Closed
Bug 617288
Opened 14 years ago
Closed 14 years ago
JM: incorrect scoping in "for (let x = ... in [])"
Categories
(Core :: JavaScript Engine, defect, P2)
Core
JavaScript Engine
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)
14.37 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
(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).
Assignee | ||
Comment 1•14 years ago
|
||
Blocking on investigation, in case this bug is problematic on the web.
Assignee: general → dvander
Status: NEW → ASSIGNED
blocking2.0: --- → betaN+
Assignee | ||
Comment 2•14 years ago
|
||
Bleh. I can derive an exploit from this, so fix coming.
Group: core-security
Reporter | ||
Updated•14 years ago
|
Whiteboard: [sg:critical?]
Assignee | ||
Comment 3•14 years ago
|
||
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]
Assignee | ||
Comment 4•14 years ago
|
||
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.
Comment 5•14 years ago
|
||
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
Updated•14 years ago
|
OS: Mac OS X → All
Priority: -- → P2
Hardware: x86 → All
Target Milestone: --- → mozilla2.0
Comment 6•14 years ago
|
||
Why is this bug security-sensitive and sg:critical?
/be
Comment 7•14 years ago
|
||
(In reply to comment #2)
> Bleh. I can derive an exploit from this, so fix coming.
Testcase? Need it to avoid regressing.
/be
Assignee | ||
Comment 8•14 years ago
|
||
(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
Comment 9•14 years ago
|
||
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
Assignee | ||
Comment 10•14 years ago
|
||
(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.
Comment 11•14 years ago
|
||
Gotcha -- thanks, I think I know how to generate 32-bit builds (overriding CXX and CC and LD to pass -m32, IIRC; gross!).
/be
Updated•14 years ago
|
Whiteboard: [sg:critical] → [sg:critical][hardblocker]
Updated•14 years ago
|
status1.9.1:
--- → unaffected
status1.9.2:
--- → unaffected
Assignee | ||
Comment 12•14 years ago
|
||
Stealing back with permission, Brendan says we can just forbid this initializer syntax with |let|.
Assignee: brendan → dvander
Assignee | ||
Comment 13•14 years ago
|
||
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)
Assignee | ||
Comment 14•14 years ago
|
||
Most of the diffnoise is unindenting.
Comment 15•14 years ago
|
||
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
Assignee | ||
Comment 16•14 years ago
|
||
diff -b, with comment #15 addressed
Attachment #502655 -
Attachment is obsolete: true
Attachment #502672 -
Flags: review?(brendan)
Attachment #502655 -
Flags: review?(brendan)
Comment 17•14 years ago
|
||
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+
Comment 18•14 years ago
|
||
Great to have a simple fix that removes code, too.
/be
Assignee | ||
Comment 19•14 years ago
|
||
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]
Comment 20•14 years ago
|
||
Me too. I see your little dvander 8-bit avatar powering up right now (like in Scott Pilgrim).
/be
Comment 21•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•