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.
I'll submit a patch also.
Created attachment 245424 [details] Patch
Comment on attachment 245424 [details] 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!
Thank you for tracking this down. Do we have any other cases where a similar mistake is being made?
Comment on attachment 245424 [details] 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!
> 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.
> 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);
(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.
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.
<rdar://problem/19635602>
(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.
(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?
Comment on attachment 245424 [details] Patch r=me. We can deal with the Assertions.cpp change elsewhere.
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!
Comment on attachment 245424 [details] Patch Clearing flags on attachment: 245424 Committed r179325: <http://trac.webkit.org/changeset/179325>
All reviewed patches have been landed. Closing bug.
(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.
(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.