Extracted from https://bugs.webkit.org/show_bug.cgi?id=91151 The ASAN-ified debug build trips over an OOB read in an assert() which (obviously) is not present in the release build. Although this has no user impact, it limits the use of ASAN debug builds to diagnose issues. The call stack looks like: ================================================================= ==6469== ERROR: AddressSanitizer stack-buffer-overflow on address 0x7fffaf275165 at pc 0x1200649 bp 0x7fffaf274c30 sp 0x7fffaf274c10 READ of size 1 at 0x7fffaf275165 thread T0 #0 0x1200649 in __interceptor_strlen #1 0x7f5f5ded5a6a in WTF::double_conversion::StringBuilder::AddSubstring(char const*, int) third_party/WebKit/Source/WTF/wtf/dtoa/utils.h:231 #2 0x7f5f5dec6155 in WTF::double_conversion::DoubleToStringConverter::CreateExponentialRepresentation(char const*, int, int, WTF::double_conversion::StringBuilder*) const third_party/WebKit/Source/WTF/wtf/dtoa/double-conversion.cc:113 #3 0x7f5f5dec8a1b in WTF::double_conversion::DoubleToStringConverter::ToShortest(double, WTF::double_conversion::StringBuilder*) const third_party/WebKit/Source/WTF/wtf/dtoa/double-conversion.cc:195 #4 0x7f5f5de5fb8b in WTF::numberToString(double, char*) third_party/WebKit/Source/WTF/wtf/dtoa.cpp:1235 #5 0x7f5f5f6825ce in WebCore::Decimal::fromDouble(double) third_party/WebKit/Source/WebCore/platform/Decimal.cpp:686 #6 0x7f5f5f3f23bb in WebCore::parseToDecimalForNumberType(WTF::String const&, WebCore::Decimal const&) third_party/WebKit/Source/WebCore/html/parser/HTMLParserIdioms.cpp:96 This appears to be introduced in 94452. The reason for this is that in wtf/dtoa/utils.h, StringBuilder::addSubstring() takes both a pointer and a length, but asserts: ASSERT(static_cast<size_t>(n) <= strlen(s)); which implies that the input |s| must be NUL-terminated despite otherwise being fully described by the length. DoubleToStringConverter::CreateExponentialRepresentation() declares an array char buffer[kMaxExponentLength]; but doesn't NUL-terminate it before passing it to addSubstring(). The other callers of addSubstring() in this file appear be passing NUL-terminated buffers, so the easiest fix is to NUL-terminate this one as well. I'll cobble up a patch to correct this as it's blocking some of my other ASAN work.
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.