From 155d539634571bcd97f07884f28c61ef18b863c2 Mon Sep 17 00:00:00 2001 From: Ryan Mitchell Date: Mon, 10 Feb 2020 13:35:24 -0800 Subject: [PATCH] Sort bag by attribute key when using libs When shared libraries are assigned package ids in a different order than compile order, bag resources that use attributes from both multiple libraries will not be sorted in ascending attribute id order. This change detects when the attribute ids are not in order and sorts the bag entries accordingly. The change is designed to be less invasive. Deduping the GetBag logic should probably be spun off in a separate bug. Bug: 147674078 Test: libandroidfw_tests Change-Id: Id8ce8e9c7ef294fcc312b77468136067d392dbd0 --- libs/androidfw/AssetManager2.cpp | 29 ++++++++++++++++-- libs/androidfw/tests/AssetManager2_test.cpp | 21 +++++++++++++ libs/androidfw/tests/data/lib_two/R.h | 12 ++++++-- libs/androidfw/tests/data/lib_two/lib_two.apk | Bin 1426 -> 1586 bytes .../tests/data/lib_two/res/values/values.xml | 11 ++++--- libs/androidfw/tests/data/libclient/R.h | 1 + .../tests/data/libclient/libclient.apk | Bin 1982 -> 2168 bytes .../data/libclient/res/values/values.xml | 6 ++++ 8 files changed, 70 insertions(+), 10 deletions(-) diff --git a/libs/androidfw/AssetManager2.cpp b/libs/androidfw/AssetManager2.cpp index 8cfd2d8ca696f..32086625a7267 100644 --- a/libs/androidfw/AssetManager2.cpp +++ b/libs/androidfw/AssetManager2.cpp @@ -992,6 +992,11 @@ const ResolvedBag* AssetManager2::GetBag(uint32_t resid) { return bag; } +static bool compare_bag_entries(const ResolvedBag::Entry& entry1, + const ResolvedBag::Entry& entry2) { + return entry1.key < entry2.key; +} + const ResolvedBag* AssetManager2::GetBag(uint32_t resid, std::vector& child_resids) { auto cached_iter = cached_bags_.find(resid); if (cached_iter != cached_bags_.end()) { @@ -1027,13 +1032,15 @@ const ResolvedBag* AssetManager2::GetBag(uint32_t resid, std::vector& child_resids.push_back(resid); uint32_t parent_resid = dtohl(map->parent.ident); - if (parent_resid == 0 || std::find(child_resids.begin(), child_resids.end(), parent_resid) + if (parent_resid == 0U || std::find(child_resids.begin(), child_resids.end(), parent_resid) != child_resids.end()) { - // There is no parent or that a circular dependency exist, meaning there is nothing to - // inherit and we can do a simple copy of the entries in the map. + // There is no parent or a circular dependency exist, meaning there is nothing to inherit and + // we can do a simple copy of the entries in the map. const size_t entry_count = map_entry_end - map_entry; util::unique_cptr new_bag{reinterpret_cast( malloc(sizeof(ResolvedBag) + (entry_count * sizeof(ResolvedBag::Entry))))}; + + bool sort_entries = false; ResolvedBag::Entry* new_entry = new_bag->entries; for (; map_entry != map_entry_end; ++map_entry) { uint32_t new_key = dtohl(map_entry->name.ident); @@ -1059,8 +1066,15 @@ const ResolvedBag* AssetManager2::GetBag(uint32_t resid, std::vector& new_entry->value.data, new_key); return nullptr; } + sort_entries = sort_entries || + (new_entry != new_bag->entries && (new_entry->key < (new_entry - 1U)->key)); ++new_entry; } + + if (sort_entries) { + std::sort(new_bag->entries, new_bag->entries + entry_count, compare_bag_entries); + } + new_bag->type_spec_flags = entry.type_flags; new_bag->entry_count = static_cast(entry_count); ResolvedBag* result = new_bag.get(); @@ -1091,6 +1105,7 @@ const ResolvedBag* AssetManager2::GetBag(uint32_t resid, std::vector& const ResolvedBag::Entry* const parent_entry_end = parent_entry + parent_bag->entry_count; // The keys are expected to be in sorted order. Merge the two bags. + bool sort_entries = false; while (map_entry != map_entry_end && parent_entry != parent_entry_end) { uint32_t child_key = dtohl(map_entry->name.ident); if (!is_internal_resid(child_key)) { @@ -1123,6 +1138,8 @@ const ResolvedBag* AssetManager2::GetBag(uint32_t resid, std::vector& memcpy(new_entry, parent_entry, sizeof(*new_entry)); } + sort_entries = sort_entries || + (new_entry != new_bag->entries && (new_entry->key < (new_entry - 1U)->key)); if (child_key >= parent_entry->key) { // Move to the next parent entry if we used it or it was overridden. ++parent_entry; @@ -1153,6 +1170,8 @@ const ResolvedBag* AssetManager2::GetBag(uint32_t resid, std::vector& new_entry->value.dataType, new_entry->value.data, new_key); return nullptr; } + sort_entries = sort_entries || + (new_entry != new_bag->entries && (new_entry->key < (new_entry - 1U)->key)); ++map_entry; ++new_entry; } @@ -1172,6 +1191,10 @@ const ResolvedBag* AssetManager2::GetBag(uint32_t resid, std::vector& new_bag.release(), sizeof(ResolvedBag) + (actual_count * sizeof(ResolvedBag::Entry))))); } + if (sort_entries) { + std::sort(new_bag->entries, new_bag->entries + actual_count, compare_bag_entries); + } + // Combine flags from the parent and our own bag. new_bag->type_spec_flags = entry.type_flags | parent_bag->type_spec_flags; new_bag->entry_count = static_cast(actual_count); diff --git a/libs/androidfw/tests/AssetManager2_test.cpp b/libs/androidfw/tests/AssetManager2_test.cpp index 2f6f3dfcaf1c8..35fea7ab86cb6 100644 --- a/libs/androidfw/tests/AssetManager2_test.cpp +++ b/libs/androidfw/tests/AssetManager2_test.cpp @@ -285,6 +285,27 @@ TEST_F(AssetManager2Test, FindsBagResourceFromSharedLibrary) { EXPECT_EQ(0x03, get_package_id(bag->entries[1].key)); } +TEST_F(AssetManager2Test, FindsBagResourceFromMultipleSharedLibraries) { + AssetManager2 assetmanager; + + // libclient is built with lib_one and then lib_two in order. + // Reverse the order to test that proper package ID re-assignment is happening. + assetmanager.SetApkAssets( + {lib_two_assets_.get(), lib_one_assets_.get(), libclient_assets_.get()}); + + const ResolvedBag* bag = assetmanager.GetBag(libclient::R::style::ThemeMultiLib); + ASSERT_NE(nullptr, bag); + ASSERT_EQ(bag->entry_count, 2u); + + // First attribute comes from lib_two. + EXPECT_EQ(2, bag->entries[0].cookie); + EXPECT_EQ(0x02, get_package_id(bag->entries[0].key)); + + // The next two attributes come from lib_one. + EXPECT_EQ(2, bag->entries[1].cookie); + EXPECT_EQ(0x03, get_package_id(bag->entries[1].key)); +} + TEST_F(AssetManager2Test, FindsStyleResourceWithParentFromSharedLibrary) { AssetManager2 assetmanager; diff --git a/libs/androidfw/tests/data/lib_two/R.h b/libs/androidfw/tests/data/lib_two/R.h index 92b9cc10e7a87..fd5a910961cb0 100644 --- a/libs/androidfw/tests/data/lib_two/R.h +++ b/libs/androidfw/tests/data/lib_two/R.h @@ -30,16 +30,22 @@ struct R { }; }; + struct integer { + enum : uint32_t { + bar = 0x02020000, // default + }; + }; + struct string { enum : uint32_t { - LibraryString = 0x02020000, // default - foo = 0x02020001, // default + LibraryString = 0x02030000, // default + foo = 0x02030001, // default }; }; struct style { enum : uint32_t { - Theme = 0x02030000, // default + Theme = 0x02040000, // default }; }; }; diff --git a/libs/androidfw/tests/data/lib_two/lib_two.apk b/libs/androidfw/tests/data/lib_two/lib_two.apk index 486c23000276df564f5e0cbdaa4c3b569db41284..8193db637eed81bb8b647516c77548083ab578c9 100644 GIT binary patch delta 408 zcmYjNu}T9`5S;hkor!u#3PnY+FbGyb2nbhsjfIt>wfy7}z1I7T-JU}c1#4HSn3?&RDlkYGovokP&bQMgV z!z3*ZvQh_#&48E{qK2V}!FaL@v*qM^<{nK6pg0p011Sd(W@HLssDX-s - + + 1337 + + Hi from library two - + Foo from lib_two - + diff --git a/libs/androidfw/tests/data/libclient/R.h b/libs/androidfw/tests/data/libclient/R.h index 43d1f9bb68e79..e21b3ebae826b 100644 --- a/libs/androidfw/tests/data/libclient/R.h +++ b/libs/androidfw/tests/data/libclient/R.h @@ -34,6 +34,7 @@ struct R { struct style { enum : uint32_t { Theme = 0x7f020000, // default + ThemeMultiLib = 0x7f020001, // default }; }; diff --git a/libs/androidfw/tests/data/libclient/libclient.apk b/libs/androidfw/tests/data/libclient/libclient.apk index 17990248e86254d09cb82c2d3c648d1b0b630be4..4b9a8833c64a7fb8e3f06592e2102292643843e0 100644 GIT binary patch delta 789 zcmdnT|3jcYz?+#xgn@~HgMp!8K4Z76sjLkn1H%<21_ogU1_sBxl%o916yL7f~idZs)$b0X`Z{|Om`dsfSsji0Q( z#QTov{>FE)XTMC(`hU{$cbuh?nn7)m(HE|~!}AmGYid>|f0^=L@uSpLx83!x?*;cq zZ2UGcsjBmy^VZVSmZtS;Ta$mx`Fu>H>~6)e^!sn;Y+T4Jdurdjug~+IuCq?AeeACm zG&`c@*pvl{x2mSq>c70aeZo&G+b{X|K3&Y3c;()=Ifdqr1YF{tiw9jU_C3#f-ePm?su7N=33616+heZus9^RfguYHZjld4ysni5b^kPHwwfG{Id2*Vm610;r`gHdC037fWt z00T1+LI6Y`BNH&Su`@6*1G$XM5H=I@pKh|0U6cNzC6A&&aus}5-vR2 zdAX+IQH}8R|9AF&zF}YT_qajDBzv31cE3$;i$j6}=zDNbT;1ll{|T!c1H%(m1_nN$ zSW#+merZv1YO!8oQE@U80}oJU@_a@$);&N4^^@Pfv@-3!#%?h9fpfDU5 zz*!6oTtK!65Gw(($>cO<)yXTFRW_eyHeeF=Vdw!00s|iyBMb~7Kx{FYk;!^;Ae)0K zNDD|5Bnt!%j7&ZZAZuV^0YElHH6y#mQplKyQF=nV5xM_9*KoJh0K9CGB-r0d_ zm_U3MD4&smi8;WVkx85xTgsUHpWR#<5;};$0qO(-1%_>n(vt%@6xp@com.android.lib_one:string/foo + + + @com.android.lib_one:string/foo