From 4e9a922ede24f7f7bfe793321f7328623ee2a061 Mon Sep 17 00:00:00 2001 From: Ryan Mitchell Date: Tue, 13 Nov 2018 10:40:07 -0800 Subject: [PATCH] Fix loaded apk string pool order Loading in an APk changed the ordering of strings in the string pool. When loading an apk, assign the strings to the same index as they are in the ResStringPool. Bug: 118831219 Test: "aapt2 dump strings left.apk" prints in the correct order, "aapt2 convert left.apk --output-format binary -o left_binary.apk" has entries in the correct order, and aapt2_tests Change-Id: I00014c02195f39c1152a110e90083d9b14e9216e --- tools/aapt2/Debug.cpp | 4 +- tools/aapt2/ResourceUtils.cpp | 4 +- tools/aapt2/StringPool.cpp | 22 ++- tools/aapt2/StringPool.h | 6 +- tools/aapt2/StringPool_test.cpp | 18 ++ tools/aapt2/cmd/Convert.cpp | 182 ++++++++++--------- tools/aapt2/cmd/Convert.h | 6 +- tools/aapt2/format/binary/TableFlattener.cpp | 20 +- tools/aapt2/format/binary/TableFlattener.h | 3 + 9 files changed, 158 insertions(+), 107 deletions(-) diff --git a/tools/aapt2/Debug.cpp b/tools/aapt2/Debug.cpp index 0bc5221245ca6..583f14ac0cbda 100644 --- a/tools/aapt2/Debug.cpp +++ b/tools/aapt2/Debug.cpp @@ -431,7 +431,7 @@ void Debug::DumpHex(const void* data, size_t len) { void Debug::DumpResStringPool(const android::ResStringPool* pool, text::Printer* printer) { using namespace android; - + if (pool->getError() == NO_INIT) { printer->Print("String pool is unitialized.\n"); return; @@ -460,7 +460,7 @@ void Debug::DumpResStringPool(const android::ResStringPool* pool, text::Printer* const size_t NS = pool->size(); for (size_t s=0; sstring8ObjectAt(s); - printer->Print(StringPrintf("String #%zd : %s\n", s, str.string())); + printer->Print(StringPrintf("String #%zd: %s\n", s, str.string())); } } diff --git a/tools/aapt2/ResourceUtils.cpp b/tools/aapt2/ResourceUtils.cpp index dbe5ac5adb983..da22e885b9171 100644 --- a/tools/aapt2/ResourceUtils.cpp +++ b/tools/aapt2/ResourceUtils.cpp @@ -718,7 +718,7 @@ std::unique_ptr ParseBinaryResValue(const ResourceType& type, const Config // This must be a FileReference. std::unique_ptr file_ref = util::make_unique(dst_pool->MakeRef( - str, StringPool::Context(StringPool::Context::kHighPriority, config))); + str, StringPool::Context(StringPool::Context::kHighPriority, config), data)); if (type == ResourceType::kRaw) { file_ref->type = ResourceFile::Type::kUnknown; } else if (util::EndsWith(*file_ref->path, ".xml")) { @@ -730,7 +730,7 @@ std::unique_ptr ParseBinaryResValue(const ResourceType& type, const Config } // There are no styles associated with this string, so treat it as a simple string. - return util::make_unique(dst_pool->MakeRef(str, StringPool::Context(config))); + return util::make_unique(dst_pool->MakeRef(str, StringPool::Context(config), data)); } } break; diff --git a/tools/aapt2/StringPool.cpp b/tools/aapt2/StringPool.cpp index 8eabd3225d87a..a8c26668b3a5c 100644 --- a/tools/aapt2/StringPool.cpp +++ b/tools/aapt2/StringPool.cpp @@ -165,12 +165,13 @@ StringPool::Ref StringPool::MakeRef(const StringPiece& str) { return MakeRefImpl(str, Context{}, true); } -StringPool::Ref StringPool::MakeRef(const StringPiece& str, const Context& context) { - return MakeRefImpl(str, context, true); +StringPool::Ref StringPool::MakeRef(const StringPiece& str, const Context& context, + Maybe index) { + return MakeRefImpl(str, context, true, index); } StringPool::Ref StringPool::MakeRefImpl(const StringPiece& str, const Context& context, - bool unique) { + bool unique, Maybe index) { if (unique) { auto range = indexed_strings_.equal_range(str); for (auto iter = range.first; iter != range.second; ++iter) { @@ -180,15 +181,26 @@ StringPool::Ref StringPool::MakeRefImpl(const StringPiece& str, const Context& c } } + const size_t size = strings_.size(); + // Insert the string at the end of the string vector if no index is specified + const size_t insertion_index = index ? index.value() : size; + std::unique_ptr entry(new Entry()); entry->value = str.to_string(); entry->context = context; - entry->index_ = strings_.size(); + entry->index_ = insertion_index; entry->ref_ = 0; entry->pool_ = this; Entry* borrow = entry.get(); - strings_.emplace_back(std::move(entry)); + if (insertion_index == size) { + strings_.emplace_back(std::move(entry)); + } else { + // Allocate enough space for the string at the index + strings_.resize(std::max(insertion_index + 1, size)); + strings_[insertion_index] = std::move(entry); + } + indexed_strings_.insert(std::make_pair(StringPiece(borrow->value), borrow)); return Ref(borrow); } diff --git a/tools/aapt2/StringPool.h b/tools/aapt2/StringPool.h index 1006ca970dc5b..115d5d315b8f8 100644 --- a/tools/aapt2/StringPool.h +++ b/tools/aapt2/StringPool.h @@ -166,7 +166,8 @@ class StringPool { // Adds a string to the pool, unless it already exists, with a context object that can be used // when sorting the string pool. Returns a reference to the string in the pool. - Ref MakeRef(const android::StringPiece& str, const Context& context); + Ref MakeRef(const android::StringPiece& str, const Context& context, + Maybe index = {}); // Adds a string from another string pool. Returns a reference to the string in the string pool. Ref MakeRef(const Ref& ref); @@ -210,7 +211,8 @@ class StringPool { static bool Flatten(BigBuffer* out, const StringPool& pool, bool utf8, IDiagnostics* diag); - Ref MakeRefImpl(const android::StringPiece& str, const Context& context, bool unique); + Ref MakeRefImpl(const android::StringPiece& str, const Context& context, bool unique, + Maybe index = {}); void ReAssignIndices(); std::vector> strings_; diff --git a/tools/aapt2/StringPool_test.cpp b/tools/aapt2/StringPool_test.cpp index 9a7238b584ba9..648be7d337542 100644 --- a/tools/aapt2/StringPool_test.cpp +++ b/tools/aapt2/StringPool_test.cpp @@ -84,6 +84,24 @@ TEST(StringPoolTest, MaintainInsertionOrderIndex) { EXPECT_THAT(ref_c.index(), Eq(2u)); } +TEST(StringPoolTest, AssignStringIndex) { + StringPool pool; + + StringPool::Ref ref_a = pool.MakeRef("0", StringPool::Context{}, 0u); + StringPool::Ref ref_b = pool.MakeRef("1", StringPool::Context{}, 1u); + StringPool::Ref ref_c = pool.MakeRef("5", StringPool::Context{}, 5u); + StringPool::Ref ref_d = pool.MakeRef("2", StringPool::Context{}, 2u); + StringPool::Ref ref_e = pool.MakeRef("4", StringPool::Context{}, 4u); + StringPool::Ref ref_f = pool.MakeRef("3", StringPool::Context{}, 3u); + + EXPECT_THAT(ref_a.index(), Eq(0u)); + EXPECT_THAT(ref_b.index(), Eq(1u)); + EXPECT_THAT(ref_d.index(), Eq(2u)); + EXPECT_THAT(ref_f.index(), Eq(3u)); + EXPECT_THAT(ref_e.index(), Eq(4u)); + EXPECT_THAT(ref_c.index(), Eq(5u)); +} + TEST(StringPoolTest, PruneStringsWithNoReferences) { StringPool pool; diff --git a/tools/aapt2/cmd/Convert.cpp b/tools/aapt2/cmd/Convert.cpp index 3ea17552ea7ce..4492f6b49cf02 100644 --- a/tools/aapt2/cmd/Convert.cpp +++ b/tools/aapt2/cmd/Convert.cpp @@ -57,78 +57,6 @@ class IApkSerializer { Source source_; }; -bool ConvertApk(IAaptContext* context, unique_ptr apk, IApkSerializer* serializer, - IArchiveWriter* writer) { - io::IFile* manifest = apk->GetFileCollection()->FindFile(kAndroidManifestPath); - if (!serializer->SerializeXml(apk->GetManifest(), kAndroidManifestPath, true /*utf16*/, writer, - (manifest != nullptr && manifest->WasCompressed()) - ? ArchiveEntry::kCompress : 0u)) { - context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) - << "failed to serialize AndroidManifest.xml"); - return false; - } - - if (apk->GetResourceTable() != nullptr) { - // The table might be modified by below code. - auto converted_table = apk->GetResourceTable(); - - // Resources - for (const auto& package : converted_table->packages) { - for (const auto& type : package->types) { - for (const auto& entry : type->entries) { - for (const auto& config_value : entry->values) { - FileReference* file = ValueCast(config_value->value.get()); - if (file != nullptr) { - if (file->file == nullptr) { - context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) - << "no file associated with " << *file); - return false; - } - - if (!serializer->SerializeFile(file, writer)) { - context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) - << "failed to serialize file " << *file->path); - return false; - } - } // file - } // config_value - } // entry - } // type - } // package - - // Converted resource table - if (!serializer->SerializeTable(converted_table, writer)) { - context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) - << "failed to serialize the resource table"); - return false; - } - } - - // Other files - std::unique_ptr iterator = apk->GetFileCollection()->Iterator(); - while (iterator->HasNext()) { - io::IFile* file = iterator->Next(); - std::string path = file->GetSource().path; - - // Manifest, resource table and resources have already been taken care of. - if (path == kAndroidManifestPath || - path == kApkResourceTablePath || - path == kProtoResourceTablePath || - path.find("res/") == 0) { - continue; - } - - if (!io::CopyFileToArchivePreserveCompression(context, file, path, writer)) { - context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) - << "failed to copy file " << path); - return false; - } - } - - return true; -} - - class BinaryApkSerializer : public IApkSerializer { public: BinaryApkSerializer(IAaptContext* context, const Source& source, @@ -323,12 +251,97 @@ class Context : public IAaptContext { StdErrDiagnostics diag_; }; +int Convert(IAaptContext* context, LoadedApk* apk, IArchiveWriter* output_writer, + ApkFormat output_format, TableFlattenerOptions& options) { + // Do not change the ordering of strings in the values string pool + options.sort_stringpool_entries = false; + + unique_ptr serializer; + if (output_format == ApkFormat::kBinary) { + serializer.reset(new BinaryApkSerializer(context, apk->GetSource(), options)); + } else if (output_format == ApkFormat::kProto) { + serializer.reset(new ProtoApkSerializer(context, apk->GetSource())); + } else { + context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) + << "Cannot convert APK to unknown format"); + return 1; + } + + io::IFile* manifest = apk->GetFileCollection()->FindFile(kAndroidManifestPath); + if (!serializer->SerializeXml(apk->GetManifest(), kAndroidManifestPath, true /*utf16*/, + output_writer, (manifest != nullptr && manifest->WasCompressed()) + ? ArchiveEntry::kCompress : 0u)) { + context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) + << "failed to serialize AndroidManifest.xml"); + return 1; + } + + if (apk->GetResourceTable() != nullptr) { + // The table might be modified by below code. + auto converted_table = apk->GetResourceTable(); + + // Resources + for (const auto& package : converted_table->packages) { + for (const auto& type : package->types) { + for (const auto& entry : type->entries) { + for (const auto& config_value : entry->values) { + FileReference* file = ValueCast(config_value->value.get()); + if (file != nullptr) { + if (file->file == nullptr) { + context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) + << "no file associated with " << *file); + return 1; + } + + if (!serializer->SerializeFile(file, output_writer)) { + context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) + << "failed to serialize file " << *file->path); + return 1; + } + } // file + } // config_value + } // entry + } // type + } // package + + // Converted resource table + if (!serializer->SerializeTable(converted_table, output_writer)) { + context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) + << "failed to serialize the resource table"); + return 1; + } + } + + // Other files + std::unique_ptr iterator = apk->GetFileCollection()->Iterator(); + while (iterator->HasNext()) { + io::IFile* file = iterator->Next(); + std::string path = file->GetSource().path; + + // Manifest, resource table and resources have already been taken care of. + if (path == kAndroidManifestPath || + path == kApkResourceTablePath || + path == kProtoResourceTablePath || + path.find("res/") == 0) { + continue; + } + + if (!io::CopyFileToArchivePreserveCompression(context, file, path, output_writer)) { + context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) + << "failed to copy file " << path); + return 1; + } + } + + return 0; +} + const char* ConvertCommand::kOutputFormatProto = "proto"; const char* ConvertCommand::kOutputFormatBinary = "binary"; int ConvertCommand::Action(const std::vector& args) { if (args.size() != 1) { - std::cerr << "must supply a single proto APK\n"; + std::cerr << "must supply a single APK\n"; Usage(&std::cerr); return 1; } @@ -341,34 +354,31 @@ int ConvertCommand::Action(const std::vector& args) { return 1; } - Maybe app_info = - ExtractAppInfoFromBinaryManifest(*apk->GetManifest(), context.GetDiagnostics()); + Maybe app_info = ExtractAppInfoFromBinaryManifest(*apk->GetManifest(), + context.GetDiagnostics()); if (!app_info) { return 1; } context.package_ = app_info.value().package; - - unique_ptr writer = - CreateZipFileArchiveWriter(context.GetDiagnostics(), output_path_); + unique_ptr writer = CreateZipFileArchiveWriter(context.GetDiagnostics(), + output_path_); if (writer == nullptr) { return 1; } - unique_ptr serializer; + ApkFormat format; if (!output_format_ || output_format_.value() == ConvertCommand::kOutputFormatBinary) { - - serializer.reset(new BinaryApkSerializer(&context, apk->GetSource(), options_)); + format = ApkFormat::kBinary; } else if (output_format_.value() == ConvertCommand::kOutputFormatProto) { - serializer.reset(new ProtoApkSerializer(&context, apk->GetSource())); + format = ApkFormat::kProto; } else { - context.GetDiagnostics()->Error(DiagMessage(path) - << "Invalid value for flag --output-format: " - << output_format_.value()); + context.GetDiagnostics()->Error(DiagMessage(path) << "Invalid value for flag --output-format: " + << output_format_.value()); return 1; } - return ConvertApk(&context, std::move(apk), serializer.get(), writer.get()) ? 0 : 1; + return Convert(&context, apk.get(), writer.get(), format, options_); } } // namespace aapt diff --git a/tools/aapt2/cmd/Convert.h b/tools/aapt2/cmd/Convert.h index fcec23d16f785..6a6719c91b58a 100644 --- a/tools/aapt2/cmd/Convert.h +++ b/tools/aapt2/cmd/Convert.h @@ -18,6 +18,7 @@ #define AAPT2_CONVERT_H #include "Command.h" +#include "LoadedApk.h" #include "format/binary/TableFlattener.h" namespace aapt { @@ -49,6 +50,9 @@ class ConvertCommand : public Command { bool verbose_ = false; }; -}// namespace aapt +int Convert(IAaptContext* context, LoadedApk* input, IArchiveWriter* output_writer, + ApkFormat output_format, TableFlattenerOptions& options); + +} // namespace aapt #endif //AAPT2_CONVERT_H diff --git a/tools/aapt2/format/binary/TableFlattener.cpp b/tools/aapt2/format/binary/TableFlattener.cpp index 8a86f63a30c16..6c1a9ba2cbad7 100644 --- a/tools/aapt2/format/binary/TableFlattener.cpp +++ b/tools/aapt2/format/binary/TableFlattener.cpp @@ -573,15 +573,17 @@ class PackageFlattener { } // namespace bool TableFlattener::Consume(IAaptContext* context, ResourceTable* table) { - // We must do this before writing the resources, since the string pool IDs may change. - table->string_pool.Prune(); - table->string_pool.Sort([](const StringPool::Context& a, const StringPool::Context& b) -> int { - int diff = util::compare(a.priority, b.priority); - if (diff == 0) { - diff = a.config.compare(b.config); - } - return diff; - }); + if (options_.sort_stringpool_entries) { + // We must do this before writing the resources, since the string pool IDs may change. + table->string_pool.Prune(); + table->string_pool.Sort([](const StringPool::Context &a, const StringPool::Context &b) -> int { + int diff = util::compare(a.priority, b.priority); + if (diff == 0) { + diff = a.config.compare(b.config); + } + return diff; + }); + } // Write the ResTable header. ChunkWriter table_writer(buffer_); diff --git a/tools/aapt2/format/binary/TableFlattener.h b/tools/aapt2/format/binary/TableFlattener.h index c2e1d4b4ce8cd..635cb21f514cf 100644 --- a/tools/aapt2/format/binary/TableFlattener.h +++ b/tools/aapt2/format/binary/TableFlattener.h @@ -43,6 +43,9 @@ struct TableFlattenerOptions { // Set of whitelisted resource names to avoid altering in key stringpool std::set whitelisted_resources; + + // When true, sort the entries in the values string pool by priority and configuration. + bool sort_stringpool_entries = true; }; class TableFlattener : public IResourceTableConsumer {