De-duplicate entries written with AAPT2 convert
There was no check for whether we had already written a specific file path to the APK when using the convert command.
If the resources table points 2 resource IDs at the same file on disk, the convert command would write the file twice, creating two entries.
This holds a set of file paths already written and ignores duplicates.
Bug: 123271593
Test: Ran convert on linked bug's weird.apk
Test: aapt2_tests case for duplicate entries
Change-Id: Ia22515bf8e8297624aaadbf6a9e47159026c63e5
diff --git a/tools/aapt2/Android.bp b/tools/aapt2/Android.bp
index 7783e10..8f75287 100644
--- a/tools/aapt2/Android.bp
+++ b/tools/aapt2/Android.bp
@@ -182,7 +182,8 @@
defaults: ["aapt2_defaults"],
data: [
"integration-tests/CompileTest/**/*",
- "integration-tests/CommandTests/**/*"
+ "integration-tests/CommandTests/**/*",
+ "integration-tests/ConvertTest/**/*"
],
}
diff --git a/tools/aapt2/cmd/Convert.cpp b/tools/aapt2/cmd/Convert.cpp
index 85f9080..7a74ba9 100644
--- a/tools/aapt2/cmd/Convert.cpp
+++ b/tools/aapt2/cmd/Convert.cpp
@@ -284,6 +284,8 @@
// The table might be modified by below code.
auto converted_table = apk->GetResourceTable();
+ std::unordered_set<std::string> files_written;
+
// Resources
for (const auto& package : converted_table->packages) {
for (const auto& type : package->types) {
@@ -297,10 +299,14 @@
return 1;
}
- if (!serializer->SerializeFile(file, output_writer)) {
- context->GetDiagnostics()->Error(DiagMessage(apk->GetSource())
- << "failed to serialize file " << *file->path);
- return 1;
+ // Only serialize if we haven't seen this file before
+ if (files_written.insert(*file->path).second) {
+ if (!serializer->SerializeFile(file, output_writer)) {
+ context->GetDiagnostics()->Error(DiagMessage(apk->GetSource())
+ << "failed to serialize file "
+ << *file->path);
+ return 1;
+ }
}
} // file
} // config_value
diff --git a/tools/aapt2/cmd/Convert_test.cpp b/tools/aapt2/cmd/Convert_test.cpp
index 2e43150..77cdc41 100644
--- a/tools/aapt2/cmd/Convert_test.cpp
+++ b/tools/aapt2/cmd/Convert_test.cpp
@@ -18,6 +18,7 @@
#include "LoadedApk.h"
#include "test/Test.h"
+#include "ziparchive/zip_archive.h"
using testing::Eq;
using testing::Ne;
@@ -95,4 +96,45 @@
EXPECT_THAT(util::GetString(tree.getStrings(), static_cast<size_t>(raw_index)), Eq("007"));
}
+TEST_F(ConvertTest, DuplicateEntriesWrittenOnce) {
+ StdErrDiagnostics diag;
+ const std::string apk_path =
+ file::BuildPath({android::base::GetExecutableDirectory(),
+ "integration-tests", "ConvertTest", "duplicate_entries.apk"});
+
+ const std::string out_convert_apk = GetTestPath("out_convert.apk");
+ std::vector<android::StringPiece> convert_args = {
+ "-o", out_convert_apk,
+ "--output-format", "proto",
+ apk_path
+ };
+ ASSERT_THAT(ConvertCommand().Execute(convert_args, &std::cerr), Eq(0));
+
+ ZipArchiveHandle handle;
+ ASSERT_THAT(OpenArchive(out_convert_apk.c_str(), &handle), Eq(0));
+
+ void* cookie = nullptr;
+
+ ZipString prefix("res/theme/10");
+ int32_t result = StartIteration(handle, &cookie, &prefix, nullptr);
+
+ // If this is -5, that means we've found a duplicate entry and this test has failed
+ EXPECT_THAT(result, Eq(0));
+
+ // But if read succeeds, verify only one res/theme/10 entry
+ int count = 0;
+
+ // Can't pass nullptrs into Next()
+ ZipString zip_name;
+ ZipEntry zip_data;
+
+ while ((result = Next(cookie, &zip_data, &zip_name)) == 0) {
+ count++;
+ }
+
+ EndIteration(cookie);
+
+ EXPECT_THAT(count, Eq(1));
+}
+
} // namespace aapt
\ No newline at end of file
diff --git a/tools/aapt2/integration-tests/ConvertTest/duplicate_entries.apk b/tools/aapt2/integration-tests/ConvertTest/duplicate_entries.apk
new file mode 100644
index 0000000..c558a33
--- /dev/null
+++ b/tools/aapt2/integration-tests/ConvertTest/duplicate_entries.apk
Binary files differ