Last Comment Bug 646968 - let-block variable initializers are statically outside the let-scope but dynamically inside it
: let-block variable initializers are statically outside the let-scope but dyna...
Status: RESOLVED FIXED
[sg:critical?] [landed m-c 7/15][qa-...
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla8
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
: 647695 (view as bug list)
Depends on: 672804 673070
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-31 13:18 PDT by Jason Orendorff [:jorendorff]
Modified: 2015-10-16 11:38 PDT (History)
15 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
-
wontfix
-
wontfix
+
fixed
+
fixed
+
fixed
-
wontfix


Attachments
WIP 1 (10.66 KB, patch)
2011-06-29 20:46 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v2 (12.38 KB, patch)
2011-07-01 07:13 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v3 (13.17 KB, patch)
2011-07-01 07:15 PDT, Jason Orendorff [:jorendorff]
brendan: review+
asa: approval‑mozilla‑aurora-
asa: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2011-03-31 13:18:08 PDT
The EXPR in `let (x = EXPR) foo();` is supposed to be outside the scope of the lexical variable x being declared.

The compiler gets this right for names it can bind statically, but it still emits the bytecode for the expression *after* the JSOP_ENTERBLOCK instruction. That means the scope is wrong for plain old JSOP_NAME opcodes:

    var x = 5;
    let (x = x)
        assertEq(x, 5);                    // PASS
    let (x = eval("x"))
        assertEq(x, 5);                    // FAIL
    let (x = function () { with ({}) return x; })
        assertEq(x(), 5);                  // FAIL

    (let (x = x) assertEq(x, 5));          // PASS
    (let (x = eval("x")) assertEq(x, 5));  // FAIL
    (let (x = function () { with ({}) return x; }) assertEq(x(), 5)); // FAIL

I can't think of a clever way to fix this offhand.
Comment 1 Jason Orendorff [:jorendorff] 2011-03-31 13:57:48 PDT
dvander points out that

  - you don't need with/eval to emit NAME opcodes; an escaping function
    with upvars is enough

  - this could cause a JSOP_NAME/JSOP_SETNAME to read or write an inner
    variable that the static analysis thinks isn't being read/written.

He thinks it can probably be exploited in the JIT.
Comment 2 Jason Orendorff [:jorendorff] 2011-04-01 08:29:25 PDT
We could emit the right-hand sides first, then ENTERBLOCK. We would modify ENTERBLOCK to use the already-populated operand stack slots as storage for the varibles in the new scope, instead of populating new slots with undefined.

    Disassembly for `let (x = EXPR1, y = EXPR2) EXPR3` would look like this:
      EXPR1                           # v1
      EXPR2                           # v1 v2
      enterblock (x, y)               # x=v1 y=v2
      EXPR3                           # x y v3
      leaveblockexpr 2                # v3

This might be a little tricky for destructuring let. More on that in a minute.
Comment 3 Jason Orendorff [:jorendorff] 2011-04-01 08:51:54 PDT
It's not so bad.

`let ([x, y] = obj) ...`

    push        # set aside a slot for x
    push        # and one for y
    getgname obj
    dup
    zero
    getelem
    setlocalpop 0  # set x = obj[0]
    dup
    one
    getelem
    setlocalpop 1  # set y = obj[1]
    enterblock (x, y)

  The only difficulty I anticipate here is getting the decompiler to
  understand which names those setlocalpop instructions are assigning to.
  Worst-case, we add a JSOP_NOP with a magic source note.

`let ([x, y] = [a, b]) ...`

  Using the existing group destructuring code with this new approach, we
  would get:
    push        # set aside a slot for x
    push        # and one for y
    getgname a
    getgname b
    getlocal 2
    setlocal 0  # store x = a
    getlocal 3
    setlocal 1  # store y = b
    popn 2
    enterblock (x, y)

  (Of course all that amounts to:
    getgname a
    getgname b
    enterblock (x, y)
  which the decompiler would happily decompile to `let (x=a, y=b)`. Cute, but
  not worth implementing, at least not in this bug.)
Comment 4 Daniel Veditz [:dveditz] 2011-05-12 13:48:52 PDT
(In reply to comment #1)
>   - this could cause a JSOP_NAME/JSOP_SETNAME to read or write an inner
>     variable that the static analysis thinks isn't being read/written.

Is the variable potentially written to still a "variable"? or could that bit of memory now be used for something else entirely where now an attacker can cause us to execute the wrong stuff. I guess even as a variable it could be a reference to some object on which you call methods, replaced with a value that points at a fake object the attacker lays out in a typedarray or something like that?
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2011-06-08 16:25:56 PDT
David, per comment 1 you're suggesting that this is a potentially exploitable bug. Can you please suggest a sg: rating per https://wiki.mozilla.org/Security_Severity_Ratings
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2011-06-15 17:26:24 PDT
David says that while it would be tricky to get the jit to generate exploitable code here he thinks it's possible, and suggests sg:critical? for this bug. He also thought Jason would be a good owner here, so reassigning to Jason for now.
Comment 7 Daniel Veditz [:dveditz] 2011-06-16 13:39:31 PDT
can we get a testcase or js shell script that exhibits the problem? We'll need this to verify the bug and create a regression test.

Does this affect the 1.9.2 branch as well, or is this a problem that crept in with the method JIT?
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2011-06-23 13:09:42 PDT
Jason, do you think you're the right owner here? And if so, when do you think you can get to this?
Comment 9 Daniel Veditz [:dveditz] 2011-06-23 13:14:03 PDT
Jason: Only a couple weeks left before mozilla6 goes to beta channel, will this be done by then?
Comment 10 Jason Orendorff [:jorendorff] 2011-06-27 09:00:18 PDT
Yeah, this is on my list. I'll patch it this week.
Comment 11 Dave Herman [:dherman] 2011-06-28 14:53:05 PDT
FWIW, I believe Jason is right that the intended scoping semantics of let expressions and let statements was that in the form:

    let (x1[= e1], ..., xn[= en]) body

all the expressions e1 ... en are in the outer scope, and the x1 ... xn variables are only in scope in the body. (This corresponds to Scheme's LET, as opposed to LET* or LETREC.)

But this is from ES4 and is not in Harmony, at least not currently. So there isn't really a definitive spec.

Dave
Comment 12 Brendan Eich [:brendan] 2011-06-28 14:56:42 PDT
Scoping should be like Scheme let, not let* or letrec.

/be
Comment 13 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-28 15:39:05 PDT
I was always partial to let* allowing easy abbreviation of intermediate steps, myself.  But I imagine let is easiest and least complicated to implement, because you don't have a gazillion different possible scope-object-munges.  Is simplicity the reason we would want let rather than let*?
Comment 14 Dave Herman [:dherman] 2011-06-28 16:44:18 PDT
Jeff: I pretty much agree with you. I like let* for the same reason. I think I originally proposed let instead of let* for ES4 without really thinking it through. And now it looks like we're schedule-bound to get this patched quickly.

But we might actually be able to get away with changing this down the road, if we decided we preferred the let* semantics; I think people use let-declarations far more often than let-expressions and let-statements, so it might not break much. And we could easily do some simple scope analysis of Firefox and addon code to see how much bustage it would cause.

Dave
Comment 15 Dave Herman [:dherman] 2011-06-28 16:46:16 PDT
FWIW, we could probably do scope analysis to merge frames in many cases, so let* would often devolve to being no more expensive than let.

Dave
Comment 16 Jason Orendorff [:jorendorff] 2011-06-28 17:14:50 PDT
The code for emitting switch statements contains this:

	/* Emit JSOP_ENTERBLOCK before code to evaluate the discriminant. */
        if (!EmitEnterBlock(cx, pn2, cg))
            return JS_FALSE;

That seems like the same kind of bug.
Comment 17 Jason Orendorff [:jorendorff] 2011-06-29 12:09:18 PDT
Heh, after all that noise in comments 2 and 3, I think all I had to do was emit the block chain in a couple extra places.
Comment 18 Jason Orendorff [:jorendorff] 2011-06-29 12:11:44 PDT
Whoops, posted that prematurely. I guess I might as well explain.

What I mean is, there's already code in EmitVariables to handle let-heads and other similar places specially. My plan: in the places where that code hacks the CodeGenerator, also emit JSOP_BLOCKCHAIN/NULLBLOCKCHAIN annotations for GetBlockChain's benefit.
Comment 19 Jason Orendorff [:jorendorff] 2011-06-29 20:46:18 PDT
Created attachment 543048 [details] [diff] [review]
WIP 1

This passes the tests with let-expressions and let-blocks, but it still fails
for(let x = eval('x');;) and switch (eval('x')) { let x; }.
Comment 20 Jason Orendorff [:jorendorff] 2011-07-01 07:13:00 PDT
Created attachment 543428 [details] [diff] [review]
v2

Phew. This patch was harder to write than it looks. :-|

Unfortunately there is still this:

  var s = '', x = {a: 1, b: 2, c: 3};
  for (let x in let (y = 0) x)
      s += x;
  assertEq(s, 'abc');

  typein:4: Error: Assertion failed: got "", expected "abc"

I think this is a bug too, but it seems to be a bug in the jsparse code that builds the def-use chains, not jsemit. Assuming this patch gets review, I'll file a follow-up bug for that.

Maybe I should remove the for-in part of this patch, though, so that I at least leave things consistent. Your thoughts?
Comment 21 Jason Orendorff [:jorendorff] 2011-07-01 07:15:18 PDT
Created attachment 543429 [details] [diff] [review]
v3

Identical to v2 except for bumping the bytecode version.
Comment 22 Jason Orendorff [:jorendorff] 2011-07-01 10:47:39 PDT
Went ahead and filed followup bug 668848.
Comment 23 Brian Hackett (:bhackett) 2011-07-02 06:47:51 PDT
*** Bug 647695 has been marked as a duplicate of this bug. ***
Comment 24 Johnny Stenback (:jst, jst@mozilla.com) 2011-07-07 13:33:30 PDT
Jason and Brendan, we're considering this for Firefox 6, which is already in beta. Any chance of getting this reviewed and landed reasonably soon to get plenty of testing during beta for this?
Comment 25 Brendan Eich [:brendan] 2011-07-08 23:35:20 PDT
Comment on attachment 543429 [details] [diff] [review]
v3

Why the JSOP_NOP emits before the EmitBlockChain calls?

/be
Comment 26 Jason Orendorff [:jorendorff] 2011-07-11 18:50:12 PDT
Sorry I didn't write a nice comment about that.

The current block chain is *not* always computed by looking at the most recent JSOP_BLOCKCHAIN/NULLBLOCKCHAIN/ENTERBLOCK instruction. Sometimes we call GetBlockChainFast. It looks at the op immediately *following* the op at pc. If it is JSOP_BLOCKCHAIN or JSOP_NULLBLOCKCHAIN, then we don't bother looking at instructions before the pc.

So in this patch, I emit JSOP_NOPs in order to avoid accidentally confusing GetBlockChainFast when pc points to the preceding instruction. (!)

I think it would be better to separate these two competely different meanings of JSOP_BLOCKCHAIN/NULLBLOCKCHAIN (modifying the immediately preceding opcode vs. modifying only subsequent opcodes), but I thought it best not to refactor all that in this bug.
Comment 27 Jason Orendorff [:jorendorff] 2011-07-11 18:51:47 PDT
It might also be better if GetBlockChainFast could assert that the looked-for annotation is there. We should know that statically at each call site, right?
Comment 28 Brendan Eich [:brendan] 2011-07-13 11:16:15 PDT
Comment on attachment 543429 [details] [diff] [review]
v3

Please FIXME-comment the NOPs citing the followup bug. Thanks,

/be
Comment 29 Johnny Stenback (:jst, jst@mozilla.com) 2011-07-14 13:21:15 PDT
Can we get this landed on the trunk so that we can tell whether this is something we can take late in a beta cycle for 6?
Comment 31 Jason Orendorff [:jorendorff] 2011-07-15 09:54:56 PDT
Comment on attachment 543429 [details] [diff] [review]
v3

I assume I still need to get these bits even though this is tracking-firefox6+ and tracking-firefox7+.
Comment 32 Jason Orendorff [:jorendorff] 2011-07-15 11:49:59 PDT
(btw, followup bug 671360.)
Comment 33 Asa Dotzler [:asa] 2011-07-17 20:57:04 PDT
jorendorff, yes, and we'll get to these approval requests on Monday's 2pm PT triage.
Comment 34 Asa Dotzler [:asa] 2011-07-18 14:22:45 PDT
Is this the kind of thing we'd expect to see independently discovered in the wild? If not, can we just let it climb up the release chain normally?
Comment 35 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-18 14:33:54 PDT
Hasn't this been in since the start of let, i.e. 2006 or so?  Waiting's always a gamble, but five years of gambling has, er, not bitten us yet.  I'd at least lean toward waiting.
Comment 36 Asa Dotzler [:asa] 2011-07-19 14:40:34 PDT
Comment on attachment 543429 [details] [diff] [review]
v3

We're not willing to take the risk into Aurora and Beta -- worried especially about add-on compatibility. Given that this is internally found, we'd prefer to take the extra 6 weeks to let this climb the normal rout through the channels.
Comment 37 Johnny Stenback (:jst, jst@mozilla.com) 2011-07-21 13:44:10 PDT
Tracking for 8 since there were regressions from this.
Comment 38 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-13 09:54:45 PDT
qa- as nothing to do for QA verification -- please reset to qa+ if a testcase can be provided.
Comment 39 Daniel Veditz [:dveditz] 2011-10-19 17:13:45 PDT
Does this bug affect 1.9.2? at least part of the patch would apply to jsemit
Comment 40 Jason Orendorff [:jorendorff] 2011-10-27 12:54:48 PDT
I think 1.9.2 is affected, but we never found a way to exploit this. The way we imagined it might be exploitable involved the methodjit, which does not exist in 1.9.2.

Combine that with the size of the patch and the number of regressions it introduced, and I can confidently say that 1.9.2 is better off without it.

Note You need to log in before you can comment on or make changes to this bug.