From 38665547bb10f65f72d3358fb16cdc63d5febf15 Mon Sep 17 00:00:00 2001 From: Adam Lesinski Date: Wed, 28 Dec 2016 12:25:46 -0500 Subject: [PATCH] AAPT2: Treat non-AAPT attributes as Attributes with no format specified. Attributes in XML like "class" should be treated as attributes with no format specified. That means that something like 'version="1"' will be interpreted as an Integer value instead of as a string. This is more in line with the old AAPT behaviour. Bug: 30763349 Test: make libaapt2_tests Change-Id: I02fe41617e3c62c354714056198ef480d62afee7 --- tools/aapt2/link/XmlReferenceLinker.cpp | 117 ++++++----- tools/aapt2/link/XmlReferenceLinker_test.cpp | 193 +++++++++---------- 2 files changed, 142 insertions(+), 168 deletions(-) diff --git a/tools/aapt2/link/XmlReferenceLinker.cpp b/tools/aapt2/link/XmlReferenceLinker.cpp index a8198318b69eb..1dbe53c02ecfd 100644 --- a/tools/aapt2/link/XmlReferenceLinker.cpp +++ b/tools/aapt2/link/XmlReferenceLinker.cpp @@ -16,6 +16,8 @@ #include "link/Linkers.h" +#include "androidfw/ResourceTypes.h" + #include "Diagnostics.h" #include "ResourceUtils.h" #include "SdkConstants.h" @@ -40,17 +42,12 @@ class ReferenceVisitor : public ValueVisitor { public: using ValueVisitor::Visit; - ReferenceVisitor(IAaptContext* context, SymbolTable* symbols, - xml::IPackageDeclStack* decls, CallSite* callsite) - : context_(context), - symbols_(symbols), - decls_(decls), - callsite_(callsite), - error_(false) {} + ReferenceVisitor(IAaptContext* context, SymbolTable* symbols, xml::IPackageDeclStack* decls, + CallSite* callsite) + : context_(context), symbols_(symbols), decls_(decls), callsite_(callsite), error_(false) {} void Visit(Reference* ref) override { - if (!ReferenceLinker::LinkReference(ref, context_, symbols_, decls_, - callsite_)) { + if (!ReferenceLinker::LinkReference(ref, context_, symbols_, decls_, callsite_)) { error_ = true; } } @@ -84,73 +81,72 @@ class XmlVisitor : public xml::PackageAwareVisitor { reference_visitor_(context, symbols, this, callsite) {} void Visit(xml::Element* el) override { + // The default Attribute allows everything except enums or flags. + constexpr const static uint32_t kDefaultTypeMask = + 0xffffffffu & ~(android::ResTable_map::TYPE_ENUM | android::ResTable_map::TYPE_FLAGS); + const static Attribute kDefaultAttribute(true /* weak */, kDefaultTypeMask); + const Source source = source_.WithLine(el->line_number); for (xml::Attribute& attr : el->attributes) { - Maybe maybe_package = - xml::ExtractPackageFromNamespace(attr.namespace_uri); - if (maybe_package) { - // There is a valid package name for this attribute. We will look this - // up. - StringPiece package = maybe_package.value().package; - if (package.empty()) { + // If the attribute has no namespace, interpret values as if + // they were assigned to the default Attribute. + + const Attribute* attribute = &kDefaultAttribute; + std::string attribute_package; + + if (Maybe maybe_package = + xml::ExtractPackageFromNamespace(attr.namespace_uri)) { + // There is a valid package name for this attribute. We will look this up. + attribute_package = maybe_package.value().package; + if (attribute_package.empty()) { // Empty package means the 'current' or 'local' package. - package = context_->GetCompilationPackage(); + attribute_package = context_->GetCompilationPackage(); } - Reference attr_ref( - ResourceNameRef(package, ResourceType::kAttr, attr.name)); + Reference attr_ref(ResourceNameRef(attribute_package, ResourceType::kAttr, attr.name)); attr_ref.private_reference = maybe_package.value().private_namespace; std::string err_str; attr.compiled_attribute = ReferenceLinker::CompileXmlAttribute( - attr_ref, context_->GetNameMangler(), symbols_, callsite_, - &err_str); + attr_ref, context_->GetNameMangler(), symbols_, callsite_, &err_str); - // Convert the string value into a compiled Value if this is a valid - // attribute. - if (attr.compiled_attribute) { - if (attr.compiled_attribute.value().id) { - // Record all SDK levels from which the attributes were defined. - const size_t sdk_level = FindAttributeSdkLevel( - attr.compiled_attribute.value().id.value()); - if (sdk_level > 1) { - sdk_levels_found_->insert(sdk_level); - } - } - - const Attribute* attribute = - &attr.compiled_attribute.value().attribute; - attr.compiled_value = - ResourceUtils::TryParseItemForAttribute(attr.value, attribute); - if (!attr.compiled_value && - !(attribute->type_mask & android::ResTable_map::TYPE_STRING)) { - // We won't be able to encode this as a string. - context_->GetDiagnostics()->Error( - DiagMessage(source) << "'" << attr.value << "' " - << "is incompatible with attribute " - << package << ":" << attr.name << " " - << *attribute); - error_ = true; - } - - } else { - context_->GetDiagnostics()->Error(DiagMessage(source) - << "attribute '" << package << ":" - << attr.name << "' " << err_str); + if (!attr.compiled_attribute) { + context_->GetDiagnostics()->Error(DiagMessage(source) << "attribute '" + << attribute_package << ":" + << attr.name << "' " << err_str); error_ = true; + continue; } - } else if (!attr.compiled_value) { - // We still encode references, but only if we haven't manually set this - // to - // another compiled value. - attr.compiled_value = ResourceUtils::TryParseReference(attr.value); + + // Find this compiled attribute's SDK level. + const xml::AaptAttribute& aapt_attr = attr.compiled_attribute.value(); + if (aapt_attr.id) { + // Record all SDK levels from which the attributes were defined. + const size_t sdk_level = FindAttributeSdkLevel(aapt_attr.id.value()); + if (sdk_level > 1) { + sdk_levels_found_->insert(sdk_level); + } + } + attribute = &aapt_attr.attribute; } + attr.compiled_value = ResourceUtils::TryParseItemForAttribute(attr.value, attribute); if (attr.compiled_value) { // With a compiledValue, we must resolve the reference and assign it an // ID. attr.compiled_value->SetSource(source); attr.compiled_value->Accept(&reference_visitor_); + } else if ((attribute->type_mask & android::ResTable_map::TYPE_STRING) == 0) { + // We won't be able to encode this as a string. + DiagMessage msg(source); + msg << "'" << attr.value << "' " + << "is incompatible with attribute "; + if (!attribute_package.empty()) { + msg << attribute_package << ":"; + } + msg << attr.name << " " << *attribute; + context_->GetDiagnostics()->Error(msg); + error_ = true; } } @@ -174,12 +170,11 @@ class XmlVisitor : public xml::PackageAwareVisitor { } // namespace -bool XmlReferenceLinker::Consume(IAaptContext* context, - xml::XmlResource* resource) { +bool XmlReferenceLinker::Consume(IAaptContext* context, xml::XmlResource* resource) { sdk_levels_found_.clear(); CallSite callsite = {resource->file.name}; - XmlVisitor visitor(context, context->GetExternalSymbols(), - resource->file.source, &sdk_levels_found_, &callsite); + XmlVisitor visitor(context, context->GetExternalSymbols(), resource->file.source, + &sdk_levels_found_, &callsite); if (resource->root) { resource->root->Accept(&visitor); return !visitor.HasError(); diff --git a/tools/aapt2/link/XmlReferenceLinker_test.cpp b/tools/aapt2/link/XmlReferenceLinker_test.cpp index 810f63c0410ef..cc59416cb2b94 100644 --- a/tools/aapt2/link/XmlReferenceLinker_test.cpp +++ b/tools/aapt2/link/XmlReferenceLinker_test.cpp @@ -23,63 +23,52 @@ namespace aapt { class XmlReferenceLinkerTest : public ::testing::Test { public: void SetUp() override { - context_ = - test::ContextBuilder() - .SetCompilationPackage("com.app.test") - .SetNameManglerPolicy( - NameManglerPolicy{"com.app.test", {"com.android.support"}}) - .AddSymbolSource( - test::StaticSymbolSourceBuilder() - .AddPublicSymbol( - "android:attr/layout_width", ResourceId(0x01010000), - test::AttributeBuilder() - .SetTypeMask(android::ResTable_map::TYPE_ENUM | - android::ResTable_map::TYPE_DIMENSION) - .AddItem("match_parent", 0xffffffff) - .Build()) - .AddPublicSymbol( - "android:attr/background", ResourceId(0x01010001), - test::AttributeBuilder() - .SetTypeMask(android::ResTable_map::TYPE_COLOR) - .Build()) - .AddPublicSymbol("android:attr/attr", - ResourceId(0x01010002), - test::AttributeBuilder().Build()) - .AddPublicSymbol( - "android:attr/text", ResourceId(0x01010003), - test::AttributeBuilder() - .SetTypeMask(android::ResTable_map::TYPE_STRING) - .Build()) + context_ = test::ContextBuilder() + .SetCompilationPackage("com.app.test") + .SetNameManglerPolicy(NameManglerPolicy{"com.app.test", {"com.android.support"}}) + .AddSymbolSource( + test::StaticSymbolSourceBuilder() + .AddPublicSymbol("android:attr/layout_width", ResourceId(0x01010000), + test::AttributeBuilder() + .SetTypeMask(android::ResTable_map::TYPE_ENUM | + android::ResTable_map::TYPE_DIMENSION) + .AddItem("match_parent", 0xffffffff) + .Build()) + .AddPublicSymbol("android:attr/background", ResourceId(0x01010001), + test::AttributeBuilder() + .SetTypeMask(android::ResTable_map::TYPE_COLOR) + .Build()) + .AddPublicSymbol("android:attr/attr", ResourceId(0x01010002), + test::AttributeBuilder().Build()) + .AddPublicSymbol("android:attr/text", ResourceId(0x01010003), + test::AttributeBuilder() + .SetTypeMask(android::ResTable_map::TYPE_STRING) + .Build()) - // Add one real symbol that was introduces in v21 - .AddPublicSymbol("android:attr/colorAccent", - ResourceId(0x01010435), - test::AttributeBuilder().Build()) + // Add one real symbol that was introduces in v21 + .AddPublicSymbol("android:attr/colorAccent", ResourceId(0x01010435), + test::AttributeBuilder().Build()) - // Private symbol. - .AddSymbol("android:color/hidden", ResourceId(0x01020001)) + // Private symbol. + .AddSymbol("android:color/hidden", ResourceId(0x01020001)) - .AddPublicSymbol("android:id/id", ResourceId(0x01030000)) - .AddSymbol("com.app.test:id/id", ResourceId(0x7f030000)) - .AddSymbol("com.app.test:color/green", - ResourceId(0x7f020000)) - .AddSymbol("com.app.test:color/red", ResourceId(0x7f020001)) - .AddSymbol( - "com.app.test:attr/colorAccent", ResourceId(0x7f010000), - test::AttributeBuilder() - .SetTypeMask(android::ResTable_map::TYPE_COLOR) - .Build()) - .AddPublicSymbol( - "com.app.test:attr/com.android.support$colorAccent", - ResourceId(0x7f010001), - test::AttributeBuilder() - .SetTypeMask(android::ResTable_map::TYPE_COLOR) - .Build()) - .AddPublicSymbol("com.app.test:attr/attr", - ResourceId(0x7f010002), - test::AttributeBuilder().Build()) - .Build()) - .Build(); + .AddPublicSymbol("android:id/id", ResourceId(0x01030000)) + .AddSymbol("com.app.test:id/id", ResourceId(0x7f030000)) + .AddSymbol("com.app.test:color/green", ResourceId(0x7f020000)) + .AddSymbol("com.app.test:color/red", ResourceId(0x7f020001)) + .AddSymbol("com.app.test:attr/colorAccent", ResourceId(0x7f010000), + test::AttributeBuilder() + .SetTypeMask(android::ResTable_map::TYPE_COLOR) + .Build()) + .AddPublicSymbol("com.app.test:attr/com.android.support$colorAccent", + ResourceId(0x7f010001), + test::AttributeBuilder() + .SetTypeMask(android::ResTable_map::TYPE_COLOR) + .Build()) + .AddPublicSymbol("com.app.test:attr/attr", ResourceId(0x7f010002), + test::AttributeBuilder().Build()) + .Build()) + .Build(); } protected: @@ -87,12 +76,13 @@ class XmlReferenceLinkerTest : public ::testing::Test { }; TEST_F(XmlReferenceLinkerTest, LinkBasicAttributes) { - std::unique_ptr doc = - test::BuildXmlDomForPackageName(context_.get(), R"EOF( + std::unique_ptr doc = test::BuildXmlDomForPackageName(context_.get(), R"EOF( )EOF"); XmlReferenceLinker linker; @@ -101,38 +91,44 @@ TEST_F(XmlReferenceLinkerTest, LinkBasicAttributes) { xml::Element* view_el = xml::FindRootElement(doc.get()); ASSERT_NE(view_el, nullptr); - xml::Attribute* xml_attr = - view_el->FindAttribute(xml::kSchemaAndroid, "layout_width"); + xml::Attribute* xml_attr = view_el->FindAttribute(xml::kSchemaAndroid, "layout_width"); ASSERT_NE(xml_attr, nullptr); AAPT_ASSERT_TRUE(xml_attr->compiled_attribute); AAPT_ASSERT_TRUE(xml_attr->compiled_attribute.value().id); - EXPECT_EQ(xml_attr->compiled_attribute.value().id.value(), - ResourceId(0x01010000)); + EXPECT_EQ(xml_attr->compiled_attribute.value().id.value(), ResourceId(0x01010000)); ASSERT_NE(xml_attr->compiled_value, nullptr); - ASSERT_NE(ValueCast(xml_attr->compiled_value.get()), - nullptr); + ASSERT_NE(ValueCast(xml_attr->compiled_value.get()), nullptr); xml_attr = view_el->FindAttribute(xml::kSchemaAndroid, "background"); ASSERT_NE(xml_attr, nullptr); AAPT_ASSERT_TRUE(xml_attr->compiled_attribute); AAPT_ASSERT_TRUE(xml_attr->compiled_attribute.value().id); - EXPECT_EQ(xml_attr->compiled_attribute.value().id.value(), - ResourceId(0x01010001)); + EXPECT_EQ(xml_attr->compiled_attribute.value().id.value(), ResourceId(0x01010001)); ASSERT_NE(xml_attr->compiled_value, nullptr); Reference* ref = ValueCast(xml_attr->compiled_value.get()); ASSERT_NE(ref, nullptr); AAPT_ASSERT_TRUE(ref->name); - EXPECT_EQ(ref->name.value(), - test::ParseNameOrDie("color/green")); // Make sure the name - // didn't change. + EXPECT_EQ(ref->name.value(), test::ParseNameOrDie("color/green")); // Make sure the name + // didn't change. AAPT_ASSERT_TRUE(ref->id); EXPECT_EQ(ref->id.value(), ResourceId(0x7f020000)); xml_attr = view_el->FindAttribute(xml::kSchemaAndroid, "text"); ASSERT_NE(xml_attr, nullptr); AAPT_ASSERT_TRUE(xml_attr->compiled_attribute); - ASSERT_FALSE( - xml_attr->compiled_value); // Strings don't get compiled for memory sake. + ASSERT_FALSE(xml_attr->compiled_value); // Strings don't get compiled for memory sake. + + xml_attr = view_el->FindAttribute("", "nonAaptAttr"); + ASSERT_NE(nullptr, xml_attr); + AAPT_ASSERT_FALSE(xml_attr->compiled_attribute); + ASSERT_NE(nullptr, xml_attr->compiled_value); + ASSERT_NE(nullptr, ValueCast(xml_attr->compiled_value.get())); + + xml_attr = view_el->FindAttribute("", "nonAaptAttrRef"); + ASSERT_NE(nullptr, xml_attr); + AAPT_ASSERT_FALSE(xml_attr->compiled_attribute); + ASSERT_NE(nullptr, xml_attr->compiled_value); + ASSERT_NE(nullptr, ValueCast(xml_attr->compiled_value.get())); xml_attr = view_el->FindAttribute("", "class"); ASSERT_NE(xml_attr, nullptr); @@ -141,8 +137,7 @@ TEST_F(XmlReferenceLinkerTest, LinkBasicAttributes) { } TEST_F(XmlReferenceLinkerTest, PrivateSymbolsAreNotLinked) { - std::unique_ptr doc = - test::BuildXmlDomForPackageName(context_.get(), R"EOF( + std::unique_ptr doc = test::BuildXmlDomForPackageName(context_.get(), R"EOF( )EOF"); @@ -150,10 +145,8 @@ TEST_F(XmlReferenceLinkerTest, PrivateSymbolsAreNotLinked) { ASSERT_FALSE(linker.Consume(context_.get(), doc.get())); } -TEST_F(XmlReferenceLinkerTest, - PrivateSymbolsAreLinkedWhenReferenceHasStarPrefix) { - std::unique_ptr doc = - test::BuildXmlDomForPackageName(context_.get(), R"EOF( +TEST_F(XmlReferenceLinkerTest, PrivateSymbolsAreLinkedWhenReferenceHasStarPrefix) { + std::unique_ptr doc = test::BuildXmlDomForPackageName(context_.get(), R"EOF( )EOF"); @@ -162,8 +155,7 @@ TEST_F(XmlReferenceLinkerTest, } TEST_F(XmlReferenceLinkerTest, SdkLevelsAreRecorded) { - std::unique_ptr doc = - test::BuildXmlDomForPackageName(context_.get(), R"EOF( + std::unique_ptr doc = test::BuildXmlDomForPackageName(context_.get(), R"EOF( )EOF"); @@ -173,8 +165,7 @@ TEST_F(XmlReferenceLinkerTest, SdkLevelsAreRecorded) { } TEST_F(XmlReferenceLinkerTest, LinkMangledAttributes) { - std::unique_ptr doc = - test::BuildXmlDomForPackageName(context_.get(), R"EOF( + std::unique_ptr doc = test::BuildXmlDomForPackageName(context_.get(), R"EOF( )EOF"); @@ -184,20 +175,17 @@ TEST_F(XmlReferenceLinkerTest, LinkMangledAttributes) { xml::Element* view_el = xml::FindRootElement(doc.get()); ASSERT_NE(view_el, nullptr); - xml::Attribute* xml_attr = view_el->FindAttribute( - xml::BuildPackageNamespace("com.android.support"), "colorAccent"); + xml::Attribute* xml_attr = + view_el->FindAttribute(xml::BuildPackageNamespace("com.android.support"), "colorAccent"); ASSERT_NE(xml_attr, nullptr); AAPT_ASSERT_TRUE(xml_attr->compiled_attribute); AAPT_ASSERT_TRUE(xml_attr->compiled_attribute.value().id); - EXPECT_EQ(xml_attr->compiled_attribute.value().id.value(), - ResourceId(0x7f010001)); - ASSERT_NE(ValueCast(xml_attr->compiled_value.get()), - nullptr); + EXPECT_EQ(xml_attr->compiled_attribute.value().id.value(), ResourceId(0x7f010001)); + ASSERT_NE(ValueCast(xml_attr->compiled_value.get()), nullptr); } TEST_F(XmlReferenceLinkerTest, LinkAutoResReference) { - std::unique_ptr doc = - test::BuildXmlDomForPackageName(context_.get(), R"EOF( + std::unique_ptr doc = test::BuildXmlDomForPackageName(context_.get(), R"EOF( )EOF"); @@ -207,13 +195,11 @@ TEST_F(XmlReferenceLinkerTest, LinkAutoResReference) { xml::Element* view_el = xml::FindRootElement(doc.get()); ASSERT_NE(view_el, nullptr); - xml::Attribute* xml_attr = - view_el->FindAttribute(xml::kSchemaAuto, "colorAccent"); + xml::Attribute* xml_attr = view_el->FindAttribute(xml::kSchemaAuto, "colorAccent"); ASSERT_NE(xml_attr, nullptr); AAPT_ASSERT_TRUE(xml_attr->compiled_attribute); AAPT_ASSERT_TRUE(xml_attr->compiled_attribute.value().id); - EXPECT_EQ(xml_attr->compiled_attribute.value().id.value(), - ResourceId(0x7f010000)); + EXPECT_EQ(xml_attr->compiled_attribute.value().id.value(), ResourceId(0x7f010000)); Reference* ref = ValueCast(xml_attr->compiled_value.get()); ASSERT_NE(ref, nullptr); AAPT_ASSERT_TRUE(ref->name); @@ -222,8 +208,7 @@ TEST_F(XmlReferenceLinkerTest, LinkAutoResReference) { } TEST_F(XmlReferenceLinkerTest, LinkViewWithShadowedPackageAlias) { - std::unique_ptr doc = - test::BuildXmlDomForPackageName(context_.get(), R"EOF( + std::unique_ptr doc = test::BuildXmlDomForPackageName(context_.get(), R"EOF( FindAttribute(xml::kSchemaAndroid, "attr"); + xml::Attribute* xml_attr = view_el->FindAttribute(xml::kSchemaAndroid, "attr"); ASSERT_NE(xml_attr, nullptr); AAPT_ASSERT_TRUE(xml_attr->compiled_attribute); AAPT_ASSERT_TRUE(xml_attr->compiled_attribute.value().id); - EXPECT_EQ(xml_attr->compiled_attribute.value().id.value(), - ResourceId(0x01010002)); + EXPECT_EQ(xml_attr->compiled_attribute.value().id.value(), ResourceId(0x01010002)); Reference* ref = ValueCast(xml_attr->compiled_value.get()); ASSERT_NE(ref, nullptr); AAPT_ASSERT_TRUE(ref->id); @@ -256,13 +239,11 @@ TEST_F(XmlReferenceLinkerTest, LinkViewWithShadowedPackageAlias) { // All attributes and references in this element should be referring to // "com.app.test" (0x7f). - xml_attr = view_el->FindAttribute(xml::BuildPackageNamespace("com.app.test"), - "attr"); + xml_attr = view_el->FindAttribute(xml::BuildPackageNamespace("com.app.test"), "attr"); ASSERT_NE(xml_attr, nullptr); AAPT_ASSERT_TRUE(xml_attr->compiled_attribute); AAPT_ASSERT_TRUE(xml_attr->compiled_attribute.value().id); - EXPECT_EQ(xml_attr->compiled_attribute.value().id.value(), - ResourceId(0x7f010002)); + EXPECT_EQ(xml_attr->compiled_attribute.value().id.value(), ResourceId(0x7f010002)); ref = ValueCast(xml_attr->compiled_value.get()); ASSERT_NE(ref, nullptr); AAPT_ASSERT_TRUE(ref->id); @@ -270,8 +251,7 @@ TEST_F(XmlReferenceLinkerTest, LinkViewWithShadowedPackageAlias) { } TEST_F(XmlReferenceLinkerTest, LinkViewWithLocalPackageAndAliasOfTheSameName) { - std::unique_ptr doc = - test::BuildXmlDomForPackageName(context_.get(), R"EOF( + std::unique_ptr doc = test::BuildXmlDomForPackageName(context_.get(), R"EOF( )EOF"); @@ -283,13 +263,12 @@ TEST_F(XmlReferenceLinkerTest, LinkViewWithLocalPackageAndAliasOfTheSameName) { // All attributes and references in this element should be referring to // "com.app.test" (0x7f). - xml::Attribute* xml_attr = view_el->FindAttribute( - xml::BuildPackageNamespace("com.app.test"), "attr"); + xml::Attribute* xml_attr = + view_el->FindAttribute(xml::BuildPackageNamespace("com.app.test"), "attr"); ASSERT_NE(xml_attr, nullptr); AAPT_ASSERT_TRUE(xml_attr->compiled_attribute); AAPT_ASSERT_TRUE(xml_attr->compiled_attribute.value().id); - EXPECT_EQ(xml_attr->compiled_attribute.value().id.value(), - ResourceId(0x7f010002)); + EXPECT_EQ(xml_attr->compiled_attribute.value().id.value(), ResourceId(0x7f010002)); Reference* ref = ValueCast(xml_attr->compiled_value.get()); ASSERT_NE(ref, nullptr); AAPT_ASSERT_TRUE(ref->id);