Closed Bug 355834 Opened 18 years ago Closed 18 years ago

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

Categories

(Core :: JavaScript Engine, defect)

PowerPC
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: igor)

Details

(Keywords: crash, testcase, verified1.8.1, Whiteboard: [sg:critical?] js1.7)

Crash Data

Attachments

(2 files, 3 obsolete files)

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.
Severity: normal → critical
Whiteboard: [sg:critical?]
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
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
"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.
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:
Attached patch Fix v1 (obsolete) — Splinter Review
Using common code for tree byte code generation. AFAICS js_EmitFunctionBytecode can also take care about js_AllocTryNotes, but I am not 100% sure.
Assignee: general → igor.bukanov
Status: NEW → ASSIGNED
Attachment #241543 - Flags: review?(brendan)
Comment on attachment 241543 [details] [diff] [review]
Fix v1

Yeah, js_AllocTryNotes can be commoned too.

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

/be
Flags: blocking1.8.1?
Attached patch Fix v2 (obsolete) — Splinter Review
Attachment #241543 - Attachment is obsolete: true
Attachment #241571 - Flags: review?(brendan)
Attachment #241543 - Flags: review?(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
Attachment #241571 - Flags: review?(brendan) → review+
Attached patch Fix v2bSplinter Review
Patch to commit with updated comments.
Attachment #241571 - Attachment is obsolete: true
Attachment #241576 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Attachment #241576 - Flags: approval1.8.1.1?
Attachment #241576 - Flags: approval1.8.1?
Attachment #241576 - Flags: approval1.8.1.1?
Flags: blocking1.8.1? → blocking1.8.1+
Be I'm assuming we want for 181?
Attached file js1_7/geniter/regress-355834.js (obsolete) —
I can't reproduce the problems with this test. Jesse can you verify?
Flags: in-testsuite+
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
Is this needed for 1.8.0.8 as well? "yield" is js1.7, but the emit patch looks pretty generic and might apply.
Flags: blocking1.8.0.8?
Nevermind, no TCF_FUN_IS_GENERATOR on the 1.8.0 branch
Flags: blocking1.8.0.8? → blocking1.8.0.8-
Whiteboard: [sg:critical?] → [sg:critical?] js1.7
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?
Status: RESOLVED → VERIFIED
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.
Attachment #241709 - Attachment is obsolete: true
Comment on attachment 241576 [details] [diff] [review]
Fix v2b

a=beltzner on behalf of drivers for 1.8.1 RC 3
Attachment #241576 - Flags: approval1.8.1? → approval1.8.1+
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
Keywords: fixed1.8.1
verified fixed 1.8 20061010 windows/mac*/linux
Group: security
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-355834.js,v  <--  regress-355834.js
Crash Signature: [@ js_LookupPropertyWithFlags]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: