SVN: rev202088 Build with: perl Tools/Scripts/build-jsc --gtk --debug The following test case triggers this assertion error: --- ASSERTION FAILED: m_buffer->length() >= m_length --- Test case: --- var s = "a".repeat(0x7ffffff) var p = new Array(50).fill(s); var r = JSON.stringify(p) --- Stack trace: --- #0 0x00007ffff6de7098 in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:317 #1 0x00007ffff6e21883 in WTF::StringBuilder::appendUninitializedSlow<unsigned char> (this=0x7fffffffc790, requiredLength=4160749631) at ../../Source/WTF/wtf/text/StringBuilder.cpp:227 #2 0x00007ffff6e214d6 in WTF::StringBuilder::appendUninitialized<unsigned char> (this=0x7fffffffc790, length=1) at ../../Source/WTF/wtf/text/StringBuilder.cpp:215 #3 0x00007ffff6e1fc39 in WTF::StringBuilder::append (this=0x7fffffffc790, characters=0x7fffffffc4e4 ",\177", length=1) at ../../Source/WTF/wtf/text/StringBuilder.cpp:283 #4 0x00007ffff5f80add in WTF::StringBuilder::append (this=0x7fffffffc790, c=44 ',') at ../../Source/WTF/wtf/text/StringBuilder.h:144 #5 0x00007ffff5f80b0c in WTF::StringBuilder::append (this=0x7fffffffc790, c=44 ',') at ../../Source/WTF/wtf/text/StringBuilder.h:149 #6 0x00007ffff6c074c9 in JSC::Stringifier::Holder::appendNextProperty (this=0x7fffffffc8e8, stringifier=..., builder=...) at ../../Source/JavaScriptCore/runtime/JSONObject.cpp:493 #7 0x00007ffff6c06e53 in JSC::Stringifier::appendStringifiedValue (this=0x7fffffffc860, builder=..., value=..., holder=0x7fffaf1dbf40, propertyName=...) at ../../Source/JavaScriptCore/runtime/JSONObject.cpp:384 #8 0x00007ffff6c064c5 in JSC::Stringifier::stringify (this=0x7fffffffc860, value=...) at ../../Source/JavaScriptCore/runtime/JSONObject.cpp:259 #9 0x00007ffff6c08ee5 in JSC::JSONProtoFuncStringify (exec=0x7fffffffcb20) at ../../Source/JavaScriptCore/runtime/JSONObject.cpp:782 ... ---
<rdar://problem/26826254>
I can reproduce this easily. Looks like a simple case of needing to detect an imminent out of memory error.
(In reply to comment #2) > I can reproduce this easily. Looks like a simple case of needing to detect > an imminent out of memory error. Early detection of imminent out of memory error aside, this bug isn't what I expected. Fix coming soon.
Created attachment 281529 [details] proposed patch.
Comment on attachment 281529 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=281529&action=review > Source/WTF/wtf/text/StringBuilder.cpp:425 > + Checked<unsigned, RecordOverflow> stringLength = string.length(); Why is this also checked? Did you want to check if this*6 overflows? > Source/WTF/wtf/text/StringBuilder.cpp:426 > + Checked<unsigned, RecordOverflow> maximumCapacityRequired = length(); What happens when multiplying with a Checked<> that's overflowed? > Source/WTF/wtf/text/StringBuilder.cpp:428 > + RELEASE_ASSERT(!maximumCapacityRequired.hasOverflowed()); Is there a mode of Checked<> where it just crashes on overflow?
Comment on attachment 281529 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=281529&action=review >> Source/WTF/wtf/text/StringBuilder.cpp:425 >> + Checked<unsigned, RecordOverflow> stringLength = string.length(); > > Why is this also checked? > Did you want to check if this*6 overflows? Yes. This is to make sure that stringLength * 6 does not overflow. >> Source/WTF/wtf/text/StringBuilder.cpp:426 >> + Checked<unsigned, RecordOverflow> maximumCapacityRequired = length(); > > What happens when multiplying with a Checked<> that's overflowed? It carries the overflow status over to the result. >> Source/WTF/wtf/text/StringBuilder.cpp:428 >> + RELEASE_ASSERT(!maximumCapacityRequired.hasOverflowed()); > > Is there a mode of Checked<> where it just crashes on overflow? Good point. I had initially tried to make this function not crash on overflow, but that turned out to be the wrong goal for now (given that StringBuilder crashes elsewhere where overflow is detected). I will change the Checked instance to crash on overflow.
Created attachment 281567 [details] Patch for landing: addressed Saam's feedback.
Comment on attachment 281567 [details] Patch for landing: addressed Saam's feedback. View in context: https://bugs.webkit.org/attachment.cgi?id=281567&action=review > Source/WTF/wtf/text/StringBuilder.cpp:427 > + maximumCapacityRequired += 2 + stringLength * 6; I don't think this line will do as much checking as we want. a += b + c * d will evaluate: *, +, +=. The * and + will not be checked.
Comment on attachment 281567 [details] Patch for landing: addressed Saam's feedback. View in context: https://bugs.webkit.org/attachment.cgi?id=281567&action=review >> Source/WTF/wtf/text/StringBuilder.cpp:427 >> + maximumCapacityRequired += 2 + stringLength * 6; > > I don't think this line will do as much checking as we want. > > a += b + c * d will evaluate: *, +, +=. The * and + will not be checked. Sorry. I didn't see initially that stringLength was also Checked. This is fine.
Comment on attachment 281567 [details] Patch for landing: addressed Saam's feedback. Attachment 281567 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1519211 New failing tests: security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html
Created attachment 281572 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
(In reply to comment #10) > Comment on attachment 281567 [details] > Patch for landing: addressed Saam's feedback. > > Attachment 281567 [details] did not pass mac-ews (mac): > Output: http://webkit-queues.webkit.org/results/1519211 > > New failing tests: > security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star. > html The failure is an "ImageOnlyFailure", which can't be due to this patch which only adds checked arithmetic to StringBuilder.
Thanks for the reviews. Landed in r202173: <http://trac.webkit.org/r202173>.