91642 2012-07-18 10:54:49 -0700 OOB read of stack buffer below DoubleToStringConverter::CreateExponentialRepresentation() in debug builds 2012-07-18 15:16:26 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 tsepez tsepez darin eae inferno leviw mhahnenberg webkit.review.bot oldest_to_newest 672526 0 tsepez 2012-07-18 10:54:49 -0700 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. 672700 1 153076 tsepez 2012-07-18 13:28:06 -0700 Created attachment 153076 Patch 672722 2 153076 inferno 2012-07-18 13:52:50 -0700 Comment on attachment 153076 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. 672732 3 tsepez 2012-07-18 14:11:52 -0700 (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. 672739 4 153076 inferno 2012-07-18 14:18:05 -0700 Comment on attachment 153076 Patch Thanks for the explanation. 672750 5 tsepez 2012-07-18 14:23:05 -0700 (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). 672825 6 153076 webkit.review.bot 2012-07-18 15:16:21 -0700 Comment on attachment 153076 Patch Clearing flags on attachment: 153076 Committed r123027: <http://trac.webkit.org/changeset/123027> 672826 7 webkit.review.bot 2012-07-18 15:16:26 -0700 All reviewed patches have been landed. Closing bug. 153076 2012-07-18 13:28:06 -0700 2012-07-18 15:16:21 -0700 Patch patch-91642.txt text/plain 1414 tsepez SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAxMjMwMDYpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDEzIEBACisyMDEyLTA3LTE4ICBUb20gU2VwZXogIDx0c2VwZXpA Y2hyb21pdW0ub3JnPgorCisgICAgICAgIE9PQiByZWFkIG9mIHN0YWNrIGJ1ZmZlciBiZWxvdyBE b3VibGVUb1N0cmluZ0NvbnZlcnRlcjo6Q3JlYXRlRXhwb25lbnRpYWxSZXByZXNlbnRhdGlvbigp IGluIGRlYnVnIGJ1aWxkcworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9OTE2NDIKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICAqIHd0Zi9kdG9hL2RvdWJsZS1jb252ZXJzaW9uLmNjOgorICAgICAgICAoRG91Ymxl VG9TdHJpbmdDb252ZXJ0ZXI6OkNyZWF0ZUV4cG9uZW50aWFsUmVwcmVzZW50YXRpb24pOiBOVUwt dGVybWluYXRlIHN0cmluZyBidWZmZXIgYmVmb3JlIHBhc3NpbmcgaXQgdG8gU3RyaW5nQnVpbGRl cjo6QWRkU3Vic3RyaW5nKCkKKyAgICAgICAgCiAyMDEyLTA3LTE4ICBSb2IgQnVpcyAgPHJidWlz QHJpbS5jb20+CiAKICAgICAgICAgQWxpZ25tZW50IGNyYXNoIGluIE1JTUVTbmlmZmVyCkluZGV4 OiBTb3VyY2UvV1RGL3d0Zi9kdG9hL2RvdWJsZS1jb252ZXJzaW9uLmNjCj09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0t IFNvdXJjZS9XVEYvd3RmL2R0b2EvZG91YmxlLWNvbnZlcnNpb24uY2MJKHJldmlzaW9uIDEyMzAw MCkKKysrIFNvdXJjZS9XVEYvd3RmL2R0b2EvZG91YmxlLWNvbnZlcnNpb24uY2MJKHdvcmtpbmcg Y29weSkKQEAgLTEwMiw4ICsxMDIsOSBAQCBuYW1lc3BhY2UgZG91YmxlX2NvbnZlcnNpb24gewog ICAgICAgICB9CiAgICAgICAgIEFTU0VSVChleHBvbmVudCA8IDFlNCk7CiAgICAgICAgIGNvbnN0 IGludCBrTWF4RXhwb25lbnRMZW5ndGggPSA1OwotICAgICAgICBjaGFyIGJ1ZmZlcltrTWF4RXhw b25lbnRMZW5ndGhdOworICAgICAgICBjaGFyIGJ1ZmZlcltrTWF4RXhwb25lbnRMZW5ndGggKyAx XTsKICAgICAgICAgaW50IGZpcnN0X2NoYXJfcG9zID0ga01heEV4cG9uZW50TGVuZ3RoOworICAg ICAgICBidWZmZXJbZmlyc3RfY2hhcl9wb3NdID0gJ1wwJzsKICAgICAgICAgd2hpbGUgKGV4cG9u ZW50ID4gMCkgewogICAgICAgICAgICAgYnVmZmVyWy0tZmlyc3RfY2hhcl9wb3NdID0gJzAnICsg KGV4cG9uZW50ICUgMTApOwogICAgICAgICAgICAgZXhwb25lbnQgLz0gMTA7Cg==