148620 2015-08-30 14:40:28 -0700 Undefined behavior in SQLiteStatement::getColumnText 2015-12-03 02:44:32 -0800 1 1 1 Unclassified WebKit WebCore Misc. Other PC Linux RESOLVED INVALID P2 Normal --- 1 mcatanzaro mcatanzaro beidson cgarcia darin mcatanzaro mrobinson oldest_to_newest 1122077 0 mcatanzaro 2015-08-30 14:40:28 -0700 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. 1122080 1 260253 mcatanzaro 2015-08-30 14:58:01 -0700 Created attachment 260253 Patch 1127397 2 mcatanzaro 2015-09-19 11:05:48 -0700 Note: I checked the rest of this file to make sure this pattern doesn't occur elsewhere. 1145572 3 mcatanzaro 2015-12-02 08:38:26 -0800 (Easy review here!) 1145642 4 beidson 2015-12-02 11:30:50 -0800 (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. 1145717 5 260253 darin 2015-12-02 13:58:33 -0800 Comment on attachment 260253 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. 1145718 6 darin 2015-12-02 14:00:21 -0800 (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. 1145909 7 mcatanzaro 2015-12-03 02:44:32 -0800 I think you're right. Thanks for the thorough review! 260253 2015-08-30 14:58:01 -0700 2015-12-02 13:58:33 -0800 Patch bug-148620-20150830165745.patch text/plain 2535 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMTg5MTU5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggODk4ZjRiY2ViOTRiYjc1 MjcwYjJhNmM4ZmRiNmViNWRlNTFhMzllYi4uYWI2YjM3Y2IzODVkZDcwZTgwOGRkODIzN2JmMzM3 NDE1ZGFkZTY4MCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI0IEBACisyMDE1LTA4LTMwICBNaWNo YWVsIENhdGFuemFybyAgPG1jYXRhbnphcm9AaWdhbGlhLmNvbT4KKworICAgICAgICBVbmRlZmlu ZWQgYmVoYXZpb3IgaW4gU1FMaXRlU3RhdGVtZW50OjpnZXRDb2x1bW5UZXh0CisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNDg2MjAKKworICAgICAgICBS ZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGVyZSBpcyB1bmRlZmluZWQg YmVoYXZpb3IgaW4gU1FMaXRlU3RhdGVtZW50OjpnZXRDb2x1bW5UZXh0LiBzcWxpdGUzX2NvbHVt bl90ZXh0MTYgY2FuCisgICAgICAgIHRyaWdnZXIgYW4gaW50ZXJuYWwgdHlwZSBjb252ZXJzaW9u LCB3aGljaCB3b3VsZCBpbnZhbGlkYXRlIHRoZSByZXN1bHQgb2YKKyAgICAgICAgc3FsaXRlM19j b2x1bW5fYnl0ZXMxNi4gSGVyZSB0aGV5IGFyZSBjYWxsZWQgYXMgdHdvIGRpZmZlcmVudCBwYXJh bWV0ZXJzIHRvIHRoZSBzYW1lCisgICAgICAgIGZ1bmN0aW9uLCBzbyB0aGUgb3JkZXIgb2YgZXhl Y3V0aW9uIGlzIHVuZGVmaW5lZC4gQnV0IHRoZSBjb3JyZWN0bmVzcyBvZiB0aGlzIGZ1bmN0aW9u CisgICAgICAgIHJlbGllcyBvbiBzcWxpdGUzX2NvbHVtbl90ZXh0MTYgYmVpbmcgY2FsbGVkIGJl Zm9yZSBzcWxpdGUzX2NvbHVtbl9ieXRlczE2LiBPdGhlcndpc2UsIHdlJ2xsCisgICAgICAgIHdp bmQgdXAgZWl0aGVyIHRydW5jYXRpbmcgdGhlIHN0cmluZyB3ZSBjcmVhdGUgaWYgdGhlIHNpemUg aXMgdG9vIHNob3J0LCBvciBlbHNlIGNyZWF0aW5nIGl0CisgICAgICAgIHdpdGggcmFuZG9tIG1l bW9yeSBpZiBpdCdzIHRvbyBiaWcuIEZpeCB0aGlzIGJ5IGNhbGxpbmcgdGhlIGZ1bmN0aW9ucyBz ZXF1ZW50aWFsbHkgaW5zdGVhZAorICAgICAgICBvZiBkb2luZyBpdCBhbGwgaW4gb25lIHN0YXRl bWVudC4KKworICAgICAgICBObyBuZXcgdGVzdHMgYmVjYXVzZSBpdCB3b3JrcyBmb3IgbWUuIFRo aXMgaXMganVzdCBhIHRoZW9yZXRpY2FsIGlzc3VlLgorCisgICAgICAgICogcGxhdGZvcm0vc3Fs L1NRTGl0ZVN0YXRlbWVudC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpTUUxpdGVTdGF0ZW1lbnQ6 OmdldENvbHVtblRleHQpOgorCiAyMDE1LTA4LTI5ICBKZXNzaWUgQmVybGluICA8YmVybGluQGFw cGxlLmNvbT4KIAogICAgICAgICBFbCBDYXBpdGFuIGJ1aWxkIGZpeC4KZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJDb3JlL3BsYXRmb3JtL3NxbC9TUUxpdGVTdGF0ZW1lbnQuY3BwIGIvU291cmNlL1dl YkNvcmUvcGxhdGZvcm0vc3FsL1NRTGl0ZVN0YXRlbWVudC5jcHAKaW5kZXggMTM1YzAwYjE2ZDIz ODQ0ZTA4OTM5NjI4NGM3YmY3YjgwYWVkNjVkNy4uNmQ4MWFjMjBkMWI3ZmViNjEzZDkwOTJmNzM0 NTFlNjMzMjI4MjgwOCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vc3FsL1NR TGl0ZVN0YXRlbWVudC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vc3FsL1NRTGl0 ZVN0YXRlbWVudC5jcHAKQEAgLTM0MSw3ICszNDEsMTIgQEAgU3RyaW5nIFNRTGl0ZVN0YXRlbWVu dDo6Z2V0Q29sdW1uVGV4dChpbnQgY29sKQogICAgICAgICAgICAgcmV0dXJuIFN0cmluZygpOwog ICAgIGlmIChjb2x1bW5Db3VudCgpIDw9IGNvbCkKICAgICAgICAgcmV0dXJuIFN0cmluZygpOwot ICAgIHJldHVybiBTdHJpbmcocmVpbnRlcnByZXRfY2FzdDxjb25zdCBVQ2hhcio+KHNxbGl0ZTNf Y29sdW1uX3RleHQxNihtX3N0YXRlbWVudCwgY29sKSksIHNxbGl0ZTNfY29sdW1uX2J5dGVzMTYo bV9zdGF0ZW1lbnQsIGNvbCkgLyBzaXplb2YoVUNoYXIpKTsKKworICAgIC8vIE5vdGUgdGhhdCBz cWxpdGUzX2NvbHVtbl90ZXh0MTYgY2FuIGludGVybmFsbHkgcGVyZm9ybSBkYXRhIHR5cGUgY29u dmVyc2lvbnMsIGNoYW5naW5nIHRoZQorICAgIC8vIHNpemUgb2YgdGhlIHJlc3VsdCwgc28gaXQg bXVzdCBzdHJpY3RseSBwcmVjZWVkIHRoZSBjYWxsIHRvIHNxbGl0ZTNfY29sdW1uX2J5dGVzMTYu CisgICAgYXV0byB0ZXh0ID0gc3RhdGljX2Nhc3Q8Y29uc3QgVUNoYXIqPihzcWxpdGUzX2NvbHVt bl90ZXh0MTYobV9zdGF0ZW1lbnQsIGNvbCkpOworICAgIGludCBsZW5ndGggPSBzcWxpdGUzX2Nv bHVtbl9ieXRlczE2KG1fc3RhdGVtZW50LCBjb2wpIC8gc2l6ZW9mKFVDaGFyKTsKKyAgICByZXR1 cm4gU3RyaW5nKHRleHQsIGxlbmd0aCk7CiB9CiAgICAgCiBkb3VibGUgU1FMaXRlU3RhdGVtZW50 OjpnZXRDb2x1bW5Eb3VibGUoaW50IGNvbCkK