From 9de6776321b80d387e6108683547bc043f868925 Mon Sep 17 00:00:00 2001 From: Sergio Giro Date: Wed, 20 Jul 2016 20:01:33 +0100 Subject: [PATCH 1/2] Unicode: specify destination length in utf8_to_utf16 methods String16(const char *utf8) now returns the empty string in case a string ends halfway throw a utf8 character. Bug: 29267949 Clean cherry-pick from 1dcc0c82394ec9cd6887c7ca39f9b5024db01ac9 Change-Id: I5223caa7d42f4582a982609a898a02043265c6d3 --- include/utils/Unicode.h | 51 +++++++++++++++-------------- libutils/String16.cpp | 2 +- libutils/Unicode.cpp | 58 ++++++++++++++++----------------- libutils/tests/Unicode_test.cpp | 13 +++++++- 4 files changed, 68 insertions(+), 56 deletions(-) diff --git a/include/utils/Unicode.h b/include/utils/Unicode.h index a006082b0..7316665e6 100644 --- a/include/utils/Unicode.h +++ b/include/utils/Unicode.h @@ -31,7 +31,7 @@ char16_t *strcpy16(char16_t *, const char16_t *); char16_t *strncpy16(char16_t *, const char16_t *, size_t); char16_t *strstr16(const char16_t*, const char16_t*); -// Version of comparison that supports embedded nulls. +// Version of comparison that supports embedded NULs. // This is different than strncmp() because we don't stop // at a nul character and consider the strings to be different // if the lengths are different (thus we need to supply the @@ -58,7 +58,7 @@ ssize_t utf32_to_utf8_length(const char32_t *src, size_t src_len); * large enough to store the string, the part of the "src" string is stored * into "dst" as much as possible. See the examples for more detail. * Returns the size actually used for storing the string. - * dst" is not null-terminated when dst_len is fully used (like strncpy). + * dst" is not nul-terminated when dst_len is fully used (like strncpy). * * Example 1 * "src" == \u3042\u3044 (\xE3\x81\x82\xE3\x81\x84) @@ -67,7 +67,7 @@ ssize_t utf32_to_utf8_length(const char32_t *src, size_t src_len); * -> * Returned value == 6 * "dst" becomes \xE3\x81\x82\xE3\x81\x84\0 - * (note that "dst" is null-terminated) + * (note that "dst" is nul-terminated) * * Example 2 * "src" == \u3042\u3044 (\xE3\x81\x82\xE3\x81\x84) @@ -76,7 +76,7 @@ ssize_t utf32_to_utf8_length(const char32_t *src, size_t src_len); * -> * Returned value == 3 * "dst" becomes \xE3\x81\x82\0 - * (note that "dst" is null-terminated, but \u3044 is not stored in "dst" + * (note that "dst" is nul-terminated, but \u3044 is not stored in "dst" * since "dst" does not have enough size to store the character) * * Example 3 @@ -86,7 +86,7 @@ ssize_t utf32_to_utf8_length(const char32_t *src, size_t src_len); * -> * Returned value == 6 * "dst" becomes \xE3\x81\x82\xE3\x81\x84 - * (note that "dst" is NOT null-terminated, like strncpy) + * (note that "dst" is NOT nul-terminated, like strncpy) */ void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst); @@ -108,7 +108,7 @@ ssize_t utf16_to_utf8_length(const char16_t *src, size_t src_len); /** * Converts a UTF-16 string to UTF-8. The destination buffer must be large * enough to fit the UTF-16 as measured by utf16_to_utf8_length with an added - * NULL terminator. + * NUL terminator. */ void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst); @@ -118,7 +118,7 @@ void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst); * is an invalid string. * * This function should be used to determine whether "src" is valid UTF-8 - * characters with valid unicode codepoints. "src" must be null-terminated. + * characters with valid unicode codepoints. "src" must be nul-terminated. * * If you are going to use other utf8_to_... functions defined in this header * with string which may not be valid UTF-8 with valid codepoint (form 0 to @@ -138,35 +138,38 @@ size_t utf8_to_utf32_length(const char *src, size_t src_len); /** * Stores a UTF-32 string converted from "src" in "dst". "dst" must be large * enough to store the entire converted string as measured by - * utf8_to_utf32_length plus space for a NULL terminator. + * utf8_to_utf32_length plus space for a NUL terminator. */ void utf8_to_utf32(const char* src, size_t src_len, char32_t* dst); /** - * Returns the UTF-16 length of UTF-8 string "src". + * Returns the UTF-16 length of UTF-8 string "src". Returns -1 in case + * it's invalid utf8. No buffer over-read occurs because of bound checks. Using overreadIsFatal you + * can ask to log a message and fail in case the invalid utf8 could have caused an override if no + * bound checks were used (otherwise -1 is returned). */ -ssize_t utf8_to_utf16_length(const uint8_t* src, size_t srcLen); +ssize_t utf8_to_utf16_length(const uint8_t* src, size_t srcLen, bool overreadIsFatal = false); /** * Convert UTF-8 to UTF-16 including surrogate pairs. - * Returns a pointer to the end of the string (where a null terminator might go - * if you wanted to add one). + * Returns a pointer to the end of the string (where a NUL terminator might go + * if you wanted to add one). At most dstLen characters are written; it won't emit half a surrogate + * pair. If dstLen == 0 nothing is written and dst is returned. If dstLen > SSIZE_MAX it aborts + * (this being probably a negative number returned as an error and casted to unsigned). */ -char16_t* utf8_to_utf16_no_null_terminator(const uint8_t* src, size_t srcLen, char16_t* dst); +char16_t* utf8_to_utf16_no_null_terminator( + const uint8_t* src, size_t srcLen, char16_t* dst, size_t dstLen); /** - * Convert UTF-8 to UTF-16 including surrogate pairs. The destination buffer - * must be large enough to hold the result as measured by utf8_to_utf16_length - * plus an added NULL terminator. + * Convert UTF-8 to UTF-16 including surrogate pairs. At most dstLen - 1 + * characters are written; it won't emit half a surrogate pair; and a NUL terminator is appended + * after. dstLen - 1 can be measured beforehand using utf8_to_utf16_length. Aborts if dstLen == 0 + * (at least one character is needed for the NUL terminator) or dstLen > SSIZE_MAX (the latter + * case being likely a negative number returned as an error and casted to unsigned) . Returns a + * pointer to the NUL terminator. */ -void utf8_to_utf16(const uint8_t* src, size_t srcLen, char16_t* dst); - -/** - * Like utf8_to_utf16_no_null_terminator, but you can supply a maximum length of the - * decoded string. The decoded string will fill up to that length; if it is longer - * the returned pointer will be to the character after dstLen. - */ -char16_t* utf8_to_utf16_n(const uint8_t* src, size_t srcLen, char16_t* dst, size_t dstLen); +char16_t *utf8_to_utf16( + const uint8_t* src, size_t srcLen, char16_t* dst, size_t dstLen); } diff --git a/libutils/String16.cpp b/libutils/String16.cpp index ac12d8aa7..9f5cfeab4 100644 --- a/libutils/String16.cpp +++ b/libutils/String16.cpp @@ -71,7 +71,7 @@ static char16_t* allocFromUTF8(const char* u8str, size_t u8len) u8cur = (const uint8_t*) u8str; char16_t* u16str = (char16_t*)buf->data(); - utf8_to_utf16(u8cur, u8len, u16str); + utf8_to_utf16(u8cur, u8len, u16str, ((size_t) u16len) + 1); //printf("Created UTF-16 string from UTF-8 \"%s\":", in); //printHexData(1, str, buf->size(), 16, 1); diff --git a/libutils/Unicode.cpp b/libutils/Unicode.cpp index f1f8bc939..1aca8e782 100644 --- a/libutils/Unicode.cpp +++ b/libutils/Unicode.cpp @@ -16,6 +16,7 @@ #include +#include #include #if defined(_WIN32) @@ -535,7 +536,7 @@ static inline uint32_t utf8_to_utf32_codepoint(const uint8_t *src, size_t length //printf("Char at %p: len=%d, utf-16=%p\n", src, length, (void*)result); } -ssize_t utf8_to_utf16_length(const uint8_t* u8str, size_t u8len) +ssize_t utf8_to_utf16_length(const uint8_t* u8str, size_t u8len, bool overreadIsFatal) { const uint8_t* const u8end = u8str + u8len; const uint8_t* u8cur = u8str; @@ -545,6 +546,20 @@ ssize_t utf8_to_utf16_length(const uint8_t* u8str, size_t u8len) while (u8cur < u8end) { u16measuredLen++; int u8charLen = utf8_codepoint_len(*u8cur); + // Malformed utf8, some characters are beyond the end. + // Cases: + // If u8charLen == 1, this becomes u8cur >= u8end, which cannot happen as u8cur < u8end, + // then this condition fail and we continue, as expected. + // If u8charLen == 2, this becomes u8cur + 1 >= u8end, which fails only if + // u8cur == u8end - 1, that is, there was only one remaining character to read but we need + // 2 of them. This condition holds and we return -1, as expected. + if (u8cur + u8charLen - 1 >= u8end) { + if (overreadIsFatal) { + LOG_ALWAYS_FATAL("Attempt to overread computing length of utf8 string"); + } else { + return -1; + } + } uint32_t codepoint = utf8_to_utf32_codepoint(u8cur, u8charLen); if (codepoint > 0xFFFF) u16measuredLen++; // this will be a surrogate pair in utf16 u8cur += u8charLen; @@ -561,38 +576,21 @@ ssize_t utf8_to_utf16_length(const uint8_t* u8str, size_t u8len) return u16measuredLen; } -char16_t* utf8_to_utf16_no_null_terminator(const uint8_t* u8str, size_t u8len, char16_t* u16str) -{ - const uint8_t* const u8end = u8str + u8len; - const uint8_t* u8cur = u8str; - char16_t* u16cur = u16str; - - while (u8cur < u8end) { - size_t u8len = utf8_codepoint_len(*u8cur); - uint32_t codepoint = utf8_to_utf32_codepoint(u8cur, u8len); - - // Convert the UTF32 codepoint to one or more UTF16 codepoints - if (codepoint <= 0xFFFF) { - // Single UTF16 character - *u16cur++ = (char16_t) codepoint; - } else { - // Multiple UTF16 characters with surrogates - codepoint = codepoint - 0x10000; - *u16cur++ = (char16_t) ((codepoint >> 10) + 0xD800); - *u16cur++ = (char16_t) ((codepoint & 0x3FF) + 0xDC00); - } - - u8cur += u8len; - } - return u16cur; -} - -void utf8_to_utf16(const uint8_t* u8str, size_t u8len, char16_t* u16str) { - char16_t* end = utf8_to_utf16_no_null_terminator(u8str, u8len, u16str); +char16_t* utf8_to_utf16(const uint8_t* u8str, size_t u8len, char16_t* u16str, size_t u16len) { + // A value > SSIZE_MAX is probably a negative value returned as an error and casted. + LOG_ALWAYS_FATAL_IF(u16len == 0 || u16len > SSIZE_MAX, "u16len is %zu", u16len); + char16_t* end = utf8_to_utf16_no_null_terminator(u8str, u8len, u16str, u16len - 1); *end = 0; + return end; } -char16_t* utf8_to_utf16_n(const uint8_t* src, size_t srcLen, char16_t* dst, size_t dstLen) { +char16_t* utf8_to_utf16_no_null_terminator( + const uint8_t* src, size_t srcLen, char16_t* dst, size_t dstLen) { + if (dstLen == 0) { + return dst; + } + // A value > SSIZE_MAX is probably a negative value returned as an error and casted. + LOG_ALWAYS_FATAL_IF(dstLen > SSIZE_MAX, "dstLen is %zu", dstLen); const uint8_t* const u8end = src + srcLen; const uint8_t* u8cur = src; const char16_t* const u16end = dst + dstLen; diff --git a/libutils/tests/Unicode_test.cpp b/libutils/tests/Unicode_test.cpp index c263f75e2..d23e43a71 100644 --- a/libutils/tests/Unicode_test.cpp +++ b/libutils/tests/Unicode_test.cpp @@ -98,7 +98,7 @@ TEST_F(UnicodeTest, UTF8toUTF16Normal) { char16_t output[1 + 1 + 1 + 2 + 1]; // Room for NULL - utf8_to_utf16(str, sizeof(str), output); + utf8_to_utf16(str, sizeof(str), output, sizeof(output) / sizeof(output[0])); EXPECT_EQ(0x0030, output[0]) << "should be U+0030"; @@ -147,4 +147,15 @@ TEST_F(UnicodeTest, strstr16TargetNotPresent) { EXPECT_EQ(nullptr, result); } +// http://b/29267949 +// Test that overreading in utf8_to_utf16_length is detected +TEST_F(UnicodeTest, InvalidUtf8OverreadDetected) { + // An utf8 char starting with \xc4 is two bytes long. + // Add extra zeros so no extra memory is read in case the code doesn't + // work as expected. + static char utf8[] = "\xc4\x00\x00\x00"; + ASSERT_DEATH(utf8_to_utf16_length((uint8_t *) utf8, strlen(utf8), + true /* overreadIsFatal */), "" /* regex for ASSERT_DEATH */); +} + } From 1cfa56d46c7c31300c25dafd722ff60a5294c3b3 Mon Sep 17 00:00:00 2001 From: Sergio Giro Date: Tue, 28 Jun 2016 18:02:29 +0100 Subject: [PATCH 2/2] 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 7316665e6..a13f34779 100644 --- a/include/utils/Unicode.h +++ b/include/utils/Unicode.h @@ -88,7 +88,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 nul-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". @@ -110,7 +110,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 * NUL 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 ad4528291..cacaf91b8 100644 --- a/libutils/String8.cpp +++ b/libutils/String8.cpp @@ -104,20 +104,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) @@ -126,21 +127,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 1aca8e782..5f96efa79 100644 --- a/libutils/Unicode.cpp +++ b/libutils/Unicode.cpp @@ -14,6 +14,7 @@ * limitations under the License. */ +#include #include #include @@ -183,7 +184,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; @@ -194,9 +195,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'; } @@ -349,7 +353,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; @@ -370,9 +374,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'; } @@ -433,10 +440,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 01e64f60a..3947a5fb9 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 @@ -77,4 +78,22 @@ TEST_F(String8Test, SetToSizeMaxReturnsNoMemory) { EXPECT_EQ(NO_MEMORY, String8("").setTo(in, SIZE_MAX)); } +// 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()); +} + }