From 3411dd445778eff725dde736679b55314039a724 Mon Sep 17 00:00:00 2001 From: Sergio Giro Date: Tue, 28 Jun 2016 18:02:29 +0100 Subject: [PATCH] libutils/Unicode.cpp: Correct length computation and add checks for utf16->utf8 Inconsistent behaviour between utf16_to_utf8 and utf16_to_utf8_length is causing a heap overflow. Correcting the length computation and adding bound checks to the conversion functions. Test: ran libutils_tests Bug: 29250543 Change-Id: I6115e3357141ed245c63c6eb25fc0fd0a9a7a2bb (cherry picked from commit c4966a363e46d2e1074d1a365e232af0dcedd6a1) --- include/utils/Unicode.h | 4 ++-- libutils/String8.cpp | 25 +++++++++++++------------ libutils/Unicode.cpp | 15 +++++++++++---- libutils/tests/String8_test.cpp | 19 +++++++++++++++++++ 4 files changed, 45 insertions(+), 18 deletions(-) diff --git a/include/utils/Unicode.h b/include/utils/Unicode.h index b76a5e268..4e17cc3d9 100644 --- a/include/utils/Unicode.h +++ b/include/utils/Unicode.h @@ -87,7 +87,7 @@ ssize_t utf32_to_utf8_length(const char32_t *src, size_t src_len); * "dst" becomes \xE3\x81\x82\xE3\x81\x84 * (note that "dst" is NOT null-terminated, like strncpy) */ -void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst); +void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst, size_t dst_len); /** * Returns the unicode value at "index". @@ -109,7 +109,7 @@ ssize_t utf16_to_utf8_length(const char16_t *src, size_t src_len); * enough to fit the UTF-16 as measured by utf16_to_utf8_length with an added * NULL terminator. */ -void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst); +void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst, size_t dst_len); /** * Returns the length of "src" when "src" is valid UTF-8 string. diff --git a/libutils/String8.cpp b/libutils/String8.cpp index ad65fdb88..75dfa2994 100644 --- a/libutils/String8.cpp +++ b/libutils/String8.cpp @@ -102,20 +102,21 @@ static char* allocFromUTF16(const char16_t* in, size_t len) { if (len == 0) return getEmptyString(); - const ssize_t bytes = utf16_to_utf8_length(in, len); - if (bytes < 0) { + // Allow for closing '\0' + const ssize_t resultStrLen = utf16_to_utf8_length(in, len) + 1; + if (resultStrLen < 1) { return getEmptyString(); } - SharedBuffer* buf = SharedBuffer::alloc(bytes+1); + SharedBuffer* buf = SharedBuffer::alloc(resultStrLen); ALOG_ASSERT(buf, "Unable to allocate shared buffer"); if (!buf) { return getEmptyString(); } - char* str = (char*)buf->data(); - utf16_to_utf8(in, len, str); - return str; + char* resultStr = (char*)buf->data(); + utf16_to_utf8(in, len, resultStr, resultStrLen); + return resultStr; } static char* allocFromUTF32(const char32_t* in, size_t len) @@ -124,21 +125,21 @@ static char* allocFromUTF32(const char32_t* in, size_t len) return getEmptyString(); } - const ssize_t bytes = utf32_to_utf8_length(in, len); - if (bytes < 0) { + const ssize_t resultStrLen = utf32_to_utf8_length(in, len) + 1; + if (resultStrLen < 1) { return getEmptyString(); } - SharedBuffer* buf = SharedBuffer::alloc(bytes+1); + SharedBuffer* buf = SharedBuffer::alloc(resultStrLen); ALOG_ASSERT(buf, "Unable to allocate shared buffer"); if (!buf) { return getEmptyString(); } - char* str = (char*) buf->data(); - utf32_to_utf8(in, len, str); + char* resultStr = (char*) buf->data(); + utf32_to_utf8(in, len, resultStr, resultStrLen); - return str; + return resultStr; } // --------------------------------------------------------------------------- diff --git a/libutils/Unicode.cpp b/libutils/Unicode.cpp index fb876c91f..2b5293e74 100644 --- a/libutils/Unicode.cpp +++ b/libutils/Unicode.cpp @@ -14,6 +14,7 @@ * limitations under the License. */ +#include #include #include @@ -182,7 +183,7 @@ ssize_t utf32_to_utf8_length(const char32_t *src, size_t src_len) return ret; } -void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst) +void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst, size_t dst_len) { if (src == NULL || src_len == 0 || dst == NULL) { return; @@ -193,9 +194,12 @@ void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst) char *cur = dst; while (cur_utf32 < end_utf32) { size_t len = utf32_codepoint_utf8_length(*cur_utf32); + LOG_ALWAYS_FATAL_IF(dst_len < len, "%zu < %zu", dst_len, len); utf32_codepoint_to_utf8((uint8_t *)cur, *cur_utf32++, len); cur += len; + dst_len -= len; } + LOG_ALWAYS_FATAL_IF(dst_len < 1, "dst_len < 1: %zu < 1", dst_len); *cur = '\0'; } @@ -324,7 +328,7 @@ int strzcmp16_h_n(const char16_t *s1H, size_t n1, const char16_t *s2N, size_t n2 : 0); } -void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst) +void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst, size_t dst_len) { if (src == NULL || src_len == 0 || dst == NULL) { return; @@ -345,9 +349,12 @@ void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst) utf32 = (char32_t) *cur_utf16++; } const size_t len = utf32_codepoint_utf8_length(utf32); + LOG_ALWAYS_FATAL_IF(dst_len < len, "%zu < %zu", dst_len, len); utf32_codepoint_to_utf8((uint8_t*)cur, utf32, len); cur += len; + dst_len -= len; } + LOG_ALWAYS_FATAL_IF(dst_len < 1, "%zu < 1", dst_len); *cur = '\0'; } @@ -408,10 +415,10 @@ ssize_t utf16_to_utf8_length(const char16_t *src, size_t src_len) const char16_t* const end = src + src_len; while (src < end) { if ((*src & 0xFC00) == 0xD800 && (src + 1) < end - && (*++src & 0xFC00) == 0xDC00) { + && (*(src + 1) & 0xFC00) == 0xDC00) { // surrogate pairs are always 4 bytes. ret += 4; - src++; + src += 2; } else { ret += utf32_codepoint_utf8_length((char32_t) *src++); } diff --git a/libutils/tests/String8_test.cpp b/libutils/tests/String8_test.cpp index c42c68dce..7cd67d314 100644 --- a/libutils/tests/String8_test.cpp +++ b/libutils/tests/String8_test.cpp @@ -17,6 +17,7 @@ #define LOG_TAG "String8_test" #include #include +#include #include @@ -72,4 +73,22 @@ TEST_F(String8Test, OperatorPlusEquals) { EXPECT_STREQ(src3, " Verify me."); } +// http://b/29250543 +TEST_F(String8Test, CorrectInvalidSurrogate) { + // d841d8 is an invalid start for a surrogate pair. Make sure this is handled by ignoring the + // first character in the pair and handling the rest correctly. + String16 string16(u"\xd841\xd841\xdc41\x0000"); + String8 string8(string16); + + EXPECT_EQ(4U, string8.length()); +} + +TEST_F(String8Test, CheckUtf32Conversion) { + // Since bound checks were added, check the conversion can be done without fatal errors. + // The utf8 lengths of these are chars are 1 + 2 + 3 + 4 = 10. + const char32_t string32[] = U"\x0000007f\x000007ff\x0000911\x0010fffe"; + String8 string8(string32); + EXPECT_EQ(10U, string8.length()); +} + }