Bug 141883 – Crash in DFGFrozenValue

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Han Choongwoo]

----
(function() {
var b=!2;
for(i = 0; i< 1e5; i++) {
b[b=this];
for (var i = 0; i < 1e5; i++) {
  if (a = b*3) {
  }
}
}
})()
----

This script crashes.

Assertion fail on DFGFrozenValue.h:53

-> RELEASE_ASSERT(!value || !value.isCell());

found with afl fuzz

* thread #2: tid = 0x535a38, 0x000000010054372e JavaScriptCore`WTFCrash + 62 at Assertions.cpp:321, name = 'DFG Worklist Worker Thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef)
    frame #0: 0x000000010054372e JavaScriptCore`WTFCrash + 62 at Assertions.cpp:321
   318 	        globalHook();
   319
   320 	    WTFReportBacktrace();
-> 321 	    *(int *)(uintptr_t)0xbbadbeef = 0;
   322 	    // More reliable, but doesn't say BBADBEEF.
   323 	#if COMPILER(CLANG)
   324 	    __builtin_trap();
(lldb) bt
* thread #2: tid = 0x535a38, 0x000000010054372e JavaScriptCore`WTFCrash + 62 at Assertions.cpp:321, name = 'DFG Worklist Worker Thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef)
  * frame #0: 0x000000010054372e JavaScriptCore`WTFCrash + 62 at Assertions.cpp:321
    frame #1: 0x00000001000d1462 JavaScriptCore`JSC::DFG::AbstractInterpreter<JSC::DFG::InPlaceAbstractState>::executeEffects(unsigned int, JSC::DFG::Node*) [inlined] JSC::DFG::FrozenValue::FrozenValue(JSC::JSValue) + 1522 at DFGFrozenValue.h:53
    frame #2: 0x00000001000d1454 JavaScriptCore`JSC::DFG::AbstractInterpreter<JSC::DFG::InPlaceAbstractState>::executeEffects(unsigned int, JSC::DFG::Node*) [inlined] JSC::DFG::FrozenValue::FrozenValue(JSC::JSValue) at DFGFrozenValue.h:54
    frame #3: 0x00000001000d1454 JavaScriptCore`JSC::DFG::AbstractInterpreter<JSC::DFG::InPlaceAbstractState>::executeEffects(this=0x0000000105609260, clobberLimit=<unavailable>, node=0x00000001043e2d00) + 1508 at DFGAbstractInterpreterInlines.h:311
    frame #4: 0x00000001000cfddd JavaScriptCore`JSC::DFG::CFAPhase::performBlockCFA(JSC::DFG::BasicBlock*) [inlined] JSC::DFG::AbstractInterpreter<JSC::DFG::InPlaceAbstractState>::execute(this=<unavailable>) + 637 at DFGAbstractInterpreterInlines.h:2048
    frame #5: 0x00000001000cfd90 JavaScriptCore`JSC::DFG::CFAPhase::performBlockCFA(this=0x0000000105608fc8, block=0x0000000102041140) + 560 at DFGCFAPhase.cpp:125
    frame #6: 0x00000001000cfa05 JavaScriptCore`JSC::DFG::CFAPhase::run() [inlined] JSC::DFG::CFAPhase::performForwardCFA(this=0x0000000105608fc8) + 93 at DFGCFAPhase.cpp:152
    frame #7: 0x00000001000cf9a8 JavaScriptCore`JSC::DFG::CFAPhase::run(this=0x0000000105608fc8) + 120 at DFGCFAPhase.cpp:79
    frame #8: 0x00000001000cf879 JavaScriptCore`bool JSC::DFG::runPhase<JSC::DFG::CFAPhase>(JSC::DFG::Graph&) [inlined] bool JSC::DFG::runAndLog<JSC::DFG::CFAPhase>(phase=0x0000000105609750) + 8 at DFGPhase.h:77
    frame #9: 0x00000001000cf871 JavaScriptCore`bool JSC::DFG::runPhase<JSC::DFG::CFAPhase>(graph=<unavailable>) + 33 at DFGPhase.h:87
    frame #10: 0x00000001000cf849 JavaScriptCore`JSC::DFG::performCFA(graph=<unavailable>) + 9 at DFGCFAPhase.cpp:168
    frame #11: 0x000000010016b6cf JavaScriptCore`JSC::DFG::Plan::compileInThreadImpl(this=0x00000001037fdc00, longLivedState=<unavailable>) + 719 at DFGPlan.cpp:263
    frame #12: 0x000000010016b19d JavaScriptCore`JSC::DFG::Plan::compileInThread(this=0x00000001037fdc00, longLivedState=0x0000000105609e18, threadData=<unavailable>) + 493 at DFGPlan.cpp:164
    frame #13: 0x00000001001f38b2 JavaScriptCore`JSC::DFG::Worklist::runThread(this=0x00000001037e6000, data=0x0000000100d01160) + 546 at DFGWorklist.cpp:358
    frame #14: 0x000000010056e283 JavaScriptCore`WTF::threadEntryPoint(void*) [inlined] std::__1::function<void (this=0x00000001005e7340)>::operator()() const + 179 at functional:1755
    frame #15: 0x000000010056e279 JavaScriptCore`WTF::threadEntryPoint(contextData=<unavailable>) + 169 at Threading.cpp:58
    frame #16: 0x000000010056e76f JavaScriptCore`WTF::wtfThreadEntryPoint(param=0x0000000103ff81c0) + 15 at ThreadingPthreads.cpp:170
    frame #17: 0x00007fff8290a268 libsystem_pthread.dylib`_pthread_body + 131
    frame #18: 0x00007fff8290a1e5 libsystem_pthread.dylib`_pthread_start + 176
    frame #19: 0x00007fff8290841d libsystem_pthread.dylib`thread_start + 13

[Filip Pizlo]

Yup, I see it.  Looking now.

[Filip Pizlo]

I have a fix.

[Filip Pizlo]

Created attachment 247103 [details]
the patch

[Benjamin Poulain]

Comment on attachment 247103 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247103&action=review

I am a bit confused by the test, but the patch looks correct :)

> Source/JavaScriptCore/tests/stress/regress-141883.js:6
> +for (var i = 0; i < n; i++) {

Did you intend to reuse "i" here?

> Source/JavaScriptCore/tests/stress/regress-141883.js:7
> +  if (a = b*3) {

Any reason for the if() here?

[Filip Pizlo]

(In reply to comment #4)
> Comment on attachment 247103 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247103&action=review
> 
> I am a bit confused by the test, but the patch looks correct :)

This is the presumably fuzz-generated test that the bug reporter (Han) provided.  It's a 100% repro case.  I only changed the number of iterations to ensure it runs fast.

> 
> > Source/JavaScriptCore/tests/stress/regress-141883.js:6
> > +for (var i = 0; i < n; i++) {
> 
> Did you intend to reuse "i" here?

No idea. ;-)

> 
> > Source/JavaScriptCore/tests/stress/regress-141883.js:7
> > +  if (a = b*3) {
> 
> Any reason for the if() here?

Lol, no idea. ;-)

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/180505

[Michael Saboff]

Comment on attachment 247103 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247103&action=review

> Source/JavaScriptCore/ChangeLog:8
> +        If a value might be a cell, then we have to have Graph freeze it rather than trying to

Is the second "have" redundant?

> Source/JavaScriptCore/tests/stress/regress-141883.js:11
> +(function() {
> +var b=!2;
> +var n = 1e4;
> +for(i = 0; i< n; i++) {
> +b[b=this];
> +for (var i = 0; i < n; i++) {
> +  if (a = b*3) {
> +  }
> +}
> +}
> +})()

Is there a reason this isn't indented properly?  Also, the formatting wrt spaces is inconsistent.
Did you intend to reuse "i" as the loop variable for the inner loop?
Could you add comments as to what the test is doing?

[Filip Pizlo]

(In reply to comment #7)
> Comment on attachment 247103 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247103&action=review
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        If a value might be a cell, then we have to have Graph freeze it rather than trying to
> 
> Is the second "have" redundant?

I don't think so.  "I have to have a cheeseburger" is a valid sentence, isn't it?

> 
> > Source/JavaScriptCore/tests/stress/regress-141883.js:11
> > +(function() {
> > +var b=!2;
> > +var n = 1e4;
> > +for(i = 0; i< n; i++) {
> > +b[b=this];
> > +for (var i = 0; i < n; i++) {
> > +  if (a = b*3) {
> > +  }
> > +}
> > +}
> > +})()
> 
> Is there a reason this isn't indented properly?  Also, the formatting wrt
> spaces is inconsistent.
> Did you intend to reuse "i" as the loop variable for the inner loop?
> Could you add comments as to what the test is doing?

I don't know what this test is doing.  I do know that it's a 100% repro case.  It was provided by the bug reporter, see the bug description.

[Saam Barati]

Out of curiosity, I played with it and it's reproducible like this:

(function() {
    var b = false;
    var n = 1e4;
    for(i = 0; i< n; i++) {
        b[{}];
        b = {};
        for (var j = 0; j < n; j++)
            b*3;
    }
})()

For it to reproduce, "i" needed to be global, "b" needed to start of as bool and switch to object, and multiplication with "b" needed to be in an inner loop.

[Benjamin Poulain]

(In reply to comment #9)
> Out of curiosity, I played with it and it's reproducible like this:
> 
> (function() {
>     var b = false;
>     var n = 1e4;
>     for(i = 0; i< n; i++) {
>         b[{}];
>         b = {};
>         for (var j = 0; j < n; j++)
>             b*3;
>     }
> })()
> 
> For it to reproduce, "i" needed to be global, "b" needed to start of as bool
> and switch to object, and multiplication with "b" needed to be in an inner
> loop.

May I suggest to submit your test too? Having more tests is great :)

r=me for your test above.