646968 - let-block variable initializers are statically outside the let-scope but dynamically inside it

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

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]]

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]]

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]]

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]]

(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)]

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)]

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]]

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)]

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]]

Jason: Only a couple weeks left before mozilla6 goes to beta channel, will this be done by then?

Comment 10

[Jason Orendorff [:jorendorff]]

Yeah, this is on my list. I'll patch it this week.

Comment 11

[Dave Herman [:dherman]]

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]]

Scoping should be like Scheme let, not let* or letrec.

/be

Comment 13

[Jeff Walden [:Waldo]]

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]]

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]]

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]]

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]]

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]]

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]]

Attachment: 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]]

Attachment: 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]]

Attachment: v3

Identical to v2 except for bumping the bytecode version.

Comment 22

[Jason Orendorff [:jorendorff]]

Went ahead and filed followup bug 668848.

Comment 24

[Johnny Stenback (:jst)]

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]]

Comment on attachment 543429 [details] [diff] [review]
v3

Why the JSOP_NOP emits before the EmitBlockChain calls?

/be

Comment 26

[Jason Orendorff [:jorendorff]]

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]]

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]]

Comment on attachment 543429 [details] [diff] [review]
v3

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

/be

Comment 29

[Johnny Stenback (:jst)]

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 30

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/69dfa4008531

Comment 31

[Jason Orendorff [:jorendorff]]

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]]

(btw, followup bug 671360.)

Comment 33

[Asa Dotzler [:asa]]

jorendorff, yes, and we'll get to these approval requests on Monday's 2pm PT triage.

Comment 34

[Asa Dotzler [:asa]]

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]]

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]]

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)]

Tracking for 8 since there were regressions from this.

Comment 38

[u279076]

qa- as nothing to do for QA verification -- please reset to qa+ if a testcase can be provided.

Comment 39

[Daniel Veditz [:dveditz]]

Does this bug affect 1.9.2? at least part of the patch would apply to jsemit

Comment 40

[Jason Orendorff [:jorendorff]]

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.