140917 2015-01-26 19:05:38 -0800 Windows return -1 when calling vsnprintf with arguments that exceed target buffer size 2015-01-30 05:22:19 -0800 1 1 1 Unclassified WebKit Web Template Framework 528+ (Nightly build) PC Windows 7 RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=140945 https://bugs.webkit.org/show_bug.cgi?id=140847 https://bugs.webkit.org/show_bug.cgi?id=141078 InRadar P2 Normal --- 1 nakim webkit-unassigned benjamin bfulgham cmarcelo commit-queue webkit-bug-importer oldest_to_newest 1064345 0 nakim 2015-01-26 19:05:38 -0800 According to https://msdn.microsoft.com/en-us/library/1kt27hek.aspx windows return -1 when calling vsnprintf with arguments that exceed target buffer size. e.g.) char buf[5]; int return = vsnprintf(buf, 5, "%s", "0123456789"); This API return -1 while other platforms (linux, bsd) return 10 instead. Other platform's reference can be found below. bsd - https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man3/vsnprintf.3.html linux - http://man7.org/linux/man-pages/man3/vsnprintf.3.html I found this bug while testing JSC::Profiler::Database which announced in https://docs.google.com/document/d/18MQU5Dm31g4cVweuQuGofQAxfbenAAsE_njeTUuKOVA/edit from windows platform. Current WebKit's vsnprintf use looks safe to not overflow the buffer. But when this profiler tries to dump its source code by using WTF::StringPrintStream::toCString(), windows' vsnprintf's -1 return value eventually *broke* WTF::StringPrintStream::vprintf() function's buffer grow logic. Since WTF::StringPrintStream::toCString() buffer grow logic depends to other (linux, bsd) platform's vsnprintf behavior, we need to fix return value of wtf_vsnprintf (which is windows' polyfill of vsnprintf) to same as other platform. 1064346 1 nakim 2015-01-26 19:06:35 -0800 I'll submit a patch also. 1064430 2 245424 nakim 2015-01-26 22:31:33 -0800 Created attachment 245424 Patch 1064748 3 245424 bfulgham 2015-01-27 18:09:57 -0800 Comment on attachment 245424 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245424&action=review Thank you for noticing this! I had to do some reading just now to understand your change. > Source/WTF/wtf/StringExtras.h:65 > + result = _vscprintf(format, args); I see; MSVC returns negative in the case of a 'buffer overrun converted to a truncation', while POSIX systems return the count as if nothing bad happened. Very nice fix! 1064749 4 bfulgham 2015-01-27 18:10:38 -0800 Thank you for tracking this down. Do we have any other cases where a similar mistake is being made? 1064750 5 245424 bfulgham 2015-01-27 18:13:38 -0800 Comment on attachment 245424 Patch Looking over this file, it seems like our 'snprintf' (a few lines above) suffers from the exact same problem. Could you please make the same correction there? Then we can land the patch in one go. r- to request the additional fix be done for 'snprintf'. Otherwise, this looks great! 1064762 6 nakim 2015-01-27 18:39:02 -0800 > Thank you for tracking this down. Do we have any other cases where a similar mistake is being made? MSVC version of vprintf_stderr_common (in Assertions.cpp) has similar (or worse) problem. If result debug formatted string length is exactly 1024 bytes, there is no null character placed in the buffer. It results following OutputDebugStringA call prints some garbage bytes also. I think we simply fix it by calling vsnprintf (which is polyfilled by wtf_vsnprintf when using windows) instead of _vsnprintf. 1064763 7 nakim 2015-01-27 18:43:56 -0800 > Looking over this file, it seems like our 'snprintf' (a few lines above) suffers from the exact same problem. Could you please make the same correction there? Then we can land the patch in one go. I fixed it too. Correcting handled before va_end calling. See below hunk please. inline int snprintf(char* buffer, size_t count, const char* format, ...) { int result; va_list args; va_start(args, format); result = _vsnprintf(buffer, count, format, args); + if (result < 0 && errno != EINVAL) + result = _vscprintf(format, args); va_end(args); 1064942 8 bfulgham 2015-01-28 10:06:26 -0800 (In reply to comment #7) > > Looking over this file, it seems like our 'snprintf' (a few lines above) suffers from the exact same problem. Could you please make the same correction there? Then we can land the patch in one go. > > I fixed it too. Correcting handled before va_end calling. See below hunk > please. > > inline int snprintf(char* buffer, size_t count, const char* format, ...) > { > int result; > va_list args; > va_start(args, format); > result = _vsnprintf(buffer, count, format, args); > + if (result < 0 && errno != EINVAL) > + result = _vscprintf(format, args); > va_end(args); Were you planning up submitting an updated patch? Let me know and I'll try to get this in as soon as you have an update. 1065002 9 bfulgham 2015-01-28 12:49:48 -0800 This bug could have the following impacts: 1. Our NSAPI plugin tests. 2. Tests involving EditingDelegate dump() operations. 3. Stuff passing through JSC's globalFuncEscape method. 4. JSC date functions (dateProtoFuncToISOString) 5. XML Document parser 6. Windows clipboard utilities. 7. TextStream/TextCodec clients It will be interesting to see if any of the Windows JSC Date tests start working once this fix is in place. 1065004 10 webkit-bug-importer 2015-01-28 12:51:25 -0800 <rdar://problem/19635602> 1065092 11 nakim 2015-01-28 17:41:57 -0800 (In reply to comment #8) > (In reply to comment #7) > > > Looking over this file, it seems like our 'snprintf' (a few lines above) suffers from the exact same problem. Could you please make the same correction there? Then we can land the patch in one go. > > > > I fixed it too. Correcting handled before va_end calling. See below hunk > > please. > > > > inline int snprintf(char* buffer, size_t count, const char* format, ...) > > { > > int result; > > va_list args; > > va_start(args, format); > > result = _vsnprintf(buffer, count, format, args); > > + if (result < 0 && errno != EINVAL) > > + result = _vscprintf(format, args); > > va_end(args); > > Were you planning up submitting an updated patch? Let me know and I'll try > to get this in as soon as you have an update. What I mean is, that hunk already in the patch I submitted. https://bugs.webkit.org/attachment.cgi?id=245424&action=diff If I miss understand something on this context, please let me know. 1065093 12 bfulgham 2015-01-28 17:46:56 -0800 (In reply to comment #11) > What I mean is, that hunk already in the patch I submitted. > https://bugs.webkit.org/attachment.cgi?id=245424&action=diff > > If I miss understand something on this context, please let me know. Ah! you are right. Sorry! But I also though you mentioned that something about a similar error in Assertions.cpp that needed correcting? 1065097 13 245424 bfulgham 2015-01-28 17:49:57 -0800 Comment on attachment 245424 Patch r=me. We can deal with the Assertions.cpp change elsewhere. 1065098 14 bfulgham 2015-01-28 17:50:35 -0800 I'll land this as-is. Could you please also package the Assertion.cpp change you mentioned as a separate bug and we'll get that landed, too! 1065119 15 245424 commit-queue 2015-01-28 18:32:19 -0800 Comment on attachment 245424 Patch Clearing flags on attachment: 245424 Committed r179325: <http://trac.webkit.org/changeset/179325> 1065120 16 commit-queue 2015-01-28 18:32:23 -0800 All reviewed patches have been landed. Closing bug. 1065187 17 nakim 2015-01-29 01:34:30 -0800 (In reply to comment #14) > I'll land this as-is. Could you please also package the Assertion.cpp change > you mentioned as a separate bug and we'll get that landed, too! Thank you. I'll pile a bug for that. And I'll check related bugs you linked also. 1065259 18 bfulgham 2015-01-29 09:06:07 -0800 (In reply to comment #9) > This bug could have the following impacts: > > 1. Our NSAPI plugin tests. > 2. Tests involving EditingDelegate dump() operations. > 3. Stuff passing through JSC's globalFuncEscape method. > 4. JSC date functions (dateProtoFuncToISOString) > 5. XML Document parser > 6. Windows clipboard utilities. > 7. TextStream/TextCodec clients > > It will be interesting to see if any of the Windows JSC Date tests start > working once this fix is in place. I reran the JSC date function tests, but unfortunately did not see any progressions after applying this patch. Furthermore, the main test units have not shown any new progressions since this change. So, this is a very useful fix but I don't think our test infrastructure was triggering any bad behavior due to this problem. 245424 2015-01-26 22:31:33 -0800 2015-01-28 18:32:19 -0800 Patch bug-140917-20150127153136.patch text/plain 2430 nakim U3VidmVyc2lvbiBSZXZpc2lvbjogMTc5MTU3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IGY4Yjk1OTcyOTgyMzliMmNmODYyYzU2 MjZhMzFmMjJlZjg4NDYzNzMuLjAxZDJhYmM1YjE0ZDhiODY4NzQwOWI0ZDNmNzA0YWI2Mzg4MDlh NGQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTUtMDEtMjYgIE5hbWhvb24gS2ltICA8bmFta2lt QGVhLmNvbT4KKworICAgICAgICBXaW5kb3dzIHJldHVybiAtMSB3aGVuIGNhbGxpbmcgdnNucHJp bnRmIHdpdGggYXJndW1lbnRzIHRoYXQgZXhjZWVkIHRhcmdldCBidWZmZXIgc2l6ZQorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQwOTE3CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgRml4IHJldHVybiB2YWx1 ZSBvZiB2c25wcmludGYgd2hlbiB3aW5kb3dzIEFQSSByZXR1cm4gLTEgdG8gZGVub3RlCisgICAg ICAgIHJlcXVlc3RlZCBidWZmZXIgZXhjZWVkZWQuIFJlcGxhY2UgcmV0dXJuIHZhbHVlIGJ5IGNh bGxpbmcgdnNjcHJpbnRmLgorCisgICAgICAgICogd3RmL1N0cmluZ0V4dHJhcy5oOgorICAgICAg ICAoc25wcmludGYpOiBGaXggcmV0dXJuIHZhbHVlIGJ5IGNhbGxpbmcgdnNjcHJpbnRmIHdoZW4g YnVmZmVyIGV4Y2VlZGVkLgorICAgICAgICAod3RmX3ZzbnByaW50Zik6IERpdHRvLgorCiAyMDE1 LTAxLTI0ICBDaHJpcyBEdW1leiAgPGNkdW1lekBhcHBsZS5jb20+CiAKICAgICAgICAgUHJvdmlk ZSBpbXBsZW1lbnRhdGlvbiBmb3IgV1RGOjpEZWZhdWx0SGFzaDxib29sPgpkaWZmIC0tZ2l0IGEv U291cmNlL1dURi93dGYvU3RyaW5nRXh0cmFzLmggYi9Tb3VyY2UvV1RGL3d0Zi9TdHJpbmdFeHRy YXMuaAppbmRleCBiMWFlYTUxOTA1MmYyZmY0NjgxZjc0YTY2ZDE3ZTI1YjZlZTE3MjA1Li4xOWEx MjI3MmU4NmQ3NTU3MjkwZTU5N2IwMzRmMWU5NzJjMTNkMGFkIDEwMDY0NAotLS0gYS9Tb3VyY2Uv V1RGL3d0Zi9TdHJpbmdFeHRyYXMuaAorKysgYi9Tb3VyY2UvV1RGL3d0Zi9TdHJpbmdFeHRyYXMu aApAQCAtMSw1ICsxLDYgQEAKIC8qCiAgKiBDb3B5cmlnaHQgKEMpIDIwMDYsIDIwMTAgQXBwbGUg SW5jLiBBbGwgcmlnaHRzIHJlc2VydmVkLgorICogQ29weXJpZ2h0IChDKSAyMDE1IEVsZWN0cm9u aWMgQXJ0cywgSW5jLiBBbGwgcmlnaHRzIHJlc2VydmVkLgogICoKICAqIFJlZGlzdHJpYnV0aW9u IGFuZCB1c2UgaW4gc291cmNlIGFuZCBiaW5hcnkgZm9ybXMsIHdpdGggb3Igd2l0aG91dAogICog bW9kaWZpY2F0aW9uLCBhcmUgcGVybWl0dGVkIHByb3ZpZGVkIHRoYXQgdGhlIGZvbGxvd2luZyBj b25kaXRpb25zCkBAIC0zNywxMiArMzgsMTYgQEAKICNpZiBDT01QSUxFUihNU1ZDKQogLy8gRklY TUU6IHdoeSBhIENPTVBJTEVSIGNoZWNrIGluc3RlYWQgb2YgT1M/IGFsc28sIHRoZXNlIHNob3Vs ZCBiZSBIQVZFIGNoZWNrcwogCisjaW5jbHVkZSA8ZXJybm8uaD4KKwogaW5saW5lIGludCBzbnBy aW50ZihjaGFyKiBidWZmZXIsIHNpemVfdCBjb3VudCwgY29uc3QgY2hhciogZm9ybWF0LCAuLi4p IAogewogICAgIGludCByZXN1bHQ7CiAgICAgdmFfbGlzdCBhcmdzOwogICAgIHZhX3N0YXJ0KGFy Z3MsIGZvcm1hdCk7CiAgICAgcmVzdWx0ID0gX3ZzbnByaW50ZihidWZmZXIsIGNvdW50LCBmb3Jt YXQsIGFyZ3MpOworICAgIGlmIChyZXN1bHQgPCAwICYmIGVycm5vICE9IEVJTlZBTCkKKyAgICAg ICAgcmVzdWx0ID0gX3ZzY3ByaW50Zihmb3JtYXQsIGFyZ3MpOwogICAgIHZhX2VuZChhcmdzKTsK IAogICAgIC8vIEluIHRoZSBjYXNlIHdoZXJlIHRoZSBzdHJpbmcgZW50aXJlbHkgZmlsbGVkIHRo ZSBidWZmZXIsIF92c25wcmludGYgd2lsbCBub3QKQEAgLTU2LDYgKzYxLDggQEAgaW5saW5lIGlu dCBzbnByaW50ZihjaGFyKiBidWZmZXIsIHNpemVfdCBjb3VudCwgY29uc3QgY2hhciogZm9ybWF0 LCAuLi4pCiBpbmxpbmUgZG91YmxlIHd0Zl92c25wcmludGYoY2hhciogYnVmZmVyLCBzaXplX3Qg Y291bnQsIGNvbnN0IGNoYXIqIGZvcm1hdCwgdmFfbGlzdCBhcmdzKQogewogICAgIGludCByZXN1 bHQgPSBfdnNucHJpbnRmKGJ1ZmZlciwgY291bnQsIGZvcm1hdCwgYXJncyk7CisgICAgaWYgKHJl c3VsdCA8IDAgJiYgZXJybm8gIT0gRUlOVkFMKQorICAgICAgICByZXN1bHQgPSBfdnNjcHJpbnRm KGZvcm1hdCwgYXJncyk7CiAKICAgICAvLyBJbiB0aGUgY2FzZSB3aGVyZSB0aGUgc3RyaW5nIGVu dGlyZWx5IGZpbGxlZCB0aGUgYnVmZmVyLCBfdnNucHJpbnRmIHdpbGwgbm90CiAgICAgLy8gbnVs bC10ZXJtaW5hdGUgaXQsIGJ1dCB2c25wcmludGYgbXVzdC4K