From 8a0b238b1344dae0042bbb17b71c0c3b9b881f22 Mon Sep 17 00:00:00 2001 From: Adam Lesinski Date: Wed, 18 Oct 2017 15:07:33 -0700 Subject: AAPT2: Ensure strings are sorted by configuration Keep strings sorted by configuration so that strings likely to be selected (all match the same locale, for instance) are close together. Bug: 67958501 Test: make aapt2_tests Change-Id: Id17d93bf2e03ce408a6f619d3ea6dc313e393b76 --- tools/aapt2/StringPool.cpp | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'tools/aapt2/StringPool.cpp') diff --git a/tools/aapt2/StringPool.cpp b/tools/aapt2/StringPool.cpp index 705b1ab052af..3a1a18c01e1c 100644 --- a/tools/aapt2/StringPool.cpp +++ b/tools/aapt2/StringPool.cpp @@ -191,6 +191,13 @@ StringPool::Ref StringPool::MakeRefImpl(const StringPiece& str, const Context& c return Ref(borrow); } +StringPool::Ref StringPool::MakeRef(const Ref& ref) { + if (ref.entry_->pool_ == this) { + return ref; + } + return MakeRef(ref.entry_->value, ref.entry_->context); +} + StringPool::StyleRef StringPool::MakeRef(const StyleString& str) { return MakeRef(str, Context{}); } -- cgit v1.2.3-59-g8ed1b From 70414f22dcab1d4ce3c2e9d981f3256a9ba62515 Mon Sep 17 00:00:00 2001 From: Ryan Mitchell Date: Mon, 26 Mar 2018 11:05:31 -0700 Subject: Check the size of the strings in the StringPool before flattening. Test: Tested for normal functionality when string does not exceed maximum length and tests for detection of string that is too lonhg for UTF8i Bug: b/74176037 Change-Id: Ic71d3671a069e7012e8ca107e79e071499eebbf6 (cherry picked from commit a15c2a8957b9883cb293fdacaeabd7f2e037a0a5) --- tools/aapt2/LoadedApk.cpp | 2 +- tools/aapt2/StringPool.cpp | 107 +++++++++++++++++------ tools/aapt2/StringPool.h | 7 +- tools/aapt2/StringPool_test.cpp | 61 ++++++++++++- tools/aapt2/cmd/Compile.cpp | 2 +- tools/aapt2/cmd/Convert.cpp | 2 +- tools/aapt2/cmd/Link.cpp | 2 +- tools/aapt2/format/binary/TableFlattener.cpp | 7 +- tools/aapt2/format/binary/XmlFlattener.cpp | 4 +- tools/aapt2/format/proto/ProtoSerialize.cpp | 9 +- tools/aapt2/format/proto/ProtoSerialize.h | 4 +- tools/aapt2/format/proto/ProtoSerialize_test.cpp | 5 +- 12 files changed, 159 insertions(+), 53 deletions(-) (limited to 'tools/aapt2/StringPool.cpp') diff --git a/tools/aapt2/LoadedApk.cpp b/tools/aapt2/LoadedApk.cpp index 4a4260d1d323..1dd46ba813db 100644 --- a/tools/aapt2/LoadedApk.cpp +++ b/tools/aapt2/LoadedApk.cpp @@ -225,7 +225,7 @@ bool LoadedApk::WriteToArchive(IAaptContext* context, ResourceTable* split_table } } else if (format_ == ApkFormat::kProto && path == kProtoResourceTablePath) { pb::ResourceTable pb_table; - SerializeTableToPb(*split_table, &pb_table); + SerializeTableToPb(*split_table, &pb_table, context->GetDiagnostics()); if (!io::CopyProtoToArchive(context, &pb_table, path, diff --git a/tools/aapt2/StringPool.cpp b/tools/aapt2/StringPool.cpp index 3a1a18c01e1c..b0ce9e1ec947 100644 --- a/tools/aapt2/StringPool.cpp +++ b/tools/aapt2/StringPool.cpp @@ -332,6 +332,25 @@ static T* EncodeLength(T* data, size_t length) { return data; } +/** + * Returns the maximum possible string length that can be successfully encoded + * using 2 units of the specified T. + * EncodeLengthMax -> maximum unit length of 0x7FFF + * EncodeLengthMax -> maximum unit length of 0x7FFFFFFF + **/ +template +static size_t EncodeLengthMax() { + static_assert(std::is_integral::value, "wat."); + + constexpr size_t kMask = 1 << ((sizeof(T) * 8 * 2) - 1); + constexpr size_t max = kMask - 1; + return max; +} + +/** + * Returns the number of units (1 or 2) needed to encode the string length + * before writing the string. + */ template static size_t EncodedLengthUnits(size_t length) { static_assert(std::is_integral::value, "wat."); @@ -341,15 +360,30 @@ static size_t EncodedLengthUnits(size_t length) { return length > kMaxSize ? 2 : 1; } -static void EncodeString(const std::string& str, const bool utf8, BigBuffer* out) { +const std::string kStringTooLarge = "STRING_TOO_LARGE"; + +static bool EncodeString(const std::string& str, const bool utf8, BigBuffer* out, + IDiagnostics* diag) { if (utf8) { const std::string& encoded = str; - const ssize_t utf16_length = - utf8_to_utf16_length(reinterpret_cast(str.data()), str.size()); + const ssize_t utf16_length = utf8_to_utf16_length( + reinterpret_cast(encoded.data()), encoded.size()); CHECK(utf16_length >= 0); - const size_t total_size = EncodedLengthUnits(utf16_length) + - EncodedLengthUnits(encoded.length()) + encoded.size() + 1; + // Make sure the lengths to be encoded do not exceed the maximum length that + // can be encoded using chars + if ((((size_t)encoded.size()) > EncodeLengthMax()) + || (((size_t)utf16_length) > EncodeLengthMax())) { + + diag->Error(DiagMessage() << "string too large to encode using UTF-8 " + << "written instead as '" << kStringTooLarge << "'"); + + EncodeString(kStringTooLarge, utf8, out, diag); + return false; + } + + const size_t total_size = EncodedLengthUnits(utf16_length) + + EncodedLengthUnits(encoded.size()) + encoded.size() + 1; char* data = out->NextBlock(total_size); @@ -357,32 +391,47 @@ static void EncodeString(const std::string& str, const bool utf8, BigBuffer* out data = EncodeLength(data, utf16_length); // Now encode the size of the real UTF8 string. - data = EncodeLength(data, encoded.length()); + data = EncodeLength(data, encoded.size()); strncpy(data, encoded.data(), encoded.size()); - } else { - const std::u16string encoded = util::Utf8ToUtf16(str); - const ssize_t utf16_length = encoded.size(); + } else { + const std::u16string encoded = util::Utf8ToUtf16(str); + const ssize_t utf16_length = encoded.size(); - // Total number of 16-bit words to write. - const size_t total_size = EncodedLengthUnits(utf16_length) + encoded.size() + 1; + // Make sure the length to be encoded does not exceed the maximum possible + // length that can be encoded + if (((size_t)utf16_length) > EncodeLengthMax()) { + diag->Error(DiagMessage() << "string too large to encode using UTF-16 " + << "written instead as '" << kStringTooLarge << "'"); - char16_t* data = out->NextBlock(total_size); + EncodeString(kStringTooLarge, utf8, out, diag); + return false; + } - // Encode the actual UTF16 string length. - data = EncodeLength(data, utf16_length); - const size_t byte_length = encoded.size() * sizeof(char16_t); + // Total number of 16-bit words to write. + const size_t total_size = EncodedLengthUnits(utf16_length) + + encoded.size() + 1; - // NOTE: For some reason, strncpy16(data, entry->value.data(), - // entry->value.size()) truncates the string. - memcpy(data, encoded.data(), byte_length); + char16_t* data = out->NextBlock(total_size); - // The null-terminating character is already here due to the block of data - // being set to 0s on allocation. - } + // Encode the actual UTF16 string length. + data = EncodeLength(data, utf16_length); + const size_t byte_length = encoded.size() * sizeof(char16_t); + + // NOTE: For some reason, strncpy16(data, entry->value.data(), + // entry->value.size()) truncates the string. + memcpy(data, encoded.data(), byte_length); + + // The null-terminating character is already here due to the block of data + // being set to 0s on allocation. + } + + return true; } -bool StringPool::Flatten(BigBuffer* out, const StringPool& pool, bool utf8) { +bool StringPool::Flatten(BigBuffer* out, const StringPool& pool, bool utf8, + IDiagnostics* diag) { + bool no_error = true; const size_t start_index = out->size(); android::ResStringPool_header* header = out->NextBlock(); header->header.type = util::HostToDevice16(android::RES_STRING_POOL_TYPE); @@ -403,12 +452,12 @@ bool StringPool::Flatten(BigBuffer* out, const StringPool& pool, bool utf8) { // Styles always come first. for (const std::unique_ptr& entry : pool.styles_) { *indices++ = out->size() - before_strings_index; - EncodeString(entry->value, utf8, out); + no_error = EncodeString(entry->value, utf8, out, diag) && no_error; } for (const std::unique_ptr& entry : pool.strings_) { *indices++ = out->size() - before_strings_index; - EncodeString(entry->value, utf8, out); + no_error = EncodeString(entry->value, utf8, out, diag) && no_error; } out->Align4(); @@ -446,15 +495,15 @@ bool StringPool::Flatten(BigBuffer* out, const StringPool& pool, bool utf8) { out->Align4(); } header->header.size = util::HostToDevice32(out->size() - start_index); - return true; + return no_error; } -bool StringPool::FlattenUtf8(BigBuffer* out, const StringPool& pool) { - return Flatten(out, pool, true); +bool StringPool::FlattenUtf8(BigBuffer* out, const StringPool& pool, IDiagnostics* diag) { + return Flatten(out, pool, true, diag); } -bool StringPool::FlattenUtf16(BigBuffer* out, const StringPool& pool) { - return Flatten(out, pool, false); +bool StringPool::FlattenUtf16(BigBuffer* out, const StringPool& pool, IDiagnostics* diag) { + return Flatten(out, pool, false, diag); } } // namespace aapt diff --git a/tools/aapt2/StringPool.h b/tools/aapt2/StringPool.h index 3c1f3dc3a1bb..f5b464de2ea5 100644 --- a/tools/aapt2/StringPool.h +++ b/tools/aapt2/StringPool.h @@ -27,6 +27,7 @@ #include "androidfw/StringPiece.h" #include "ConfigDescription.h" +#include "Diagnostics.h" #include "util/BigBuffer.h" namespace aapt { @@ -152,8 +153,8 @@ class StringPool { int ref_; }; - static bool FlattenUtf8(BigBuffer* out, const StringPool& pool); - static bool FlattenUtf16(BigBuffer* out, const StringPool& pool); + static bool FlattenUtf8(BigBuffer* out, const StringPool& pool, IDiagnostics* diag); + static bool FlattenUtf16(BigBuffer* out, const StringPool& pool, IDiagnostics* diag); StringPool() = default; StringPool(StringPool&&) = default; @@ -207,7 +208,7 @@ class StringPool { private: DISALLOW_COPY_AND_ASSIGN(StringPool); - static bool Flatten(BigBuffer* out, const StringPool& pool, bool utf8); + static bool Flatten(BigBuffer* out, const StringPool& pool, bool utf8, IDiagnostics* diag); Ref MakeRefImpl(const android::StringPiece& str, const Context& context, bool unique); void ReAssignIndices(); diff --git a/tools/aapt2/StringPool_test.cpp b/tools/aapt2/StringPool_test.cpp index b1e5ce2e28a8..58a03de60f93 100644 --- a/tools/aapt2/StringPool_test.cpp +++ b/tools/aapt2/StringPool_test.cpp @@ -20,6 +20,7 @@ #include "androidfw/StringPiece.h" +#include "Diagnostics.h" #include "test/Test.h" #include "util/Util.h" @@ -188,10 +189,11 @@ TEST(StringPoolTest, StylesAndStringsAreSeparateAfterSorting) { TEST(StringPoolTest, FlattenEmptyStringPoolUtf8) { using namespace android; // For NO_ERROR on Windows. + StdErrDiagnostics diag; StringPool pool; BigBuffer buffer(1024); - StringPool::FlattenUtf8(&buffer, pool); + StringPool::FlattenUtf8(&buffer, pool, &diag); std::unique_ptr data = util::Copy(buffer); ResStringPool test; @@ -200,11 +202,12 @@ TEST(StringPoolTest, FlattenEmptyStringPoolUtf8) { TEST(StringPoolTest, FlattenOddCharactersUtf16) { using namespace android; // For NO_ERROR on Windows. + StdErrDiagnostics diag; StringPool pool; pool.MakeRef("\u093f"); BigBuffer buffer(1024); - StringPool::FlattenUtf16(&buffer, pool); + StringPool::FlattenUtf16(&buffer, pool, &diag); std::unique_ptr data = util::Copy(buffer); ResStringPool test; @@ -225,6 +228,7 @@ constexpr const char* sLongString = TEST(StringPoolTest, Flatten) { using namespace android; // For NO_ERROR on Windows. + StdErrDiagnostics diag; StringPool pool; @@ -244,8 +248,8 @@ TEST(StringPoolTest, Flatten) { EXPECT_THAT(ref_d.index(), Eq(4u)); BigBuffer buffers[2] = {BigBuffer(1024), BigBuffer(1024)}; - StringPool::FlattenUtf8(&buffers[0], pool); - StringPool::FlattenUtf16(&buffers[1], pool); + StringPool::FlattenUtf8(&buffers[0], pool, &diag); + StringPool::FlattenUtf16(&buffers[1], pool, &diag); // Test both UTF-8 and UTF-16 buffers. for (const BigBuffer& buffer : buffers) { @@ -288,4 +292,53 @@ TEST(StringPoolTest, Flatten) { } } + +TEST(StringPoolTest, MaxEncodingLength) { + StdErrDiagnostics diag; + using namespace android; // For NO_ERROR on Windows. + ResStringPool test; + + StringPool pool; + pool.MakeRef("aaaaaaaaaa"); + BigBuffer buffers[2] = {BigBuffer(1024), BigBuffer(1024)}; + + // Make sure a UTF-8 string under the maximum length does not produce an error + EXPECT_THAT(StringPool::FlattenUtf8(&buffers[0], pool, &diag), Eq(true)); + std::unique_ptr data = util::Copy(buffers[0]); + test.setTo(data.get(), buffers[0].size()); + EXPECT_THAT(util::GetString(test, 0), Eq("aaaaaaaaaa")); + + // Make sure a UTF-16 string under the maximum length does not produce an error + EXPECT_THAT(StringPool::FlattenUtf16(&buffers[1], pool, &diag), Eq(true)); + data = util::Copy(buffers[1]); + test.setTo(data.get(), buffers[1].size()); + EXPECT_THAT(util::GetString16(test, 0), Eq(u"aaaaaaaaaa")); + + StringPool pool2; + std::string longStr(50000, 'a'); + pool2.MakeRef("this fits1"); + pool2.MakeRef(longStr); + pool2.MakeRef("this fits2"); + BigBuffer buffers2[2] = {BigBuffer(1024), BigBuffer(1024)}; + + // Make sure a string that exceeds the maximum length of UTF-8 produces an + // error and writes a shorter error string instead + EXPECT_THAT(StringPool::FlattenUtf8(&buffers2[0], pool2, &diag), Eq(false)); + data = util::Copy(buffers2[0]); + test.setTo(data.get(), buffers2[0].size()); + EXPECT_THAT(util::GetString(test, 0), "this fits1"); + EXPECT_THAT(util::GetString(test, 1), "STRING_TOO_LARGE"); + EXPECT_THAT(util::GetString(test, 2), "this fits2"); + + // Make sure a string that a string that exceeds the maximum length of UTF-8 + // but not UTF-16 does not error for UTF-16 + StringPool pool3; + std::u16string longStr16(50000, 'a'); + pool3.MakeRef(longStr); + EXPECT_THAT(StringPool::FlattenUtf16(&buffers2[1], pool3, &diag), Eq(true)); + data = util::Copy(buffers2[1]); + test.setTo(data.get(), buffers2[1].size()); + EXPECT_THAT(util::GetString16(test, 0), Eq(longStr16)); +} + } // namespace aapt diff --git a/tools/aapt2/cmd/Compile.cpp b/tools/aapt2/cmd/Compile.cpp index 101f74eafaf4..2d83a14e541d 100644 --- a/tools/aapt2/cmd/Compile.cpp +++ b/tools/aapt2/cmd/Compile.cpp @@ -258,7 +258,7 @@ static bool CompileTable(IAaptContext* context, const CompileOptions& options, ContainerWriter container_writer(©ing_adaptor, 1u); pb::ResourceTable pb_table; - SerializeTableToPb(table, &pb_table); + SerializeTableToPb(table, &pb_table, context->GetDiagnostics()); if (!container_writer.AddResTableEntry(pb_table)) { context->GetDiagnostics()->Error(DiagMessage(output_path) << "failed to write"); return false; diff --git a/tools/aapt2/cmd/Convert.cpp b/tools/aapt2/cmd/Convert.cpp index 7f956c525bed..eb307fb1ddce 100644 --- a/tools/aapt2/cmd/Convert.cpp +++ b/tools/aapt2/cmd/Convert.cpp @@ -226,7 +226,7 @@ class ProtoApkSerializer : public IApkSerializer { bool SerializeTable(ResourceTable* table, IArchiveWriter* writer) override { pb::ResourceTable pb_table; - SerializeTableToPb(*table, &pb_table); + SerializeTableToPb(*table, &pb_table, context_->GetDiagnostics()); return io::CopyProtoToArchive(context_, &pb_table, kProtoResourceTablePath, ArchiveEntry::kCompress, writer); } diff --git a/tools/aapt2/cmd/Link.cpp b/tools/aapt2/cmd/Link.cpp index 0839f6ff87ad..818ad2089d8a 100644 --- a/tools/aapt2/cmd/Link.cpp +++ b/tools/aapt2/cmd/Link.cpp @@ -1073,7 +1073,7 @@ class LinkCommand { case OutputFormat::kProto: { pb::ResourceTable pb_table; - SerializeTableToPb(*table, &pb_table); + SerializeTableToPb(*table, &pb_table, context_->GetDiagnostics()); return io::CopyProtoToArchive(context_, &pb_table, kProtoResourceTablePath, ArchiveEntry::kCompress, writer); } break; diff --git a/tools/aapt2/format/binary/TableFlattener.cpp b/tools/aapt2/format/binary/TableFlattener.cpp index 24a4112e181d..6fd4c8d2c135 100644 --- a/tools/aapt2/format/binary/TableFlattener.cpp +++ b/tools/aapt2/format/binary/TableFlattener.cpp @@ -255,10 +255,10 @@ class PackageFlattener { FlattenTypes(&type_buffer); pkg_header->typeStrings = util::HostToDevice32(pkg_writer.size()); - StringPool::FlattenUtf16(pkg_writer.buffer(), type_pool_); + StringPool::FlattenUtf16(pkg_writer.buffer(), type_pool_, diag_); pkg_header->keyStrings = util::HostToDevice32(pkg_writer.size()); - StringPool::FlattenUtf8(pkg_writer.buffer(), key_pool_); + StringPool::FlattenUtf8(pkg_writer.buffer(), key_pool_, diag_); // Append the types. buffer->AppendBuffer(std::move(type_buffer)); @@ -590,7 +590,8 @@ bool TableFlattener::Consume(IAaptContext* context, ResourceTable* table) { table_header->packageCount = util::HostToDevice32(table->packages.size()); // Flatten the values string pool. - StringPool::FlattenUtf8(table_writer.buffer(), table->string_pool); + StringPool::FlattenUtf8(table_writer.buffer(), table->string_pool, + context->GetDiagnostics()); BigBuffer package_buffer(1024); diff --git a/tools/aapt2/format/binary/XmlFlattener.cpp b/tools/aapt2/format/binary/XmlFlattener.cpp index 781b9fe8bc29..d897941da6c4 100644 --- a/tools/aapt2/format/binary/XmlFlattener.cpp +++ b/tools/aapt2/format/binary/XmlFlattener.cpp @@ -333,9 +333,9 @@ bool XmlFlattener::Flatten(IAaptContext* context, const xml::Node* node) { // Flatten the StringPool. if (options_.use_utf16) { - StringPool::FlattenUtf16(buffer_, visitor.pool); + StringPool::FlattenUtf16(buffer_, visitor.pool, context->GetDiagnostics()); } else { - StringPool::FlattenUtf8(buffer_, visitor.pool); + StringPool::FlattenUtf8(buffer_, visitor.pool, context->GetDiagnostics()); } { diff --git a/tools/aapt2/format/proto/ProtoSerialize.cpp b/tools/aapt2/format/proto/ProtoSerialize.cpp index 1d00852eab73..2e5635956a0d 100644 --- a/tools/aapt2/format/proto/ProtoSerialize.cpp +++ b/tools/aapt2/format/proto/ProtoSerialize.cpp @@ -21,9 +21,9 @@ namespace aapt { -void SerializeStringPoolToPb(const StringPool& pool, pb::StringPool* out_pb_pool) { +void SerializeStringPoolToPb(const StringPool& pool, pb::StringPool* out_pb_pool, IDiagnostics* diag) { BigBuffer buffer(1024); - StringPool::FlattenUtf8(&buffer, pool); + StringPool::FlattenUtf8(&buffer, pool, diag); std::string* data = out_pb_pool->mutable_data(); data->reserve(buffer.size()); @@ -270,7 +270,8 @@ void SerializeConfig(const ConfigDescription& config, pb::Configuration* out_pb_ out_pb_config->set_sdk_version(config.sdkVersion); } -void SerializeTableToPb(const ResourceTable& table, pb::ResourceTable* out_table) { +void SerializeTableToPb(const ResourceTable& table, pb::ResourceTable* out_table, + IDiagnostics* diag) { StringPool source_pool; for (const std::unique_ptr& package : table.packages) { pb::Package* pb_package = out_table->add_package(); @@ -323,7 +324,7 @@ void SerializeTableToPb(const ResourceTable& table, pb::ResourceTable* out_table } } } - SerializeStringPoolToPb(source_pool, out_table->mutable_source_pool()); + SerializeStringPoolToPb(source_pool, out_table->mutable_source_pool(), diag); } static pb::Reference_Type SerializeReferenceTypeToPb(Reference::Type type) { diff --git a/tools/aapt2/format/proto/ProtoSerialize.h b/tools/aapt2/format/proto/ProtoSerialize.h index 95dd413c1713..951494ca8a0d 100644 --- a/tools/aapt2/format/proto/ProtoSerialize.h +++ b/tools/aapt2/format/proto/ProtoSerialize.h @@ -46,13 +46,13 @@ void SerializeXmlResourceToPb(const xml::XmlResource& resource, pb::XmlNode* out // Serializes a StringPool into its protobuf representation, which is really just the binary // ResStringPool representation stuffed into a bytes field. -void SerializeStringPoolToPb(const StringPool& pool, pb::StringPool* out_pb_pool); +void SerializeStringPoolToPb(const StringPool& pool, pb::StringPool* out_pb_pool, IDiagnostics* diag); // Serializes a ConfigDescription into its protobuf representation. void SerializeConfig(const ConfigDescription& config, pb::Configuration* out_pb_config); // Serializes a ResourceTable into its protobuf representation. -void SerializeTableToPb(const ResourceTable& table, pb::ResourceTable* out_table); +void SerializeTableToPb(const ResourceTable& table, pb::ResourceTable* out_table, IDiagnostics* diag); // Serializes a ResourceFile into its protobuf representation. void SerializeCompiledFileToPb(const ResourceFile& file, pb::internal::CompiledFile* out_file); diff --git a/tools/aapt2/format/proto/ProtoSerialize_test.cpp b/tools/aapt2/format/proto/ProtoSerialize_test.cpp index 9081ab6abd0a..6366a3dd4e68 100644 --- a/tools/aapt2/format/proto/ProtoSerialize_test.cpp +++ b/tools/aapt2/format/proto/ProtoSerialize_test.cpp @@ -95,7 +95,7 @@ TEST(ProtoSerializeTest, SerializeSinglePackage) { Overlayable{}, test::GetDiagnostics())); pb::ResourceTable pb_table; - SerializeTableToPb(*table, &pb_table); + SerializeTableToPb(*table, &pb_table, context->GetDiagnostics()); test::TestFile file_a("res/layout/main.xml"); MockFileCollection files; @@ -255,6 +255,7 @@ TEST(ProtoSerializeTest, SerializeAndDeserializeXml) { } TEST(ProtoSerializeTest, SerializeAndDeserializePrimitives) { + std::unique_ptr context = test::ContextBuilder().Build(); std::unique_ptr table = test::ResourceTableBuilder() .AddValue("android:bool/boolean_true", @@ -274,7 +275,7 @@ TEST(ProtoSerializeTest, SerializeAndDeserializePrimitives) { .Build(); pb::ResourceTable pb_table; - SerializeTableToPb(*table, &pb_table); + SerializeTableToPb(*table, &pb_table, context->GetDiagnostics()); test::TestFile file_a("res/layout/main.xml"); MockFileCollection files; -- cgit v1.2.3-59-g8ed1b From 35ecb89a8aa68f24d2e991df5bb9964ad15075dc Mon Sep 17 00:00:00 2001 From: y Date: Fri, 13 Apr 2018 11:25:12 -0700 Subject: AAPT: Modified StringPool uniqueness detection b/77862560 detected that when converting an apk to binary using aapt2, all resource ids of attributes that have been replaced with resource identifiers become set to the identifier of the first attribute. This is because the attribute names are all empty because the names are not necessary since the resource ids are present. The empty attribute names all map to the same string pool reference and cause all the ids to be the first empty string into the string pool. Bug: 77862560 Test: Converted apk in listed bug from proto to binary and observed correct resource ids and correct badging. Change-Id: I635c13cd1ad7a395fe40a57198cfe5ec91602d01 --- tools/aapt2/StringPool.cpp | 7 ++++--- tools/aapt2/StringPool_test.cpp | 12 +++++++++++- 2 files changed, 15 insertions(+), 4 deletions(-) (limited to 'tools/aapt2/StringPool.cpp') diff --git a/tools/aapt2/StringPool.cpp b/tools/aapt2/StringPool.cpp index b0ce9e1ec947..73a8259a3a87 100644 --- a/tools/aapt2/StringPool.cpp +++ b/tools/aapt2/StringPool.cpp @@ -172,9 +172,10 @@ StringPool::Ref StringPool::MakeRef(const StringPiece& str, const Context& conte StringPool::Ref StringPool::MakeRefImpl(const StringPiece& str, const Context& context, bool unique) { if (unique) { - auto iter = indexed_strings_.find(str); - if (iter != std::end(indexed_strings_)) { - return Ref(iter->second); + for (auto& indexed_str : indexed_strings_) { + if (str == indexed_str.first && context.priority == indexed_str.second->context.priority) { + return Ref(indexed_str.second); + } } } diff --git a/tools/aapt2/StringPool_test.cpp b/tools/aapt2/StringPool_test.cpp index 58a03de60f93..5f7d3d6b1d03 100644 --- a/tools/aapt2/StringPool_test.cpp +++ b/tools/aapt2/StringPool_test.cpp @@ -61,6 +61,17 @@ TEST(StringPoolTest, DoNotInsertNewDuplicateString) { EXPECT_THAT(pool.size(), Eq(1u)); } +TEST(StringPoolTest, DoNotDedupeSameStringDifferentPriority) { + StringPool pool; + + StringPool::Ref ref_a = pool.MakeRef("wut", StringPool::Context(1)); + StringPool::Ref ref_b = pool.MakeRef("wut", StringPool::Context(2)); + + EXPECT_THAT(*ref_a, Eq("wut")); + EXPECT_THAT(*ref_b, Eq("wut")); + EXPECT_THAT(pool.size(), Eq(2u)); +} + TEST(StringPoolTest, MaintainInsertionOrderIndex) { StringPool pool; @@ -292,7 +303,6 @@ TEST(StringPoolTest, Flatten) { } } - TEST(StringPoolTest, MaxEncodingLength) { StdErrDiagnostics diag; using namespace android; // For NO_ERROR on Windows. -- cgit v1.2.3-59-g8ed1b From 61ffd4029032862c871c98ac04f12d4141d59383 Mon Sep 17 00:00:00 2001 From: Ryan Mitchell Date: Mon, 16 Apr 2018 18:21:14 +0000 Subject: Revert "AAPT: Modified StringPool uniqueness detection" This reverts commit 35ecb89a8aa68f24d2e991df5bb9964ad15075dc. Reason for revert: Change-Id: I10d1cf53ca3054d40e23b06368ebaff6af69beab --- tools/aapt2/StringPool.cpp | 7 +++---- tools/aapt2/StringPool_test.cpp | 12 +----------- 2 files changed, 4 insertions(+), 15 deletions(-) (limited to 'tools/aapt2/StringPool.cpp') diff --git a/tools/aapt2/StringPool.cpp b/tools/aapt2/StringPool.cpp index 73a8259a3a87..b0ce9e1ec947 100644 --- a/tools/aapt2/StringPool.cpp +++ b/tools/aapt2/StringPool.cpp @@ -172,10 +172,9 @@ StringPool::Ref StringPool::MakeRef(const StringPiece& str, const Context& conte StringPool::Ref StringPool::MakeRefImpl(const StringPiece& str, const Context& context, bool unique) { if (unique) { - for (auto& indexed_str : indexed_strings_) { - if (str == indexed_str.first && context.priority == indexed_str.second->context.priority) { - return Ref(indexed_str.second); - } + auto iter = indexed_strings_.find(str); + if (iter != std::end(indexed_strings_)) { + return Ref(iter->second); } } diff --git a/tools/aapt2/StringPool_test.cpp b/tools/aapt2/StringPool_test.cpp index 5f7d3d6b1d03..58a03de60f93 100644 --- a/tools/aapt2/StringPool_test.cpp +++ b/tools/aapt2/StringPool_test.cpp @@ -61,17 +61,6 @@ TEST(StringPoolTest, DoNotInsertNewDuplicateString) { EXPECT_THAT(pool.size(), Eq(1u)); } -TEST(StringPoolTest, DoNotDedupeSameStringDifferentPriority) { - StringPool pool; - - StringPool::Ref ref_a = pool.MakeRef("wut", StringPool::Context(1)); - StringPool::Ref ref_b = pool.MakeRef("wut", StringPool::Context(2)); - - EXPECT_THAT(*ref_a, Eq("wut")); - EXPECT_THAT(*ref_b, Eq("wut")); - EXPECT_THAT(pool.size(), Eq(2u)); -} - TEST(StringPoolTest, MaintainInsertionOrderIndex) { StringPool pool; @@ -303,6 +292,7 @@ TEST(StringPoolTest, Flatten) { } } + TEST(StringPoolTest, MaxEncodingLength) { StdErrDiagnostics diag; using namespace android; // For NO_ERROR on Windows. -- cgit v1.2.3-59-g8ed1b From 4602926f83d7aa3b52b190122955b5b0d6d8089d Mon Sep 17 00:00:00 2001 From: y Date: Mon, 16 Apr 2018 18:13:14 -0700 Subject: AAPT2: Modified StringPool uniqueness detection #2 b/77862560 detected that when converting an apk to binary using aapt2, all resource ids of attributes that have been replaced with resource identifiers become set to the identifier of the first attribute. This is because the attribute names are all empty because the names are not necessary since the resource ids are present. The empty attribute names all map to the same string pool reference and cause all the ids to be the first empty string into the string pool. The ag/3897499 approach to fix the specified bug was extremely inefficient and caused hour long builds. This change takes advantage of the multimap data structure to do lookups efficiently. Bug: 77862560 Test: Converted apk in listed bug from proto to binary and observed correct resource ids and correct badging. Also built the Android tree to check for regressions in build time. Change-Id: I27a9ee4ffbed8b9ff6f238ad315cdf87b588947c --- tools/aapt2/StringPool.cpp | 8 +++++--- tools/aapt2/StringPool_test.cpp | 11 +++++++++++ 2 files changed, 16 insertions(+), 3 deletions(-) (limited to 'tools/aapt2/StringPool.cpp') diff --git a/tools/aapt2/StringPool.cpp b/tools/aapt2/StringPool.cpp index b0ce9e1ec947..b37e1fbd9693 100644 --- a/tools/aapt2/StringPool.cpp +++ b/tools/aapt2/StringPool.cpp @@ -172,9 +172,11 @@ StringPool::Ref StringPool::MakeRef(const StringPiece& str, const Context& conte StringPool::Ref StringPool::MakeRefImpl(const StringPiece& str, const Context& context, bool unique) { if (unique) { - auto iter = indexed_strings_.find(str); - if (iter != std::end(indexed_strings_)) { - return Ref(iter->second); + auto range = indexed_strings_.equal_range(str); + for (auto iter = range.first; iter != range.second; ++iter) { + if (context.priority == iter->second->context.priority) { + return Ref(iter->second); + } } } diff --git a/tools/aapt2/StringPool_test.cpp b/tools/aapt2/StringPool_test.cpp index 58a03de60f93..4b3afe2bb962 100644 --- a/tools/aapt2/StringPool_test.cpp +++ b/tools/aapt2/StringPool_test.cpp @@ -61,6 +61,17 @@ TEST(StringPoolTest, DoNotInsertNewDuplicateString) { EXPECT_THAT(pool.size(), Eq(1u)); } +TEST(StringPoolTest, DoNotDedupeSameStringDifferentPriority) { + StringPool pool; + + StringPool::Ref ref_a = pool.MakeRef("wut", StringPool::Context(0x81010001)); + StringPool::Ref ref_b = pool.MakeRef("wut", StringPool::Context(0x81010002)); + + EXPECT_THAT(*ref_a, Eq("wut")); + EXPECT_THAT(*ref_b, Eq("wut")); + EXPECT_THAT(pool.size(), Eq(2u)); +} + TEST(StringPoolTest, MaintainInsertionOrderIndex) { StringPool pool; -- cgit v1.2.3-59-g8ed1b