From 4cb4624f9f7b8e37229f197cc093a1a0a174663c Mon Sep 17 00:00:00 2001 From: Brandon Liu Date: Thu, 16 Feb 2023 02:55:56 +0000 Subject: [PATCH] Fail framework build when staged type id overlap public type id Added more elegant error message to fail the build when staged type id overlap public type id. And also added logic to handle the case when staging type is assigned the same type with same id as public resource. Bug: b/268793902 Test: Added and verified affected atests pass Change-Id: Ic245d703bc34a8f7c2c4c266085a7a09c89918bf --- tools/aapt2/cmd/Link_test.cpp | 12 ++++---- tools/aapt2/compile/IdAssigner.cpp | 39 ++++++++++++++++++++++--- tools/aapt2/compile/IdAssigner_test.cpp | 30 ++++++++++++++----- 3 files changed, 63 insertions(+), 18 deletions(-) diff --git a/tools/aapt2/cmd/Link_test.cpp b/tools/aapt2/cmd/Link_test.cpp index 28fcc1a4800ec..e629bafbdd259 100644 --- a/tools/aapt2/cmd/Link_test.cpp +++ b/tools/aapt2/cmd/Link_test.cpp @@ -441,8 +441,8 @@ static void BuildNonFinalizedSDK(const std::string& apk_path, const std::string& R"( - - + + @@ -480,8 +480,8 @@ static void BuildFinalizedSDK(const std::string& apk_path, const std::string& ja - - + + @@ -551,7 +551,7 @@ TEST_F(LinkTest, StagedAndroidApi) { EXPECT_THAT(android_r_contents, HasSubstr("public static final int finalized_res=0x01010001;")); EXPECT_THAT( android_r_contents, - HasSubstr("public static final int staged_s_res; static { staged_s_res=0x01010050; }")); + HasSubstr("public static final int staged_s_res; static { staged_s_res=0x01fc0050; }")); EXPECT_THAT( android_r_contents, HasSubstr("public static final int staged_s_string; static { staged_s_string=0x01fd0080; }")); @@ -583,7 +583,7 @@ TEST_F(LinkTest, StagedAndroidApi) { result = am.GetResourceId("android:attr/staged_s_res"); ASSERT_TRUE(result.has_value()); - EXPECT_THAT(*result, Eq(0x01010050)); + EXPECT_THAT(*result, Eq(0x01fc0050)); result = am.GetResourceId("android:string/staged_s_string"); ASSERT_TRUE(result.has_value()); diff --git a/tools/aapt2/compile/IdAssigner.cpp b/tools/aapt2/compile/IdAssigner.cpp index b3f98a9d3e300..5421abde3689e 100644 --- a/tools/aapt2/compile/IdAssigner.cpp +++ b/tools/aapt2/compile/IdAssigner.cpp @@ -37,6 +37,7 @@ using Result = expected; template struct NextIdFinder { + std::map pre_assigned_ids_; explicit NextIdFinder(Id start_id = 0u) : next_id_(start_id){}; // Attempts to reserve an identifier for the specified key. @@ -55,7 +56,6 @@ struct NextIdFinder { Id next_id_; bool next_id_called_ = false; bool exhausted_ = false; - std::map pre_assigned_ids_; typename std::map::iterator next_preassigned_id_; }; @@ -158,7 +158,7 @@ bool IdAssigner::Consume(IAaptContext* context, ResourceTable* table) { } if (assigned_id_map_) { - // Reserve all the IDs mentioned in the stable ID map. That way we won't assig IDs that were + // Reserve all the IDs mentioned in the stable ID map. That way we won't assign IDs that were // listed in the map if they don't exist in the table. for (const auto& stable_id_entry : *assigned_id_map_) { const ResourceName& pre_assigned_name = stable_id_entry.first; @@ -191,6 +191,11 @@ bool IdAssigner::Consume(IAaptContext* context, ResourceTable* table) { } namespace { +static const std::string_view staged_type_overlap_error = + "Staged public resource type IDs have conflict with non staged public resources type " + "IDs, please restart staged resource type ID assignment at 0xff in public-staging.xml " + "and also delete all the overlapping groups in public-final.xml"; + template Result NextIdFinder::ReserveId(Key key, Id id) { CHECK(!next_id_called_) << "ReserveId cannot be called after NextId"; @@ -282,8 +287,20 @@ bool IdAssignerContext::ReserveId(const ResourceName& name, ResourceId id, // another type. auto assign_result = type_id_finder_.ReserveId(key, id.type_id()); if (!assign_result.has_value()) { - diag->Error(android::DiagMessage() << "can't assign ID " << id << " to resource " << name - << " because type " << assign_result.error()); + auto pre_assigned_type = type_id_finder_.pre_assigned_ids_[id.type_id()].type; + bool pre_assigned_type_staged = + non_staged_type_ids_.find(pre_assigned_type) == non_staged_type_ids_.end(); + auto hex_type_id = fmt::format("{:#04x}", (int)id.type_id()); + bool current_type_staged = visibility.staged_api; + diag->Error(android::DiagMessage() + << "can't assign type ID " << hex_type_id << " to " + << (current_type_staged ? "staged type " : "non staged type ") << name.type.type + << " because this type ID have been assigned to " + << (pre_assigned_type_staged ? "staged type " : "non staged type ") + << pre_assigned_type); + if (pre_assigned_type_staged || current_type_staged) { + diag->Error(android::DiagMessage() << staged_type_overlap_error); + } return false; } type = types_.emplace(key, TypeGroup(package_id_, id.type_id())).first; @@ -298,6 +315,20 @@ bool IdAssignerContext::ReserveId(const ResourceName& name, ResourceId id, << " because type already has ID " << std::hex << (int)id.type_id()); return false; } + } else { + // Ensure that staged public resources cannot have the same type name and type id with + // non staged public resources. + auto non_staged_type = non_staged_type_ids_.find(name.type.type); + if (non_staged_type != non_staged_type_ids_.end() && non_staged_type->second == id.type_id()) { + diag->Error( + android::DiagMessage() + << "can`t assign type ID " << fmt::format("{:#04x}", (int)id.type_id()) + << " to staged type " << name.type.type << " because type ID " + << fmt::format("{:#04x}", (int)id.type_id()) + << " already has been assigned to a non staged resource type with the same type name"); + diag->Error(android::DiagMessage() << staged_type_overlap_error); + return false; + } } auto assign_result = type->second.ReserveId(name, id); diff --git a/tools/aapt2/compile/IdAssigner_test.cpp b/tools/aapt2/compile/IdAssigner_test.cpp index 8911dad394701..ce45b7c1df048 100644 --- a/tools/aapt2/compile/IdAssigner_test.cpp +++ b/tools/aapt2/compile/IdAssigner_test.cpp @@ -117,14 +117,28 @@ TEST_F(IdAssignerTests, FailWhenTypeHasTwoNonStagedIds) { } TEST_F(IdAssignerTests, FailWhenTypeHasTwoNonStagedIdsRegardlessOfStagedId) { - auto table = test::ResourceTableBuilder() - .AddSimple("android:attr/foo", ResourceId(0x01050000)) - .AddSimple("android:attr/bar", ResourceId(0x01ff0006)) - .Add(NewResourceBuilder("android:attr/staged_baz") - .SetId(0x01ff0000) - .SetVisibility({.staged_api = true}) - .Build()) - .Build(); + auto table = + test::ResourceTableBuilder() + .AddSimple("android:attr/foo", ResourceId(0x01050000)) + .AddSimple("android:attr/bar", ResourceId(0x01ff0006)) + .Add(NewResourceBuilder("android:attr/staged_baz") + .SetId(0x01ff0000) + .SetVisibility({.staged_api = true, .level = Visibility::Level::kPublic}) + .Build()) + .Build(); + IdAssigner assigner; + ASSERT_FALSE(assigner.Consume(context.get(), table.get())); +} + +TEST_F(IdAssignerTests, FailWhenTypeHaveBothStagedAndNonStagedIds) { + auto table = + test::ResourceTableBuilder() + .AddSimple("android:attr/foo", ResourceId(0x01010000)) + .Add(NewResourceBuilder("android:bool/staged_baz") + .SetId(0x01010001) + .SetVisibility({.staged_api = true, .level = Visibility::Level::kPublic}) + .Build()) + .Build(); IdAssigner assigner; ASSERT_FALSE(assigner.Consume(context.get(), table.get())); }