From f852dd0eb86e461f1f039f6a292e62123757dad8 Mon Sep 17 00:00:00 2001 From: Adam Lesinski Date: Fri, 18 Aug 2017 19:49:58 -0700 Subject: [PATCH] AAPT2: Fix regression in Manifest.java permissions Permissions defined with the same leaf name emit the same string symbol, which causes collisions. AAPT would override the symbol with the last one seen. Do the same thing as AAPT, but emit a warning. Bug: 64472942 Test: make aapt2_tests Change-Id: I17b9dc7e8d8bd80db98869394c93695cb453bebd --- tools/aapt2/java/ClassDefinition.cpp | 11 ++++ tools/aapt2/java/ClassDefinition.h | 63 ++++++++++++++++--- tools/aapt2/java/ManifestClassGenerator.cpp | 5 +- .../java/ManifestClassGenerator_test.cpp | 15 +++++ 4 files changed, 84 insertions(+), 10 deletions(-) diff --git a/tools/aapt2/java/ClassDefinition.cpp b/tools/aapt2/java/ClassDefinition.cpp index 45130a4d76812..6ad0dd68cb6ae 100644 --- a/tools/aapt2/java/ClassDefinition.cpp +++ b/tools/aapt2/java/ClassDefinition.cpp @@ -39,6 +39,17 @@ void MethodDefinition::WriteToStream(const StringPiece& prefix, bool final, *out << prefix << "}"; } +ClassDefinition::Result ClassDefinition::AddMember(std::unique_ptr member) { + Result result = Result::kAdded; + auto iter = members_.find(member); + if (iter != members_.end()) { + members_.erase(iter); + result = Result::kOverridden; + } + members_.insert(std::move(member)); + return result; +} + bool ClassDefinition::empty() const { for (const std::unique_ptr& member : members_) { if (!member->empty()) { diff --git a/tools/aapt2/java/ClassDefinition.h b/tools/aapt2/java/ClassDefinition.h index ca76421390d6a..6c4bcad83d2ab 100644 --- a/tools/aapt2/java/ClassDefinition.h +++ b/tools/aapt2/java/ClassDefinition.h @@ -18,6 +18,7 @@ #define AAPT_JAVA_CLASSDEFINITION_H #include +#include #include #include "android-base/macros.h" @@ -37,10 +38,14 @@ class ClassMember { public: virtual ~ClassMember() = default; - AnnotationProcessor* GetCommentBuilder() { return &processor_; } + AnnotationProcessor* GetCommentBuilder() { + return &processor_; + } virtual bool empty() const = 0; + virtual const std::string& GetName() const = 0; + // Writes the class member to the out stream. Subclasses should derive this method // to write their own data. Call this base method from the subclass to write out // this member's comments/annotations. @@ -57,7 +62,13 @@ class PrimitiveMember : public ClassMember { PrimitiveMember(const android::StringPiece& name, const T& val) : name_(name.to_string()), val_(val) {} - bool empty() const override { return false; } + bool empty() const override { + return false; + } + + const std::string& GetName() const override { + return name_; + } void WriteToStream(const android::StringPiece& prefix, bool final, std::ostream* out) const override { @@ -83,7 +94,13 @@ class PrimitiveMember : public ClassMember { PrimitiveMember(const android::StringPiece& name, const std::string& val) : name_(name.to_string()), val_(val) {} - bool empty() const override { return false; } + bool empty() const override { + return false; + } + + const std::string& GetName() const override { + return name_; + } void WriteToStream(const android::StringPiece& prefix, bool final, std::ostream* out) const override { @@ -109,9 +126,17 @@ class PrimitiveArrayMember : public ClassMember { public: explicit PrimitiveArrayMember(const android::StringPiece& name) : name_(name.to_string()) {} - void AddElement(const T& val) { elements_.push_back(val); } + void AddElement(const T& val) { + elements_.push_back(val); + } - bool empty() const override { return false; } + bool empty() const override { + return false; + } + + const std::string& GetName() const override { + return name_; + } void WriteToStream(const android::StringPiece& prefix, bool final, std::ostream* out) const override { @@ -154,6 +179,11 @@ class MethodDefinition : public ClassMember { // formatting may be broken. void AppendStatement(const android::StringPiece& statement); + // Not quite the same as a name, but good enough. + const std::string& GetName() const override { + return signature_; + } + // Even if the method is empty, we always want to write the method signature. bool empty() const override { return false; } @@ -175,19 +205,34 @@ class ClassDefinition : public ClassMember { ClassDefinition(const android::StringPiece& name, ClassQualifier qualifier, bool createIfEmpty) : name_(name.to_string()), qualifier_(qualifier), create_if_empty_(createIfEmpty) {} - void AddMember(std::unique_ptr member) { - members_.push_back(std::move(member)); - } + enum class Result { + kAdded, + kOverridden, + }; + + Result AddMember(std::unique_ptr member); bool empty() const override; + + const std::string& GetName() const override { + return name_; + } + void WriteToStream(const android::StringPiece& prefix, bool final, std::ostream* out) const override; private: + struct ClassMemberCompare { + using T = std::unique_ptr; + bool operator()(const T& a, const T& b) const { + return a->GetName() < b->GetName(); + } + }; + std::string name_; ClassQualifier qualifier_; bool create_if_empty_; - std::vector> members_; + std::set, ClassMemberCompare> members_; DISALLOW_COPY_AND_ASSIGN(ClassDefinition); }; diff --git a/tools/aapt2/java/ManifestClassGenerator.cpp b/tools/aapt2/java/ManifestClassGenerator.cpp index 4ef32c92dd848..c4b36176aa71a 100644 --- a/tools/aapt2/java/ManifestClassGenerator.cpp +++ b/tools/aapt2/java/ManifestClassGenerator.cpp @@ -68,7 +68,10 @@ static bool WriteSymbol(const Source& source, IDiagnostics* diag, xml::Element* util::make_unique(result.value(), attr->value); string_member->GetCommentBuilder()->AppendComment(el->comment); - class_def->AddMember(std::move(string_member)); + if (class_def->AddMember(std::move(string_member)) == ClassDefinition::Result::kOverridden) { + diag->Warn(DiagMessage(source.WithLine(el->line_number)) + << "duplicate definitions of '" << result.value() << "', overriding previous"); + } return true; } diff --git a/tools/aapt2/java/ManifestClassGenerator_test.cpp b/tools/aapt2/java/ManifestClassGenerator_test.cpp index c744e9bd4fc9b..ada563409d19b 100644 --- a/tools/aapt2/java/ManifestClassGenerator_test.cpp +++ b/tools/aapt2/java/ManifestClassGenerator_test.cpp @@ -121,6 +121,21 @@ TEST(ManifestClassGeneratorTest, CommentsAndAnnotationsArePresent) { EXPECT_THAT(actual, HasSubstr(expected_test)); } +// This is bad but part of public API behaviour so we need to preserve it. +TEST(ManifestClassGeneratorTest, LastSeenPermissionWithSameLeafNameTakesPrecedence) { + std::unique_ptr context = test::ContextBuilder().Build(); + std::unique_ptr manifest = test::BuildXmlDom(R"( + + + + )"); + + std::string actual; + ASSERT_TRUE(GetManifestClassText(context.get(), manifest.get(), &actual)); + EXPECT_THAT(actual, HasSubstr("ACCESS_INTERNET=\"com.android.aapt.test.ACCESS_INTERNET\";")); + EXPECT_THAT(actual, Not(HasSubstr("ACCESS_INTERNET=\"android.permission.ACCESS_INTERNET\";"))); +} + static ::testing::AssertionResult GetManifestClassText(IAaptContext* context, xml::XmlResource* res, std::string* out_str) { std::unique_ptr manifest_class =