Closed Bug 617288 Opened 11 years ago Closed 11 years ago

JM: incorrect scoping in "for (let x = ... in [])"

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla2.0
Tracking Status
blocking2.0 --- betaN+
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: jruderman, Assigned: dvander)

References

Details

(Keywords: regression, testcase, Whiteboard: [sg:critical][hardblocker][fixed-in-tracemonkey])

Attachments

(1 file, 1 obsolete file)

(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.
Assignee: general → dvander
Status: NEW → ASSIGNED
blocking2.0: --- → betaN+
Bleh. I can derive an exploit from this, so fix coming.
Group: core-security
Whiteboard: [sg:critical?]
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.
Whiteboard: [sg:critical?] → [sg:critical]
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
Assignee: dvander → brendan
OS: Mac OS X → All
Priority: -- → P2
Hardware: x86 → All
Target Milestone: --- → mozilla2.0
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
Whiteboard: [sg:critical] → [sg:critical][hardblocker]
Stealing back with permission, Brendan says we can just forbid this initializer syntax with |let|.
Assignee: brendan → dvander
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #502655 - Flags: review?(brendan)
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
Attached patch v2Splinter Review
diff -b, with comment #15 addressed
Attachment #502655 - Attachment is obsolete: true
Attachment #502672 - Flags: review?(brendan)
Attachment #502655 - Flags: review?(brendan)
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
Attachment #502672 - Flags: review?(brendan) → review+
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
Whiteboard: [sg:critical][hardblocker] → [sg:critical][hardblocker][fixed-in-tracemonkey]
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.