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
diff --git a/tools/aapt2/java/ClassDefinition.cpp b/tools/aapt2/java/ClassDefinition.cpp
index 45130a4..6ad0dd6 100644
--- a/tools/aapt2/java/ClassDefinition.cpp
+++ b/tools/aapt2/java/ClassDefinition.cpp
@@ -39,6 +39,17 @@
*out << prefix << "}";
}
+ClassDefinition::Result ClassDefinition::AddMember(std::unique_ptr<ClassMember> 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<ClassMember>& member : members_) {
if (!member->empty()) {
diff --git a/tools/aapt2/java/ClassDefinition.h b/tools/aapt2/java/ClassDefinition.h
index ca76421..6c4bcad 100644
--- a/tools/aapt2/java/ClassDefinition.h
+++ b/tools/aapt2/java/ClassDefinition.h
@@ -18,6 +18,7 @@
#define AAPT_JAVA_CLASSDEFINITION_H
#include <ostream>
+#include <set>
#include <string>
#include "android-base/macros.h"
@@ -37,10 +38,14 @@
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 @@
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 @@
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 @@
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 @@
// 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 @@
ClassDefinition(const android::StringPiece& name, ClassQualifier qualifier, bool createIfEmpty)
: name_(name.to_string()), qualifier_(qualifier), create_if_empty_(createIfEmpty) {}
- void AddMember(std::unique_ptr<ClassMember> member) {
- members_.push_back(std::move(member));
- }
+ enum class Result {
+ kAdded,
+ kOverridden,
+ };
+
+ Result AddMember(std::unique_ptr<ClassMember> 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<ClassMember>;
+ 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<std::unique_ptr<ClassMember>> members_;
+ std::set<std::unique_ptr<ClassMember>, ClassMemberCompare> members_;
DISALLOW_COPY_AND_ASSIGN(ClassDefinition);
};
diff --git a/tools/aapt2/java/ManifestClassGenerator.cpp b/tools/aapt2/java/ManifestClassGenerator.cpp
index 4ef32c9..c4b3617 100644
--- a/tools/aapt2/java/ManifestClassGenerator.cpp
+++ b/tools/aapt2/java/ManifestClassGenerator.cpp
@@ -68,7 +68,10 @@
util::make_unique<StringMember>(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 c744e9b..ada5634 100644
--- a/tools/aapt2/java/ManifestClassGenerator_test.cpp
+++ b/tools/aapt2/java/ManifestClassGenerator_test.cpp
@@ -121,6 +121,21 @@
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<IAaptContext> context = test::ContextBuilder().Build();
+ std::unique_ptr<xml::XmlResource> manifest = test::BuildXmlDom(R"(
+ <manifest xmlns:android="http://schemas.android.com/apk/res/android">
+ <permission android:name="android.permission.ACCESS_INTERNET" />
+ <permission android:name="com.android.aapt.test.ACCESS_INTERNET" />
+ </manifest>)");
+
+ 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<ClassDefinition> manifest_class =