Summary: | OOB read of stack buffer below DoubleToStringConverter::CreateExponentialRepresentation() in debug builds | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Thomas Sepez <tsepez> | ||||
Component: | WebCore Misc. | Assignee: | Thomas Sepez <tsepez> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | darin, eae, inferno, leviw, mhahnenberg, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Thomas Sepez
2012-07-18 10:54:49 PDT
Created attachment 153076 [details]
Patch
Comment on attachment 153076 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153076&action=review Was the testcase minimizable ? i know it looked ugly on ClusterFuzz. > Source/WTF/ChangeLog:9 > + (DoubleToStringConverter::CreateExponentialRepresentation): NUL-terminate string buffer before passing it to StringBuilder::AddSubstring() typo s/NUL/NULL > Source/WTF/wtf/dtoa/double-conversion.cc:107 > + buffer[first_char_pos] = '\0'; nit: it will be more readable to use kMaxExponentLength instead of first_char_pos. (In reply to comment #2) > (From update of attachment 153076 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153076&action=review > > Was the testcase minimizable ? i know it looked ugly on ClusterFuzz. > > > Source/WTF/ChangeLog:9 > > + (DoubleToStringConverter::CreateExponentialRepresentation): NUL-terminate string buffer before passing it to StringBuilder::AddSubstring() > > typo s/NUL/NULL I've always believed that NULL is a pointer but NUL is the character (3-letter abbr from the TTY days, see man ascii). There's some use of this form in WebCore/icu for example. Let me know which way you want it. > > > Source/WTF/wtf/dtoa/double-conversion.cc:107 > > + buffer[first_char_pos] = '\0'; > > nit: it will be more readable to use kMaxExponentLength instead of first_char_pos. Perhaps. But the rest of the assignments are relative to first_char_pos. If later on, someone decides to initialize first_char_pos to some other value, then this code still works unchanged and we don't get a gap between the characters from the loop below and the terminator. Or so was my thinking when I did it this way. Let me know which way you want it. Comment on attachment 153076 [details]
Patch
Thanks for the explanation.
(In reply to comment #4) > (From update of attachment 153076 [details]) > Thanks for the explanation. RE: testcase. It's not practical without automated ASAN/Valgrind debug binary setup to test this. Had there been one, this would go off all over the place as lots of existing code/tests hits this path (opening the devtools window does it pretty reliably on my chromium binary). Comment on attachment 153076 [details] Patch Clearing flags on attachment: 153076 Committed r123027: <http://trac.webkit.org/changeset/123027> All reviewed patches have been landed. Closing bug. |