From 57cd195c4303ff248c6e016d51e3b7632cf69bae Mon Sep 17 00:00:00 2001 From: y Date: Thu, 12 Apr 2018 14:26:23 -0700 Subject: [PATCH] AAPT2: GetBag infinite recursion fix Style resources with circular parental dependencies caused infinite recursion when calling AssetManager2::GetBag. This fix allows recursion to cease when a circular dependency is found. Bug: 77928512 Change-Id: Ib900c36ab1aef5da5b03234a9484c4dad3b63c02 Test: Manual test of b/77928512 and duplicates of 74493983 --- libs/androidfw/AssetManager2.cpp | 19 ++++++++++++++---- .../include/androidfw/AssetManager2.h | 4 ++++ libs/androidfw/tests/AssetManager2_test.cpp | 11 ++++++++++ libs/androidfw/tests/data/styles/R.h | 3 +++ .../tests/data/styles/res/values/styles.xml | 16 +++++++++++++++ libs/androidfw/tests/data/styles/styles.apk | Bin 2350 -> 2550 bytes 6 files changed, 49 insertions(+), 4 deletions(-) diff --git a/libs/androidfw/AssetManager2.cpp b/libs/androidfw/AssetManager2.cpp index 5460b3b4c914f..9c1629bc36f58 100644 --- a/libs/androidfw/AssetManager2.cpp +++ b/libs/androidfw/AssetManager2.cpp @@ -18,6 +18,7 @@ #include "androidfw/AssetManager2.h" +#include #include #include @@ -567,6 +568,11 @@ ApkAssetsCookie AssetManager2::ResolveReference(ApkAssetsCookie cookie, Res_valu } const ResolvedBag* AssetManager2::GetBag(uint32_t resid) { + auto found_resids = std::vector(); + return GetBag(resid, found_resids); +} + +const ResolvedBag* AssetManager2::GetBag(uint32_t resid, std::vector& child_resids) { ATRACE_NAME("AssetManager::GetBag"); auto cached_iter = cached_bags_.find(resid); @@ -595,10 +601,15 @@ const ResolvedBag* AssetManager2::GetBag(uint32_t resid) { reinterpret_cast(reinterpret_cast(map) + map->size); const ResTable_map* const map_entry_end = map_entry + dtohl(map->count); + // Keep track of ids that have already been seen to prevent infinite loops caused by circular + // dependencies between bags + child_resids.push_back(resid); + uint32_t parent_resid = dtohl(map->parent.ident); - if (parent_resid == 0 || parent_resid == resid) { - // There is no parent, meaning there is nothing to inherit and we can do a simple - // copy of the entries in the map. + if (parent_resid == 0 || 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. 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))))}; @@ -639,7 +650,7 @@ const ResolvedBag* AssetManager2::GetBag(uint32_t resid) { entry.dynamic_ref_table->lookupResourceId(&parent_resid); // Get the parent and do a merge of the keys. - const ResolvedBag* parent_bag = GetBag(parent_resid); + const ResolvedBag* parent_bag = GetBag(parent_resid, child_resids); if (parent_bag == nullptr) { // Failed to get the parent that should exist. LOG(ERROR) << base::StringPrintf("Failed to find parent 0x%08x of bag 0x%08x.", parent_resid, diff --git a/libs/androidfw/include/androidfw/AssetManager2.h b/libs/androidfw/include/androidfw/AssetManager2.h index ef08897d997a6..ad31f69404389 100644 --- a/libs/androidfw/include/androidfw/AssetManager2.h +++ b/libs/androidfw/include/androidfw/AssetManager2.h @@ -276,6 +276,10 @@ class AssetManager2 { // This should always be called when mutating the AssetManager's configuration or ApkAssets set. void RebuildFilterList(); + // AssetManager2::GetBag(resid) wraps this function to track which resource ids have already + // been seen while traversing bag parents. + const ResolvedBag* GetBag(uint32_t resid, std::vector& child_resids); + // The ordered list of ApkAssets to search. These are not owned by the AssetManager, and must // have a longer lifetime. std::vector apk_assets_; diff --git a/libs/androidfw/tests/AssetManager2_test.cpp b/libs/androidfw/tests/AssetManager2_test.cpp index 7cac2b3417b57..3118009fef902 100644 --- a/libs/androidfw/tests/AssetManager2_test.cpp +++ b/libs/androidfw/tests/AssetManager2_test.cpp @@ -329,6 +329,17 @@ TEST_F(AssetManager2Test, MergesStylesWithParentFromSingleApkAssets) { EXPECT_EQ(0, bag_two->entries[5].cookie); } +TEST_F(AssetManager2Test, MergeStylesCircularDependency) { + AssetManager2 assetmanager; + assetmanager.SetApkAssets({style_assets_.get()}); + + // GetBag should stop traversing the parents of styles when a circular + // dependency is detected + const ResolvedBag* bag_one = assetmanager.GetBag(app::R::style::StyleFour); + ASSERT_NE(nullptr, bag_one); + ASSERT_EQ(3u, bag_one->entry_count); +} + TEST_F(AssetManager2Test, ResolveReferenceToResource) { AssetManager2 assetmanager; assetmanager.SetApkAssets({basic_assets_.get()}); diff --git a/libs/androidfw/tests/data/styles/R.h b/libs/androidfw/tests/data/styles/R.h index 05073a8869ecb..538a84717176e 100644 --- a/libs/androidfw/tests/data/styles/R.h +++ b/libs/androidfw/tests/data/styles/R.h @@ -48,6 +48,9 @@ struct R { StyleOne = 0x7f020000u, StyleTwo = 0x7f020001u, StyleThree = 0x7f020002u, + StyleFour = 0x7f020003u, + StyleFive = 0x7f020004u, + StyleSix = 0x7f020005u, }; }; }; diff --git a/libs/androidfw/tests/data/styles/res/values/styles.xml b/libs/androidfw/tests/data/styles/res/values/styles.xml index 3c90317a62c6d..1a231768dadee 100644 --- a/libs/androidfw/tests/data/styles/res/values/styles.xml +++ b/libs/androidfw/tests/data/styles/res/values/styles.xml @@ -63,4 +63,20 @@ 5 + + + + + + + + + + diff --git a/libs/androidfw/tests/data/styles/styles.apk b/libs/androidfw/tests/data/styles/styles.apk index 72abf8ff2b408a01c88baf9dbe22a08c15490c8c..cd5c7a1c6c12c06a3d7528b527b3a8028c21c641 100644 GIT binary patch delta 850 zcmZ1{^i6nzQT+s8zNP@4mi_8dH-8F?>)AAN7d|{xA>Cvzp)B-(LGFNag~j`YmzTR8 zu{gWr?H)gG4vv*F7HW4I_~g%Ie+`J~5dWxE)_F1_xFBggkFC&E_q1N|*Tt+po-0Kx zwUt~o-A}pAi}?!=%k-(DRYu zg0uD~u@_g=-Hx=(5Gr=!csl3qjLmM-TCYtF%U+VXL*~p;rM~M^_caxVUO(~q(#|T) z)XmyoJxyD!-mhKx?dZ8VOIGz?dX=_J?;Jz9nn4YF>`qq}^#g2*&Mr2dhF6c=KY73I zXWX1$in8~f`QLhV|H$v^gUNHvPuP97()arFZvNi+Ki~Mz69@TXs^!b2`+&X(VPs$k z@MdNKGC`nWKI4_>M~n>YKsNj2zl_pMKuU@!ifIYk<}#)>MnM^%dI7LfAOU2@Oy*@V z=bZ%P%md<;K)h|TD@&&mCueX;WlpMFerXW{jGbAQ%D}+^77xy>n0%H+eDY%!W8o06 zBY$mF~@g_V(01IS|nVz$X2S)@f7nL-$1fC|APK+znS+R10wI5_73 zc{_mk%H)e|^8*DKm>IZ%JQfC~dXU*b*VY5g7XYzAay&qq6)MLFl>?azlH&!^%uqQd zs2ua;Ty{yITQk6NV81g&0NG%-vT^|3#5egSyCPHtC_F(nA**0u;GgWvAQN7_n^lgUDVse}I zWN#_l`tI$5<%vtBE{BC%_xKd3tv`EK$`f693q#e3JOG6 z4we;uD(_g`CbiOI_9Qo%uj)VgzXnBXHR;#fdCK}O&O&7yPr{6coCz&{Pu3b{`1o{M zZdBRUGUb%zZu6;bGTYZ`uesK-=v$FdIQO~G*yN2Gb5@&2UVGB27dm;>tBkWhXFOYW zCZzV{n!K=AuXtK+%>Bw+VbJ+RIQEI0z#N9zTO^qtxjqVM-9Neh$EUqcuY|4koYvp< z>i(1G)oYKlGAn_sSIpvIf<3|r6rT)S&fB#7<_%}Wr8FgiVJSHG! z24Wy^fMG@^ABHbL4oofr$Oh|T)c~?eCY!R)*JNT6U=RWFKzc!@1AzlbUjmQ;6AJ*c zA^JFgW^qrR$)N~z4@d>b(;!`N6_W)xSyXv|B0yI%gJrxKLDmELKoMZrz~g?hC8vcn lau|w&l)=H1$vvDJ(uj~`VBmwxff!FGpXXF$ivR@%0|0tI@OA(I