Improve how we exempt resources from getting their names collapsed.
Removed the --whitelist-path flag, which is poorly named and we can
already specify these resources with --resources-config-path.
Renamed TableFlattenerOptions.whitelisted_resources to keep_resources.
It holds ResourceName instead of std::string. This lets us include type
when specifying what gets exempted, for correctness.
Bug: 111115201
Test: make aapt2_tests
Change-Id: Ifa5df924b5e2265c32cdcf8ca7dfa4a3992a0468
diff --git a/tools/aapt2/cmd/Optimize.cpp b/tools/aapt2/cmd/Optimize.cpp
index 5e06818..e36668e 100644
--- a/tools/aapt2/cmd/Optimize.cpp
+++ b/tools/aapt2/cmd/Optimize.cpp
@@ -53,9 +53,9 @@
using ::android::ResTable_config;
using ::android::StringPiece;
using ::android::base::ReadFileToString;
-using ::android::base::WriteStringToFile;
using ::android::base::StringAppendF;
using ::android::base::StringPrintf;
+using ::android::base::WriteStringToFile;
namespace aapt {
@@ -300,29 +300,7 @@
OptimizeContext* context_;
};
-bool ExtractObfuscationWhitelistFromConfig(const std::string& path, OptimizeContext* context,
- OptimizeOptions* options) {
- std::string contents;
- if (!ReadFileToString(path, &contents, true)) {
- context->GetDiagnostics()->Error(DiagMessage()
- << "failed to parse whitelist from config file: " << path);
- return false;
- }
- for (StringPiece resource_name : util::Tokenize(contents, ',')) {
- options->table_flattener_options.whitelisted_resources.insert(
- resource_name.to_string());
- }
- return true;
-}
-
-bool ExtractConfig(const std::string& path, OptimizeContext* context,
- OptimizeOptions* options) {
- std::string content;
- if (!android::base::ReadFileToString(path, &content, true /*follow_symlinks*/)) {
- context->GetDiagnostics()->Error(DiagMessage(path) << "failed reading whitelist");
- return false;
- }
-
+bool ParseConfig(const std::string& content, IAaptContext* context, OptimizeOptions* options) {
size_t line_no = 0;
for (StringPiece line : util::Tokenize(content, '\n')) {
line_no++;
@@ -351,15 +329,24 @@
for (StringPiece directive : util::Tokenize(directives, ',')) {
if (directive == "remove") {
options->resources_blacklist.insert(resource_name.ToResourceName());
- } else if (directive == "no_obfuscate") {
- options->table_flattener_options.whitelisted_resources.insert(
- resource_name.entry.to_string());
+ } else if (directive == "no_collapse" || directive == "no_obfuscate") {
+ options->table_flattener_options.name_collapse_exemptions.insert(
+ resource_name.ToResourceName());
}
}
}
return true;
}
+bool ExtractConfig(const std::string& path, IAaptContext* context, OptimizeOptions* options) {
+ std::string content;
+ if (!android::base::ReadFileToString(path, &content, true /*follow_symlinks*/)) {
+ context->GetDiagnostics()->Error(DiagMessage(path) << "failed reading config file");
+ return false;
+ }
+ return ParseConfig(content, context, options);
+}
+
bool ExtractAppDataFromManifest(OptimizeContext* context, const LoadedApk* apk,
OptimizeOptions* out_options) {
const xml::XmlResource* manifest = apk->GetManifest();
@@ -467,15 +454,6 @@
}
}
- if (options_.table_flattener_options.collapse_key_stringpool) {
- if (whitelist_path_) {
- std::string& path = whitelist_path_.value();
- if (!ExtractObfuscationWhitelistFromConfig(path, &context, &options_)) {
- return 1;
- }
- }
- }
-
if (resources_config_path_) {
std::string& path = resources_config_path_.value();
if (!ExtractConfig(path, &context, &options_)) {
diff --git a/tools/aapt2/cmd/Optimize.h b/tools/aapt2/cmd/Optimize.h
index 0be7dad..1bb6b82 100644
--- a/tools/aapt2/cmd/Optimize.h
+++ b/tools/aapt2/cmd/Optimize.h
@@ -78,10 +78,6 @@
"All the resources that would be unused on devices of the given densities will be \n"
"removed from the APK.",
&target_densities_);
- AddOptionalFlag("--whitelist-path",
- "Path to the whitelist.cfg file containing whitelisted resources \n"
- "whose names should not be altered in final resource tables.",
- &whitelist_path_);
AddOptionalFlag("--resources-config-path",
"Path to the resources.cfg file containing the list of resources and \n"
"directives to each resource. \n"
@@ -125,7 +121,6 @@
const std::string &file_path);
Maybe<std::string> config_path_;
- Maybe<std::string> whitelist_path_;
Maybe<std::string> resources_config_path_;
Maybe<std::string> target_densities_;
std::vector<std::string> configs_;
diff --git a/tools/aapt2/cmd/Optimize_test.cpp b/tools/aapt2/cmd/Optimize_test.cpp
new file mode 100644
index 0000000..ac681e8
--- /dev/null
+++ b/tools/aapt2/cmd/Optimize_test.cpp
@@ -0,0 +1,68 @@
+/*
+ * Copyright (C) 2019 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "Optimize.h"
+
+#include "AppInfo.h"
+#include "Diagnostics.h"
+#include "LoadedApk.h"
+#include "Resource.h"
+#include "test/Test.h"
+
+using testing::Contains;
+using testing::Eq;
+
+namespace aapt {
+
+bool ParseConfig(const std::string&, IAaptContext*, OptimizeOptions*);
+
+using OptimizeTest = CommandTestFixture;
+
+TEST_F(OptimizeTest, ParseConfigWithNoCollapseExemptions) {
+ const std::string& content = R"(
+string/foo#no_collapse
+dimen/bar#no_collapse
+)";
+ aapt::test::Context context;
+ OptimizeOptions options;
+ ParseConfig(content, &context, &options);
+
+ const std::set<ResourceName>& name_collapse_exemptions =
+ options.table_flattener_options.name_collapse_exemptions;
+
+ ASSERT_THAT(name_collapse_exemptions.size(), Eq(2));
+ EXPECT_THAT(name_collapse_exemptions, Contains(ResourceName({}, ResourceType::kString, "foo")));
+ EXPECT_THAT(name_collapse_exemptions, Contains(ResourceName({}, ResourceType::kDimen, "bar")));
+}
+
+TEST_F(OptimizeTest, ParseConfigWithNoObfuscateExemptions) {
+ const std::string& content = R"(
+string/foo#no_obfuscate
+dimen/bar#no_obfuscate
+)";
+ aapt::test::Context context;
+ OptimizeOptions options;
+ ParseConfig(content, &context, &options);
+
+ const std::set<ResourceName>& name_collapse_exemptions =
+ options.table_flattener_options.name_collapse_exemptions;
+
+ ASSERT_THAT(name_collapse_exemptions.size(), Eq(2));
+ EXPECT_THAT(name_collapse_exemptions, Contains(ResourceName({}, ResourceType::kString, "foo")));
+ EXPECT_THAT(name_collapse_exemptions, Contains(ResourceName({}, ResourceType::kDimen, "bar")));
+}
+
+} // namespace aapt
diff --git a/tools/aapt2/format/binary/TableFlattener.cpp b/tools/aapt2/format/binary/TableFlattener.cpp
index b932117..58e232c 100644
--- a/tools/aapt2/format/binary/TableFlattener.cpp
+++ b/tools/aapt2/format/binary/TableFlattener.cpp
@@ -228,14 +228,15 @@
public:
PackageFlattener(IAaptContext* context, ResourceTablePackage* package,
const std::map<size_t, std::string>* shared_libs, bool use_sparse_entries,
- bool collapse_key_stringpool, const std::set<std::string>& whitelisted_resources)
+ bool collapse_key_stringpool,
+ const std::set<ResourceName>& name_collapse_exemptions)
: context_(context),
diag_(context->GetDiagnostics()),
package_(package),
shared_libs_(shared_libs),
use_sparse_entries_(use_sparse_entries),
collapse_key_stringpool_(collapse_key_stringpool),
- whitelisted_resources_(whitelisted_resources) {
+ name_collapse_exemptions_(name_collapse_exemptions) {
}
bool FlattenPackage(BigBuffer* buffer) {
@@ -652,11 +653,12 @@
for (ResourceEntry* entry : sorted_entries) {
uint32_t local_key_index;
+ ResourceName resource_name({}, type->type, entry->name);
if (!collapse_key_stringpool_ ||
- whitelisted_resources_.find(entry->name) != whitelisted_resources_.end()) {
+ name_collapse_exemptions_.find(resource_name) != name_collapse_exemptions_.end()) {
local_key_index = (uint32_t)key_pool_.MakeRef(entry->name).index();
} else {
- // resource isn't whitelisted, add it as obfuscated value
+ // resource isn't exempt from collapse, add it as obfuscated value
local_key_index = (uint32_t)key_pool_.MakeRef(obfuscated_resource_name).index();
}
// Group values by configuration.
@@ -712,7 +714,7 @@
StringPool type_pool_;
StringPool key_pool_;
bool collapse_key_stringpool_;
- const std::set<std::string>& whitelisted_resources_;
+ const std::set<ResourceName>& name_collapse_exemptions_;
};
} // namespace
@@ -760,7 +762,7 @@
PackageFlattener flattener(context, package.get(), &table->included_packages_,
options_.use_sparse_entries, options_.collapse_key_stringpool,
- options_.whitelisted_resources);
+ options_.name_collapse_exemptions);
if (!flattener.FlattenPackage(&package_buffer)) {
return false;
}
diff --git a/tools/aapt2/format/binary/TableFlattener.h b/tools/aapt2/format/binary/TableFlattener.h
index 73c1729..4360db1 100644
--- a/tools/aapt2/format/binary/TableFlattener.h
+++ b/tools/aapt2/format/binary/TableFlattener.h
@@ -19,6 +19,7 @@
#include "android-base/macros.h"
+#include "Resource.h"
#include "ResourceTable.h"
#include "process/IResourceTableConsumer.h"
#include "util/BigBuffer.h"
@@ -41,8 +42,8 @@
// have name indices that point to this single value
bool collapse_key_stringpool = false;
- // Set of whitelisted resource names to avoid altering in key stringpool
- std::set<std::string> whitelisted_resources;
+ // Set of resources to avoid collapsing to a single entry in key stringpool.
+ std::set<ResourceName> name_collapse_exemptions;
// Map from original resource paths to shortened resource paths.
std::map<std::string, std::string> shortened_path_map;
diff --git a/tools/aapt2/format/binary/TableFlattener_test.cpp b/tools/aapt2/format/binary/TableFlattener_test.cpp
index a940923..8fbdd7f 100644
--- a/tools/aapt2/format/binary/TableFlattener_test.cpp
+++ b/tools/aapt2/format/binary/TableFlattener_test.cpp
@@ -518,7 +518,7 @@
ASSERT_FALSE(Flatten(context.get(), {}, table.get(), &result));
}
-TEST_F(TableFlattenerTest, ObfuscatingResourceNamesNoWhitelistSucceeds) {
+TEST_F(TableFlattenerTest, ObfuscatingResourceNamesNoNameCollapseExemptionsSucceeds) {
std::unique_ptr<ResourceTable> table =
test::ResourceTableBuilder()
.SetPackageId("com.app.test", 0x7f)
@@ -572,7 +572,7 @@
ResourceId(0x7f050000), {}, Res_value::TYPE_STRING, (uint32_t)idx, 0u));
}
-TEST_F(TableFlattenerTest, ObfuscatingResourceNamesWithWhitelistSucceeds) {
+TEST_F(TableFlattenerTest, ObfuscatingResourceNamesWithNameCollapseExemptionsSucceeds) {
std::unique_ptr<ResourceTable> table =
test::ResourceTableBuilder()
.SetPackageId("com.app.test", 0x7f)
@@ -591,21 +591,22 @@
TableFlattenerOptions options;
options.collapse_key_stringpool = true;
- options.whitelisted_resources.insert("test");
- options.whitelisted_resources.insert("three");
+ options.name_collapse_exemptions.insert(ResourceName({}, ResourceType::kId, "one"));
+ options.name_collapse_exemptions.insert(ResourceName({}, ResourceType::kString, "test"));
ResTable res_table;
ASSERT_TRUE(Flatten(context_.get(), options, table.get(), &res_table));
- EXPECT_TRUE(Exists(&res_table, "com.app.test:id/0_resource_name_obfuscated",
+ EXPECT_TRUE(Exists(&res_table, "com.app.test:id/one",
ResourceId(0x7f020000), {}, Res_value::TYPE_INT_BOOLEAN, 0u, 0u));
EXPECT_TRUE(Exists(&res_table, "com.app.test:id/0_resource_name_obfuscated",
ResourceId(0x7f020001), {}, Res_value::TYPE_INT_BOOLEAN, 0u, 0u));
- EXPECT_TRUE(Exists(&res_table, "com.app.test:id/three", ResourceId(0x7f020002), {},
- Res_value::TYPE_REFERENCE, 0x7f020000u, 0u));
+ EXPECT_TRUE(Exists(&res_table, "com.app.test:id/0_resource_name_obfuscated",
+ ResourceId(0x7f020002), {}, Res_value::TYPE_REFERENCE, 0x7f020000u, 0u));
+ // Note that this resource is also named "one", but it's a different type, so gets obfuscated.
EXPECT_TRUE(Exists(&res_table, "com.app.test:integer/0_resource_name_obfuscated",
ResourceId(0x7f030000), {}, Res_value::TYPE_INT_DEC, 1u,
ResTable_config::CONFIG_VERSION));