Bug 59949 – Avoid potential buffer overflow in WTFLog() and WTFLogVerbose()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeff Miller]

Avoid potential buffer underrun in WTFLog() and WTFLogVerbose()

[Jeff Miller]

Created attachment 91941 [details]
Avoid potential buffer underrun in WTFLog() and WTFLogVerbose()

[Alexey Proskuryakov]

Comment on attachment 91941 [details]
Avoid potential buffer underrun in WTFLog() and WTFLogVerbose()

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

> Source/JavaScriptCore/ChangeLog:5
> +        Avoid potential buffer underrun in WTFLog() and WTFLogVerbose()

I think it's a buffer overrun, not underrun - <http://en.wikipedia.org/wiki/Buffer_underrun>.

> Source/JavaScriptCore/wtf/Assertions.cpp:277
> +    if (!format)
> +        return;

I'm not sure if this is a good time to make this check after calling vprintf_stderr_common. Or if it's really necessary - crashing on null ptr access if fairly safe, even if we were guarding against malicious input, which we aren't really.

> Source/JavaScriptCore/wtf/Assertions.cpp:279
> +    size_t formatLen = strlen(format);

Please don't abbreviate.

> Source/JavaScriptCore/wtf/Assertions.cpp:294
> +    if (!format) {

Same comment about necessity of the check.

[Jeff Miller]

(In reply to comment #2)
> (From update of attachment 91941 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91941&action=review
> 
> > Source/JavaScriptCore/ChangeLog:5
> > +        Avoid potential buffer underrun in WTFLog() and WTFLogVerbose()
> 
> I think it's a buffer overrun, not underrun - <http://en.wikipedia.org/wiki/Buffer_underrun>.

Heh, actually according to that article it's always a buffer overflow when talking about memory.

> 
> > Source/JavaScriptCore/wtf/Assertions.cpp:277
> > +    if (!format)
> > +        return;
> 
> I'm not sure if this is a good time to make this check after calling vprintf_stderr_common. Or if it's really necessary - crashing on null ptr access if fairly safe, even if we were guarding against malicious input, which we aren't really.

OK, I'll remove the check.

> 
> > Source/JavaScriptCore/wtf/Assertions.cpp:279
> > +    size_t formatLen = strlen(format);
> 
> Please don't abbreviate.

Changed to formatLength.

> 
> > Source/JavaScriptCore/wtf/Assertions.cpp:294
> > +    if (!format) {
> 
> Same comment about necessity of the check.

Also removed this check.

[Jeff Miller]

Landed http://trac.webkit.org/changeset/85496