libandroidfw: Remove pre-verification
This added more up-front cost to loading an APK and didn't provide
a significant benefit to resource retrieval.
Test: make libandroidfw_tests
Change-Id: Idbf993abc433fa8c8950d106c66469b310b66f7f
diff --git a/libs/androidfw/AssetManager2.cpp b/libs/androidfw/AssetManager2.cpp
index 94a05b2..415d3e3 100644
--- a/libs/androidfw/AssetManager2.cpp
+++ b/libs/androidfw/AssetManager2.cpp
@@ -299,10 +299,9 @@
const PackageGroup& package_group = package_groups_[idx];
const size_t package_count = package_group.packages_.size();
+ FindEntryResult current_entry;
for (size_t i = 0; i < package_count; i++) {
const LoadedPackage* loaded_package = package_group.packages_[i];
-
- FindEntryResult current_entry;
if (!loaded_package->FindEntry(type_idx, entry_id, *desired_config, ¤t_entry)) {
continue;
}
@@ -394,7 +393,7 @@
return kInvalidCookie;
}
- if (dtohl(entry.entry->flags) & ResTable_entry::FLAG_COMPLEX) {
+ if (dtohs(entry.entry->flags) & ResTable_entry::FLAG_COMPLEX) {
if (!may_be_bag) {
LOG(ERROR) << base::StringPrintf("Resource %08x is a complex map type.", resid);
return kInvalidCookie;
diff --git a/libs/androidfw/LoadedArsc.cpp b/libs/androidfw/LoadedArsc.cpp
index 6ee5e86..28548e2 100644
--- a/libs/androidfw/LoadedArsc.cpp
+++ b/libs/androidfw/LoadedArsc.cpp
@@ -129,9 +129,14 @@
// Precondition: The header passed in has already been verified, so reading any fields and trusting
// the ResChunk_header is safe.
static bool VerifyResTableType(const ResTable_type* header) {
+ if (header->id == 0) {
+ LOG(ERROR) << "RES_TABLE_TYPE_TYPE has invalid ID 0.";
+ return false;
+ }
+
const size_t entry_count = dtohl(header->entryCount);
if (entry_count > std::numeric_limits<uint16_t>::max()) {
- LOG(ERROR) << "Too many entries in RES_TABLE_TYPE_TYPE.";
+ LOG(ERROR) << "RES_TABLE_TYPE_TYPE has too many entries (" << entry_count << ").";
return false;
}
@@ -141,17 +146,17 @@
const size_t offsets_length = sizeof(uint32_t) * entry_count;
if (offsets_offset > entries_offset || entries_offset - offsets_offset < offsets_length) {
- LOG(ERROR) << "Entry offsets overlap actual entry data.";
+ LOG(ERROR) << "RES_TABLE_TYPE_TYPE entry offsets overlap actual entry data.";
return false;
}
if (entries_offset > dtohl(header->header.size)) {
- LOG(ERROR) << "Entry offsets extend beyond chunk.";
+ LOG(ERROR) << "RES_TABLE_TYPE_TYPE entry offsets extend beyond chunk.";
return false;
}
if (entries_offset & 0x03) {
- LOG(ERROR) << "Entries start at unaligned address.";
+ LOG(ERROR) << "RES_TABLE_TYPE_TYPE entries start at unaligned address.";
return false;
}
return true;
@@ -236,7 +241,6 @@
return true;
}
-template <bool Verified>
bool LoadedPackage::FindEntry(const TypeSpecPtr& type_spec_ptr, uint16_t entry_idx,
const ResTable_config& config, FindEntryResult* out_entry) const {
const ResTable_config* best_config = nullptr;
@@ -254,13 +258,6 @@
const size_t entry_count = dtohl(type_chunk->entryCount);
const size_t offsets_offset = dtohs(type_chunk->header.headerSize);
- // If the package hasn't been verified, do bounds checking.
- if (!Verified) {
- if (!VerifyResTableType(type_chunk)) {
- continue;
- }
- }
-
// Check if there is the desired entry in this type.
if (type_chunk->flags & ResTable_type::FLAG_SPARSE) {
@@ -282,7 +279,7 @@
// Extract the offset from the entry. Each offset must be a multiple of 4 so we store it as
// the real offset divided by 4.
- best_offset = dtohs(result->offset) * 4u;
+ best_offset = uint32_t{dtohs(result->offset)} * 4u;
} else {
if (entry_idx >= entry_count) {
// This entry cannot be here.
@@ -309,10 +306,8 @@
return false;
}
- if (!Verified) {
- if (!VerifyResTableEntry(best_type, best_offset, entry_idx)) {
- return false;
- }
+ if (UNLIKELY(!VerifyResTableEntry(best_type, best_offset, entry_idx))) {
+ return false;
}
const ResTable_entry* best_entry = reinterpret_cast<const ResTable_entry*>(
@@ -334,7 +329,7 @@
// If the type IDs are offset in this package, we need to take that into account when searching
// for a type.
const TypeSpecPtr& ptr = type_specs_[type_idx - type_id_offset_];
- if (ptr == nullptr) {
+ if (UNLIKELY(ptr == nullptr)) {
return false;
}
@@ -345,54 +340,7 @@
return false;
}
}
-
- // Don't bother checking if the entry ID is larger than the number of entries.
- if (entry_idx >= dtohl(ptr->type_spec->entryCount)) {
- return false;
- }
-
- if (verified_) {
- return FindEntry<true>(ptr, entry_idx, config, out_entry);
- }
- return FindEntry<false>(ptr, entry_idx, config, out_entry);
-}
-
-static bool VerifyType(const Chunk& chunk) {
- ATRACE_CALL();
- const ResTable_type* header = chunk.header<ResTable_type, kResTableTypeMinSize>();
-
- if (!VerifyResTableType(header)) {
- return false;
- }
-
- const size_t entry_count = dtohl(header->entryCount);
- const size_t offsets_offset = chunk.header_size();
-
- if (header->flags & ResTable_type::FLAG_SPARSE) {
- // This is encoded as a sparse map, so perform a binary search.
- const ResTable_sparseTypeEntry* sparse_indices =
- reinterpret_cast<const ResTable_sparseTypeEntry*>(reinterpret_cast<const uint8_t*>(header) +
- offsets_offset);
- for (size_t i = 0; i < entry_count; i++) {
- const uint32_t offset = uint32_t{dtohs(sparse_indices[i].offset)} * 4u;
- if (!VerifyResTableEntry(header, offset, i)) {
- return false;
- }
- }
- } else {
- // Check each entry offset.
- const uint32_t* offsets = reinterpret_cast<const uint32_t*>(
- reinterpret_cast<const uint8_t*>(header) + offsets_offset);
- for (size_t i = 0; i < entry_count; i++) {
- uint32_t offset = dtohl(offsets[i]);
- if (offset != ResTable_type::NO_ENTRY) {
- if (!VerifyResTableEntry(header, offset, i)) {
- return false;
- }
- }
- }
- }
- return true;
+ return FindEntry(ptr, entry_idx, config, out_entry);
}
void LoadedPackage::CollectConfigurations(bool exclude_mipmap,
@@ -507,7 +455,7 @@
sizeof(ResTable_package) - sizeof(ResTable_package::typeIdOffset);
const ResTable_package* header = chunk.header<ResTable_package, kMinPackageSize>();
if (header == nullptr) {
- LOG(ERROR) << "Chunk RES_TABLE_PACKAGE_TYPE is too small.";
+ LOG(ERROR) << "RES_TABLE_PACKAGE_TYPE too small.";
return {};
}
@@ -529,7 +477,7 @@
if (header->header.headerSize >= sizeof(ResTable_package)) {
uint32_t type_id_offset = dtohl(header->typeIdOffset);
if (type_id_offset > std::numeric_limits<uint8_t>::max()) {
- LOG(ERROR) << "Type ID offset in RES_TABLE_PACKAGE_TYPE is too large.";
+ LOG(ERROR) << "RES_TABLE_PACKAGE_TYPE type ID offset too large.";
return {};
}
loaded_package->type_id_offset_ = static_cast<int>(type_id_offset);
@@ -560,7 +508,7 @@
status_t err = loaded_package->type_string_pool_.setTo(
child_chunk.header<ResStringPool_header>(), child_chunk.size());
if (err != NO_ERROR) {
- LOG(ERROR) << "Corrupt package type string pool.";
+ LOG(ERROR) << "RES_STRING_POOL_TYPE for types corrupt.";
return {};
}
} else if (pool_address == header_address + dtohl(header->keyStrings)) {
@@ -568,11 +516,11 @@
status_t err = loaded_package->key_string_pool_.setTo(
child_chunk.header<ResStringPool_header>(), child_chunk.size());
if (err != NO_ERROR) {
- LOG(ERROR) << "Corrupt package key string pool.";
+ LOG(ERROR) << "RES_STRING_POOL_TYPE for keys corrupt.";
return {};
}
} else {
- LOG(WARNING) << "Too many string pool chunks found in package.";
+ LOG(WARNING) << "Too many RES_STRING_POOL_TYPEs found in RES_TABLE_PACKAGE_TYPE.";
}
} break;
@@ -603,18 +551,18 @@
const ResTable_typeSpec* type_spec = child_chunk.header<ResTable_typeSpec>();
if (type_spec == nullptr) {
- LOG(ERROR) << "Chunk RES_TABLE_TYPE_SPEC_TYPE is too small.";
+ LOG(ERROR) << "RES_TABLE_TYPE_SPEC_TYPE too small.";
return {};
}
if (type_spec->id == 0) {
- LOG(ERROR) << "Chunk RES_TABLE_TYPE_SPEC_TYPE has invalid ID 0.";
+ LOG(ERROR) << "RES_TABLE_TYPE_SPEC_TYPE has invalid ID 0.";
return {};
}
if (loaded_package->type_id_offset_ + static_cast<int>(type_spec->id) >
std::numeric_limits<uint8_t>::max()) {
- LOG(ERROR) << "Chunk RES_TABLE_TYPE_SPEC_TYPE has out of range ID.";
+ LOG(ERROR) << "RES_TABLE_TYPE_SPEC_TYPE has out of range ID.";
return {};
}
@@ -626,12 +574,12 @@
// There can only be 2^16 entries in a type, because that is the ID
// space for entries (EEEE) in the resource ID 0xPPTTEEEE.
if (entry_count > std::numeric_limits<uint16_t>::max()) {
- LOG(ERROR) << "Too many entries in RES_TABLE_TYPE_SPEC_TYPE: " << entry_count << ".";
+ LOG(ERROR) << "RES_TABLE_TYPE_SPEC_TYPE has too many entries (" << entry_count << ").";
return {};
}
if (entry_count * sizeof(uint32_t) > chunk.data_size()) {
- LOG(ERROR) << "Chunk too small to hold entries in RES_TABLE_TYPE_SPEC_TYPE.";
+ LOG(ERROR) << "RES_TABLE_TYPE_SPEC_TYPE too small to hold entries.";
return {};
}
@@ -650,41 +598,32 @@
case RES_TABLE_TYPE_TYPE: {
const ResTable_type* type = child_chunk.header<ResTable_type, kResTableTypeMinSize>();
if (type == nullptr) {
- LOG(ERROR) << "Chunk RES_TABLE_TYPE_TYPE is too small.";
+ LOG(ERROR) << "RES_TABLE_TYPE_TYPE too small.";
return {};
}
- if (type->id == 0) {
- LOG(ERROR) << "Chunk RES_TABLE_TYPE_TYPE has invalid ID 0.";
+ if (!VerifyResTableType(type)) {
return {};
}
// Type chunks must be preceded by their TypeSpec chunks.
if (!types_builder || type->id - 1 != last_type_idx) {
- LOG(ERROR) << "Found RES_TABLE_TYPE_TYPE chunk without RES_TABLE_TYPE_SPEC_TYPE.";
+ LOG(ERROR) << "RES_TABLE_TYPE_TYPE found without preceding RES_TABLE_TYPE_SPEC_TYPE.";
return {};
}
- // Only verify the type if we haven't already failed verification.
- if (loaded_package->verified_) {
- if (!VerifyType(child_chunk)) {
- LOG(WARNING) << "Package failed verification, resource retrieval may be slower";
- loaded_package->verified_ = false;
- }
- }
-
types_builder->AddType(type);
} break;
case RES_TABLE_LIBRARY_TYPE: {
const ResTable_lib_header* lib = child_chunk.header<ResTable_lib_header>();
if (lib == nullptr) {
- LOG(ERROR) << "Chunk RES_TABLE_LIBRARY_TYPE is too small.";
+ LOG(ERROR) << "RES_TABLE_LIBRARY_TYPE too small.";
return {};
}
if (child_chunk.data_size() / sizeof(ResTable_lib_entry) < dtohl(lib->count)) {
- LOG(ERROR) << "Chunk too small to hold entries in RES_TABLE_LIBRARY_TYPE.";
+ LOG(ERROR) << "RES_TABLE_LIBRARY_TYPE too small to hold entries.";
return {};
}
@@ -751,7 +690,7 @@
const uint8_t type_id = get_type_id(resid);
const uint16_t entry_id = get_entry_id(resid);
- if (type_id == 0) {
+ if (UNLIKELY(type_id == 0)) {
LOG(ERROR) << base::StringPrintf("Invalid ID 0x%08x.", resid);
return false;
}
@@ -769,7 +708,7 @@
ATRACE_CALL();
const ResTable_header* header = chunk.header<ResTable_header>();
if (header == nullptr) {
- LOG(ERROR) << "Chunk RES_TABLE_TYPE is too small.";
+ LOG(ERROR) << "RES_TABLE_TYPE too small.";
return false;
}
@@ -788,11 +727,11 @@
status_t err = global_string_pool_.setTo(child_chunk.header<ResStringPool_header>(),
child_chunk.size());
if (err != NO_ERROR) {
- LOG(ERROR) << "Corrupt string pool.";
+ LOG(ERROR) << "RES_STRING_POOL_TYPE corrupt.";
return false;
}
} else {
- LOG(WARNING) << "Multiple string pool chunks found in resource table.";
+ LOG(WARNING) << "Multiple RES_STRING_POOL_TYPEs found in RES_TABLE_TYPE.";
}
break;
diff --git a/libs/androidfw/include/androidfw/LoadedArsc.h b/libs/androidfw/include/androidfw/LoadedArsc.h
index 377735b..965e2db 100644
--- a/libs/androidfw/include/androidfw/LoadedArsc.h
+++ b/libs/androidfw/include/androidfw/LoadedArsc.h
@@ -119,11 +119,6 @@
return overlay_;
}
- // Returns true if this package is verified to be valid.
- inline bool IsVerified() const {
- return verified_;
- }
-
// Returns the map of package name to package ID used in this LoadedPackage. At runtime, a
// package could have been assigned a different package ID than what this LoadedPackage was
// compiled with. AssetManager rewrites the package IDs so that they are compatible at runtime.
@@ -145,7 +140,6 @@
LoadedPackage();
- template <bool Verified>
bool FindEntry(const util::unique_cptr<TypeSpec>& type_spec_ptr, uint16_t entry_idx,
const ResTable_config& config, FindEntryResult* out_entry) const;
@@ -157,7 +151,6 @@
bool dynamic_ = false;
bool system_ = false;
bool overlay_ = false;
- bool verified_ = true;
ByteBucketArray<util::unique_cptr<TypeSpec>> type_specs_;
std::vector<DynamicPackageEntry> dynamic_package_map_;
diff --git a/libs/androidfw/tests/ApkAssets_test.cpp b/libs/androidfw/tests/ApkAssets_test.cpp
index ba5844b..6c43a67 100644
--- a/libs/androidfw/tests/ApkAssets_test.cpp
+++ b/libs/androidfw/tests/ApkAssets_test.cpp
@@ -39,7 +39,6 @@
const LoadedPackage* loaded_package = loaded_arsc->GetPackageForId(0x7f010000);
ASSERT_NE(nullptr, loaded_package);
- EXPECT_TRUE(loaded_package->IsVerified());
std::unique_ptr<Asset> asset = loaded_apk->Open("res/layout/main.xml");
ASSERT_NE(nullptr, asset);
@@ -59,7 +58,6 @@
const LoadedPackage* loaded_package = loaded_arsc->GetPackageForId(0x7f010000);
ASSERT_NE(nullptr, loaded_package);
- EXPECT_TRUE(loaded_package->IsVerified());
std::unique_ptr<Asset> asset = loaded_apk->Open("res/layout/main.xml");
ASSERT_NE(nullptr, asset);
@@ -114,20 +112,6 @@
ASSERT_NE(nullptr, loaded_overlay_apk);
}
-TEST(ApkAssetsTest, LoadUnverifiableApk) {
- std::unique_ptr<const ApkAssets> loaded_apk =
- ApkAssets::Load(GetTestDataPath() + "/unverified/unverified.apk");
- ASSERT_NE(nullptr, loaded_apk);
-
- const LoadedArsc* loaded_arsc = loaded_apk->GetLoadedArsc();
- ASSERT_NE(nullptr, loaded_arsc);
-
- const LoadedPackage* loaded_package = loaded_arsc->GetPackageForId(0x7f010000);
- ASSERT_NE(nullptr, loaded_package);
-
- EXPECT_FALSE(loaded_package->IsVerified());
-}
-
TEST(ApkAssetsTest, CreateAndDestroyAssetKeepsApkAssetsOpen) {
std::unique_ptr<const ApkAssets> loaded_apk =
ApkAssets::Load(GetTestDataPath() + "/basic/basic.apk");
diff --git a/libs/androidfw/tests/AssetManager2_bench.cpp b/libs/androidfw/tests/AssetManager2_bench.cpp
index ea9d427f..85e8f25 100644
--- a/libs/androidfw/tests/AssetManager2_bench.cpp
+++ b/libs/androidfw/tests/AssetManager2_bench.cpp
@@ -26,12 +26,10 @@
#include "data/basic/R.h"
#include "data/libclient/R.h"
#include "data/styles/R.h"
-#include "data/unverified/R.h"
namespace app = com::android::app;
namespace basic = com::android::basic;
namespace libclient = com::android::libclient;
-namespace unverified = com::android::unverified;
namespace android {
@@ -95,12 +93,6 @@
}
BENCHMARK(BM_AssetManagerGetResourceOld);
-static void BM_AssetManagerGetResourceUnverified(benchmark::State& state) {
- GetResourceBenchmark({GetTestDataPath() + "/unverified/unverified.apk"}, nullptr /*config*/,
- unverified::R::integer::number1, state);
-}
-BENCHMARK(BM_AssetManagerGetResourceUnverified);
-
static void BM_AssetManagerGetLibraryResource(benchmark::State& state) {
GetResourceBenchmark(
{GetTestDataPath() + "/lib_two/lib_two.apk", GetTestDataPath() + "/lib_one/lib_one.apk",
@@ -183,30 +175,6 @@
}
BENCHMARK(BM_AssetManagerGetBagOld);
-static void BM_AssetManagerGetBagUnverified(benchmark::State& state) {
- std::unique_ptr<const ApkAssets> apk =
- ApkAssets::Load(GetTestDataPath() + "/unverified/unverified.apk");
- if (apk == nullptr) {
- state.SkipWithError("Failed to load assets");
- return;
- }
-
- AssetManager2 assets;
- assets.SetApkAssets({apk.get()});
-
- while (state.KeepRunning()) {
- const ResolvedBag* bag = assets.GetBag(unverified::R::array::integerArray1);
- const auto bag_end = end(bag);
- for (auto iter = begin(bag); iter != bag_end; ++iter) {
- uint32_t key = iter->key;
- Res_value value = iter->value;
- benchmark::DoNotOptimize(key);
- benchmark::DoNotOptimize(value);
- }
- }
-}
-BENCHMARK(BM_AssetManagerGetBagUnverified);
-
static void BM_AssetManagerGetResourceLocales(benchmark::State& state) {
std::unique_ptr<const ApkAssets> apk = ApkAssets::Load(kFrameworkPath);
if (apk == nullptr) {
diff --git a/libs/androidfw/tests/AssetManager2_test.cpp b/libs/androidfw/tests/AssetManager2_test.cpp
index 567adfe..92462a6 100644
--- a/libs/androidfw/tests/AssetManager2_test.cpp
+++ b/libs/androidfw/tests/AssetManager2_test.cpp
@@ -28,7 +28,6 @@
#include "data/libclient/R.h"
#include "data/styles/R.h"
#include "data/system/R.h"
-#include "data/unverified/R.h"
namespace app = com::android::app;
namespace appaslib = com::android::appaslib::app;
@@ -36,7 +35,6 @@
namespace lib_one = com::android::lib_one;
namespace lib_two = com::android::lib_two;
namespace libclient = com::android::libclient;
-namespace unverified = com::android::unverified;
namespace android {
@@ -452,30 +450,4 @@
TEST_F(AssetManager2Test, OpensFileFromMultipleApkAssets) {}
-TEST_F(AssetManager2Test, OperateOnUnverifiedApkAssets) {
- std::unique_ptr<const ApkAssets> unverified_assets =
- ApkAssets::Load(GetTestDataPath() + "/unverified/unverified.apk");
- ASSERT_NE(nullptr, unverified_assets);
-
- AssetManager2 assetmanager;
- assetmanager.SetApkAssets({unverified_assets.get()});
-
- Res_value value;
- ResTable_config config;
- uint32_t flags;
-
- EXPECT_EQ(kInvalidCookie,
- assetmanager.GetResource(unverified::R::string::test1, false /*may_be_bag*/, 0u, &value,
- &config, &flags));
- EXPECT_EQ(kInvalidCookie,
- assetmanager.GetResource(unverified::R::string::test2, false /*may_be_bag*/, 0u, &value,
- &config, &flags));
- EXPECT_NE(kInvalidCookie,
- assetmanager.GetResource(unverified::R::integer::number1, false /*may_be_bag*/, 0u,
- &value, &config, &flags));
-
- EXPECT_EQ(nullptr, assetmanager.GetBag(unverified::R::style::Theme1));
- EXPECT_NE(nullptr, assetmanager.GetBag(unverified::R::array::integerArray1));
-}
-
} // namespace android
diff --git a/libs/androidfw/tests/data/unverified/R.h b/libs/androidfw/tests/data/unverified/R.h
deleted file mode 100644
index b734b49..0000000
--- a/libs/androidfw/tests/data/unverified/R.h
+++ /dev/null
@@ -1,32 +0,0 @@
-/*
- * Copyright (C) 2016 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-#ifndef TESTS_DATA_UNVERIFIED_R_H_
-#define TESTS_DATA_UNVERIFIED_R_H_
-
-#include <cstdint>
-
-#include "tests/data/basic/R.h"
-
-namespace com {
-namespace android {
-
-namespace unverified = basic;
-
-} // namespace android
-} // namespace com
-
-#endif /* TESTS_DATA_UNVERIFIED_R_H_ */
diff --git a/libs/androidfw/tests/data/unverified/unverified.apk b/libs/androidfw/tests/data/unverified/unverified.apk
deleted file mode 100644
index 234b390..0000000
--- a/libs/androidfw/tests/data/unverified/unverified.apk
+++ /dev/null
Binary files differ