AAPT2: Fix issue with String flattening in XmlFlattener

Compiled Strings (previously not encountered) in an XML resource
were using a different StringPool than the one being referred to
in the XmlFlattener, and so the indices were all wrong.

Bug: 72700446
Test: make aapt2_tests
Change-Id: I663924f8fad50fd4c69cfa196318dc63fb641a25

3 files changed, 138 insertions, 28 deletions

diff --git a/tools/aapt2/format/binary/XmlFlattener.cpp b/tools/aapt2/format/binary/XmlFlattener.cpp
index 345cc95cfb29..067372b99b5c 100644
--- a/tools/aapt2/format/binary/XmlFlattener.cpp
+++ b/tools/aapt2/format/binary/XmlFlattener.cpp
@@ -26,6 +26,7 @@
 #include "utils/misc.h"
 
 #include "SdkConstants.h"
+#include "ValueVisitor.h"
 #include "format/binary/ChunkWriter.h"
 #include "format/binary/ResourceTypeExtensions.h"
 #include "xml/XmlDom.h"
@@ -153,6 +154,9 @@ class XmlFlattenerVisitor : public xml::ConstVisitor {
  private:
   DISALLOW_COPY_AND_ASSIGN(XmlFlattenerVisitor);
 
+  // We are adding strings to a StringPool whose strings will be sorted and merged with other
+  // string pools. That means we can't encode the ID of a string directly. Instead, we defer the
+  // writing of the ID here, until after the StringPool is merged and sorted.
   void AddString(const StringPiece& str, uint32_t priority, android::ResStringPool_ref* dest,
                  bool treat_empty_string_as_null = false) {
     if (str.empty() && treat_empty_string_as_null) {
@@ -164,6 +168,9 @@ class XmlFlattenerVisitor : public xml::ConstVisitor {
     }
   }
 
+  // We are adding strings to a StringPool whose strings will be sorted and merged with other
+  // string pools. That means we can't encode the ID of a string directly. Instead, we defer the
+  // writing of the ID here, until after the StringPool is merged and sorted.
   void AddString(const StringPool::Ref& ref, android::ResStringPool_ref* dest) {
     string_refs.push_back(StringFlattenDest{ref, dest});
   }
@@ -248,30 +255,39 @@ class XmlFlattenerVisitor : public xml::ConstVisitor {
         AddString(name_ref, &flat_attr->name);
       }
 
-      // Process plain strings to make sure they get properly escaped.
-      StringPiece raw_value = xml_attr->value;
-
-      util::StringBuilder str_builder(true /*preserve_spaces*/);
-      str_builder.Append(xml_attr->value);
-
-      if (!options_.keep_raw_values) {
-        raw_value = str_builder.ToString();
-      }
-
-      if (options_.keep_raw_values || !xml_attr->compiled_value) {
-        // Keep raw values if the value is not compiled or
-        // if we're building a static library (need symbols).
-        AddString(raw_value, kLowPriority, &flat_attr->rawValue);
+      std::string processed_str;
+      Maybe<StringPiece> compiled_text;
+      if (xml_attr->compiled_value != nullptr) {
+        // Make sure we're not flattening a String. A String can be referencing a string from
+        // a different StringPool than we're using here to build the binary XML.
+        String* string_value = ValueCast<String>(xml_attr->compiled_value.get());
+        if (string_value != nullptr) {
+          // Mark the String's text as needing to be serialized.
+          compiled_text = StringPiece(*string_value->value);
+        } else {
+          // Serialize this compiled value safely.
+          CHECK(xml_attr->compiled_value->Flatten(&flat_attr->typedValue));
+        }
+      } else {
+        // There is no compiled value, so treat the raw string as compiled, once it is processed to
+        // make sure escape sequences are properly interpreted.
+        processed_str =
+            util::StringBuilder(true /*preserve_spaces*/).Append(xml_attr->value).ToString();
+        compiled_text = StringPiece(processed_str);
       }
 
-      if (xml_attr->compiled_value) {
-        CHECK(xml_attr->compiled_value->Flatten(&flat_attr->typedValue));
-      } else {
-        // Flatten as a regular string type.
+      if (compiled_text) {
+        // Write out the compiled text and raw_text.
         flat_attr->typedValue.dataType = android::Res_value::TYPE_STRING;
-
-        AddString(str_builder.ToString(), kLowPriority,
-                  (ResStringPool_ref*)&flat_attr->typedValue.data);
+        AddString(compiled_text.value(), kLowPriority,
+                  reinterpret_cast<ResStringPool_ref*>(&flat_attr->typedValue.data));
+        if (options_.keep_raw_values) {
+          AddString(xml_attr->value, kLowPriority, &flat_attr->rawValue);
+        } else {
+          AddString(compiled_text.value(), kLowPriority, &flat_attr->rawValue);
+        }
+      } else if (options_.keep_raw_values && !xml_attr->value.empty()) {
+        AddString(xml_attr->value, kLowPriority, &flat_attr->rawValue);
       }
 
       flat_attr->typedValue.size = util::HostToDevice16(sizeof(flat_attr->typedValue));
diff --git a/tools/aapt2/format/binary/XmlFlattener_test.cpp b/tools/aapt2/format/binary/XmlFlattener_test.cpp
index 0450f6c16de5..08243feb3769 100644
--- a/tools/aapt2/format/binary/XmlFlattener_test.cpp
+++ b/tools/aapt2/format/binary/XmlFlattener_test.cpp
@@ -286,4 +286,92 @@ TEST_F(XmlFlattenerTest, ProcessEscapedStrings) {
   EXPECT_THAT(tree.getText(&len), StrEq(u"\\d{5}"));
 }
 
+TEST_F(XmlFlattenerTest, FlattenRawValueOnlyMakesCompiledValueToo) {
+  std::unique_ptr<xml::XmlResource> doc = test::BuildXmlDom(R"(<element foo="bar" />)");
+
+  // Raw values are kept when encoding an attribute with no compiled value, regardless of option.
+  XmlFlattenerOptions options;
+  options.keep_raw_values = false;
+
+  android::ResXMLTree tree;
+  ASSERT_TRUE(Flatten(doc.get(), &tree, options));
+
+  while (tree.next() != android::ResXMLTree::START_TAG) {
+    ASSERT_THAT(tree.getEventType(), Ne(android::ResXMLTree::BAD_DOCUMENT));
+    ASSERT_THAT(tree.getEventType(), Ne(android::ResXMLTree::END_DOCUMENT));
+  }
+
+  ASSERT_THAT(tree.getAttributeCount(), Eq(1u));
+  EXPECT_THAT(tree.getAttributeValueStringID(0), Ge(0));
+  EXPECT_THAT(tree.getAttributeDataType(0), Eq(android::Res_value::TYPE_STRING));
+  EXPECT_THAT(tree.getAttributeValueStringID(0), Eq(tree.getAttributeData(0)));
+}
+
+TEST_F(XmlFlattenerTest, FlattenCompiledStringValuePreservesRawValue) {
+  std::unique_ptr<xml::XmlResource> doc = test::BuildXmlDom(R"(<element foo="bar" />)");
+  doc->root->attributes[0].compiled_value =
+      util::make_unique<String>(doc->string_pool.MakeRef("bar"));
+
+  // Raw values are kept when encoding a string anyways.
+  XmlFlattenerOptions options;
+  options.keep_raw_values = false;
+
+  android::ResXMLTree tree;
+  ASSERT_TRUE(Flatten(doc.get(), &tree, options));
+
+  while (tree.next() != android::ResXMLTree::START_TAG) {
+    ASSERT_THAT(tree.getEventType(), Ne(android::ResXMLTree::BAD_DOCUMENT));
+    ASSERT_THAT(tree.getEventType(), Ne(android::ResXMLTree::END_DOCUMENT));
+  }
+
+  ASSERT_THAT(tree.getAttributeCount(), Eq(1u));
+  EXPECT_THAT(tree.getAttributeValueStringID(0), Ge(0));
+  EXPECT_THAT(tree.getAttributeDataType(0), Eq(android::Res_value::TYPE_STRING));
+  EXPECT_THAT(tree.getAttributeValueStringID(0), Eq(tree.getAttributeData(0)));
+}
+
+TEST_F(XmlFlattenerTest, FlattenCompiledValueExcludesRawValueWithKeepRawOptionFalse) {
+  std::unique_ptr<xml::XmlResource> doc = test::BuildXmlDom(R"(<element foo="true" />)");
+  doc->root->attributes[0].compiled_value = ResourceUtils::MakeBool(true);
+
+  XmlFlattenerOptions options;
+  options.keep_raw_values = false;
+
+  android::ResXMLTree tree;
+  ASSERT_TRUE(Flatten(doc.get(), &tree, options));
+
+  while (tree.next() != android::ResXMLTree::START_TAG) {
+    ASSERT_THAT(tree.getEventType(), Ne(android::ResXMLTree::BAD_DOCUMENT));
+    ASSERT_THAT(tree.getEventType(), Ne(android::ResXMLTree::END_DOCUMENT));
+  }
+
+  ASSERT_THAT(tree.getAttributeCount(), Eq(1u));
+  EXPECT_THAT(tree.getAttributeValueStringID(0), Eq(-1));
+  EXPECT_THAT(tree.getAttributeDataType(0), Eq(android::Res_value::TYPE_INT_BOOLEAN));
+}
+
+TEST_F(XmlFlattenerTest, FlattenCompiledValueExcludesRawValueWithKeepRawOptionTrue) {
+  std::unique_ptr<xml::XmlResource> doc = test::BuildXmlDom(R"(<element foo="true" />)");
+  doc->root->attributes[0].compiled_value = ResourceUtils::MakeBool(true);
+
+  XmlFlattenerOptions options;
+  options.keep_raw_values = true;
+
+  android::ResXMLTree tree;
+  ASSERT_TRUE(Flatten(doc.get(), &tree, options));
+
+  while (tree.next() != android::ResXMLTree::START_TAG) {
+    ASSERT_THAT(tree.getEventType(), Ne(android::ResXMLTree::BAD_DOCUMENT));
+    ASSERT_THAT(tree.getEventType(), Ne(android::ResXMLTree::END_DOCUMENT));
+  }
+
+  ASSERT_THAT(tree.getAttributeCount(), Eq(1u));
+  EXPECT_THAT(tree.getAttributeValueStringID(0), Ge(0));
+
+  size_t len;
+  EXPECT_THAT(tree.getAttributeStringValue(0, &len), StrEq(u"true"));
+
+  EXPECT_THAT(tree.getAttributeDataType(0), Eq(android::Res_value::TYPE_INT_BOOLEAN));
+}
+
 }  // namespace aapt
diff --git a/tools/aapt2/xml/XmlDom.cpp b/tools/aapt2/xml/XmlDom.cpp
index 7b748ce78cbc..b6cd08697545 100644
--- a/tools/aapt2/xml/XmlDom.cpp
+++ b/tools/aapt2/xml/XmlDom.cpp
@@ -248,8 +248,14 @@ static void CopyAttributes(Element* el, android::ResXMLParser* parser, StringPoo
 
       android::Res_value res_value;
       if (parser->getAttributeValue(i, &res_value) > 0) {
-        attr.compiled_value = ResourceUtils::ParseBinaryResValue(
-            ResourceType::kAnim, {}, parser->getStrings(), res_value, out_pool);
+        // Only compile the value if it is not a string, or it is a string that differs from
+        // the raw attribute value.
+        int32_t raw_value_idx = parser->getAttributeValueStringID(i);
+        if (res_value.dataType != android::Res_value::TYPE_STRING || raw_value_idx < 0 ||
+            static_cast<uint32_t>(raw_value_idx) != res_value.data) {
+          attr.compiled_value = ResourceUtils::ParseBinaryResValue(
+              ResourceType::kAnim, {}, parser->getStrings(), res_value, out_pool);
+        }
       }
 
       el->attributes.push_back(std::move(attr));
@@ -262,8 +268,8 @@ std::unique_ptr<XmlResource> Inflate(const void* data, size_t len, std::string*
   // an enum, which causes errors when qualifying it with android::
   using namespace android;
 
-  StringPool string_pool;
-  std::unique_ptr<Element> root;
+  std::unique_ptr<XmlResource> xml_resource = util::make_unique<XmlResource>();
+
   std::stack<Element*> node_stack;
   std::unique_ptr<Element> pending_element;
 
@@ -322,12 +328,12 @@ std::unique_ptr<XmlResource> Inflate(const void* data, size_t len, std::string*
         }
 
         Element* this_el = el.get();
-        CopyAttributes(el.get(), &tree, &string_pool);
+        CopyAttributes(el.get(), &tree, &xml_resource->string_pool);
 
         if (!node_stack.empty()) {
           node_stack.top()->AppendChild(std::move(el));
         } else {
-          root = std::move(el);
+          xml_resource->root = std::move(el);
         }
         node_stack.push(this_el);
         break;
@@ -359,7 +365,7 @@ std::unique_ptr<XmlResource> Inflate(const void* data, size_t len, std::string*
         break;
     }
   }
-  return util::make_unique<XmlResource>(ResourceFile{}, std::move(string_pool), std::move(root));
+  return xml_resource;
 }
 
 std::unique_ptr<XmlResource> XmlResource::Clone() const {