Bug 158794 – OOM Assertion failure in JSON.stringify

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[André Bargull]

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
...
---

[Radar WebKit Bug Importer]

<rdar://problem/26826254>

[Mark Lam]

I can reproduce this easily.  Looks like a simple case of needing to detect an imminent out of memory error.

[Mark Lam]

(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.

[Mark Lam]

Created attachment 281529 [details]
proposed patch.

[Saam Barati]

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?

[Mark Lam]

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.

[Mark Lam]

Created attachment 281567 [details]
Patch for landing: addressed Saam's feedback.

[Geoffrey Garen]

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.

[Geoffrey Garen]

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.

[Build Bot]

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

[Build Bot]

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

[Mark Lam]

(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.

[Mark Lam]

Thanks for the reviews.  Landed in r202173: <http://trac.webkit.org/r202173>.