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
diff --git a/tools/aapt2/format/binary/XmlFlattener.cpp b/tools/aapt2/format/binary/XmlFlattener.cpp
index 345cc95..067372b 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 @@
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 @@
}
}
+ // 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 @@
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);
- }
-
- if (xml_attr->compiled_value) {
- CHECK(xml_attr->compiled_value->Flatten(&flat_attr->typedValue));
+ 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 {
- // Flatten as a regular string type.
- flat_attr->typedValue.dataType = android::Res_value::TYPE_STRING;
+ // 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);
+ }
- AddString(str_builder.ToString(), kLowPriority,
- (ResStringPool_ref*)&flat_attr->typedValue.data);
+ if (compiled_text) {
+ // Write out the compiled text and raw_text.
+ flat_attr->typedValue.dataType = android::Res_value::TYPE_STRING;
+ 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 0450f6c..08243fe 100644
--- a/tools/aapt2/format/binary/XmlFlattener_test.cpp
+++ b/tools/aapt2/format/binary/XmlFlattener_test.cpp
@@ -286,4 +286,92 @@
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 7b748ce..b6cd086 100644
--- a/tools/aapt2/xml/XmlDom.cpp
+++ b/tools/aapt2/xml/XmlDom.cpp
@@ -248,8 +248,14 @@
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 @@
// 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 @@
}
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 @@
break;
}
}
- return util::make_unique<XmlResource>(ResourceFile{}, std::move(string_pool), std::move(root));
+ return xml_resource;
}
std::unique_ptr<XmlResource> XmlResource::Clone() const {