Undefined behavior in SQLiteStatement::getColumnText

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 260253 [details]
Patch

[Michael Catanzaro]

Note: I checked the rest of this file to make sure this pattern doesn't occur elsewhere.

[Michael Catanzaro]

(Easy review here!)

[Brady Eidson]

(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.

[Darin Adler]

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.

[Darin Adler]

(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.

[Michael Catanzaro]

I think you're right. Thanks for the thorough review!