Closed Bug 908920 Opened 11 years ago Closed 11 years ago

Crash [@ js::CloseIterator] or Assertion failure: hasScript(), at jsfun.h

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla26
Tracking Status
firefox23 --- unaffected
firefox24 --- unaffected
firefox25 --- unaffected
firefox26 + verified
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: gkw, Assigned: wingo)

References

Details

(5 keywords, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(2 files, 3 obsolete files)

Attached file stack
Function("\
    x = (function() { yield })();\
    Set(x);\
    schedulegc(1);\
    print( /x/ );\
    for (p in x) {}\
")()

asserts js debug shell on m-c changeset 17143a9a0d83 without any CLI arguments at Assertion failure: hasScript(), at jsfun.h

Setting s-s because schedulegc is in the testcase. This is reproducible very often.
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/32e6af3f6a05
user:        Andy Wingo
date:        Fri Aug 23 11:07:10 2013 -0400
summary:     Bug 904701 - Implement prototype madness for ES6 generators. r=bhackett, r=jorendorff
Blocks: 904701
Similar testcases also cause crashes at js::CloseIterator, some reduced forms hit this assertion eventually.
Crash Signature: [@ js::CloseIterator]
Flags: needinfo?(wingo)
Keywords: crash
Summary: Assertion failure: hasScript(), at jsfun.h → Crash [@ js::CloseIterator] or Assertion failure: hasScript(), at jsfun.h
Thanks for isolating this bug, and apologies for introducing it.  I didn't realize you could have a generator without a script.  It seems GC can throw away scripts for generators also?

Part of the solution for this bug is bug 907744 comment 10: using different classes for legacy and star generators.  But I wonder, are we sure that generator functions should have GC-able scripts and, later on, JIT code?  Generators are activations on the heap, and like functions with activations on the stack, it seems that you would not want to collect their scripts.

I'll work on a patch.
Flags: needinfo?(wingo)
This patch makes legacy (JS 1.7) and star (ES6) generators have
different classes, avoiding the need to poke through to the frame's
script to see which kind they are.  This steps around a failure to get
a frame's script after GC, when the script was thrown away, while also
preparing for bug 907744.
Assignee: general → wingo
Attachment #795350 - Flags: review?(jorendorff)
How bad of a security issue is this?  Will it touch dead memory, or is it just going to hit a null pointer or something?
I think it's better classified as a normal bug -- I just introduced it on Friday after all, and the debugging asserts caught it.  However I think it results in interpreting a LazyScript* as a JSScript*.
(In reply to Andy Wingo from comment #6)
> I think it's better classified as a normal bug -- I just introduced it on
> Friday after all, and the debugging asserts caught it.  However I think it
> results in interpreting a LazyScript* as a JSScript*.

Neither the time when it was introduced, nor the fact that debug asserts detect it, are a reason to not classify as a security bug.

If this is misinterpreting one type for another, then we'll have to assume that bad things could happen, unless these objects are meant to be used that way.
Ok, I'll set this to sec-high just to be conservative.
As decoder said, type confusion can lead to all sorts of badness, such as interpreting a pointer as an int or vice versa, which can lead to arbitrary code execution in the worst case.
Looking pretty green over at https://tbpl.mozilla.org/?tree=Try&rev=d77199283a08.

This was my first try build and I didn't think through the "embargo" implications.  My apologies.  The result is that the test case and its fix are effectively public now.
This is trunk-only so it isn't a huge deal.

People often push security fixes to try, though I think it is good to not push the test cases and just test that locally.
Oddly, I was not able to reproduce this with a debugging, non-optimized build -- only with an optimized debugging build.  Here's the BT:

js> Function("\
    x = (function() { yield })();\
    Set(x);\
    schedulegc(1);\
    print( /x/ );\
    for (p in x) {}\
")()
typein:2:22 warning: yield without a value is deprecated, and illegal in ES6 (use 'yield undefined' instead):
typein:2:22 warning:     x = (function() { yield })();    Set(x);    schedulegc(1);    print( /x/ );   
typein:2:22 warning: ......................^
/x/
Assertion failure: hasScript(), at ../jsfun.h:262

Program received signal SIGSEGV, Segmentation fault.
0x00000000006d6850 in nonLazyScript (this=<optimized out>) at ../jsfun.h:262
262	        JS_ASSERT(hasScript());
(gdb) bt
#0  0x00000000006d6850 in nonLazyScript (this=<optimized out>) at ../jsfun.h:262
#1  script (this=<optimized out>) at ../vm/Stack.h:596
#2  IsLegacyGenerator (obj=..., obj@entry=...) at /home/wingo/src/mozilla-central/js/src/jsiter.cpp:1014
#3  0x00000000006d7513 in js::CloseIterator (cx=cx@entry=0x1682980, obj=..., obj@entry=...) at /home/wingo/src/mozilla-central/js/src/jsiter.cpp:1047
#4  0x00000000004b7581 in Interpret (cx=cx@entry=0x1682980, state=...) at /home/wingo/src/mozilla-central/js/src/vm/Interpreter.cpp:1859
#5  0x00000000004bf74f in js::RunScript (cx=cx@entry=0x1682980, state=...) at /home/wingo/src/mozilla-central/js/src/vm/Interpreter.cpp:446
#6  0x00000000004bfa4b in js::ExecuteKernel (cx=cx@entry=0x1682980, script=..., script@entry=..., scopeChainArg=..., thisv=..., type=type@entry=js::EXECUTE_GLOBAL, evalInFrame=evalInFrame@entry=..., 
    result=result@entry=0x7fffffffdea0) at /home/wingo/src/mozilla-central/js/src/vm/Interpreter.cpp:630
#7  0x00000000004c0366 in js::Execute (cx=cx@entry=0x1682980, script=..., script@entry=..., scopeChainArg=..., rval=rval@entry=0x7fffffffdea0) at /home/wingo/src/mozilla-central/js/src/vm/Interpreter.cpp:667
#8  0x0000000000623583 in JS_ExecuteScript (cx=cx@entry=0x1682980, objArg=<optimized out>, scriptArg=0x7ffff69551c0, rval=rval@entry=0x7fffffffdea0) at /home/wingo/src/mozilla-central/js/src/jsapi.cpp:5173
#9  0x00000000004509c0 in EvalAndPrint (out=0x7ffff6f87160 <_IO_2_1_stdout_>, compileOnly=false, lineno=2, length=125, bytes=<optimized out>, global=..., cx=0x1682980)
    at /home/wingo/src/mozilla-central/js/src/shell/js.cpp:465
#10 ReadEvalPrintLoop (compileOnly=false, out=0x7ffff6f87160 <_IO_2_1_stdout_>, in=0x7ffff6f87240 <_IO_2_1_stdin_>, global=..., cx=0x1682980) at /home/wingo/src/mozilla-central/js/src/shell/js.cpp:529
#11 Process (cx=cx@entry=0x1682980, obj_=<optimized out>, filename=<optimized out>, filename@entry=0x0, forceTTY=forceTTY@entry=true) at /home/wingo/src/mozilla-central/js/src/shell/js.cpp:575
#12 0x00000000004519b2 in ProcessArgs (op=0x7fffffffe0f0, obj_=<optimized out>, cx=0x1682980) at /home/wingo/src/mozilla-central/js/src/shell/js.cpp:5270
#13 Shell (cx=cx@entry=0x1682980, op=op@entry=0x7fffffffe0f0, envp=envp@entry=0x7fffffffe2d8) at /home/wingo/src/mozilla-central/js/src/shell/js.cpp:5307
#14 0x000000000043f42d in main (argc=<optimized out>, argv=<optimized out>, envp=0x7fffffffe2d8) at /home/wingo/src/mozilla-central/js/src/shell/js.cpp:5551
(gdb) quit
Attached patch causes this bug to reproduce in debug mode.
Attachment #796063 - Flags: review?(jorendorff)
Stack trace in non-optimized debug build:

js> Function("\
    x = (function() { yield })();\
    Set(x);\
    schedulegc(1);\
    print( /x/ );\
    for (p in x) {}\
")()
typein:1:22 warning: yield without a value is deprecated, and illegal in ES6 (use 'yield undefined' instead):
typein:1:22 warning:     x = (function() { yield })();    Set(x);    schedulegc(1);    print( /x/ );   
typein:1:22 warning: ......................^

Program received signal SIGSEGV, Segmentation fault.
0x000000000041c6e0 in js::StackFrame::isFunctionFrame (this=0x0) at ../../vm/Stack.h:390
390	        return !!(flags_ & FUNCTION);
(gdb) bt
#0  0x000000000041c6e0 in js::StackFrame::isFunctionFrame (this=0x0) at ../../vm/Stack.h:390
#1  0x000000000041c71c in js::StackFrame::script (this=0x0) at ../../vm/Stack.h:592
#2  0x000000000074dddf in IsLegacyGenerator (obj=...) at /home/wingo/src/mozilla-central/js/src/jsiter.cpp:1014
#3  0x000000000074df40 in js::CloseIterator (cx=0x1a29980, obj=...) at /home/wingo/src/mozilla-central/js/src/jsiter.cpp:1047
#4  0x00000000004b8240 in js::ForOfIterator::close (this=0x7fffffffbce0) at ../jsiter.h:320
#5  0x00000000004d5663 in js::SetObject::construct (cx=0x1a29980, argc=1, vp=0x1ad3278) at /home/wingo/src/mozilla-central/js/src/builtin/MapObject.cpp:1677
#6  0x00000000004b93c5 in js::CallJSNative (cx=0x1a29980, native=0x4d5388 <js::SetObject::construct(JSContext*, unsigned int, JS::Value*)>, args=...) at ../jscntxtinlines.h:219
#7  0x00000000004a46db in js::Invoke (cx=0x1a29980, args=..., construct=js::NO_CONSTRUCT) at /home/wingo/src/mozilla-central/js/src/vm/Interpreter.cpp:489
#8  0x00000000004acea3 in Interpret (cx=0x1a29980, state=...) at /home/wingo/src/mozilla-central/js/src/vm/Interpreter.cpp:2484
#9  0x00000000004a4387 in js::RunScript (cx=0x1a29980, state=...) at /home/wingo/src/mozilla-central/js/src/vm/Interpreter.cpp:446
Comment on attachment 796063 [details] [diff] [review]
Closing a JSGenerator clears its stack frame in debug mode

Review of attachment 796063 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but of course this has to be merged with the other patch.

(Landing it alone would make the bug happen all the time instead of only in specific GC-related cases.)

::: js/src/jsiter.cpp
@@ +1413,5 @@
> +    MakeRangeGCSafe(gen->fp->generatorArgsSnapshotBegin(),
> +                    gen->fp->generatorArgsSnapshotEnd());
> +    MakeRangeGCSafe(gen->fp->generatorSlotsSnapshotBegin(),
> +                    gen->regs.sp);
> +    mozilla::PodZero(&gen->regs, sizeof (gen->regs));

Also set gen->fp = NULL.

What I want to establish here is an invariant about JSGenerator structs. Depending on gen->state, all the fields that are meaningless for that state should be NULL (or perhaps poisoned would be better? see generator_finalize) in debug builds.
Attachment #796063 - Flags: review?(jorendorff) → review+
Comment on attachment 795350 [details] [diff] [review]
Don't assume suspended generator activations have a script

Review of attachment 795350 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this is much better!
Attachment #795350 - Flags: review?(jorendorff) → review+
This patch makes legacy (JS 1.7) and star (ES6) generators have
different classes, avoiding the need to poke through to the frame's
script to see which kind they are.  This steps around a failure to get
a frame's script after GC, when the script was thrown away, while also
preparing for bug 907744.

To detect errors in the future, also make accesses to gen->fp->script()
cause a null pointer deref in debug mode.  This caught another bug in
generator_close_impl(), in which with GGC it could be possible to
reference a return value from the recently dead frame.  This was easily
fixed because that value is always undefined.
Attachment #796063 - Attachment is obsolete: true
Comment on attachment 796097 [details] [diff] [review]
Don't assume suspended generator activations have a script

Quick r? to jorendorff regarding the JS_POISON bits and the generator_close_impl bit.
Attachment #796097 - Flags: review?(jorendorff)
Comment on attachment 796097 [details] [diff] [review]
Don't assume suspended generator activations have a script

Review of attachment 796097 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsiter.cpp
@@ +1354,5 @@
>      JS_ASSERT(gen->state == JSGEN_NEWBORN ||
>                gen->state == JSGEN_CLOSED ||
>                gen->state == JSGEN_OPEN);
> +    // If gen->state is JSGEN_CLOSED, gen->fp may be NULL.
> +    JS_POISON(gen->fp, JS_FREE_PATTERN, gen->fp ? sizeof(StackFrame) : 0);

Go ahead and splurge on an extra line:

    if (gen->fp)
        JS_POISON(...)

C++ compilers are smart enough to optimize away the if. JS_POISON is ((void) 0) in non-debug builds.

@@ +1783,5 @@
>  
>      if (!SendToGenerator(cx, JSGENOP_CLOSE, thisObj, gen, JS::UndefinedHandleValue))
>          return false;
>  
> +    args.rval().set(JS::UndefinedHandleValue);

Please use args.rval().setUndefined() like a few lines above.
Attachment #796097 - Flags: review?(jorendorff) → review+
Attachment #795350 - Attachment is obsolete: true
Fixed nits, setting checkin-needed.
Attachment #796097 - Attachment is obsolete: true
Attachment #796107 - Flags: review+
Keywords: checkin-needed
Only trunk affected = OK to land without sec-approval.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcf00351bc97
Flags: in-testsuite+
Keywords: checkin-needed
(In reply to Andy Wingo from comment #12)
> Oddly, I was not able to reproduce this with a debugging, non-optimized
> build -- only with an optimized debugging build.

FWIW, optimized debugging builds are what is produced as TBPL debug builds (--enable-optimize --enable-debug).
Sorry, this landed under my name. Please make sure you have hg configured to create patches with your name included by default.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
https://hg.mozilla.org/mozilla-central/rev/bcf00351bc97
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #23)
> Sorry, this landed under my name. Please make sure you have hg configured to
> create patches with your name included by default.
> https://developer.mozilla.org/en-US/docs/
> Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F

Sorry about that.  I do have my mercurial configured correctly AFAIK; not sure how this patch got through like that.  Will check next time.
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: