From d51efe818b92ef83eb0539e63fb9d22b37e53696 Mon Sep 17 00:00:00 2001 From: Izabela Orlowska Date: Tue, 24 Apr 2018 18:18:29 +0100 Subject: [PATCH] AAPT2: add flag for strict visibility Will only detect whether a resource was defined as both 'public' and 'private' (but will allow overriding 'undefined' visiblity for now). Test: TableMerger_test + manual Bug: 72735798 Change-Id: If0749559c91c4d8820a6286fc9ddc80209c1e5e9 --- tools/aapt2/cmd/Link.cpp | 9 +++++++- tools/aapt2/link/TableMerger.cpp | 14 ++++++++++-- tools/aapt2/link/TableMerger.h | 2 ++ tools/aapt2/link/TableMerger_test.cpp | 31 +++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 3 deletions(-) diff --git a/tools/aapt2/cmd/Link.cpp b/tools/aapt2/cmd/Link.cpp index 75eba9e7f5daa..74edf6e803643 100644 --- a/tools/aapt2/cmd/Link.cpp +++ b/tools/aapt2/cmd/Link.cpp @@ -137,6 +137,9 @@ struct LinkOptions { // In order to work around this limitation, we allow the use of traditionally reserved // resource IDs [those between 0x02 and 0x7E]. bool allow_reserved_package_id = false; + + // Whether we should fail on definitions of a resource with conflicting visibility. + bool strict_visibility = false; }; class LinkContext : public IAaptContext { @@ -1713,6 +1716,7 @@ class LinkCommand { TableMergerOptions table_merger_options; table_merger_options.auto_add_overlay = options_.auto_add_overlay; + table_merger_options.strict_visibility = options_.strict_visibility; table_merger_ = util::make_unique(context_, &final_table_, table_merger_options); if (context_->IsVerbose()) { @@ -2209,7 +2213,10 @@ int Link(const std::vector& args, IDiagnostics* diagnostics) { .OptionalSwitch("--debug-mode", "Inserts android:debuggable=\"true\" in to the application node of the\n" "manifest, making the application debuggable even on production devices.", - &options.manifest_fixer_options.debug_mode); + &options.manifest_fixer_options.debug_mode) + .OptionalSwitch("--strict-visibility", + "Do not allow overlays with different visibility levels.", + &options.strict_visibility); if (!flags.Parse("aapt2 link", args, &std::cerr)) { return 1; diff --git a/tools/aapt2/link/TableMerger.cpp b/tools/aapt2/link/TableMerger.cpp index e819f51a5634a..91a55b3370710 100644 --- a/tools/aapt2/link/TableMerger.cpp +++ b/tools/aapt2/link/TableMerger.cpp @@ -102,7 +102,17 @@ static bool MergeType(IAaptContext* context, const Source& src, ResourceTableTyp } static bool MergeEntry(IAaptContext* context, const Source& src, bool overlay, - ResourceEntry* dst_entry, ResourceEntry* src_entry) { + ResourceEntry* dst_entry, ResourceEntry* src_entry, + bool strict_visibility) { + if (strict_visibility + && dst_entry->visibility.level != Visibility::Level::kUndefined + && src_entry->visibility.level != dst_entry->visibility.level) { + context->GetDiagnostics()->Error( + DiagMessage(src) << "cannot merge resource '" << dst_entry->name << "' with conflicting visibilities: " + << "public and private"); + return false; + } + // Copy over the strongest visibility. if (src_entry->visibility.level > dst_entry->visibility.level) { // Only copy the ID if the source is public, or else the ID is meaningless. @@ -234,7 +244,7 @@ bool TableMerger::DoMerge(const Source& src, ResourceTable* src_table, continue; } - if (!MergeEntry(context_, src, overlay, dst_entry, src_entry.get())) { + if (!MergeEntry(context_, src, overlay, dst_entry, src_entry.get(), options_.strict_visibility)) { error = true; continue; } diff --git a/tools/aapt2/link/TableMerger.h b/tools/aapt2/link/TableMerger.h index 47e23dded26f7..24c5e13292441 100644 --- a/tools/aapt2/link/TableMerger.h +++ b/tools/aapt2/link/TableMerger.h @@ -35,6 +35,8 @@ namespace aapt { struct TableMergerOptions { // If true, resources in overlays can be added without previously having existed. bool auto_add_overlay = false; + // If true, resource overlays with conflicting visibility are not allowed. + bool strict_visibility = false; }; // TableMerger takes resource tables and merges all packages within the tables that have the same diff --git a/tools/aapt2/link/TableMerger_test.cpp b/tools/aapt2/link/TableMerger_test.cpp index 34461c6b467d2..cf504c4f4ce51 100644 --- a/tools/aapt2/link/TableMerger_test.cpp +++ b/tools/aapt2/link/TableMerger_test.cpp @@ -241,6 +241,37 @@ TEST_F(TableMergerTest, FailToOverrideConflictingEntryIdsWithOverlay) { ASSERT_FALSE(merger.Merge({}, overlay.get(), true /*overlay*/)); } +TEST_F(TableMergerTest, FailConflictingVisibility) { + std::unique_ptr base = + test::ResourceTableBuilder() + .SetPackageId("", 0x7f) + .SetSymbolState("bool/foo", ResourceId(0x7f, 0x01, 0x0001), Visibility::Level::kPublic) + .Build(); + std::unique_ptr overlay = + test::ResourceTableBuilder() + .SetPackageId("", 0x7f) + .SetSymbolState("bool/foo", ResourceId(0x7f, 0x01, 0x0001), Visibility::Level::kPrivate) + .Build(); + + // It should fail if the "--strict-visibility" flag is set. + ResourceTable final_table; + TableMergerOptions options; + options.auto_add_overlay = false; + options.strict_visibility = true; + TableMerger merger(context_.get(), &final_table, options); + + ASSERT_TRUE(merger.Merge({}, base.get(), false /*overlay*/)); + ASSERT_FALSE(merger.Merge({}, overlay.get(), true /*overlay*/)); + + // But it should still pass if the flag is not set. + ResourceTable final_table2; + options.strict_visibility = false; + TableMerger merger2(context_.get(), &final_table2, options); + + ASSERT_TRUE(merger2.Merge({}, base.get(), false /*overlay*/)); + ASSERT_TRUE(merger2.Merge({}, overlay.get(), true /*overlay*/)); +} + TEST_F(TableMergerTest, MergeAddResourceFromOverlay) { std::unique_ptr table_a = test::ResourceTableBuilder().SetPackageId("", 0x7f).Build();