AAPT2: Verify positional Java String format arguments in strings
Change-Id: Id415969035a0d5712857c0e11e140155566a960c
diff --git a/tools/aapt2/ResourceParser.cpp b/tools/aapt2/ResourceParser.cpp
index 0c7a4d5..2d6c0c2 100644
--- a/tools/aapt2/ResourceParser.cpp
+++ b/tools/aapt2/ResourceParser.cpp
@@ -62,6 +62,7 @@
StyleString* outStyleString) {
std::vector<Span> spanStack;
+ bool error = false;
outRawString->clear();
outStyleString->spans.clear();
util::StringBuilder builder;
@@ -84,7 +85,6 @@
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 @@
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 @@
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 @@
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 ae3b4ff..d3c3c10 100644
--- a/tools/aapt2/ResourceUtils.cpp
+++ b/tools/aapt2/ResourceUtils.cpp
@@ -328,18 +328,36 @@
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 3c532de..851edc8 100644
--- a/tools/aapt2/ResourceUtils.h
+++ b/tools/aapt2/ResourceUtils.h
@@ -59,6 +59,11 @@
*/
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 6ef4ce5..59b8385 100644
--- a/tools/aapt2/util/Util.cpp
+++ b/tools/aapt2/util/Util.cpp
@@ -189,6 +189,105 @@
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 fd3fbb4..80552a5 100644
--- a/tools/aapt2/util/Util.h
+++ b/tools/aapt2/util/Util.h
@@ -158,6 +158,14 @@
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 cdba960..9db9fb7 100644
--- a/tools/aapt2/util/Util_test.cpp
+++ b/tools/aapt2/util/Util_test.cpp
@@ -185,4 +185,12 @@
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