From ef506c73bb841d363060d2f0b52d56f3a28eea0e Mon Sep 17 00:00:00 2001 From: Adam Lesinski Date: Mon, 6 Nov 2017 10:44:46 -0800 Subject: [PATCH 1/3] AAPT2: Differentiate between Android and Java package names Android package names are more strict (ASCII only) than Java package names. Also fixed an issue where trailing underscores were disallowed in Android package names. (cherry picked from commit 96ea08f1e737e0d19e274e9a29f71c387d81b09a) Also includes part of I357fb84941bfbb3892a8c46feb47f55b865b6649 to remove usage of FindNonAlphaNumericAndNotInSet. Bug: 79481102 Test: make aapt2_tests Change-Id: I1052e9e82b6617db6065ce448d9bf7972bb68d59 Merged-In: I1052e9e82b6617db6065ce448d9bf7972bb68d59 --- tools/aapt2/java/ManifestClassGenerator.cpp | 11 +- tools/aapt2/link/ManifestFixer.cpp | 4 +- tools/aapt2/text/Unicode.cpp | 3 +- tools/aapt2/text/Unicode_test.cpp | 5 +- tools/aapt2/util/Util.cpp | 84 ++++----- tools/aapt2/util/Util.h | 189 +++++++++----------- tools/aapt2/util/Util_test.cpp | 32 +++- 7 files changed, 157 insertions(+), 171 deletions(-) diff --git a/tools/aapt2/java/ManifestClassGenerator.cpp b/tools/aapt2/java/ManifestClassGenerator.cpp index cad4c6c7c94fd..f4391e17adb6d 100644 --- a/tools/aapt2/java/ManifestClassGenerator.cpp +++ b/tools/aapt2/java/ManifestClassGenerator.cpp @@ -22,9 +22,11 @@ #include "java/AnnotationProcessor.h" #include "java/ClassDefinition.h" #include "util/Maybe.h" +#include "text/Unicode.h" #include "xml/XmlDom.h" -using android::StringPiece; +using ::android::StringPiece; +using ::aapt::text::IsJavaIdentifier; namespace aapt { @@ -46,11 +48,8 @@ static Maybe ExtractJavaIdentifier(IDiagnostics* diag, return {}; } - iter = util::FindNonAlphaNumericAndNotInSet(result, "_"); - if (iter != result.end()) { - diag->Error(DiagMessage(source) << "invalid character '" - << StringPiece(iter, 1) << "' in '" - << result << "'"); + if (!IsJavaIdentifier(result)) { + diag->Error(DiagMessage(source) << "invalid Java identifier '" << result << "'"); return {}; } diff --git a/tools/aapt2/link/ManifestFixer.cpp b/tools/aapt2/link/ManifestFixer.cpp index 6fb1793d90ede..de4fb736758c4 100644 --- a/tools/aapt2/link/ManifestFixer.cpp +++ b/tools/aapt2/link/ManifestFixer.cpp @@ -127,9 +127,9 @@ static bool VerifyManifest(xml::Element* el, SourcePathDiagnostics* diag) { diag->Error(DiagMessage(el->line_number) << "attribute 'package' in tag must not be a reference"); return false; - } else if (!util::IsJavaPackageName(attr->value)) { + } else if (!util::IsAndroidPackageName(attr->value)) { diag->Error(DiagMessage(el->line_number) - << "attribute 'package' in tag is not a valid Java package name: '" + << "attribute 'package' in tag is not a valid Android package name: '" << attr->value << "'"); return false; } diff --git a/tools/aapt2/text/Unicode.cpp b/tools/aapt2/text/Unicode.cpp index 75eeb46c7f5ec..3735b3e841e0d 100644 --- a/tools/aapt2/text/Unicode.cpp +++ b/tools/aapt2/text/Unicode.cpp @@ -85,7 +85,8 @@ bool IsJavaIdentifier(const StringPiece& str) { return false; } - if (!IsXidStart(iter.Next())) { + const char32_t first_codepoint = iter.Next(); + if (!IsXidStart(first_codepoint) && first_codepoint != U'_' && first_codepoint != U'$') { return false; } diff --git a/tools/aapt2/text/Unicode_test.cpp b/tools/aapt2/text/Unicode_test.cpp index d47fb282bc0a3..a8e797cf3d179 100644 --- a/tools/aapt2/text/Unicode_test.cpp +++ b/tools/aapt2/text/Unicode_test.cpp @@ -44,10 +44,11 @@ TEST(UnicodeTest, IsXidContinue) { TEST(UnicodeTest, IsJavaIdentifier) { EXPECT_TRUE(IsJavaIdentifier("FøøBar_12")); EXPECT_TRUE(IsJavaIdentifier("Føø$Bar")); + EXPECT_TRUE(IsJavaIdentifier("_FøøBar")); + EXPECT_TRUE(IsJavaIdentifier("$Føø$Bar")); EXPECT_FALSE(IsJavaIdentifier("12FøøBar")); - EXPECT_FALSE(IsJavaIdentifier("_FøøBar")); - EXPECT_FALSE(IsJavaIdentifier("$Føø$Bar")); + EXPECT_FALSE(IsJavaIdentifier(".Hello")); } TEST(UnicodeTest, IsValidResourceEntryName) { diff --git a/tools/aapt2/util/Util.cpp b/tools/aapt2/util/Util.cpp index 51a75d7556ad0..7c1c1ad6bb4d2 100644 --- a/tools/aapt2/util/Util.cpp +++ b/tools/aapt2/util/Util.cpp @@ -24,6 +24,7 @@ #include "androidfw/StringPiece.h" #include "utils/Unicode.h" +#include "text/Unicode.h" #include "text/Utf8Iterator.h" #include "util/BigBuffer.h" #include "util/Maybe.h" @@ -94,72 +95,55 @@ StringPiece TrimWhitespace(const StringPiece& str) { return StringPiece(start, end - start); } -StringPiece::const_iterator FindNonAlphaNumericAndNotInSet( - const StringPiece& str, const StringPiece& allowed_chars) { - const auto end_iter = str.end(); - for (auto iter = str.begin(); iter != end_iter; ++iter) { - char c = *iter; - if ((c >= u'a' && c <= u'z') || (c >= u'A' && c <= u'Z') || - (c >= u'0' && c <= u'9')) { - continue; - } - - bool match = false; - for (char i : allowed_chars) { - if (c == i) { - match = true; - break; - } - } - - if (!match) { - return iter; +static int IsJavaNameImpl(const StringPiece& str) { + int pieces = 0; + for (const StringPiece& piece : Tokenize(str, '.')) { + pieces++; + if (!text::IsJavaIdentifier(piece)) { + return -1; } } - return end_iter; + return pieces; } bool IsJavaClassName(const StringPiece& str) { - size_t pieces = 0; - for (const StringPiece& piece : Tokenize(str, '.')) { - pieces++; - if (piece.empty()) { - return false; - } - - // Can't have starting or trailing $ character. - if (piece.data()[0] == '$' || piece.data()[piece.size() - 1] == '$') { - return false; - } - - if (FindNonAlphaNumericAndNotInSet(piece, "$_") != piece.end()) { - return false; - } - } - return pieces >= 2; + return IsJavaNameImpl(str) >= 2; } bool IsJavaPackageName(const StringPiece& str) { - if (str.empty()) { - return false; - } + return IsJavaNameImpl(str) >= 1; +} - size_t pieces = 0; +static int IsAndroidNameImpl(const StringPiece& str) { + int pieces = 0; for (const StringPiece& piece : Tokenize(str, '.')) { - pieces++; if (piece.empty()) { - return false; + return -1; } - if (piece.data()[0] == '_' || piece.data()[piece.size() - 1] == '_') { - return false; + const char first_character = piece.data()[0]; + if (!::isalpha(first_character)) { + return -1; } - if (FindNonAlphaNumericAndNotInSet(piece, "_") != piece.end()) { - return false; + bool valid = std::all_of(piece.begin() + 1, piece.end(), [](const char c) -> bool { + return ::isalnum(c) || c == '_'; + }); + + if (!valid) { + return -1; } + pieces++; } - return pieces >= 1; + return pieces; +} + +bool IsAndroidPackageName(const StringPiece& str) { + return IsAndroidNameImpl(str) > 1 || str == "android"; +} + +bool IsAndroidSplitName(const StringPiece& str) { + return IsAndroidNameImpl(str) > 0; } Maybe GetFullyQualifiedClassName(const StringPiece& package, @@ -176,7 +160,7 @@ Maybe GetFullyQualifiedClassName(const StringPiece& package, return {}; } - std::string result(package.data(), package.size()); + std::string result = package.to_string(); if (classname.data()[0] != '.') { result += '.'; } diff --git a/tools/aapt2/util/Util.h b/tools/aapt2/util/Util.h index 8f021ab8cb8a4..f12746fe4bfcc 100644 --- a/tools/aapt2/util/Util.h +++ b/tools/aapt2/util/Util.h @@ -53,48 +53,40 @@ struct Range { std::vector Split(const android::StringPiece& str, char sep); std::vector SplitAndLowercase(const android::StringPiece& str, char sep); -/** - * Returns true if the string starts with prefix. - */ +// Returns true if the string starts with prefix. bool StartsWith(const android::StringPiece& str, const android::StringPiece& prefix); -/** - * Returns true if the string ends with suffix. - */ +// Returns true if the string ends with suffix. bool EndsWith(const android::StringPiece& str, const android::StringPiece& suffix); -/** - * Creates a new StringPiece16 that points to a substring - * of the original string without leading or trailing whitespace. - */ +// Creates a new StringPiece16 that points to a substring of the original string without leading or +// trailing whitespace. android::StringPiece TrimWhitespace(const android::StringPiece& str); -/** - * Returns an iterator to the first character that is not alpha-numeric and that - * is not in the allowedChars set. - */ -android::StringPiece::const_iterator FindNonAlphaNumericAndNotInSet( - const android::StringPiece& str, const android::StringPiece& allowed_chars); - -/** - * Tests that the string is a valid Java class name. - */ +// Tests that the string is a valid Java class name. bool IsJavaClassName(const android::StringPiece& str); -/** - * Tests that the string is a valid Java package name. - */ +// Tests that the string is a valid Java package name. bool IsJavaPackageName(const android::StringPiece& str); -/** - * Converts the class name to a fully qualified class name from the given - * `package`. Ex: - * - * asdf --> package.asdf - * .asdf --> package.asdf - * .a.b --> package.a.b - * asdf.adsf --> asdf.adsf - */ +// Tests that the string is a valid Android package name. More strict than a Java package name. +// - First character of each component (separated by '.') must be an ASCII letter. +// - Subsequent characters of a component can be ASCII alphanumeric or an underscore. +// - Package must contain at least two components, unless it is 'android'. +bool IsAndroidPackageName(const android::StringPiece& str); + +// Tests that the string is a valid Android split name. +// - First character of each component (separated by '.') must be an ASCII letter. +// - Subsequent characters of a component can be ASCII alphanumeric or an underscore. +bool IsAndroidSplitName(const android::StringPiece& str); + +// Converts the class name to a fully qualified class name from the given +// `package`. Ex: +// +// asdf --> package.asdf +// .asdf --> package.asdf +// .a.b --> package.a.b +// asdf.adsf --> asdf.adsf Maybe GetFullyQualifiedClassName(const android::StringPiece& package, const android::StringPiece& class_name); @@ -108,23 +100,17 @@ typename std::enable_if::value, int>::type compare(const T return 0; } -/** - * Makes a std::unique_ptr<> with the template parameter inferred by the compiler. - * This will be present in C++14 and can be removed then. - */ +// Makes a std::unique_ptr<> with the template parameter inferred by the compiler. +// This will be present in C++14 and can be removed then. template std::unique_ptr make_unique(Args&&... args) { return std::unique_ptr(new T{std::forward(args)...}); } -/** - * Writes a set of items to the std::ostream, joining the times with the - * provided - * separator. - */ +// Writes a set of items to the std::ostream, joining the times with the provided separator. template -::std::function<::std::ostream&(::std::ostream&)> Joiner( - const Container& container, const char* sep) { +::std::function<::std::ostream&(::std::ostream&)> Joiner(const Container& container, + const char* sep) { using std::begin; using std::end; const auto begin_iter = begin(container); @@ -140,32 +126,19 @@ template }; } -/** - * Helper method to extract a UTF-16 string from a StringPool. If the string is - * stored as UTF-8, - * the conversion to UTF-16 happens within ResStringPool. - */ +// Helper method to extract a UTF-16 string from a StringPool. If the string is stored as UTF-8, +// the conversion to UTF-16 happens within ResStringPool. android::StringPiece16 GetString16(const android::ResStringPool& pool, size_t idx); -/** - * Helper method to extract a UTF-8 string from a StringPool. If the string is - * stored as UTF-16, - * the conversion from UTF-16 to UTF-8 does not happen in ResStringPool and is - * done by this method, - * which maintains no state or cache. This means we must return an std::string - * copy. - */ +// Helper method to extract a UTF-8 string from a StringPool. If the string is stored as UTF-16, +// the conversion from UTF-16 to UTF-8 does not happen in ResStringPool and is done by this method, +// which maintains no state or cache. This means we must return an std::string copy. std::string GetString(const android::ResStringPool& pool, size_t idx); -/** - * Checks that the Java string format contains no non-positional arguments - * (arguments without - * explicitly specifying an index) when there are more than one argument. This - * is an error - * because translations may rearrange the order of the arguments in the string, - * which will - * break the string interpolation. - */ +// Checks that the Java string format contains no non-positional arguments (arguments without +// explicitly specifying an index) when there are more than one argument. This is an error +// because translations may rearrange the order of the arguments in the string, which will +// break the string interpolation. bool VerifyJavaStringFormat(const android::StringPiece& str); class StringBuilder { @@ -194,36 +167,38 @@ class StringBuilder { std::string error_; }; -inline const std::string& StringBuilder::ToString() const { return str_; } +inline const std::string& StringBuilder::ToString() const { + return str_; +} -inline const std::string& StringBuilder::Error() const { return error_; } +inline const std::string& StringBuilder::Error() const { + return error_; +} -inline bool StringBuilder::IsEmpty() const { return str_.empty(); } +inline bool StringBuilder::IsEmpty() const { + return str_.empty(); +} -inline size_t StringBuilder::Utf16Len() const { return utf16_len_; } +inline size_t StringBuilder::Utf16Len() const { + return utf16_len_; +} -inline StringBuilder::operator bool() const { return error_.empty(); } +inline StringBuilder::operator bool() const { + return error_.empty(); +} -/** - * Converts a UTF8 string to a UTF16 string. - */ +// Converts a UTF8 string to a UTF16 string. std::u16string Utf8ToUtf16(const android::StringPiece& utf8); std::string Utf16ToUtf8(const android::StringPiece16& utf16); -/** - * Writes the entire BigBuffer to the output stream. - */ +// Writes the entire BigBuffer to the output stream. bool WriteAll(std::ostream& out, const BigBuffer& buffer); -/* - * Copies the entire BigBuffer into a single buffer. - */ +// Copies the entire BigBuffer into a single buffer. std::unique_ptr Copy(const BigBuffer& buffer); -/** - * A Tokenizer implemented as an iterable collection. It does not allocate - * any memory on the heap nor use standard containers. - */ +// A Tokenizer implemented as an iterable collection. It does not allocate any memory on the heap +// nor use standard containers. class Tokenizer { public: class iterator { @@ -269,38 +244,42 @@ class Tokenizer { const iterator end_; }; -inline Tokenizer Tokenize(const android::StringPiece& str, char sep) { return Tokenizer(str, sep); } +inline Tokenizer Tokenize(const android::StringPiece& str, char sep) { + return Tokenizer(str, sep); +} -inline uint16_t HostToDevice16(uint16_t value) { return htods(value); } +inline uint16_t HostToDevice16(uint16_t value) { + return htods(value); +} -inline uint32_t HostToDevice32(uint32_t value) { return htodl(value); } +inline uint32_t HostToDevice32(uint32_t value) { + return htodl(value); +} -inline uint16_t DeviceToHost16(uint16_t value) { return dtohs(value); } +inline uint16_t DeviceToHost16(uint16_t value) { + return dtohs(value); +} -inline uint32_t DeviceToHost32(uint32_t value) { return dtohl(value); } +inline uint32_t DeviceToHost32(uint32_t value) { + return dtohl(value); +} -/** - * Given a path like: res/xml-sw600dp/foo.xml - * - * Extracts "res/xml-sw600dp/" into outPrefix. - * Extracts "foo" into outEntry. - * Extracts ".xml" into outSuffix. - * - * Returns true if successful. - */ +// Given a path like: res/xml-sw600dp/foo.xml +// +// Extracts "res/xml-sw600dp/" into outPrefix. +// Extracts "foo" into outEntry. +// Extracts ".xml" into outSuffix. +// +// Returns true if successful. bool ExtractResFilePathParts(const android::StringPiece& path, android::StringPiece* out_prefix, android::StringPiece* out_entry, android::StringPiece* out_suffix); } // namespace util -/** - * Stream operator for functions. Calls the function with the stream as an - * argument. - * In the aapt namespace for lookup. - */ -inline ::std::ostream& operator<<( - ::std::ostream& out, - const ::std::function<::std::ostream&(::std::ostream&)>& f) { +// Stream operator for functions. Calls the function with the stream as an argument. +// In the aapt namespace for lookup. +inline ::std::ostream& operator<<(::std::ostream& out, + const ::std::function<::std::ostream&(::std::ostream&)>& f) { return f(out); } diff --git a/tools/aapt2/util/Util_test.cpp b/tools/aapt2/util/Util_test.cpp index adb52911ab821..2d1242ada949f 100644 --- a/tools/aapt2/util/Util_test.cpp +++ b/tools/aapt2/util/Util_test.cpp @@ -117,24 +117,46 @@ TEST(UtilTest, IsJavaClassName) { EXPECT_TRUE(util::IsJavaClassName("android.test.Class$Inner")); EXPECT_TRUE(util::IsJavaClassName("android_test.test.Class")); EXPECT_TRUE(util::IsJavaClassName("_android_.test._Class_")); - EXPECT_FALSE(util::IsJavaClassName("android.test.$Inner")); - EXPECT_FALSE(util::IsJavaClassName("android.test.Inner$")); + EXPECT_TRUE(util::IsJavaClassName("android.test.$Inner")); + EXPECT_TRUE(util::IsJavaClassName("android.test.Inner$")); + EXPECT_TRUE(util::IsJavaClassName("com.foo.FøøBar")); + EXPECT_FALSE(util::IsJavaClassName(".test.Class")); EXPECT_FALSE(util::IsJavaClassName("android")); + EXPECT_FALSE(util::IsJavaClassName("FooBar")); } TEST(UtilTest, IsJavaPackageName) { EXPECT_TRUE(util::IsJavaPackageName("android")); EXPECT_TRUE(util::IsJavaPackageName("android.test")); EXPECT_TRUE(util::IsJavaPackageName("android.test_thing")); - EXPECT_FALSE(util::IsJavaPackageName("_android")); - EXPECT_FALSE(util::IsJavaPackageName("android_")); + EXPECT_TRUE(util::IsJavaPackageName("_android")); + EXPECT_TRUE(util::IsJavaPackageName("android_")); + EXPECT_TRUE(util::IsJavaPackageName("android._test")); + EXPECT_TRUE(util::IsJavaPackageName("cøm.foo")); + EXPECT_FALSE(util::IsJavaPackageName("android.")); EXPECT_FALSE(util::IsJavaPackageName(".android")); - EXPECT_FALSE(util::IsJavaPackageName("android._test")); EXPECT_FALSE(util::IsJavaPackageName("..")); } +TEST(UtilTest, IsAndroidPackageName) { + EXPECT_TRUE(util::IsAndroidPackageName("android")); + EXPECT_TRUE(util::IsAndroidPackageName("android.test")); + EXPECT_TRUE(util::IsAndroidPackageName("com.foo")); + EXPECT_TRUE(util::IsAndroidPackageName("com.foo.test_thing")); + EXPECT_TRUE(util::IsAndroidPackageName("com.foo.testing_thing_")); + EXPECT_TRUE(util::IsAndroidPackageName("com.foo.test_99_")); + + EXPECT_FALSE(util::IsAndroidPackageName("android._test")); + EXPECT_FALSE(util::IsAndroidPackageName("com")); + EXPECT_FALSE(util::IsAndroidPackageName("_android")); + EXPECT_FALSE(util::IsAndroidPackageName("android.")); + EXPECT_FALSE(util::IsAndroidPackageName(".android")); + EXPECT_FALSE(util::IsAndroidPackageName("..")); + EXPECT_FALSE(util::IsAndroidPackageName("cøm.foo")); +} + TEST(UtilTest, FullyQualifiedClassName) { EXPECT_THAT(util::GetFullyQualifiedClassName("android", ".asdf"), Eq("android.asdf")); EXPECT_THAT(util::GetFullyQualifiedClassName("android", ".a.b"), Eq("android.a.b")); From b2b20f26db06c9d9f84b9801f27cb17ab7444e2f Mon Sep 17 00:00:00 2001 From: Adam Lesinski Date: Thu, 2 Nov 2017 12:07:08 -0700 Subject: [PATCH 2/3] AAPT2: Better error messages for ManifestFixer AAPT2 will now print the XML hierarchy where it found an unexpected element. Test: make aapt2_tests Change-Id: Iac7918b2f344fab874f0a3e7aa9c6936ecde8913 Merged-In: Iac7918b2f344fab874f0a3e7aa9c6936ecde8913 (cherry picked from commit ed37f4842ad838792b16bf19768ed9b2519b0194) --- tools/aapt2/xml/XmlActionExecutor.cpp | 25 ++++++++---- tools/aapt2/xml/XmlActionExecutor.h | 46 +++++++++------------- tools/aapt2/xml/XmlActionExecutor_test.cpp | 8 +++- 3 files changed, 41 insertions(+), 38 deletions(-) diff --git a/tools/aapt2/xml/XmlActionExecutor.cpp b/tools/aapt2/xml/XmlActionExecutor.cpp index cc664a5de7224..602a902bfc3e8 100644 --- a/tools/aapt2/xml/XmlActionExecutor.cpp +++ b/tools/aapt2/xml/XmlActionExecutor.cpp @@ -16,6 +16,8 @@ #include "xml/XmlActionExecutor.h" +using ::android::StringPiece; + namespace aapt { namespace xml { @@ -46,8 +48,8 @@ static void PrintElementToDiagMessage(const Element* el, DiagMessage* msg) { *msg << el->name << ">"; } -bool XmlNodeAction::Execute(XmlActionExecutorPolicy policy, SourcePathDiagnostics* diag, - Element* el) const { +bool XmlNodeAction::Execute(XmlActionExecutorPolicy policy, std::vector* bread_crumb, + SourcePathDiagnostics* diag, Element* el) const { bool error = false; for (const ActionFuncWithDiag& action : actions_) { error |= !action(el, diag); @@ -57,15 +59,21 @@ bool XmlNodeAction::Execute(XmlActionExecutorPolicy policy, SourcePathDiagnostic if (child_el->namespace_uri.empty()) { std::map::const_iterator iter = map_.find(child_el->name); if (iter != map_.end()) { - error |= !iter->second.Execute(policy, diag, child_el); + // Use the iterator's copy of the element name, because the element may be modified. + bread_crumb->push_back(iter->first); + error |= !iter->second.Execute(policy, bread_crumb, diag, child_el); + bread_crumb->pop_back(); continue; } if (policy == XmlActionExecutorPolicy::kWhitelist) { DiagMessage error_msg(child_el->line_number); - error_msg << "unknown element "; + error_msg << "unexpected element "; PrintElementToDiagMessage(child_el, &error_msg); - error_msg << " found"; + error_msg << " found in "; + for (const StringPiece& element : *bread_crumb) { + error_msg << "<" << element << ">"; + } diag->Error(error_msg); error = true; } @@ -90,14 +98,15 @@ bool XmlActionExecutor::Execute(XmlActionExecutorPolicy policy, IDiagnostics* di if (el->namespace_uri.empty()) { std::map::const_iterator iter = map_.find(el->name); if (iter != map_.end()) { - return iter->second.Execute(policy, &source_diag, el); + std::vector bread_crumb; + bread_crumb.push_back(iter->first); + return iter->second.Execute(policy, &bread_crumb, &source_diag, el); } if (policy == XmlActionExecutorPolicy::kWhitelist) { DiagMessage error_msg(el->line_number); - error_msg << "unknown element "; + error_msg << "unexpected root element "; PrintElementToDiagMessage(el, &error_msg); - error_msg << " found"; source_diag.Error(error_msg); return false; } diff --git a/tools/aapt2/xml/XmlActionExecutor.h b/tools/aapt2/xml/XmlActionExecutor.h index 1d70045b30239..df70100e882a5 100644 --- a/tools/aapt2/xml/XmlActionExecutor.h +++ b/tools/aapt2/xml/XmlActionExecutor.h @@ -40,56 +40,46 @@ enum class XmlActionExecutorPolicy { kWhitelist, }; -/** - * Contains the actions to perform at this XML node. This is a recursive data - * structure that - * holds XmlNodeActions for child XML nodes. - */ +// Contains the actions to perform at this XML node. This is a recursive data structure that +// holds XmlNodeActions for child XML nodes. class XmlNodeAction { public: using ActionFuncWithDiag = std::function; using ActionFunc = std::function; - /** - * Find or create a child XmlNodeAction that will be performed for the child - * element with the name `name`. - */ - XmlNodeAction& operator[](const std::string& name) { return map_[name]; } + // Find or create a child XmlNodeAction that will be performed for the child element with the + // name `name`. + XmlNodeAction& operator[](const std::string& name) { + return map_[name]; + } - /** - * Add an action to be performed at this XmlNodeAction. - */ + // Add an action to be performed at this XmlNodeAction. void Action(ActionFunc f); void Action(ActionFuncWithDiag); private: friend class XmlActionExecutor; - bool Execute(XmlActionExecutorPolicy policy, SourcePathDiagnostics* diag, Element* el) const; + bool Execute(XmlActionExecutorPolicy policy, std::vector<::android::StringPiece>* bread_crumb, + SourcePathDiagnostics* diag, Element* el) const; std::map map_; std::vector actions_; }; -/** - * Allows the definition of actions to execute at specific XML elements defined - * by their - * hierarchy. - */ +// Allows the definition of actions to execute at specific XML elements defined by their hierarchy. class XmlActionExecutor { public: XmlActionExecutor() = default; - /** - * Find or create a root XmlNodeAction that will be performed for the root XML - * element with the name `name`. - */ - XmlNodeAction& operator[](const std::string& name) { return map_[name]; } + // Find or create a root XmlNodeAction that will be performed for the root XML element with the + // name `name`. + XmlNodeAction& operator[](const std::string& name) { + return map_[name]; + } - /** - * Execute the defined actions for this XmlResource. - * Returns true if all actions return true, otherwise returns false. - */ + // Execute the defined actions for this XmlResource. + // Returns true if all actions return true, otherwise returns false. bool Execute(XmlActionExecutorPolicy policy, IDiagnostics* diag, XmlResource* doc) const; private: diff --git a/tools/aapt2/xml/XmlActionExecutor_test.cpp b/tools/aapt2/xml/XmlActionExecutor_test.cpp index 0fe7ab05ceb28..d39854e5fe4eb 100644 --- a/tools/aapt2/xml/XmlActionExecutor_test.cpp +++ b/tools/aapt2/xml/XmlActionExecutor_test.cpp @@ -56,9 +56,13 @@ TEST(XmlActionExecutorTest, FailsWhenUndefinedHierarchyExists) { XmlActionExecutor executor; executor["manifest"]["application"]; - std::unique_ptr doc = - test::BuildXmlDom(""); + std::unique_ptr doc; StdErrDiagnostics diag; + + doc = test::BuildXmlDom(""); + ASSERT_FALSE(executor.Execute(XmlActionExecutorPolicy::kWhitelist, &diag, doc.get())); + + doc = test::BuildXmlDom(""); ASSERT_FALSE(executor.Execute(XmlActionExecutorPolicy::kWhitelist, &diag, doc.get())); } From 533c09b01b175b8b59e8f83c64a61d98ca808e75 Mon Sep 17 00:00:00 2001 From: Izabela Orlowska Date: Tue, 19 Dec 2017 16:22:42 +0000 Subject: [PATCH 3/3] AAPT2: treat manifest validation errors as warnings when asked Bug: 65670329 Test: updated Change-Id: Ic554cc20134fce66aa9ddf8d16ddffe0131c50e9 Merged-In: Ic554cc20134fce66aa9ddf8d16ddffe0131c50e9 (cherry picked from commit ad9e1324ff2c459d0ee6ee571d4a3e458c02cc81) --- tools/aapt2/cmd/Link.cpp | 3 +++ tools/aapt2/link/ManifestFixer.cpp | 5 +++- tools/aapt2/link/ManifestFixer.h | 5 ++++ tools/aapt2/link/ManifestFixer_test.cpp | 32 +++++++++++++++++++++++-- tools/aapt2/xml/XmlActionExecutor.cpp | 12 +++++++--- tools/aapt2/xml/XmlActionExecutor.h | 9 +++++-- 6 files changed, 58 insertions(+), 8 deletions(-) diff --git a/tools/aapt2/cmd/Link.cpp b/tools/aapt2/cmd/Link.cpp index 7742f36f16104..036bf5d86f328 100644 --- a/tools/aapt2/cmd/Link.cpp +++ b/tools/aapt2/cmd/Link.cpp @@ -1968,6 +1968,9 @@ int Link(const std::vector& args, IDiagnostics* diagnostics) { &options.manifest_fixer_options.rename_instrumentation_target_package) .OptionalFlagList("-0", "File extensions not to compress.", &options.extensions_to_not_compress) + .OptionalSwitch("--warn-manifest-validation", + "Treat manifest validation errors as warnings.", + &options.manifest_fixer_options.warn_validation) .OptionalFlagList("--split", "Split resources matching a set of configs out to a Split APK.\n" "Syntax: path/to/output.apk:[,[...]].\n" diff --git a/tools/aapt2/link/ManifestFixer.cpp b/tools/aapt2/link/ManifestFixer.cpp index de4fb736758c4..e6271eb12c056 100644 --- a/tools/aapt2/link/ManifestFixer.cpp +++ b/tools/aapt2/link/ManifestFixer.cpp @@ -409,7 +409,10 @@ bool ManifestFixer::Consume(IAaptContext* context, xml::XmlResource* doc) { return false; } - if (!executor.Execute(xml::XmlActionExecutorPolicy::kWhitelist, context->GetDiagnostics(), doc)) { + xml::XmlActionExecutorPolicy policy = options_.warn_validation + ? xml::XmlActionExecutorPolicy::kWhitelistWarning + : xml::XmlActionExecutorPolicy::kWhitelist; + if (!executor.Execute(policy, context->GetDiagnostics(), doc)) { return false; } diff --git a/tools/aapt2/link/ManifestFixer.h b/tools/aapt2/link/ManifestFixer.h index 470f65eb01c48..dbabab179cad4 100644 --- a/tools/aapt2/link/ManifestFixer.h +++ b/tools/aapt2/link/ManifestFixer.h @@ -35,6 +35,11 @@ struct ManifestFixerOptions { Maybe rename_instrumentation_target_package; Maybe version_name_default; Maybe version_code_default; + + // Wether validation errors should be treated only as warnings. If this is 'true', then an + // incorrect node will not result in an error, but only as a warning, and the parsing will + // continue. + bool warn_validation = false; }; /** diff --git a/tools/aapt2/link/ManifestFixer_test.cpp b/tools/aapt2/link/ManifestFixer_test.cpp index da7f410b8b086..84e367d842c86 100644 --- a/tools/aapt2/link/ManifestFixer_test.cpp +++ b/tools/aapt2/link/ManifestFixer_test.cpp @@ -19,6 +19,7 @@ #include "test/Test.h" using ::android::StringPiece; +using ::testing::IsNull; using ::testing::NotNull; namespace aapt { @@ -109,7 +110,9 @@ TEST_F(ManifestFixerTest, AllowMetaData) { } TEST_F(ManifestFixerTest, UseDefaultSdkVersionsIfNonePresent) { - ManifestFixerOptions options = {std::string("8"), std::string("22")}; + ManifestFixerOptions options; + options.min_sdk_version_default = std::string("8"); + options.target_sdk_version_default = std::string("22"); std::unique_ptr doc = VerifyWithOptions(R"EOF( doc = VerifyWithOptions(R"EOF( @@ -439,4 +444,27 @@ TEST_F(ManifestFixerTest, SupportKeySets) { EXPECT_THAT(Verify(input), NotNull()); } +TEST_F(ManifestFixerTest, UnexpectedElementsInManifest) { + std::string input = R"( + + + )"; + ManifestFixerOptions options; + options.warn_validation = true; + + // Unexpected element should result in a warning if the flag is set to 'true'. + std::unique_ptr manifest = VerifyWithOptions(input, options); + ASSERT_THAT(manifest, NotNull()); + + // Unexpected element should result in an error if the flag is set to 'false'. + options.warn_validation = false; + manifest = VerifyWithOptions(input, options); + ASSERT_THAT(manifest, IsNull()); + + // By default the flag should be set to 'false'. + manifest = Verify(input); + ASSERT_THAT(manifest, IsNull()); +} + } // namespace aapt diff --git a/tools/aapt2/xml/XmlActionExecutor.cpp b/tools/aapt2/xml/XmlActionExecutor.cpp index 602a902bfc3e8..cb844f085ecc2 100644 --- a/tools/aapt2/xml/XmlActionExecutor.cpp +++ b/tools/aapt2/xml/XmlActionExecutor.cpp @@ -66,7 +66,7 @@ bool XmlNodeAction::Execute(XmlActionExecutorPolicy policy, std::vectorline_number); error_msg << "unexpected element "; PrintElementToDiagMessage(child_el, &error_msg); @@ -74,8 +74,14 @@ bool XmlNodeAction::Execute(XmlActionExecutorPolicy policy, std::vector"; } - diag->Error(error_msg); - error = true; + if (policy == XmlActionExecutorPolicy::kWhitelistWarning) { + // Treat the error only as a warning. + diag->Warn(error_msg); + } else { + // Policy is XmlActionExecutorPolicy::kWhitelist, we should fail. + diag->Error(error_msg); + error = true; + } } } } diff --git a/tools/aapt2/xml/XmlActionExecutor.h b/tools/aapt2/xml/XmlActionExecutor.h index df70100e882a5..f689b2a3eaa85 100644 --- a/tools/aapt2/xml/XmlActionExecutor.h +++ b/tools/aapt2/xml/XmlActionExecutor.h @@ -34,10 +34,15 @@ enum class XmlActionExecutorPolicy { // Actions are run if elements are matched, errors occur only when actions return false. kNone, - // The actions defined must match and run. If an element is found that does - // not match an action, an error occurs. + // The actions defined must match and run. If an element is found that does not match an action, + // an error occurs. // Note: namespaced elements are always ignored. kWhitelist, + + // The actions defined should match and run. if an element is found that does not match an + // action, a warning is printed. + // Note: namespaced elements are always ignored. + kWhitelistWarning, }; // Contains the actions to perform at this XML node. This is a recursive data structure that