(function () { var x = 4; for (let x = ((function() { print(x); })()) in []) {} })() ./js 4 ./js -m undefined The first bad revision is: changeset: 9852618873d9 user: David Anderson date: Mon Jul 12 15:00:58 2010 -0700 summary: [JAEGER] Remove stores from OOL transitions when not needed (bug 573083).

Blocking on investigation, in case this bug is problematic on the web.

Bleh. I can derive an exploit from this, so fix coming.

The inner "x" is binding to "let x", but the upvar cookie has the slot for "var x". Thus the JIT's output is correct (but for the wrong reasons). The interpreter's output is wrong, and should read "undefined". The problem is that the ENTERBLOCK occurs after evaluating the RHS, so the slot adjustment of the inner pn for "x" doesn't occur until after it's been emitted.

Okay, so maybe I'm wrong. The interpreter is right, but the parser isn't marking "var x" as closed, so it doesn't get synced in the JIT. Brendan, would you be able to clarify the semantics here? jsparse.cpp says: if (tt == TOK_LET) { /* * Hoist just the 'i' from 'for (let x = i in o)' to * before the loop, glued together via pnseq. */ It then proceeds to reorder the relevant parse tree nodes. This messes up the use-def information in the following example: > var x; > for (let x = x in o) ... Because the third "x" points to the second "x", not the first. Since all this is non-standard, we could consider changing the semantics to be consistent with "let x = x" in all other scenarios. Otherwise, maybe we should hide the LHS's name before parsing the RHS.

Ugly. I hate ES1 for lazily reusing sub-grammar and not excluding the possibility of an initializer for a var in for-in. I'll take this one. /be

Why is this bug security-sensitive and sg:critical? /be

(In reply to comment #2) > Bleh. I can derive an exploit from this, so fix coming. Testcase? Need it to avoid regressing. /be

(In reply to comment #7) > (In reply to comment #2) > > Bleh. I can derive an exploit from this, so fix coming. > > Testcase? Need it to avoid regressing. > > /be This shows it's exploitable: (function (y) { var x = { } if (1) { } x = y | 100000; for (let x = ((function() { print(x); })()) in []) {} })(2) dvander@dvander:~/mozilla/tracemonkey/js/src$ ./Debug32/js -m k.js Assertion failure: addr % sizeof(FreeCell) == 0, at ../../jsgc.h:346 Aborted

David, that looks like bug 621240 -- not sure what's up but I can't repro on 64-bit Mac shell, and I don't see how a wrong-scope problem results in this assertion botching. What happens in an opt build for the same test and -m? /be

(In reply to comment #9) > I don't see how a wrong-scope problem results in this > assertion botching. If scoping is wrong, the parser can miss marking a variable as closed, which means JM won't sync it. Across closures, that might mean a value is unsynced, which is exploitable if the payload and tag aren't in agreement. Here, there is a pointer tag with an integer value, i.e. not an aligned GC thing. I don't think it's bug 621240, it's specifically a test case for this bug based on the original one in comment #0, though bug 621240 could have the same symptoms. > I can't repro on 64-bit Mac shell Try 32-bit, the register allocator and syncing logic is a different. It's probably not exploitable on 64-bit. > What happens in an opt build for the same test and -m? It crashes.

Gotcha -- thanks, I think I know how to generate 32-bit builds (overriding CXX and CC and LD to pass -m32, IIRC; gross!). /be

Stealing back with permission, Brendan says we can just forbid this initializer syntax with |let|.

Patch introduces a new SyntaxError, and removes tests which involve the syntax. find . | xargs grep "for (let" | grep " in " | grep "=" Revealed no obvious uses in our source tree outside of the test suite, but I'll tryserver to make sure.

Most of the diffnoise is unindenting.

Comment on attachment 502655 [details] [diff] [review] fix Put this block #if JS_HAS_BLOCK_SCOPE if (tt == TOK_LET) { - /* - * Hoist just the 'i' from 'for (let x = i in o)' to - * before the loop, glued together via pnseq. - */ - JSParseNode *pn3 = UnaryNode::create(tc); - if (!pn3) + reportErrorNumber(pn2, JSREPORT_ERROR, JSMSG_INVALID_FOR_IN_INIT); + return NULL; + } +#endif /* JS_HAS_BLOCK_SCOPE */ earlier, first thing in its enclosing block before allocating and setting pnseq. For the error message, how about "for-in loop let declaration may not have an initializer" instead? Hit me with a diff -b next time and I will r+ right away. /be

diff -b, with comment #15 addressed

Comment on attachment 502672 [details] [diff] [review] v2 Great, thanks for taking this. I hate that ES1-era lazy grammar factoring spec bug. This is just the first taste of revenge! /be

Great to have a simple fix that removes code, too. /be

I always feel like I've gained karma after deleting code. http://hg.mozilla.org/tracemonkey/rev/ae34ea3455f2

Me too. I see your little dvander 8-bit avatar powering up right now (like in Scott Pilgrim). /be

http://hg.mozilla.org/mozilla-central/rev/ae34ea3455f2 YES.