355834 - (new Function('yield'))(1) causes "yield from closing generator" or crash [@ js_LookupPropertyWithFlags]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> (new Function('yield'))(1)

Opt: gives an error message that doesn't make sense to me in this context,
"TypeError: yield from closing generator (function anonymous() {yield;})"

Debug: js_Interpret calls js_DecompileValueGenerator in an attempt to create the error message above.  A few frames deeper, js_LookupPropertyWithFlags crashes dereferencing 0xdadadada.

The number passed as an argument matters.  Passing 1 makes it output the strange error or crash, but passing 0 or 2 does not.

Security-sensitive because of the 0xdadadada.

Comment 1

[Jesse Ruderman]

Thread 0 Crashed:
0   js 	0x0004903c js_LookupPropertyWithFlags + 684 (jsobj.c:3159)
1   js 	0x00048d74 js_LookupProperty + 68 (jsobj.c:3086)
2   js 	0x00049d4c js_GetProperty + 256 (jsobj.c:3371)
3   js 	0x0004e2f0 js_TryMethod + 324 (jsobj.c:4529)
4   js 	0x0007d8b0 js_ValueToSource + 404 (jsstr.c:2701)
5   js 	0x0003f194 js_DecompileValueGenerator + 3868 (jsopcode.c:4764)
6   js 	0x000b2cd8 js_Interpret + 119864 (jsinterp.c:5932)
7   js 	0x00094368 js_Execute + 936 (jsinterp.c:1622)
8   js 	0x000210d8 JS_ExecuteScript + 64 (jsapi.c:4256)
9   js 	0x00002be4 Process + 904 (js.c:265)
10  js 	0x000037ac ProcessArgs + 2304 (js.c:487)
11  js 	0x00009b9c main + 640 (js.c:3088)
12  js 	0x00001f48 _start + 340 (crt.c:272)
13  js 	0x00001df0 start + 60

Comment 2

[Jesse Ruderman]

INT_TO_JSVAL(1) is represented in memory as 0x3.  JSGEN_CLOSING is the fourth element in an enum, so it is also represented in memory as 0x3.  Maybe that has something to do with why the value 1 is special in triggering the error/crash?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsinterp.c&rev=3.297#5931

Comment 3

[Jesse Ruderman]

"new Function" omits the JSOP_GENERATOR that's supposed to be at the top of each generator function.

js> dis(new Function("yield 3;"))
flags: LAMBDA
main:
00000:  uint16 3
00003:  yield
00004:  pop
00005:  stop

The JSGenerator object that the JSOP_YIELD case looks for is created in the JSOP_GENERATOR case.  I guess since the JSGenerator isn't where the JSOP_YIELD case expects it to be, it ends up looking at the first parameter instead.

Comment 4

[Igor Bukanov]

Function('yield') is not marked as generator:

js> dis(function() { yield; })
dis(function() { yield; })
flags: LAMBDA
main:
00000:  generator
00001:  push
00002:  yield
00003:  pop
00004:  stop

Source notes:
js> dis(Function('yield'))
dis(Function('yield'))
flags: LAMBDA
main:
00000:  push
00001:  yield
00002:  pop
00003:  stop

Source notes:

Comment 5

[Igor Bukanov]

Attachment: Fix v1

Using common code for tree byte code generation. AFAICS js_EmitFunctionBytecode can also take care about js_AllocTryNotes, but I am not 100% sure.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 241543 [details] [diff] [review]
Fix v1

Yeah, js_AllocTryNotes can be commoned too.

/be

Comment 7

[Brendan Eich [:brendan]]

Putting on radar, simple fix to fuzzer-found bug in sight.

/be

Comment 8

[Igor Bukanov]

Attachment: Fix v2

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 241571 [details] [diff] [review]
Fix v2


> /*
>+ * Emit code into cg for the tree rooted at body.
>+ */
>+extern JSBool
>+js_EmitFunctionBytecode(JSContext *cx, JSCodeGenerator *cg, JSParseNode *body);

Comment could say "Emit function code ...".  r+ me with that.

/be

Comment 10

[Igor Bukanov]

Attachment: Fix v2b

Patch to commit with updated comments.

Comment 11

[Igor Bukanov]

I committed the patch from comment 10 to the trunk:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.224; previous revision: 3.223
done
Checking in jsemit.h;
/cvsroot/mozilla/js/src/jsemit.h,v  <--  jsemit.h
new revision: 3.69; previous revision: 3.68
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.256; previous revision: 3.255
done

Comment 12

[Mike Schroepfer]

Be I'm assuming we want for 181?

Comment 13

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/geniter/regress-355834.js

I can't reproduce the problems with this test. Jesse can you verify?

Comment 14

[Brendan Eich [:brendan]]

This is a very safe patch to refactor slightly so Function produces well-formed bytecode.  Jesse's fuzzer produces Function invocations and injects yield into generated source, so it will likely produce travesties that this bug bites.  We should fix it now.

/be

Comment 15

[Daniel Veditz [:dveditz]]

Is this needed for 1.8.0.8 as well? "yield" is js1.7, but the emit patch looks pretty generic and might apply.

Comment 16

[Daniel Veditz [:dveditz]]

Nevermind, no TCF_FUN_IS_GENERATOR on the 1.8.0 branch

Comment 17

[Jesse Ruderman]

Verified fixed on Mac, using opt and debug jsshell:

js> (new Function('yield'))(1)
[object Generator]

bc, if you can't reproduce the crash in older builds in the regression test framework, can you at least reproduce the lack of a returned generator?

Comment 18

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/geniter/regress-355834.js

In reply to comment #17)

> bc, if you can't reproduce the crash in older builds in the regression test
> framework, can you at least reproduce the lack of a returned generator?

yes. Modified test attached. Thanks.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 241576 [details] [diff] [review]
Fix v2b

a=beltzner on behalf of drivers for 1.8.1 RC 3

Comment 20

[Igor Bukanov]

I committed the patch from comment 10 to MOZILLA_1_8_BRANCH:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.128.2.55; previous revision: 3.128.2.54
done
Checking in jsemit.h;
/cvsroot/mozilla/js/src/jsemit.h,v  <--  jsemit.h
new revision: 3.38.20.18; previous revision: 3.38.20.17
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.142.2.66; previous revision: 3.142.2.65
done

Comment 21

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20061010 windows/mac*/linux

Comment 22

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_7/geniter/regress-355834.js,v  <--  regress-355834.js