From b23f1e077b02a1d62bcf5e34655e8dc979e124fa Mon Sep 17 00:00:00 2001 From: Adam Lesinski Date: Tue, 3 Nov 2015 12:24:17 -0800 Subject: [PATCH] AAPT2: Verify positional Java String format arguments in strings Change-Id: Id415969035a0d5712857c0e11e140155566a960c --- tools/aapt2/ResourceParser.cpp | 37 +++++++++++++--- tools/aapt2/ResourceUtils.cpp | 36 +++++++++++---- tools/aapt2/ResourceUtils.h | 5 +++ tools/aapt2/util/Util.cpp | 99 ++++++++++++++++++++++++++++++++++++++++++ tools/aapt2/util/Util.h | 8 ++++ tools/aapt2/util/Util_test.cpp | 8 ++++ 6 files changed, 179 insertions(+), 14 deletions(-) diff --git a/tools/aapt2/ResourceParser.cpp b/tools/aapt2/ResourceParser.cpp index 0c7a4d578be5..2d6c0c2d5b8c 100644 --- a/tools/aapt2/ResourceParser.cpp +++ b/tools/aapt2/ResourceParser.cpp @@ -62,6 +62,7 @@ bool ResourceParser::flattenXmlSubtree(XmlPullParser* parser, std::u16string* ou StyleString* outStyleString) { std::vector spanStack; + bool error = false; outRawString->clear(); outStyleString->spans.clear(); util::StringBuilder builder; @@ -84,7 +85,6 @@ bool ResourceParser::flattenXmlSubtree(XmlPullParser* parser, std::u16string* ou spanStack.pop_back(); } else if (event == XmlPullParser::Event::kText) { - // TODO(adamlesinski): Verify format strings. outRawString->append(parser->getText()); builder.append(parser->getText()); @@ -116,9 +116,10 @@ bool ResourceParser::flattenXmlSubtree(XmlPullParser* parser, std::u16string* ou if (builder.str().size() > std::numeric_limits::max()) { mDiag->error(DiagMessage(mSource.withLine(parser->getLineNumber())) << "style string '" << builder.str() << "' is too long"); - return false; + error = true; + } else { + spanStack.push_back(Span{ spanName, static_cast(builder.str().size()) }); } - spanStack.push_back(Span{ spanName, static_cast(builder.str().size()) }); } else if (event == XmlPullParser::Event::kComment) { // Skip @@ -129,7 +130,7 @@ bool ResourceParser::flattenXmlSubtree(XmlPullParser* parser, std::u16string* ou assert(spanStack.empty() && "spans haven't been fully processed"); outStyleString->str = builder.str(); - return true; + return !error; } bool ResourceParser::parse(XmlPullParser* parser) { @@ -437,13 +438,39 @@ std::unique_ptr ResourceParser::parseXml(XmlPullParser* parser, const uint bool ResourceParser::parseString(XmlPullParser* parser, ParsedResource* outResource) { const Source source = mSource.withLine(parser->getLineNumber()); - // TODO(adamlesinski): Read "untranslateable" attribute. + bool formatted = true; + if (Maybe formattedAttr = findAttribute(parser, u"formatted")) { + if (!ResourceUtils::tryParseBool(formattedAttr.value(), &formatted)) { + mDiag->error(DiagMessage(source) << "invalid value for 'formatted'. Must be a boolean"); + return false; + } + } + + bool untranslateable = false; + if (Maybe untranslateableAttr = findAttribute(parser, u"untranslateable")) { + if (!ResourceUtils::tryParseBool(untranslateableAttr.value(), &untranslateable)) { + mDiag->error(DiagMessage(source) + << "invalid value for 'untranslateable'. Must be a boolean"); + return false; + } + } outResource->value = parseXml(parser, android::ResTable_map::TYPE_STRING, kNoRawString); if (!outResource->value) { mDiag->error(DiagMessage(source) << "not a valid string"); return false; } + + if (formatted || untranslateable) { + if (String* stringValue = valueCast(outResource->value.get())) { + if (!util::verifyJavaStringFormat(*stringValue->value)) { + mDiag->error(DiagMessage(mSource.withLine(parser->getLineNumber())) + << "multiple substitutions specified in non-positional format; " + "did you mean to add the formatted=\"false\" attribute?"); + return false; + } + } + } return true; } diff --git a/tools/aapt2/ResourceUtils.cpp b/tools/aapt2/ResourceUtils.cpp index ae3b4ff1e363..d3c3c1044ae7 100644 --- a/tools/aapt2/ResourceUtils.cpp +++ b/tools/aapt2/ResourceUtils.cpp @@ -328,18 +328,36 @@ std::unique_ptr tryParseColor(const StringPiece16& str) { return error ? std::unique_ptr() : util::make_unique(value); } -std::unique_ptr tryParseBool(const StringPiece16& str) { +bool tryParseBool(const StringPiece16& str, bool* outValue) { StringPiece16 trimmedStr(util::trimWhitespace(str)); - uint32_t data = 0; if (trimmedStr == u"true" || trimmedStr == u"TRUE") { - data = 0xffffffffu; - } else if (trimmedStr != u"false" && trimmedStr != u"FALSE") { - return {}; + if (outValue) { + *outValue = true; + } + return true; + } else if (trimmedStr == u"false" || trimmedStr == u"FALSE") { + if (outValue) { + *outValue = false; + } + return true; } - android::Res_value value = { }; - value.dataType = android::Res_value::TYPE_INT_BOOLEAN; - value.data = data; - return util::make_unique(value); + return false; +} + +std::unique_ptr tryParseBool(const StringPiece16& str) { + bool result = false; + if (tryParseBool(str, &result)) { + android::Res_value value = {}; + value.dataType = android::Res_value::TYPE_INT_BOOLEAN; + + if (result) { + value.data = 0xffffffffu; + } else { + value.data = 0; + } + return util::make_unique(value); + } + return {}; } std::unique_ptr tryParseInt(const StringPiece16& str) { diff --git a/tools/aapt2/ResourceUtils.h b/tools/aapt2/ResourceUtils.h index 3c532de9060a..851edc89fa90 100644 --- a/tools/aapt2/ResourceUtils.h +++ b/tools/aapt2/ResourceUtils.h @@ -59,6 +59,11 @@ bool isReference(const StringPiece16& str); */ bool tryParseAttributeReference(const StringPiece16& str, ResourceNameRef* outReference); +/** + * Returns true if the value is a boolean, putting the result in `outValue`. + */ +bool tryParseBool(const StringPiece16& str, bool* outValue); + /* * Returns a Reference, or None Maybe instance if the string `str` was parsed as a * valid reference to a style. diff --git a/tools/aapt2/util/Util.cpp b/tools/aapt2/util/Util.cpp index 6ef4ce504a63..59b838587a6a 100644 --- a/tools/aapt2/util/Util.cpp +++ b/tools/aapt2/util/Util.cpp @@ -189,6 +189,105 @@ Maybe getFullyQualifiedClassName(const StringPiece16& package, return result; } +static size_t consumeDigits(const char16_t* start, const char16_t* end) { + const char16_t* c = start; + for (; c != end && *c >= u'0' && *c <= u'9'; c++) {} + return static_cast(c - start); +} + +bool verifyJavaStringFormat(const StringPiece16& str) { + const char16_t* c = str.begin(); + const char16_t* const end = str.end(); + + size_t argCount = 0; + bool nonpositional = false; + while (c != end) { + if (*c == u'%' && c + 1 < end) { + c++; + + if (*c == u'%') { + c++; + continue; + } + + argCount++; + + size_t numDigits = consumeDigits(c, end); + if (numDigits > 0) { + c += numDigits; + if (c != end && *c != u'$') { + // The digits were a size, but not a positional argument. + nonpositional = true; + } + } else if (*c == u'<') { + // Reusing last argument, bad idea since positions can be moved around + // during translation. + nonpositional = true; + + c++; + + // Optionally we can have a $ after + if (c != end && *c == u'$') { + c++; + } + } else { + nonpositional = true; + } + + // Ignore size, width, flags, etc. + while (c != end && (*c == u'-' || + *c == u'#' || + *c == u'+' || + *c == u' ' || + *c == u',' || + *c == u'(' || + (*c >= u'0' && *c <= '9'))) { + c++; + } + + /* + * This is a shortcut to detect strings that are going to Time.format() + * instead of String.format() + * + * Comparison of String.format() and Time.format() args: + * + * String: ABC E GH ST X abcdefgh nost x + * Time: DEFGHKMS W Za d hkm s w yz + * + * Therefore we know it's definitely Time if we have: + * DFKMWZkmwyz + */ + if (c != end) { + switch (*c) { + case 'D': + case 'F': + case 'K': + case 'M': + case 'W': + case 'Z': + case 'k': + case 'm': + case 'w': + case 'y': + case 'z': + return true; + } + } + } + + if (c != end) { + c++; + } + } + + if (argCount > 1 && nonpositional) { + // Multiple arguments were specified, but some or all were non positional. Translated + // strings may rearrange the order of the arguments, which will break the string. + return false; + } + return true; +} + static Maybe parseUnicodeCodepoint(const char16_t** start, const char16_t* end) { char16_t code = 0; for (size_t i = 0; i < 4 && *start != end; i++, (*start)++) { diff --git a/tools/aapt2/util/Util.h b/tools/aapt2/util/Util.h index fd3fbb4573a0..80552a540ec3 100644 --- a/tools/aapt2/util/Util.h +++ b/tools/aapt2/util/Util.h @@ -158,6 +158,14 @@ inline StringPiece16 getString(const android::ResStringPool& pool, size_t idx) { return StringPiece16(); } +/** + * 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 StringPiece16& str); + class StringBuilder { public: StringBuilder& append(const StringPiece16& str); diff --git a/tools/aapt2/util/Util_test.cpp b/tools/aapt2/util/Util_test.cpp index cdba960b8670..9db9fb7f112a 100644 --- a/tools/aapt2/util/Util_test.cpp +++ b/tools/aapt2/util/Util_test.cpp @@ -185,4 +185,12 @@ TEST(UtilTest, ExtractResourcePathComponents) { EXPECT_EQ(suffix, u"."); } +TEST(UtilTest, VerifyJavaStringFormat) { + ASSERT_TRUE(util::verifyJavaStringFormat(u"%09.34f")); + ASSERT_TRUE(util::verifyJavaStringFormat(u"%9$.34f %8$")); + ASSERT_TRUE(util::verifyJavaStringFormat(u"%% %%")); + ASSERT_FALSE(util::verifyJavaStringFormat(u"%09$f %f")); + ASSERT_FALSE(util::verifyJavaStringFormat(u"%09f %08s")); +} + } // namespace aapt -- 2.11.0