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/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..e6271eb12c056 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; } @@ -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/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")); diff --git a/tools/aapt2/xml/XmlActionExecutor.cpp b/tools/aapt2/xml/XmlActionExecutor.cpp index cc664a5de7224..cb844f085ecc2 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,17 +59,29 @@ 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) { + if (policy != XmlActionExecutorPolicy::kNone) { DiagMessage error_msg(child_el->line_number); - error_msg << "unknown element "; + error_msg << "unexpected element "; PrintElementToDiagMessage(child_el, &error_msg); - error_msg << " found"; - diag->Error(error_msg); - error = true; + error_msg << " found in "; + for (const StringPiece& element : *bread_crumb) { + error_msg << "<" << element << ">"; + } + 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; + } } } } @@ -90,14 +104,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..f689b2a3eaa85 100644 --- a/tools/aapt2/xml/XmlActionExecutor.h +++ b/tools/aapt2/xml/XmlActionExecutor.h @@ -34,62 +34,57 @@ 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 - * 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())); }