There is undefined behavior in SQLiteStatement::getColumnText. sqlite3_column_text16 can trigger an internal type conversion, which would invalidate the result of sqlite3_column_bytes16. Here they are called as two different parameters to the same function, so the order is undefined. But the correctness of this function relies on sqlite3_column_text16 being called before sqlite3_column_bytes16.
Created attachment 260253 [details] Patch
Note: I checked the rest of this file to make sure this pattern doesn't occur elsewhere.
(Easy review here!)
(In reply to comment #3) > (Easy review here!) This is such a subtle thing, I think it'd be awesome if the theoretical issue could be proven with a test.
Comment on attachment 260253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260253&action=review > Source/WebCore/ChangeLog:15 > + There is undefined behavior in SQLiteStatement::getColumnText. sqlite3_column_text16 can > + trigger an internal type conversion, which would invalidate the result of > + sqlite3_column_bytes16. Here they are called as two different parameters to the same > + function, so the order of execution is undefined. But the correctness of this function > + relies on sqlite3_column_text16 being called before sqlite3_column_bytes16. Otherwise, we'll > + wind up either truncating the string we create if the size is too short, or else creating it > + with random memory if it's too big. Fix this by calling the functions sequentially instead > + of doing it all in one statement. My reading of <https://sqlite.org/c3ref/column_blob.html> says this analysis is incorrect. Both sqlite3_column_text16 and sqlite3_column_bytes16 convert the string to UTF-16. It doesn’t matter which is called first. It’s true that the page recommends calling sqlite3_column_text16 first and then sqlite3_column_bytes16, but if you read carefully you’ll see that it’s equally safe to call sqlite3_column_bytes16 first and then sqlite3_column_text16, as long as you don’t call other functions. I think there is no bug here. > Source/WebCore/platform/sql/SQLiteStatement.cpp:346 > + // size of the result, so it must strictly preceed the call to sqlite3_column_bytes16. Spelling error here: precede is misspelled as “preceed” > Source/WebCore/platform/sql/SQLiteStatement.cpp:349 > + int length = sqlite3_column_bytes16(m_statement, col) / sizeof(UChar); > + return String(text, length); I wouldn’t split these into two separate lines.
(In reply to comment #4) > This is such a subtle thing, I think it'd be awesome if the theoretical > issue could be proven with a test. I don’t think that would be practical, because the ordering of evaluation of function arguments is undefined and so it’s compiler-dependent. If there was a bug here, it still might be impossible to reproduce on any given compiler. But as I said above, I don’t think there is a bug here.
I think you're right. Thanks for the thorough review!