Bug 94522 – Unsafe vsprintf usage in TestNetscapePlugin

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Nate Chapin]

Original report: http://code.google.com/p/chromium/issues/detail?id=141806

This is test-only code, so not marking it as a security issue.  However, it is triggering crashes in ClusterFuzz, so it seems worth it to clean up.

Using vsnprintf and a proper buffer size seems to fix the problem.

[Nate Chapin]

Created attachment 159511 [details]
patch

[Adam Barth]

Comment on attachment 159511 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159511&action=review

> Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:66
>      char message[2048] = "PLUGIN: ";
> -    vsprintf(message + strlen(message), format, args);
> +    vsnprintf(message + strlen(message), 2040, format, args);

Why 2040 and not 2048 ?  Also, these libc functions don't necessarily \0-terminate strings, so it's a good practice to explicitly write a '\0' at the end of the array after calling these functions.

[Nate Chapin]

(In reply to comment #2)
> (From update of attachment 159511 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159511&action=review
> 
> > Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:66
> >      char message[2048] = "PLUGIN: ";
> > -    vsprintf(message + strlen(message), format, args);
> > +    vsnprintf(message + strlen(message), 2040, format, args);
> 
> Why 2040 and not 2048 ?  Also, these libc functions don't necessarily \0-terminate strings, so it's a good practice to explicitly write a '\0' at the end of the array after calling these functions.

Because the first 8 bytes are the "PLUGIN: ", and I'm a terrible person who uses magic numbers. Any preference on how I make this clearer?

Good point on the '\0'.

[Thomas Sepez]

Comment on attachment 159511 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159511&action=review

>> Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:66
>> +    vsnprintf(message + strlen(message), 2040, format, args);
> 
> Why 2040 and not 2048 ?  Also, these libc functions don't necessarily \0-terminate strings, so it's a good practice to explicitly write a '\0' at the end of the array after calling these functions.

Also, so long as your calling strlen(), seems like both of these values should derive from that - as in 
2048 - strlen(message).  But don't call strlen() twice.  sizeof("PLUGIN: ") -1 is also a suitable alternate.

[Adam Barth]

Comment on attachment 159511 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159511&action=review

>>> Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:66
>>> +    vsnprintf(message + strlen(message), 2040, format, args);
>> 
>> Why 2040 and not 2048 ?  Also, these libc functions don't necessarily \0-terminate strings, so it's a good practice to explicitly write a '\0' at the end of the array after calling these functions.
> 
> Because the first 8 bytes are the "PLUGIN: ", and I'm a terrible person who uses magic numbers. Any preference on how I make this clearer?
> 
> Good point on the '\0'.

I see.  Yeah, that's easy to for dumb people like me to miss.  :)

const size_t messageBufferSize = 2048;
char message[2048] = "PLUGIN: ";
messageLength = strlen(message);
vsnprintf(message + messageLength, messageBufferSize - messageLength, format, args);
message[messageBufferSize - 1] = '\0';

[Thomas Sepez]

> char message[2048] = "PLUGIN: ";
> messageLength = sizeof("PLUGIN: ") - 1;
> vsnprintf(message + messageLength, sizeof(message) - messageLength, format, args);
> message[sizeof(message) - 1] = '\0';

[Adam Barth]

tsepez puts me to shame, as usual :)

[Thomas Sepez]

Still not happy that we say "Plugin" twice.  You should be able to tweak that string or the dimension of the buffer in one place and only one place and still have the code work.  Let's see:

> char message[2048];
> static const char plugin[] = "PLUGIN: ";
> memcpy(message, plugin, sizeof(plugin) - 1);
> vsnprintf(message + sizeof(plugin) - 1, sizeof(message) - (sizeof(plugin) - 1), format, args);
> message[sizeof(message) - 1] = '\0';

But I've not tried it.

[Nate Chapin]

Created attachment 159518 [details]
patch

[Adam Barth]

Comment on attachment 159518 [details]
patch

This isn't my favorite idiom, but whatever man.

[Nate Chapin]

(In reply to comment #10)
> (From update of attachment 159518 [details])
> This isn't my favorite idiom, but whatever man.

The way I wrote the code, or any code that matches .*printf.* ? :)

[Adam Barth]

> The way I wrote the code, or any code that matches .*printf.* ? :)

:)

[Adam Barth]

I think the root of the issue is that .*printf.* are so impossible to use correctly that they make the code that calls them super ugly, regardless of how it's written.

[WebKit Review Bot]

Comment on attachment 159518 [details]
patch

Clearing flags on attachment: 159518

Committed r126089: <http://trac.webkit.org/changeset/126089>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.