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
diff --git a/libs/androidfw/AssetManager2.cpp b/libs/androidfw/AssetManager2.cpp
index 8cfd2d8..3208662 100644
--- a/libs/androidfw/AssetManager2.cpp
+++ b/libs/androidfw/AssetManager2.cpp
@@ -992,6 +992,11 @@
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<uint32_t>& child_resids) {
auto cached_iter = cached_bags_.find(resid);
if (cached_iter != cached_bags_.end()) {
@@ -1027,13 +1032,15 @@
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<ResolvedBag> new_bag{reinterpret_cast<ResolvedBag*>(
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 @@
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<uint32_t>(entry_count);
ResolvedBag* result = new_bag.get();
@@ -1091,6 +1105,7 @@
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 @@
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 @@
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 @@
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<uint32_t>(actual_count);
diff --git a/libs/androidfw/tests/AssetManager2_test.cpp b/libs/androidfw/tests/AssetManager2_test.cpp
index 2f6f3df..35fea7a 100644
--- a/libs/androidfw/tests/AssetManager2_test.cpp
+++ b/libs/androidfw/tests/AssetManager2_test.cpp
@@ -285,6 +285,27 @@
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 92b9cc1..fd5a910 100644
--- a/libs/androidfw/tests/data/lib_two/R.h
+++ b/libs/androidfw/tests/data/lib_two/R.h
@@ -30,16 +30,22 @@
};
};
+ 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 486c230..8193db6 100644
--- a/libs/androidfw/tests/data/lib_two/lib_two.apk
+++ b/libs/androidfw/tests/data/lib_two/lib_two.apk
Binary files differ
diff --git a/libs/androidfw/tests/data/lib_two/res/values/values.xml b/libs/androidfw/tests/data/lib_two/res/values/values.xml
index 340d14c..4e1d69a 100644
--- a/libs/androidfw/tests/data/lib_two/res/values/values.xml
+++ b/libs/androidfw/tests/data/lib_two/res/values/values.xml
@@ -18,14 +18,17 @@
<public type="attr" name="attr3" id="0x00010000" />
<attr name="attr3" format="integer" />
- <public type="string" name="LibraryString" id="0x00020000" />
+ <public type="integer" name="bar" id="0x00020000" />
+ <integer name="bar">1337</integer>
+
+ <public type="string" name="LibraryString" id="0x00030000" />
<string name="LibraryString">Hi from library two</string>
- <public type="string" name="foo" id="0x00020001" />
+ <public type="string" name="foo" id="0x00030001" />
<string name="foo">Foo from lib_two</string>
- <public type="style" name="Theme" id="0x02030000" />
+ <public type="style" name="Theme" id="0x00040000" />
<style name="Theme">
- <item name="com.android.lib_two:attr3">800</item>
+ <item name="com.android.lib_two:attr3">@integer/bar</item>
</style>
</resources>
diff --git a/libs/androidfw/tests/data/libclient/R.h b/libs/androidfw/tests/data/libclient/R.h
index 43d1f9b..e21b3eb 100644
--- a/libs/androidfw/tests/data/libclient/R.h
+++ b/libs/androidfw/tests/data/libclient/R.h
@@ -34,6 +34,7 @@
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 1799024..4b9a883 100644
--- a/libs/androidfw/tests/data/libclient/libclient.apk
+++ b/libs/androidfw/tests/data/libclient/libclient.apk
Binary files differ
diff --git a/libs/androidfw/tests/data/libclient/res/values/values.xml b/libs/androidfw/tests/data/libclient/res/values/values.xml
index fead7c3..a29f473 100644
--- a/libs/androidfw/tests/data/libclient/res/values/values.xml
+++ b/libs/androidfw/tests/data/libclient/res/values/values.xml
@@ -27,6 +27,12 @@
<item name="bar">@com.android.lib_one:string/foo</item>
</style>
+ <public type="style" name="ThemeMultiLib" id="0x7f020001" />
+ <style name="ThemeMultiLib" >
+ <item name="com.android.lib_one:attr1">@com.android.lib_one:string/foo</item>
+ <item name="com.android.lib_two:attr3">@com.android.lib_two:integer/bar</item>
+ </style>
+
<public type="string" name="foo_one" id="0x7f030000" />
<string name="foo_one">@com.android.lib_one:string/foo</string>