From ed69ce84bdfa5ba82008a9b66485ef71c953657a Mon Sep 17 00:00:00 2001 From: Adam Lesinski Date: Mon, 20 Mar 2017 10:55:01 -0700 Subject: [PATCH] libandroidfw: Fix mass logspam of ResourceTypes warnings An overlay was incorrectly leaking its own resources into the framework resource package, which caused warnings for every app that tried to access framework resources (all of them). This change skips including any resources that are not overlaying anything (not present in IDMAP). Bug: 36256974 Test: make libandroidfw_tests Change-Id: I8c710af6849bb848938825aacca02799ee96c003 --- libs/androidfw/.clang-format | 5 + libs/androidfw/ResourceTypes.cpp | 85 +++++++++------ libs/androidfw/tests/Idmap_test.cpp | 98 ++++++++++-------- libs/androidfw/tests/data/overlay/overlay.apk | Bin 1254 -> 2442 bytes .../tests/data/overlay/res/values/values.xml | 2 + tools/aapt2/.clang-format | 6 +- 6 files changed, 119 insertions(+), 77 deletions(-) diff --git a/libs/androidfw/.clang-format b/libs/androidfw/.clang-format index ee1bee2bc644c..c91502a257f3f 100644 --- a/libs/androidfw/.clang-format +++ b/libs/androidfw/.clang-format @@ -1,2 +1,7 @@ BasedOnStyle: Google ColumnLimit: 100 +AllowShortBlocksOnASingleLine: false +AllowShortFunctionsOnASingleLine: false +CommentPragmas: NOLINT:.* +DerivePointerAlignment: false +PointerAlignment: Left diff --git a/libs/androidfw/ResourceTypes.cpp b/libs/androidfw/ResourceTypes.cpp index 359cface8b7f1..244c52577a1aa 100644 --- a/libs/androidfw/ResourceTypes.cpp +++ b/libs/androidfw/ResourceTypes.cpp @@ -6431,32 +6431,42 @@ status_t ResTable::parsePackage(const ResTable_package* const pkg, } if (newEntryCount > 0) { + bool addToType = true; uint8_t typeIndex = typeSpec->id - 1; ssize_t idmapIndex = idmapEntries.indexOfKey(typeSpec->id); if (idmapIndex >= 0) { typeIndex = idmapEntries[idmapIndex].targetTypeId() - 1; + } else if (header->resourceIDMap != NULL) { + // This is an overlay, but the types in this overlay are not + // overlaying anything according to the idmap. We can skip these + // as they will otherwise conflict with the other resources in the package + // without a mapping. + addToType = false; } - TypeList& typeList = group->types.editItemAt(typeIndex); - if (!typeList.isEmpty()) { - const Type* existingType = typeList[0]; - if (existingType->entryCount != newEntryCount && idmapIndex < 0) { - ALOGW("ResTable_typeSpec entry count inconsistent: given %d, previously %d", - (int) newEntryCount, (int) existingType->entryCount); - // We should normally abort here, but some legacy apps declare - // resources in the 'android' package (old bug in AAPT). + if (addToType) { + TypeList& typeList = group->types.editItemAt(typeIndex); + if (!typeList.isEmpty()) { + const Type* existingType = typeList[0]; + if (existingType->entryCount != newEntryCount && idmapIndex < 0) { + ALOGW("ResTable_typeSpec entry count inconsistent: " + "given %d, previously %d", + (int) newEntryCount, (int) existingType->entryCount); + // We should normally abort here, but some legacy apps declare + // resources in the 'android' package (old bug in AAPT). + } } - } - Type* t = new Type(header, package, newEntryCount); - t->typeSpec = typeSpec; - t->typeSpecFlags = (const uint32_t*)( - ((const uint8_t*)typeSpec) + dtohs(typeSpec->header.headerSize)); - if (idmapIndex >= 0) { - t->idmapEntries = idmapEntries[idmapIndex]; + Type* t = new Type(header, package, newEntryCount); + t->typeSpec = typeSpec; + t->typeSpecFlags = (const uint32_t*)( + ((const uint8_t*)typeSpec) + dtohs(typeSpec->header.headerSize)); + if (idmapIndex >= 0) { + t->idmapEntries = idmapEntries[idmapIndex]; + } + typeList.add(t); + group->largestTypeId = max(group->largestTypeId, typeSpec->id); } - typeList.add(t); - group->largestTypeId = max(group->largestTypeId, typeSpec->id); } else { ALOGV("Skipping empty ResTable_typeSpec for type %d", typeSpec->id); } @@ -6499,31 +6509,40 @@ status_t ResTable::parsePackage(const ResTable_package* const pkg, } if (newEntryCount > 0) { + bool addToType = true; uint8_t typeIndex = type->id - 1; ssize_t idmapIndex = idmapEntries.indexOfKey(type->id); if (idmapIndex >= 0) { typeIndex = idmapEntries[idmapIndex].targetTypeId() - 1; + } else if (header->resourceIDMap != NULL) { + // This is an overlay, but the types in this overlay are not + // overlaying anything according to the idmap. We can skip these + // as they will otherwise conflict with the other resources in the package + // without a mapping. + addToType = false; } - TypeList& typeList = group->types.editItemAt(typeIndex); - if (typeList.isEmpty()) { - ALOGE("No TypeSpec for type %d", type->id); - return (mError=BAD_TYPE); - } + if (addToType) { + TypeList& typeList = group->types.editItemAt(typeIndex); + if (typeList.isEmpty()) { + ALOGE("No TypeSpec for type %d", type->id); + return (mError=BAD_TYPE); + } - Type* t = typeList.editItemAt(typeList.size() - 1); - if (t->package != package) { - ALOGE("No TypeSpec for type %d", type->id); - return (mError=BAD_TYPE); - } + Type* t = typeList.editItemAt(typeList.size() - 1); + if (t->package != package) { + ALOGE("No TypeSpec for type %d", type->id); + return (mError=BAD_TYPE); + } - t->configs.add(type); + t->configs.add(type); - if (kDebugTableGetEntry) { - ResTable_config thisConfig; - thisConfig.copyFromDtoH(type->config); - ALOGI("Adding config to type %d: %s\n", type->id, - thisConfig.toString().string()); + if (kDebugTableGetEntry) { + ResTable_config thisConfig; + thisConfig.copyFromDtoH(type->config); + ALOGI("Adding config to type %d: %s\n", type->id, + thisConfig.toString().string()); + } } } else { ALOGV("Skipping empty ResTable_type for type %d", type->id); diff --git a/libs/androidfw/tests/Idmap_test.cpp b/libs/androidfw/tests/Idmap_test.cpp index 0928b1b976c3e..d12be184745c1 100644 --- a/libs/androidfw/tests/Idmap_test.cpp +++ b/libs/androidfw/tests/Idmap_test.cpp @@ -30,25 +30,23 @@ class IdmapTest : public ::testing::Test { protected: void SetUp() override { std::string contents; - ASSERT_TRUE(ReadFileFromZipToString(GetTestDataPath() + "/basic/basic.apk", - "resources.arsc", &contents)); - ASSERT_EQ(NO_ERROR, - target_table_.add(contents.data(), contents.size(), 0, true)); + ASSERT_TRUE(ReadFileFromZipToString(GetTestDataPath() + "/basic/basic.apk", "resources.arsc", + &contents)); + ASSERT_EQ(NO_ERROR, target_table_.add(contents.data(), contents.size(), 0, true)); - ASSERT_TRUE( - ReadFileFromZipToString(GetTestDataPath() + "/overlay/overlay.apk", - "resources.arsc", &overlay_data_)); + ASSERT_TRUE(ReadFileFromZipToString(GetTestDataPath() + "/overlay/overlay.apk", + "resources.arsc", &overlay_data_)); ResTable overlay_table; - ASSERT_EQ(NO_ERROR, - overlay_table.add(overlay_data_.data(), overlay_data_.size())); + ASSERT_EQ(NO_ERROR, overlay_table.add(overlay_data_.data(), overlay_data_.size())); char target_name[256] = "com.android.basic"; - ASSERT_EQ(NO_ERROR, - target_table_.createIdmap(overlay_table, 0, 0, target_name, - target_name, &data_, &data_size_)); + ASSERT_EQ(NO_ERROR, target_table_.createIdmap(overlay_table, 0, 0, target_name, target_name, + &data_, &data_size_)); } - void TearDown() override { ::free(data_); } + void TearDown() override { + ::free(data_); + } ResTable target_table_; std::string overlay_data_; @@ -56,13 +54,12 @@ class IdmapTest : public ::testing::Test { size_t data_size_ = 0; }; -TEST_F(IdmapTest, canLoadIdmap) { +TEST_F(IdmapTest, CanLoadIdmap) { ASSERT_EQ(NO_ERROR, - target_table_.add(overlay_data_.data(), overlay_data_.size(), data_, - data_size_)); + target_table_.add(overlay_data_.data(), overlay_data_.size(), data_, data_size_)); } -TEST_F(IdmapTest, overlayOverridesResourceValue) { +TEST_F(IdmapTest, OverlayOverridesResourceValue) { Res_value val; ssize_t block = target_table_.getResource(R::string::test2, &val, false); ASSERT_GE(block, 0); @@ -71,45 +68,60 @@ TEST_F(IdmapTest, overlayOverridesResourceValue) { ASSERT_TRUE(pool != NULL); ASSERT_LT(val.data, pool->size()); - size_t strLen; - const char16_t* targetStr16 = pool->stringAt(val.data, &strLen); - ASSERT_TRUE(targetStr16 != NULL); - ASSERT_EQ(String16("test2"), String16(targetStr16, strLen)); + size_t str_len; + const char16_t* target_str16 = pool->stringAt(val.data, &str_len); + ASSERT_TRUE(target_str16 != NULL); + ASSERT_EQ(String16("test2"), String16(target_str16, str_len)); ASSERT_EQ(NO_ERROR, - target_table_.add(overlay_data_.data(), overlay_data_.size(), data_, - data_size_)); + target_table_.add(overlay_data_.data(), overlay_data_.size(), data_, data_size_)); - ssize_t newBlock = target_table_.getResource(R::string::test2, &val, false); - ASSERT_GE(newBlock, 0); - ASSERT_NE(block, newBlock); + ssize_t new_block = target_table_.getResource(R::string::test2, &val, false); + ASSERT_GE(new_block, 0); + ASSERT_NE(block, new_block); ASSERT_EQ(Res_value::TYPE_STRING, val.dataType); - pool = target_table_.getTableStringBlock(newBlock); + pool = target_table_.getTableStringBlock(new_block); ASSERT_TRUE(pool != NULL); ASSERT_LT(val.data, pool->size()); - targetStr16 = pool->stringAt(val.data, &strLen); - ASSERT_TRUE(targetStr16 != NULL); - ASSERT_EQ(String16("test2-overlay"), String16(targetStr16, strLen)); + target_str16 = pool->stringAt(val.data, &str_len); + ASSERT_TRUE(target_str16 != NULL); + ASSERT_EQ(String16("test2-overlay"), String16(target_str16, str_len)); } -TEST_F(IdmapTest, overlaidResourceHasSameName) { +TEST_F(IdmapTest, OverlaidResourceHasSameName) { ASSERT_EQ(NO_ERROR, - target_table_.add(overlay_data_.data(), overlay_data_.size(), data_, - data_size_)); + target_table_.add(overlay_data_.data(), overlay_data_.size(), data_, data_size_)); - ResTable::resource_name resName; - ASSERT_TRUE( - target_table_.getResourceName(R::array::integerArray1, false, &resName)); + ResTable::resource_name res_name; + ASSERT_TRUE(target_table_.getResourceName(R::array::integerArray1, false, &res_name)); - ASSERT_TRUE(resName.package != NULL); - ASSERT_TRUE(resName.type != NULL); - ASSERT_TRUE(resName.name != NULL); + ASSERT_TRUE(res_name.package != NULL); + ASSERT_TRUE(res_name.type != NULL); + ASSERT_TRUE(res_name.name != NULL); - EXPECT_EQ(String16("com.android.basic"), - String16(resName.package, resName.packageLen)); - EXPECT_EQ(String16("array"), String16(resName.type, resName.typeLen)); - EXPECT_EQ(String16("integerArray1"), String16(resName.name, resName.nameLen)); + EXPECT_EQ(String16("com.android.basic"), String16(res_name.package, res_name.packageLen)); + EXPECT_EQ(String16("array"), String16(res_name.type, res_name.typeLen)); + EXPECT_EQ(String16("integerArray1"), String16(res_name.name, res_name.nameLen)); +} + +constexpr const uint32_t kNonOverlaidResourceId = 0x7fff0000u; + +TEST_F(IdmapTest, OverlayDoesNotIncludeNonOverlaidResources) { + // First check that the resource we're trying to not include when overlaid is present when + // the overlay is loaded as a standalone APK. + ResTable table; + ASSERT_EQ(NO_ERROR, table.add(overlay_data_.data(), overlay_data_.size(), 0, true)); + + Res_value val; + ssize_t block = table.getResource(kNonOverlaidResourceId, &val, false /*mayBeBag*/); + ASSERT_GE(block, 0); + + // Now add the overlay and verify that the unoverlaid resource is gone. + ASSERT_EQ(NO_ERROR, + target_table_.add(overlay_data_.data(), overlay_data_.size(), data_, data_size_)); + block = target_table_.getResource(kNonOverlaidResourceId, &val, false /*mayBeBag*/); + ASSERT_LT(block, 0); } } // namespace diff --git a/libs/androidfw/tests/data/overlay/overlay.apk b/libs/androidfw/tests/data/overlay/overlay.apk index e0e054343601fccafab1a5b8a1ae6d1ae35346ce..40bf17c5951aeb10bac70685fecb83f667d9bfeb 100644 GIT binary patch literal 2442 zcmWIWW@Zs#U}E54U}%`nc-+I1{~7}W!yQHj24NuWn3qzNpPAyDn3tKBT3n)6k(-m+ z=bhK=AmEbivq3ji_`i}^T-8+8oQe6)1xdpHCp=`Zbm^Ot{6z2MV&fAM-&cefU%eVE zo3Tb--BMTdpmMDC+dIo2FwZ%l{)6d$)*s1RZsK!84_xXlTV=WQnwQ1YqABLGg<)OQ z|7N_K8P^n-#UKA6?AyM-;nTtvJ^x{p`?2$F|1+I0dBvu;Hvj)@xBA<>i1pii8-uI1 zya{5jPXEHK=J$W%J5NCq#{E+~6B_1~eOV^ALc{TYgo1ug~?%w`Kn`1b~A9=wom&yq#0A?g%>r0|@g0g^E&(^Gl18Q;YQyi;9z( z7{-i(KIk3(tr&xo>_n;VF?fx0oiQe^0f$9F6J?$ z1Np2#xgsEhl(Fm#Ho!7Bm!SZtx{|>TD9-_u$pfm+1)Gr%HUm`707$(NZR=6MB;YEDkn+9cUqH10ukil?@~ha#Agj{tk{E01GJn;s5{u delta 144 zcmeAYe#W^$l##3J-sVCDW(EcY=E?Gm5==nKlF^Il3e)CRMlMECpaeq+BLf2ykOl!p z1{sEs$vo`RtUN%T%;YU>y2=pI03aWx3Zzm6L_h)GWF2<>&Bg2jjFZ1}1R`4*#A(D9 I%mPvd0Iw+(?EnA( diff --git a/libs/androidfw/tests/data/overlay/res/values/values.xml b/libs/androidfw/tests/data/overlay/res/values/values.xml index 3e1af9878429e..8e4417e457d18 100644 --- a/libs/androidfw/tests/data/overlay/res/values/values.xml +++ b/libs/androidfw/tests/data/overlay/res/values/values.xml @@ -20,4 +20,6 @@ 10 11 + + @null diff --git a/tools/aapt2/.clang-format b/tools/aapt2/.clang-format index 71c5ef2fcda08..c91502a257f3f 100644 --- a/tools/aapt2/.clang-format +++ b/tools/aapt2/.clang-format @@ -1,3 +1,7 @@ BasedOnStyle: Google ColumnLimit: 100 - +AllowShortBlocksOnASingleLine: false +AllowShortFunctionsOnASingleLine: false +CommentPragmas: NOLINT:.* +DerivePointerAlignment: false +PointerAlignment: Left