Merge "AAPT2: Verify positional Java String format arguments in strings"

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<Span> 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<uint32_t>::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<uint32_t>(builder.str().size()) });
             }
-            spanStack.push_back(Span{ spanName, static_cast<uint32_t>(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<Item> 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<StringPiece16> 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<StringPiece16> 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<String>(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<BinaryPrimitive> tryParseColor(const StringPiece16& str) {
     return error ? std::unique_ptr<BinaryPrimitive>() : util::make_unique<BinaryPrimitive>(value);
 }
 
-std::unique_ptr<BinaryPrimitive> 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<BinaryPrimitive>(value);
+    return false;
+}
+
+std::unique_ptr<BinaryPrimitive> 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<BinaryPrimitive>(value);
+    }
+    return {};
 }
 
 std::unique_ptr<BinaryPrimitive> 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<std::u16string> 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<size_t>(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<char16_t> 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