Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 1b4f3166 authored by Dan Albert's avatar Dan Albert
Browse files

Fix UB in ResourceTable::stringToInt. 

Was here because UBsan found integer overflow in the parsing for hex
numbers, since hex numbers here are actually unsigned but assigned to
a signed integer.

Also fixes a number of missing error conditions.

Change-Id: Iaea576daedfc6c75521cde02de3fe9dd0198a3b7
parent 47c1cf4b

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

}



}