From fb4d09cadd27a3fb1a2e268417f0f511aa92e344 Mon Sep 17 00:00:00 2001
From: Ryan Mitchell <rtmitchell@google.com>
Date: Mon, 7 Dec 2020 16:06:04 -0800
Subject: [PATCH] Read manifest values using resource id in idmap2

Idmap2 currently reads android:targetPackage and android:targetName
from overlay manifests by looking for an attribute with the name of
the attribute resource.

This fixes the divergence from package parsing by finding the
attributes using their resource ids.

Bug: 175060836
Test: libidmap2_tests
Change-Id: I09965e5880a1e6c48c3f8077db0c595484804ce7
---
 cmds/idmap2/include/idmap2/XmlParser.h        |   6 +-
 cmds/idmap2/libidmap2/ResourceUtils.cpp       |  14 ++--
 cmds/idmap2/libidmap2/XmlParser.cpp           |  62 +++++++++++-------
 cmds/idmap2/tests/ResourceUtilsTests.cpp      |  24 ++++++-
 .../data/overlay/AndroidManifestInvalid.xml   |  34 ++++++++++
 cmds/idmap2/tests/data/overlay/build          |   7 ++
 .../tests/data/overlay/overlay-invalid.apk    | Bin 0 -> 4876 bytes
 7 files changed, 119 insertions(+), 28 deletions(-)
 create mode 100644 cmds/idmap2/tests/data/overlay/AndroidManifestInvalid.xml
 create mode 100644 cmds/idmap2/tests/data/overlay/overlay-invalid.apk

diff --git a/cmds/idmap2/include/idmap2/XmlParser.h b/cmds/idmap2/include/idmap2/XmlParser.h
index 972a6d7e3427c..1c74ab3bb691b 100644
--- a/cmds/idmap2/include/idmap2/XmlParser.h
+++ b/cmds/idmap2/include/idmap2/XmlParser.h
@@ -22,6 +22,7 @@
 #include <memory>
 #include <string>
 
+#include "ResourceUtils.h"
 #include "Result.h"
 #include "android-base/macros.h"
 #include "androidfw/ResourceTypes.h"
@@ -39,8 +40,11 @@ class XmlParser {
     Event event() const;
     std::string name() const;
 
-    Result<std::string> GetAttributeStringValue(const std::string& name) const;
     Result<Res_value> GetAttributeValue(const std::string& name) const;
+    Result<Res_value> GetAttributeValue(ResourceId attr, const std::string& label) const;
+
+    Result<std::string> GetAttributeStringValue(const std::string& name) const;
+    Result<std::string> GetAttributeStringValue(ResourceId attr, const std::string& label) const;
 
     bool operator==(const Node& rhs) const;
     bool operator!=(const Node& rhs) const;
diff --git a/cmds/idmap2/libidmap2/ResourceUtils.cpp b/cmds/idmap2/libidmap2/ResourceUtils.cpp
index 52837418e7759..4e85e57513003 100644
--- a/cmds/idmap2/libidmap2/ResourceUtils.cpp
+++ b/cmds/idmap2/libidmap2/ResourceUtils.cpp
@@ -32,6 +32,12 @@ using android::idmap2::ZipFile;
 using android::util::Utf16ToUtf8;
 
 namespace android::idmap2::utils {
+namespace {
+constexpr ResourceId kAttrName = 0x01010003;
+constexpr ResourceId kAttrResourcesMap = 0x01010609;
+constexpr ResourceId kAttrTargetName = 0x0101044d;
+constexpr ResourceId kAttrTargetPackage = 0x01010021;
+}  // namespace
 
 bool IsReference(uint8_t data_type) {
   return data_type == Res_value::TYPE_REFERENCE || data_type == Res_value::TYPE_DYNAMIC_REFERENCE;
@@ -119,7 +125,7 @@ Result<OverlayManifestInfo> ExtractOverlayManifestInfo(const std::string& path,
     }
 
     OverlayManifestInfo info{};
-    if (auto result_str = it.GetAttributeStringValue("name")) {
+    if (auto result_str = it.GetAttributeStringValue(kAttrName, "android:name")) {
       if (*result_str != name) {
         // A value for android:name was found, but either a the name does not match the requested
         // name, or an <overlay> tag with no name was requested.
@@ -132,18 +138,18 @@ Result<OverlayManifestInfo> ExtractOverlayManifestInfo(const std::string& path,
       continue;
     }
 
-    if (auto result_str = it.GetAttributeStringValue("targetPackage")) {
+    if (auto result_str = it.GetAttributeStringValue(kAttrTargetPackage, "android:targetPackage")) {
       info.target_package = *result_str;
     } else {
       return Error("android:targetPackage missing from <overlay> of %s: %s", path.c_str(),
                    result_str.GetErrorMessage().c_str());
     }
 
-    if (auto result_str = it.GetAttributeStringValue("targetName")) {
+    if (auto result_str = it.GetAttributeStringValue(kAttrTargetName, "android:targetName")) {
       info.target_name = *result_str;
     }
 
-    if (auto result_value = it.GetAttributeValue("resourcesMap")) {
+    if (auto result_value = it.GetAttributeValue(kAttrResourcesMap, "android:resourcesMap")) {
       if (IsReference((*result_value).dataType)) {
         info.resource_mapping = (*result_value).data;
       } else {
diff --git a/cmds/idmap2/libidmap2/XmlParser.cpp b/cmds/idmap2/libidmap2/XmlParser.cpp
index 4030b83b3a41f..00baea46f9093 100644
--- a/cmds/idmap2/libidmap2/XmlParser.cpp
+++ b/cmds/idmap2/libidmap2/XmlParser.cpp
@@ -90,15 +90,27 @@ std::string XmlParser::Node::name() const {
   return String8(key16).c_str();
 }
 
-Result<std::string> XmlParser::Node::GetAttributeStringValue(const std::string& name) const {
-  auto value = GetAttributeValue(name);
-  if (!value) {
-    return value.GetError();
+template <typename Func>
+Result<Res_value> FindAttribute(const ResXMLParser& parser, const std::string& label,
+                                Func&& predicate) {
+  for (size_t i = 0; i < parser.getAttributeCount(); i++) {
+    if (!predicate(i)) {
+      continue;
+    }
+    Res_value res_value{};
+    if (parser.getAttributeValue(i, &res_value) == BAD_TYPE) {
+      return Error(R"(Bad value for attribute "%s")", label.c_str());
+    }
+    return res_value;
   }
+  return Error(R"(Failed to find attribute "%s")", label.c_str());
+}
 
-  switch ((*value).dataType) {
+Result<std::string> GetStringValue(const ResXMLParser& parser, const Res_value& value,
+                                   const std::string& label) {
+  switch (value.dataType) {
     case Res_value::TYPE_STRING: {
-      if (auto str = parser_.getStrings().string8ObjectAt((*value).data); str.ok()) {
+      if (auto str = parser.getStrings().string8ObjectAt(value.data); str.ok()) {
         return std::string(str->string());
       }
       break;
@@ -106,31 +118,37 @@ Result<std::string> XmlParser::Node::GetAttributeStringValue(const std::string&
     case Res_value::TYPE_INT_DEC:
     case Res_value::TYPE_INT_HEX:
     case Res_value::TYPE_INT_BOOLEAN: {
-      return std::to_string((*value).data);
+      return std::to_string(value.data);
     }
   }
+  return Error(R"(Failed to convert attribute "%s" value to a string)", label.c_str());
+}
 
-  return Error(R"(Failed to convert attribute "%s" value to a string)", name.c_str());
+Result<Res_value> XmlParser::Node::GetAttributeValue(ResourceId attr,
+                                                     const std::string& label) const {
+  return FindAttribute(parser_, label, [&](size_t index) -> bool {
+    return parser_.getAttributeNameResID(index) == attr;
+  });
 }
 
 Result<Res_value> XmlParser::Node::GetAttributeValue(const std::string& name) const {
-  size_t len;
-  for (size_t i = 0; i < parser_.getAttributeCount(); i++) {
-    const String16 key16(parser_.getAttributeName(i, &len));
+  return FindAttribute(parser_, name, [&](size_t index) -> bool {
+    size_t len;
+    const String16 key16(parser_.getAttributeName(index, &len));
     std::string key = String8(key16).c_str();
-    if (key != name) {
-      continue;
-    }
+    return key == name;
+  });
+}
 
-    Res_value res_value{};
-    if (parser_.getAttributeValue(i, &res_value) == BAD_TYPE) {
-      return Error(R"(Bad value for attribute "%s")", name.c_str());
-    }
+Result<std::string> XmlParser::Node::GetAttributeStringValue(ResourceId attr,
+                                                             const std::string& label) const {
+  auto value = GetAttributeValue(attr, label);
+  return value ? GetStringValue(parser_, *value, label) : value.GetError();
+}
 
-    return res_value;
-  }
-
-  return Error(R"(Failed to find attribute "%s")", name.c_str());
+Result<std::string> XmlParser::Node::GetAttributeStringValue(const std::string& name) const {
+  auto value = GetAttributeValue(name);
+  return value ? GetStringValue(parser_, *value, name) : value.GetError();
 }
 
 Result<std::unique_ptr<const XmlParser>> XmlParser::Create(const void* data, size_t size,
diff --git a/cmds/idmap2/tests/ResourceUtilsTests.cpp b/cmds/idmap2/tests/ResourceUtilsTests.cpp
index 9ed807ccd8f90..1f6bf49f5f0e1 100644
--- a/cmds/idmap2/tests/ResourceUtilsTests.cpp
+++ b/cmds/idmap2/tests/ResourceUtilsTests.cpp
@@ -59,4 +59,26 @@ TEST_F(ResourceUtilsTests, ResToTypeEntryNameNoSuchResourceId) {
   ASSERT_FALSE(name);
 }
 
-}  // namespace android::idmap2
+TEST_F(ResourceUtilsTests, InvalidValidOverlayNameInvalidAttributes) {
+  auto info = utils::ExtractOverlayManifestInfo(GetTestDataPath() + "/overlay/overlay-invalid.apk",
+                                                "InvalidName");
+  ASSERT_FALSE(info);
+}
+
+TEST_F(ResourceUtilsTests, ValidOverlayNameInvalidAttributes) {
+  auto info = utils::ExtractOverlayManifestInfo(GetTestDataPath() + "/overlay/overlay-invalid.apk",
+                                                "ValidName");
+  ASSERT_FALSE(info);
+}
+
+TEST_F(ResourceUtilsTests, ValidOverlayNameAndTargetPackageInvalidAttributes) {
+  auto info = utils::ExtractOverlayManifestInfo(GetTestDataPath() + "/overlay/overlay-invalid.apk",
+                                                "ValidNameAndTargetPackage");
+  ASSERT_TRUE(info);
+  ASSERT_EQ("ValidNameAndTargetPackage", info->name);
+  ASSERT_EQ("Valid", info->target_package);
+  ASSERT_EQ("", info->target_name); // Attribute resource id could not be found
+  ASSERT_EQ(0, info->resource_mapping); // Attribute resource id could not be found
+}
+
+}// namespace android::idmap2
diff --git a/cmds/idmap2/tests/data/overlay/AndroidManifestInvalid.xml b/cmds/idmap2/tests/data/overlay/AndroidManifestInvalid.xml
new file mode 100644
index 0000000000000..d61c36cad60ce
--- /dev/null
+++ b/cmds/idmap2/tests/data/overlay/AndroidManifestInvalid.xml
@@ -0,0 +1,34 @@
+<?xml version="1.0" encoding="utf-8"?>
+<!-- Copyright (C) 2018 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.
+-->
+<manifest xmlns:android="http://schemas.android.com/apk/res/android"
+          package="test.overlay">
+    <application android:hasCode="false"/>
+
+    <overlay name="InvalidName"
+         android:targetName="Invalid"
+         android:targetPackage="Invalid"
+         android:resourcesMap="@xml/overlays_swap"/>
+
+    <overlay android:name="ValidName"
+         targetName="Invalid"
+         targetPackage="Invalid"
+         resourcesMap="@xml/overlays_swap"/>
+
+    <overlay android:name="ValidNameAndTargetPackage"
+         targetName="Invalid"
+         android:targetPackage="Valid"
+         resourcesMap="@xml/overlays_swap"/>
+</manifest>
diff --git a/cmds/idmap2/tests/data/overlay/build b/cmds/idmap2/tests/data/overlay/build
index 1f1cedb052719..7b1a66f31e611 100755
--- a/cmds/idmap2/tests/data/overlay/build
+++ b/cmds/idmap2/tests/data/overlay/build
@@ -38,4 +38,11 @@ aapt2 link \
     -o overlay-legacy.apk \
     compiled.flata
 
+aapt2 link \
+    --no-resource-removal \
+    -I "$FRAMEWORK_RES_APK" \
+    --manifest AndroidManifestInvalid.xml \
+    -o overlay-invalid.apk \
+    compiled.flata
+
 rm compiled.flata
diff --git a/cmds/idmap2/tests/data/overlay/overlay-invalid.apk b/cmds/idmap2/tests/data/overlay/overlay-invalid.apk
new file mode 100644
index 0000000000000000000000000000000000000000..888c871e41019567120088b2ab93a2f213aa500b
GIT binary patch
literal 4876
zcmd^Dc{r478-K=FCPrkbv8FmIg-A)+CRvI$StG_^Xe>jsC>2RUh(mQsQ94maSqk|m
zx-1!WD%yl}G)JXHmX4&A?|z4ghVQz*zJI^xx_)yn&-32zb1%>G&PF@DD1t^nDC!ih
zV))`s9T9|r#1WDQ?Mj9(GlcHDj>@3>(O7KV9l=3I4y|LzuT+S+U^<l2Zx=<HB{kbw
z`>l>eP?Tb#<!Z}#oud9YwYDOOUu5O9i*BqmLJ~Ye2Ab7Sl_n-~t>^VkVkRj=>ByIL
zmCa2}g>9+ll@Dv4>DsBXrundGeR&zqji=s%3#qc{Q`(I;uUh2ppS`Y+Sb^$!3qMYc
zjLwN><KITf_u+PnP{{VHOq)}~?&ZFU+HpQkIrrr;t2#aT?taBC{cSE0k%JzkLoQ)o
z9Q}`+pZ3l*PPxitWq#o~OZkT~H>Gm}8tjj(xM^Z|qbF_VYfby+I(OgthT%C2DDRef
zMauZi@>-k{JCH5cMQghJ#KpPQB(5?gAjKg4uM?7~`RTkR1Ku9VnVXIpnRQ<e{;b}g
z%yCg`*ZPEaOnyQy!RM6ry4SxRP&$;%Zpd@xNUpl}VT)B>Uxm$k?uT7y@Ui|gubC<h
zmpE5#24d1>UR9W0YELS@awDU=yn*w`^vhs(P?hFY#;MkG@7o<YiTY{!(xLJ}iU-pV
zC#6TbwUW{mKg`aLdTNrGbSn$(Nqg4q^NiD;Hr%Uw)pOpb1vxs6N1m7#`=9xvv;XK5
z9lNr#FCXnIH@v2L`?6V&Ucrmq+Tk<jzO40iD|JkL>SEORrj8NGEex;pUSG&#*mj)h
zD1W6|kdYslkkH<`V`=U-PE*Fw&^r(9`prF`4{bis9$i?;Y`Efr-}74=q3od@EB5Fk
zRZnHl!qhIOzgPoOAMGk*0<Ehv*Y=)F-{vxmHvUju%VsSQ`wCU{9x6-$D3iuo1QoR?
zB#g!kqK31u@;aDiXID#B*x$JJWLkX0FT5IBb!N`1nw$#-SJ;%<#96Km@eiUGezPB1
zGPqN6nW2vSjZD?`JujHoHYdfA%#}BmRWFN@TCK&KQh()lMy~OjRJToy52XfdbLdX)
z4#)l0Q#2aeO4}cEoC<^Q9p=^yE(~m!wQ8%R8`eprJ^#zl$)kauc&KM6dl#8xYc2aW
z@lFGxEa>dU<y#-SXY;V|aQ$%7x%}FO1hbyETU+lHv5r}>Pk9%-yrpe+#Wu^_)X&4X
z$c^iZ=Ez#}yt37}*>U=6RfPT1R*;UWaZBP-rM<|cP`!`e42smP&7w}noK__^Q8WB6
z_xEpqC-GynDmz`A^C3#{IE3a-5-o}^-OrE4q%qi(P^wQL)t|<X-v(pJ8nQyhx99sV
z#d}j{wX`I}cW6~uCTNQOZm|XL?6xy7v&#MYuQi)v>!<eng-LI!P{_I%B_X?t9GtR9
zj%lHLb7{Cnetw1VtTnp%fp0pJwXc3YyW*%ya(82JoY4m>hZQOF0}?-XmJK^<wc+xa
zc}A>%bi7RcOmLYm=Z6r~<82bpJqUWKnI2fOHFa%3fanEcaVLKBX`jzFaj8=c?&`-z
z#w15N-~F=Zn^1WqTy`_B1xGW$VU@`orZa-*j8QMIO4jJ1iucmfb31rhJYHVe@-j)u
zo(8d1T9#7!)1Rn*BMdt&O7os3QK;&oTkp{^r+O6<nezm9Jv8fI*CjldYuDdYDxcl#
zd#*Uu*6C10P7{vqb}~sj`=rARLiGI`b2`e-=Qw{l^1Armi`-)l)8l6!>atLdp=Mr4
zEHPe|+Gia#uV~QO+DvWtkj%%W7t_@)H~+aq_gQ(;fJFegs97dsMmei%^FZs;ulJgQ
z-Ve9P7HlXn%)5W9H2+|40e3jEOX|mD<3AGfGr?~XY`kid`5qb)ME9Z7M%Ku~b?a-$
zBynEh@CzF;H`^(e9%amDkByI&aheZrx-_LETWz)L>zY+Nh#`M3uh?(=ZS}QSdgdCx
z<I5a8&-ULntVS~|RLJ|5SJ2T6GQRN?Z;9${Syp0sy@_|}vF*1dI_3NKf8u`i5~&!H
z^f|6%w~S|0XQtEq4i}x}P5Z5|=bvaRM{XiV@|#(N{l%jtqK#)#+PpJ0<L0k-wXb<W
zD_)oIb-BBhQ|X^Rr3)QMQ%^`Gyy)B$XTH^h5!P1Z+9M}^?dhBCQW@06^hfh9My%xg
zk-W_0#DO<kPj%W=NVoZIE4Nj0ePrg@yQ#{*x8nr5dl~nHXHxa`epNleOntzpAUtc(
zz7Xej=D~5F?XfJGeP5JK-!#y<>${?c0(hRMy$T<bv`7()KZ~5Xr8MWj-&!QK#VbT=
z|0Ztm{wNu17rRg|sG(1j>stLP?Vch@*FQ<qI?JoYw?(A(c@o)stpYKQXyfIYp`S`R
z9ZgN6N)j^-EDq>9>1i+4-b)STI(pw$BxVvlypaEGP44Ar>aSD+@u0!uT}=^RhC5|`
z%xk1ZH|`bWGYYmmrOEQj+D;ASuiMlOfi+~=x~G2qHt}K?)E7K|<f*xOS22a$*Y0z?
zR!+olFI%NAdnUKH)vU|%jOC5DX6gpS{x<4T@fPDKix_;>S_@onnWd$v11n%n{vR@L
z4jsSqPtEb$j{2rrwkMujc|TZSFumY}d;flo`GF3>_jPLe+ML5v>q_dJKI}YFSv^RL
zecAN7v)-#ve7KkOkg(|ew1I`yih3{T9WBLY%}aM4es6T2mG_Aq@z^~uSF9s1SHj5n
z&gKnj#md|=iCV?jwrYZV(mUAlBx>bjo2yBsq(jC@bQ$rOy;H?|)3+I4$bCB0I#UoH
zEGDOLS!PM<L2&iJFm)t6AsnU;lsT2j@)1Eqeh;h*ghQ%G0~kr*`3-nW0S{CPFat0L
zuma!%*b6ufs07>w5C|hD4c!+_nKF9l=;)8*4UmGuWQP)(J6Bje`R<EUR3;1@+qICI
z+JxTm>l!I3P3RZY2O<)`&sc{QHcniEi3o{k;K&jP?E#jJXov;Bx+nyN0nbE1h>F6Y
z=l`Q&b%HhaD8+cJejI_~F=4dL7cfBO0J8xSh>jTi2>SCQhmbfwmQ23hA4cHO4ipR#
z9kEG-g{=rYzrp)L1w2ql2frrufvyC&0(JsY0NDVnE}H;-00KNa51<QJ3NQiK0DJ*q
zfCPZ3D4oI9N7B-a5H^L*ps=Y-e;OOjm@!%z6ih$LQ@w*|FjCMH!rn?_LUjvde%Xan
zs6OlvCYm`@NbM8C@T2=vSag2|mCa$&ker;*h!Ef4NtCo;G<~|z$WUg8FUN<C6cvR#
zINoriAreVQKlauMkFdg7Y<P#Ns_-LxJtd6B@CA27MOkd7{s=J`Axplm{csjiRvvwv
zHE|Im8JX{$BMTQzoAzBJSgd1#Wl^vN3i%Tekwqk+kR}8r@x9?h9MNu&1o|z&lfU==
z?~E)w3Y#Xs0hJwwhRN7_1Tr?K)gWW@!FJc5Y5b4E(FZUewt!)Qog~ncf5zB<7>oI!
z0nh`G0UH5cfKWgb;8#Ev;3A+3An*~J2l&We%9zg>GhW8#K3*0RlHrc^LqE1o$B)Ns
zj+dth$=De*z8^b##>+$@S$b3+%?&>n*m%qjEVg+-F&@j&_}KFIb<q86!{YScHq16Q
zRu(AC2?^$k@fc)AC?1OeR!_eG#o~>{bh3u|QwAM^F<XHrPd4x;b=dqg0D1s20Gr$B
zobjXiN7F|1{693Xer!(%iXY8C{~rxZ9?d_(zCke=%l}3@9F_uTpqb>KkdZ#Y?<D_=
z2pbUo*cCjAkSDxI_?*YCfWj7pKLE!pB!S1m7AEdxli3vh#u~F30jp+$&9S?S@Z^L)
zSjG&6L%1gx66~Cl%}Mx<JZ8!sGBd%H;6^kyBY0qvyX2UeP)I458F#bXXeWjh0i4hh
Ms0WrM0V^H!Z+{x6e*gdg

literal 0
HcmV?d00001