Merge "Fix UB in ResourceTable::stringToInt." (571c5a26) · Commits · e / os / android_frameworks_base

include/androidfw/ResourceTypes.h

+4 −1

Original line number	Diff line number	Diff line
		@@ -372,7 +372,8 @@ struct Res_value
		};

		// The data for this item, as interpreted according to dataType.
		uint32_t data;
		typedef uint32_t data_type;
		data_type data;

		void copyFrom_dtoh(const Res_value& src);
		};
		@@ -1502,6 +1503,8 @@ private:
		KeyedVector<String16, uint8_t> mEntries;
		};

		bool U16StringToInt(const char16_t* s, size_t len, Res_value* outValue);

		/**
		* Convenience class for accessing data in a ResTable resource.
		*/

libs/androidfw/ResourceTypes.cpp

+53 −21

Original line number	Diff line number	Diff line
		@@ -17,6 +17,16 @@
		#define LOG_TAG "ResourceType"
		//#define LOG_NDEBUG 0

		#include <ctype.h>
		#include <memory.h>
		#include <stddef.h>
		#include <stdint.h>
		#include <stdlib.h>
		#include <string.h>

		#include <limits>
		#include <type_traits>

		#include <androidfw/ByteBucketArray.h>
		#include <androidfw/ResourceTypes.h>
		#include <androidfw/TypeWrappers.h>
		@@ -27,17 +37,10 @@
		#include <utils/String16.h>
		#include <utils/String8.h>

		#ifdef HAVE_ANDROID_OS
		#ifdef __ANDROID__
		#include <binder/TextOutput.h>
		#endif

		#include <stdlib.h>
		#include <string.h>
		#include <memory.h>
		#include <ctype.h>
		#include <stdint.h>
		#include <stddef.h>

		#ifndef INT32_MAX
		#define INT32_MAX ((int32_t)(2147483647))
		#endif
		@@ -4551,8 +4554,7 @@ static bool parse_unit(const char* str, Res_value* outValue,
		return false;
		}


		bool ResTable::stringToInt(const char16_t* s, size_t len, Res_value* outValue)
		bool U16StringToInt(const char16_t* s, size_t len, Res_value* outValue)
		{
		while (len > 0 && isspace16(*s)) {
		s++;
		@@ -4564,7 +4566,7 @@ bool ResTable::stringToInt(const char16_t* s, size_t len, Res_value* outValue)
		}

		size_t i = 0;
		int32_t val = 0;
		int64_t val = 0;
		bool neg = false;

		if (*s == '-') {
		@@ -4576,28 +4578,50 @@ bool ResTable::stringToInt(const char16_t* s, size_t len, Res_value* outValue)
		return false;
		}

		static_assert(std::is_same<uint32_t, Res_value::data_type>::value,
		"Res_value::data_type has changed. The range checks in this "
		"function are no longer correct.");

		// Decimal or hex?
		if (s[i] == '0' && s[i+1] == 'x') {
		if (outValue)
		outValue->dataType = outValue->TYPE_INT_HEX;
		bool isHex;
		if (len > 1 && s[i] == '0' && s[i+1] == 'x') {
		isHex = true;
		i += 2;

		if (neg) {
		return false;
		}

		if (i == len) {
		// Just u"0x"
		return false;
		}

		bool error = false;
		while (i < len && !error) {
		val = (val*16) + get_hex(s[i], &error);
		i++;

		if (val > std::numeric_limits<uint32_t>::max()) {
		return false;
		}
		}
		if (error) {
		return false;
		}
		} else {
		if (outValue)
		outValue->dataType = outValue->TYPE_INT_DEC;
		isHex = false;
		while (i < len) {
		if (s[i] < '0' \|\| s[i] > '9') {
		return false;
		}
		val = (val*10) + s[i]-'0';
		i++;

		if ((neg && -val < std::numeric_limits<int32_t>::min()) \|\|
		(!neg && val > std::numeric_limits<int32_t>::max())) {
		return false;
		}
		}
		}

		@@ -4607,13 +4631,21 @@ bool ResTable::stringToInt(const char16_t* s, size_t len, Res_value* outValue)
		i++;
		}

		if (i == len) {
		if (outValue)
		outValue->data = val;
		if (i != len) {
		return false;
		}

		if (outValue) {
		outValue->dataType =
		isHex ? outValue->TYPE_INT_HEX : outValue->TYPE_INT_DEC;
		outValue->data = static_cast<Res_value::data_type>(val);
		}
		return true;
		}

		return false;
		bool ResTable::stringToInt(const char16_t* s, size_t len, Res_value* outValue)
		{
		return U16StringToInt(s, len, outValue);
		}

		bool ResTable::stringToFloat(const char16_t* s, size_t len, Res_value* outValue)

libs/androidfw/tests/Android.mk

+17 −13

Original line number	Diff line number	Diff line
		@@ -19,6 +19,7 @@
		# targets here.
		# ==========================================================
		LOCAL_PATH:= $(call my-dir)

		testFiles := \
		AttributeFinder_test.cpp \
		ByteBucketArray_test.cpp \
		@@ -32,27 +33,33 @@ testFiles := \
		TypeWrappers_test.cpp \
		ZipUtils_test.cpp

		androidfw_test_cflags := \
		-Wall \
		-Werror \
		-Wunused \
		-Wunreachable-code \
		-Wno-missing-field-initializers \

		# gtest is broken.
		androidfw_test_cflags += -Wno-unnamed-type-template-args

		# ==========================================================
		# Build the host tests: libandroidfw_tests
		# ==========================================================
		include $(CLEAR_VARS)

		LOCAL_MODULE := libandroidfw_tests

		LOCAL_CFLAGS += -Wall -Werror -Wunused -Wunreachable-code
		# gtest is broken.
		LOCAL_CFLAGS += -Wno-unnamed-type-template-args

		LOCAL_CFLAGS := $(androidfw_test_cflags)
		LOCAL_SRC_FILES := $(testFiles)
		LOCAL_STATIC_LIBRARIES := \
		libandroidfw \
		libutils \
		libcutils \
		liblog
		liblog \
		libz \

		include $(BUILD_HOST_NATIVE_TEST)


		# ==========================================================
		# Build the device tests: libandroidfw_tests
		# ==========================================================
		@@ -60,14 +67,11 @@ ifneq ($(SDK_ONLY),true)
		include $(CLEAR_VARS)

		LOCAL_MODULE := libandroidfw_tests

		LOCAL_CFLAGS += -Wall -Werror -Wunused -Wunreachable-code
		# gtest is broken.
		LOCAL_CFLAGS += -Wno-unnamed-type-template-args

		LOCAL_CFLAGS := $(androidfw_test_cflags)
		LOCAL_SRC_FILES := $(testFiles) \
		BackupData_test.cpp \
		ObbFile_test.cpp
		ObbFile_test.cpp \

		LOCAL_SHARED_LIBRARIES := \
		libandroidfw \
		libcutils \

libs/androidfw/tests/ResTable_test.cpp

+81 −0

Original line number	Diff line number	Diff line
		@@ -16,6 +16,10 @@

		#include <androidfw/ResourceTypes.h>

		#include <codecvt>
		#include <locale>
		#include <string>

		#include <utils/String8.h>
		#include <utils/String16.h>
		#include "TestHelpers.h"
		@@ -201,4 +205,81 @@ TEST(ResTableTest, emptyTableHasSensibleDefaults) {
		ASSERT_LT(table.getResource(base::R::integer::number1, &val, MAY_NOT_BE_BAG), 0);
		}

		void testU16StringToInt(const char16_t* str, uint32_t expectedValue,
		bool expectSuccess, bool expectHex) {
		size_t len = std::char_traits<char16_t>::length(str);

		// Gtest can't print UTF-16 strings, so we have to convert to UTF-8 :(
		std::wstring_convert<std::codecvt_utf8_utf16<char16_t>, char16_t> convert;
		std::string s = convert.to_bytes(std::u16string(str, len));

		Res_value out = {};
		ASSERT_EQ(expectSuccess, U16StringToInt(str, len, &out))
		<< "Failed with " << s;

		if (!expectSuccess) {
		ASSERT_EQ(out.TYPE_NULL, out.dataType) << "Failed with " << s;
		return;
		}

		if (expectHex) {
		ASSERT_EQ(out.TYPE_INT_HEX, out.dataType) << "Failed with " << s;
		} else {
		ASSERT_EQ(out.TYPE_INT_DEC, out.dataType) << "Failed with " << s;
		}

		ASSERT_EQ(expectedValue, out.data) << "Failed with " << s;
		}

		TEST(ResTableTest, U16StringToInt) {
		testU16StringToInt(u"", 0U, false, false);
		testU16StringToInt(u" ", 0U, false, false);
		testU16StringToInt(u"\t\n", 0U, false, false);

		testU16StringToInt(u"abcd", 0U, false, false);
		testU16StringToInt(u"10abcd", 0U, false, false);
		testU16StringToInt(u"42 42", 0U, false, false);
		testU16StringToInt(u"- 42", 0U, false, false);
		testU16StringToInt(u"-", 0U, false, false);

		testU16StringToInt(u"0x", 0U, false, true);
		testU16StringToInt(u"0xnope", 0U, false, true);
		testU16StringToInt(u"0X42", 0U, false, true);
		testU16StringToInt(u"0x42 0x42", 0U, false, true);
		testU16StringToInt(u"-0x0", 0U, false, true);
		testU16StringToInt(u"-0x42", 0U, false, true);
		testU16StringToInt(u"- 0x42", 0U, false, true);

		// Note that u" 42" would pass. This preserves the old behavior, but it may
		// not be desired.
		testU16StringToInt(u"42 ", 0U, false, false);
		testU16StringToInt(u"0x42 ", 0U, false, true);

		// Decimal cases.
		testU16StringToInt(u"0", 0U, true, false);
		testU16StringToInt(u"-0", 0U, true, false);
		testU16StringToInt(u"42", 42U, true, false);
		testU16StringToInt(u" 42", 42U, true, false);
		testU16StringToInt(u"-42", static_cast<uint32_t>(-42), true, false);
		testU16StringToInt(u" -42", static_cast<uint32_t>(-42), true, false);
		testU16StringToInt(u"042", 42U, true, false);
		testU16StringToInt(u"-042", static_cast<uint32_t>(-42), true, false);

		// Hex cases.
		testU16StringToInt(u"0x0", 0x0, true, true);
		testU16StringToInt(u"0x42", 0x42, true, true);
		testU16StringToInt(u" 0x42", 0x42, true, true);

		// Just before overflow cases:
		testU16StringToInt(u"2147483647", INT_MAX, true, false);
		testU16StringToInt(u"-2147483648", static_cast<uint32_t>(INT_MIN), true,
		false);
		testU16StringToInt(u"0xffffffff", UINT_MAX, true, true);

		// Overflow cases:
		testU16StringToInt(u"2147483648", 0U, false, false);
		testU16StringToInt(u"-2147483649", 0U, false, false);
		testU16StringToInt(u"0x1ffffffff", 0U, false, true);
		}

		}