Bug 140917 – Windows return -1 when calling vsnprintf with arguments that exceed target buffer size

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Namhoon Kim]

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.

[Namhoon Kim]

I'll submit a patch also.

[Namhoon Kim]

Created attachment 245424 [details]
Patch

[Brent Fulgham]

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!

[Brent Fulgham]

Thank you for tracking this down. Do we have any other cases where a similar mistake is being made?

[Brent Fulgham]

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!

[Namhoon Kim]

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

[Namhoon Kim]

> 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);

[Brent Fulgham]

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

[Brent Fulgham]

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.

[Radar WebKit Bug Importer]

<rdar://problem/19635602>

[Namhoon Kim]

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

[Brent Fulgham]

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

[Brent Fulgham]

Comment on attachment 245424 [details]
Patch

r=me. We can deal with the Assertions.cpp change elsewhere.

[Brent Fulgham]

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!

[WebKit Commit Bot]

Comment on attachment 245424 [details]
Patch

Clearing flags on attachment: 245424

Committed r179325: <http://trac.webkit.org/changeset/179325>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Namhoon Kim]

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

[Brent Fulgham]

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