From a4fb17bbbba59c5ee92402eb8c5da4e3c52560b2 Mon Sep 17 00:00:00 2001
From: Adam Lesinski <adamlesinski@google.com>
Date: Tue, 16 Jan 2018 17:05:10 -0800
Subject: aapt2: Fix issue with Manifest duplicate handling

Fixed a memory-corruption issue that led to multiple duplicate
permission entries being generated for Manifest.java.

Bug: 71641288
Test: make aapt2_tests
Change-Id: I8cd37929c4883aaba2beebbf874c7ee3234d51d8
---
 tools/aapt2/java/ClassDefinition.cpp             | 14 ++++++++++++--
 tools/aapt2/java/ManifestClassGenerator_test.cpp |  5 ++++-
 2 files changed, 16 insertions(+), 3 deletions(-)

(limited to 'tools/aapt2/java')

diff --git a/tools/aapt2/java/ClassDefinition.cpp b/tools/aapt2/java/ClassDefinition.cpp
index b692ccf7e52d..f5f5b05491bb 100644
--- a/tools/aapt2/java/ClassDefinition.cpp
+++ b/tools/aapt2/java/ClassDefinition.cpp
@@ -45,8 +45,18 @@ ClassDefinition::Result ClassDefinition::AddMember(std::unique_ptr<ClassMember>
   Result result = Result::kAdded;
   auto iter = indexed_members_.find(member->GetName());
   if (iter != indexed_members_.end()) {
-    // Overwrite the entry.
-    ordered_members_[iter->second].reset();
+    // Overwrite the entry. Be careful, as the key in indexed_members_ is actually memory owned
+    // by the value at ordered_members_[index]. Since overwriting a value for a key doesn't replace
+    // the key (the initial key inserted into the unordered_map is kept), we must erase and then
+    // insert a new key, whose memory is being kept around. We do all this to avoid using more
+    // memory for each key.
+    size_t index = iter->second;
+
+    // Erase the key + value from the map.
+    indexed_members_.erase(iter);
+
+    // Now clear the memory that was backing the key (now erased).
+    ordered_members_[index].reset();
     result = Result::kOverridden;
   }
 
diff --git a/tools/aapt2/java/ManifestClassGenerator_test.cpp b/tools/aapt2/java/ManifestClassGenerator_test.cpp
index c324238d3ecb..f4e10ab2e584 100644
--- a/tools/aapt2/java/ManifestClassGenerator_test.cpp
+++ b/tools/aapt2/java/ManifestClassGenerator_test.cpp
@@ -129,13 +129,16 @@ TEST(ManifestClassGeneratorTest, LastSeenPermissionWithSameLeafNameTakesPreceden
   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" />
+        <permission android:name="com.android.sample.ACCESS_INTERNET" />
+        <permission android:name="com.android.permission.UNRELATED_PERMISSION" />
+        <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\";")));
+  EXPECT_THAT(actual, Not(HasSubstr("ACCESS_INTERNET=\"com.android.sample.ACCESS_INTERNET\";")));
 }
 
 static ::testing::AssertionResult GetManifestClassText(IAaptContext* context, xml::XmlResource* res,
-- 
cgit v1.2.3-59-g8ed1b