AAPT2: Verify positional Java String format arguments in strings (b23f1e07) · Commits · e / os / android_frameworks_base

tools/aapt2/ResourceParser.cpp

+32 −5

Original line number	Diff line number	Diff line
		@@ -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()) });
		}

		} 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;
		}

tools/aapt2/ResourceUtils.cpp

+27 −9

Original line number	Diff line number	Diff line
		@@ -328,19 +328,37 @@ 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;
		}
		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;
		value.data = data;

		if (result) {
		value.data = 0xffffffffu;
		} else {
		value.data = 0;
		}
		return util::make_unique<BinaryPrimitive>(value);
		}
		return {};
		}

		std::unique_ptr<BinaryPrimitive> tryParseInt(const StringPiece16& str) {
		android::Res_value value;

tools/aapt2/ResourceUtils.h

+5 −0

Original line number	Diff line number	Diff line
		@@ -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.

tools/aapt2/util/Util.cpp

+99 −0

Original line number	Diff line number	Diff line
		@@ -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)++) {

tools/aapt2/util/Util.h

+8 −0

Original line number	Diff line number	Diff line
		@@ -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);