From b1afa07745280c7a36077beda6293b69c6ba4ea2 Mon Sep 17 00:00:00 2001 From: Adam Lesinski Date: Wed, 29 Mar 2017 13:52:38 -0700 Subject: [PATCH] AAPT2: Allow arbitrary entry names with aapt2 optimize Presumably, the apps build fine for the developers, so just feed the existing names through without validation. Validation still exists when building an app from source. Bug: 36051854 Change-Id: Idc64ee91b08dce67d3c28f3c5284a7afa1312df1 Test: run aapt2 optimize on the apks from b/36051854 and build aapt2_tests --- tools/aapt2/ResourceTable.cpp | 164 +++++++++++++---------------- tools/aapt2/ResourceTable.h | 18 ++-- tools/aapt2/ResourceTable_test.cpp | 10 +- 3 files changed, 95 insertions(+), 97 deletions(-) diff --git a/tools/aapt2/ResourceTable.cpp b/tools/aapt2/ResourceTable.cpp index 6e4b450b65e54..1947628e9267d 100644 --- a/tools/aapt2/ResourceTable.cpp +++ b/tools/aapt2/ResourceTable.cpp @@ -32,22 +32,19 @@ using android::StringPiece; namespace aapt { -static bool less_than_type(const std::unique_ptr& lhs, - ResourceType rhs) { +static bool less_than_type(const std::unique_ptr& lhs, ResourceType rhs) { return lhs->type < rhs; } template -static bool less_than_struct_with_name(const std::unique_ptr& lhs, - const StringPiece& rhs) { +static bool less_than_struct_with_name(const std::unique_ptr& lhs, const StringPiece& rhs) { return lhs->name.compare(0, lhs->name.size(), rhs.data(), rhs.size()) < 0; } ResourceTablePackage* ResourceTable::FindPackage(const StringPiece& name) { const auto last = packages.end(); - auto iter = - std::lower_bound(packages.begin(), last, name, - less_than_struct_with_name); + auto iter = std::lower_bound(packages.begin(), last, name, + less_than_struct_with_name); if (iter != last && name == (*iter)->name) { return iter->get(); } @@ -63,8 +60,7 @@ ResourceTablePackage* ResourceTable::FindPackageById(uint8_t id) { return nullptr; } -ResourceTablePackage* ResourceTable::CreatePackage(const StringPiece& name, - Maybe id) { +ResourceTablePackage* ResourceTable::CreatePackage(const StringPiece& name, Maybe id) { ResourceTablePackage* package = FindOrCreatePackage(name); if (id && !package->id) { package->id = id; @@ -77,18 +73,15 @@ ResourceTablePackage* ResourceTable::CreatePackage(const StringPiece& name, return package; } -ResourceTablePackage* ResourceTable::FindOrCreatePackage( - const StringPiece& name) { +ResourceTablePackage* ResourceTable::FindOrCreatePackage(const StringPiece& name) { const auto last = packages.end(); - auto iter = - std::lower_bound(packages.begin(), last, name, - less_than_struct_with_name); + auto iter = std::lower_bound(packages.begin(), last, name, + less_than_struct_with_name); if (iter != last && name == (*iter)->name) { return iter->get(); } - std::unique_ptr new_package = - util::make_unique(); + std::unique_ptr new_package = util::make_unique(); new_package->name = name.to_string(); return packages.emplace(iter, std::move(new_package))->get(); } @@ -113,8 +106,8 @@ ResourceTableType* ResourceTablePackage::FindOrCreateType(ResourceType type) { ResourceEntry* ResourceTableType::FindEntry(const StringPiece& name) { const auto last = entries.end(); - auto iter = std::lower_bound(entries.begin(), last, name, - less_than_struct_with_name); + auto iter = + std::lower_bound(entries.begin(), last, name, less_than_struct_with_name); if (iter != last && name == (*iter)->name) { return iter->get(); } @@ -123,8 +116,8 @@ ResourceEntry* ResourceTableType::FindEntry(const StringPiece& name) { ResourceEntry* ResourceTableType::FindOrCreateEntry(const StringPiece& name) { auto last = entries.end(); - auto iter = std::lower_bound(entries.begin(), last, name, - less_than_struct_with_name); + auto iter = + std::lower_bound(entries.begin(), last, name, less_than_struct_with_name); if (iter != last && name == (*iter)->name) { return iter->get(); } @@ -140,8 +133,7 @@ struct ConfigKey { const StringPiece& product; }; -bool ltConfigKeyRef(const std::unique_ptr& lhs, - const ConfigKey& rhs) { +bool ltConfigKeyRef(const std::unique_ptr& lhs, const ConfigKey& rhs) { int cmp = lhs->config.compare(*rhs.config); if (cmp == 0) { cmp = StringPiece(lhs->product).compare(rhs.product); @@ -151,8 +143,8 @@ bool ltConfigKeyRef(const std::unique_ptr& lhs, ResourceConfigValue* ResourceEntry::FindValue(const ConfigDescription& config, const StringPiece& product) { - auto iter = std::lower_bound(values.begin(), values.end(), - ConfigKey{&config, product}, ltConfigKeyRef); + auto iter = + std::lower_bound(values.begin(), values.end(), ConfigKey{&config, product}, ltConfigKeyRef); if (iter != values.end()) { ResourceConfigValue* value = iter->get(); if (value->config == config && StringPiece(value->product) == product) { @@ -162,10 +154,10 @@ ResourceConfigValue* ResourceEntry::FindValue(const ConfigDescription& config, return nullptr; } -ResourceConfigValue* ResourceEntry::FindOrCreateValue( - const ConfigDescription& config, const StringPiece& product) { - auto iter = std::lower_bound(values.begin(), values.end(), - ConfigKey{&config, product}, ltConfigKeyRef); +ResourceConfigValue* ResourceEntry::FindOrCreateValue(const ConfigDescription& config, + const StringPiece& product) { + auto iter = + std::lower_bound(values.begin(), values.end(), ConfigKey{&config, product}, ltConfigKeyRef); if (iter != values.end()) { ResourceConfigValue* value = iter->get(); if (value->config == config && StringPiece(value->product) == product) { @@ -173,14 +165,11 @@ ResourceConfigValue* ResourceEntry::FindOrCreateValue( } } ResourceConfigValue* newValue = - values - .insert(iter, util::make_unique(config, product)) - ->get(); + values.insert(iter, util::make_unique(config, product))->get(); return newValue; } -std::vector ResourceEntry::findAllValues( - const ConfigDescription& config) { +std::vector ResourceEntry::FindAllValues(const ConfigDescription& config) { std::vector results; auto iter = values.begin(); @@ -237,8 +226,8 @@ std::vector ResourceEntry::FindValuesIf( * format for there to be * no error. */ -ResourceTable::CollisionResult ResourceTable::ResolveValueCollision( - Value* existing, Value* incoming) { +ResourceTable::CollisionResult ResourceTable::ResolveValueCollision(Value* existing, + Value* incoming) { Attribute* existing_attr = ValueCast(existing); Attribute* incoming_attr = ValueCast(incoming); if (!incoming_attr) { @@ -278,18 +267,15 @@ ResourceTable::CollisionResult ResourceTable::ResolveValueCollision( // The two attributes are both DECLs, but they are plain attributes // with the same formats. // Keep the strongest one. - return existing_attr->IsWeak() ? CollisionResult::kTakeNew - : CollisionResult::kKeepOriginal; + return existing_attr->IsWeak() ? CollisionResult::kTakeNew : CollisionResult::kKeepOriginal; } - if (existing_attr->IsWeak() && - existing_attr->type_mask == android::ResTable_map::TYPE_ANY) { + if (existing_attr->IsWeak() && existing_attr->type_mask == android::ResTable_map::TYPE_ANY) { // Any incoming attribute is better than this. return CollisionResult::kTakeNew; } - if (incoming_attr->IsWeak() && - incoming_attr->type_mask == android::ResTable_map::TYPE_ANY) { + if (incoming_attr->IsWeak() && incoming_attr->type_mask == android::ResTable_map::TYPE_ANY) { // The incoming attribute may be a USE instead of a DECL. // Keep the existing attribute. return CollisionResult::kKeepOriginal; @@ -298,15 +284,26 @@ ResourceTable::CollisionResult ResourceTable::ResolveValueCollision( } static constexpr const char* kValidNameChars = "._-"; -static constexpr const char* kValidNameMangledChars = "._-$"; + +static StringPiece ValidateName(const StringPiece& name) { + auto iter = util::FindNonAlphaNumericAndNotInSet(name, kValidNameChars); + if (iter != name.end()) { + return StringPiece(iter, 1); + } + return {}; +} + +static StringPiece SkipValidateName(const StringPiece& /*name*/) { + return {}; +} bool ResourceTable::AddResource(const ResourceNameRef& name, const ConfigDescription& config, const StringPiece& product, std::unique_ptr value, IDiagnostics* diag) { - return AddResourceImpl(name, {}, config, product, std::move(value), - kValidNameChars, ResolveValueCollision, diag); + return AddResourceImpl(name, {}, config, product, std::move(value), ValidateName, + ResolveValueCollision, diag); } bool ResourceTable::AddResource(const ResourceNameRef& name, @@ -315,8 +312,8 @@ bool ResourceTable::AddResource(const ResourceNameRef& name, const StringPiece& product, std::unique_ptr value, IDiagnostics* diag) { - return AddResourceImpl(name, res_id, config, product, std::move(value), - kValidNameChars, ResolveValueCollision, diag); + return AddResourceImpl(name, res_id, config, product, std::move(value), ValidateName, + ResolveValueCollision, diag); } bool ResourceTable::AddFileReference(const ResourceNameRef& name, @@ -324,29 +321,26 @@ bool ResourceTable::AddFileReference(const ResourceNameRef& name, const Source& source, const StringPiece& path, IDiagnostics* diag) { - return AddFileReferenceImpl(name, config, source, path, nullptr, - kValidNameChars, diag); + return AddFileReferenceImpl(name, config, source, path, nullptr, ValidateName, diag); } bool ResourceTable::AddFileReferenceAllowMangled( const ResourceNameRef& name, const ConfigDescription& config, const Source& source, const StringPiece& path, io::IFile* file, IDiagnostics* diag) { - return AddFileReferenceImpl(name, config, source, path, file, - kValidNameMangledChars, diag); + return AddFileReferenceImpl(name, config, source, path, file, SkipValidateName, diag); } -bool ResourceTable::AddFileReferenceImpl( - const ResourceNameRef& name, const ConfigDescription& config, - const Source& source, const StringPiece& path, io::IFile* file, - const char* valid_chars, IDiagnostics* diag) { +bool ResourceTable::AddFileReferenceImpl(const ResourceNameRef& name, + const ConfigDescription& config, const Source& source, + const StringPiece& path, io::IFile* file, + NameValidator name_validator, IDiagnostics* diag) { std::unique_ptr fileRef = util::make_unique(string_pool.MakeRef(path)); fileRef->SetSource(source); fileRef->file = file; - return AddResourceImpl(name, ResourceId{}, config, StringPiece{}, - std::move(fileRef), valid_chars, ResolveValueCollision, - diag); + return AddResourceImpl(name, ResourceId{}, config, StringPiece{}, std::move(fileRef), + name_validator, ResolveValueCollision, diag); } bool ResourceTable::AddResourceAllowMangled(const ResourceNameRef& name, @@ -354,8 +348,8 @@ bool ResourceTable::AddResourceAllowMangled(const ResourceNameRef& name, const StringPiece& product, std::unique_ptr value, IDiagnostics* diag) { - return AddResourceImpl(name, ResourceId{}, config, product, std::move(value), - kValidNameMangledChars, ResolveValueCollision, diag); + return AddResourceImpl(name, ResourceId{}, config, product, std::move(value), SkipValidateName, + ResolveValueCollision, diag); } bool ResourceTable::AddResourceAllowMangled(const ResourceNameRef& name, @@ -364,25 +358,24 @@ bool ResourceTable::AddResourceAllowMangled(const ResourceNameRef& name, const StringPiece& product, std::unique_ptr value, IDiagnostics* diag) { - return AddResourceImpl(name, id, config, product, std::move(value), - kValidNameMangledChars, ResolveValueCollision, diag); + return AddResourceImpl(name, id, config, product, std::move(value), SkipValidateName, + ResolveValueCollision, diag); } -bool ResourceTable::AddResourceImpl( - const ResourceNameRef& name, const ResourceId& res_id, - const ConfigDescription& config, const StringPiece& product, - std::unique_ptr value, const char* valid_chars, - const CollisionResolverFunc& conflictResolver, IDiagnostics* diag) { +bool ResourceTable::AddResourceImpl(const ResourceNameRef& name, const ResourceId& res_id, + const ConfigDescription& config, const StringPiece& product, + std::unique_ptr value, NameValidator name_validator, + const CollisionResolverFunc& conflictResolver, + IDiagnostics* diag) { CHECK(value != nullptr); CHECK(diag != nullptr); - auto bad_char_iter = - util::FindNonAlphaNumericAndNotInSet(name.entry, valid_chars); - if (bad_char_iter != name.entry.end()) { - diag->Error(DiagMessage(value->GetSource()) - << "resource '" << name << "' has invalid entry name '" - << name.entry << "'. Invalid character '" - << StringPiece(bad_char_iter, 1) << "'"); + const StringPiece bad_char = name_validator(name.entry); + if (!bad_char.empty()) { + diag->Error(DiagMessage(value->GetSource()) << "resource '" << name + << "' has invalid entry name '" << name.entry + << "'. Invalid character '" << bad_char << "'"); + return false; } @@ -450,30 +443,26 @@ bool ResourceTable::AddResourceImpl( bool ResourceTable::SetSymbolState(const ResourceNameRef& name, const ResourceId& res_id, const Symbol& symbol, IDiagnostics* diag) { - return SetSymbolStateImpl(name, res_id, symbol, kValidNameChars, diag); + return SetSymbolStateImpl(name, res_id, symbol, ValidateName, diag); } bool ResourceTable::SetSymbolStateAllowMangled(const ResourceNameRef& name, const ResourceId& res_id, const Symbol& symbol, IDiagnostics* diag) { - return SetSymbolStateImpl(name, res_id, symbol, kValidNameMangledChars, diag); + return SetSymbolStateImpl(name, res_id, symbol, SkipValidateName, diag); } -bool ResourceTable::SetSymbolStateImpl(const ResourceNameRef& name, - const ResourceId& res_id, - const Symbol& symbol, - const char* valid_chars, +bool ResourceTable::SetSymbolStateImpl(const ResourceNameRef& name, const ResourceId& res_id, + const Symbol& symbol, NameValidator name_validator, IDiagnostics* diag) { CHECK(diag != nullptr); - auto bad_char_iter = - util::FindNonAlphaNumericAndNotInSet(name.entry, valid_chars); - if (bad_char_iter != name.entry.end()) { - diag->Error(DiagMessage(symbol.source) - << "resource '" << name << "' has invalid entry name '" - << name.entry << "'. Invalid character '" - << StringPiece(bad_char_iter, 1) << "'"); + const StringPiece bad_char = name_validator(name.entry); + if (!bad_char.empty()) { + diag->Error(DiagMessage(symbol.source) << "resource '" << name << "' has invalid entry name '" + << name.entry << "'. Invalid character '" << bad_char + << "'"); return false; } @@ -532,8 +521,7 @@ bool ResourceTable::SetSymbolStateImpl(const ResourceNameRef& name, return true; } -Maybe ResourceTable::FindResource( - const ResourceNameRef& name) { +Maybe ResourceTable::FindResource(const ResourceNameRef& name) { ResourceTablePackage* package = FindPackage(name.package); if (!package) { return {}; diff --git a/tools/aapt2/ResourceTable.h b/tools/aapt2/ResourceTable.h index 6b69aaf02cbe9..b0321214c6cc4 100644 --- a/tools/aapt2/ResourceTable.h +++ b/tools/aapt2/ResourceTable.h @@ -113,8 +113,7 @@ class ResourceEntry { const android::StringPiece& product); ResourceConfigValue* FindOrCreateValue(const ConfigDescription& config, const android::StringPiece& product); - std::vector findAllValues( - const ConfigDescription& config); + std::vector FindAllValues(const ConfigDescription& config); std::vector FindValuesIf( const std::function& f); @@ -189,8 +188,7 @@ class ResourceTable { * When a collision of resources occurs, this method decides which value to * keep. */ - static CollisionResult ResolveValueCollision(Value* existing, - Value* incoming); + static CollisionResult ResolveValueCollision(Value* existing, Value* incoming); bool AddResource(const ResourceNameRef& name, const ConfigDescription& config, const android::StringPiece& product, std::unique_ptr value, @@ -274,20 +272,24 @@ class ResourceTable { std::map included_packages_; private: + // The function type that validates a symbol name. Returns a non-empty StringPiece representing + // the offending character (which may be more than one byte in UTF-8). Returns an empty string + // if the name was valid. + using NameValidator = android::StringPiece(const android::StringPiece&); + ResourceTablePackage* FindOrCreatePackage(const android::StringPiece& name); bool AddResourceImpl(const ResourceNameRef& name, const ResourceId& res_id, const ConfigDescription& config, const android::StringPiece& product, - std::unique_ptr value, const char* valid_chars, + std::unique_ptr value, NameValidator name_validator, const CollisionResolverFunc& conflict_resolver, IDiagnostics* diag); bool AddFileReferenceImpl(const ResourceNameRef& name, const ConfigDescription& config, const Source& source, const android::StringPiece& path, io::IFile* file, - const char* valid_chars, IDiagnostics* diag); + NameValidator name_validator, IDiagnostics* diag); bool SetSymbolStateImpl(const ResourceNameRef& name, const ResourceId& res_id, - const Symbol& symbol, const char* valid_chars, - IDiagnostics* diag); + const Symbol& symbol, NameValidator name_validator, IDiagnostics* diag); DISALLOW_COPY_AND_ASSIGN(ResourceTable); }; diff --git a/tools/aapt2/ResourceTable_test.cpp b/tools/aapt2/ResourceTable_test.cpp index cb3699a00f755..e2b37be994ffb 100644 --- a/tools/aapt2/ResourceTable_test.cpp +++ b/tools/aapt2/ResourceTable_test.cpp @@ -40,6 +40,14 @@ TEST(ResourceTableTest, FailToAddResourceWithBadName) { test::GetDiagnostics())); } +TEST(ResourceTableTest, AddResourceWithWeirdNameWhenAddingMangledResources) { + ResourceTable table; + + EXPECT_TRUE(table.AddResourceAllowMangled( + test::ParseNameOrDie("android:id/heythere "), ConfigDescription{}, "", + test::ValueBuilder().SetSource("test.xml", 21u).Build(), test::GetDiagnostics())); +} + TEST(ResourceTableTest, AddOneResource) { ResourceTable table; @@ -130,7 +138,7 @@ TEST(ResourceTableTest, ProductVaryingValues) { table.FindResource(test::ParseNameOrDie("android:string/foo")); AAPT_ASSERT_TRUE(sr); std::vector values = - sr.value().entry->findAllValues(test::ParseConfigOrDie("land")); + sr.value().entry->FindAllValues(test::ParseConfigOrDie("land")); ASSERT_EQ(2u, values.size()); EXPECT_EQ(std::string("phone"), values[0]->product); EXPECT_EQ(std::string("tablet"), values[1]->product);