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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Similar testcases also cause crashes at js::CloseIterator, some reduced forms hit this assertion eventually.

Comment 3

[Andy Wingo [:wingo]]

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.

Comment 4

[Andy Wingo [:wingo]]

Attachment: Don't assume suspended generator activations have a script

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.

Comment 5

[Andrew McCreight [:mccr8]]

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?

Comment 6

[Andy Wingo [:wingo]]

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

Comment 7

[Christian Holler (:decoder)]

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

Comment 8

[Andrew McCreight [:mccr8]]

Ok, I'll set this to sec-high just to be conservative.

Comment 9

[Andrew McCreight [:mccr8]]

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.

Comment 10

[Andy Wingo [:wingo]]

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.

Comment 11

[Andrew McCreight [:mccr8]]

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.

Comment 12

[Andy Wingo [:wingo]]

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

Comment 13

[Andy Wingo [:wingo]]

Attachment: Closing a JSGenerator clears its stack frame in debug mode

Attached patch causes this bug to reproduce in debug mode.

Comment 14

[Andy Wingo [:wingo]]

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 15

[Jason Orendorff [:jorendorff]]

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.

Comment 16

[Jason Orendorff [:jorendorff]]

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!

Comment 17

[Andy Wingo [:wingo]]

Attachment: Don't assume suspended generator activations have a script

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.

Comment 18

[Andy Wingo [:wingo]]

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.

Comment 19

[Jason Orendorff [: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.

Comment 20

[Andy Wingo [:wingo]]

Attachment: Don't assume suspended generator activations have a script v3

Fixed nits, setting checkin-needed.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Only trunk affected = OK to land without sec-approval.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcf00351bc97

Comment 22

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bcf00351bc97

Comment 25

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 26

[Andy Wingo [:wingo]]

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