321971 - JSOP_FINDNAME replaces JSOP_BINDNAME, does not prefix it

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

In the fix for bug 155081, I missed the case in the summary.  This results in a JSOP_BINDNAME being emitted without its immmediate operand (which is the desired format when prefixed by JSOP_LITOPX, but that's not applicable to this case), when it should not be emitted at all.  Trivial patch next.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix

Trivial given diff -w coming next.  Review that, stamp this.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: diff -w version of last patch

Comment 3

[Brendan Eich [:brendan]]

IOW, there are JOF_NAME and JOF_PROP formats that look like

op, <16-bit-atom-index>

and these are converted to xop, either JSOP_FINDNAME (for JOF_NAME format) or JSOP_LITERAL (for JOF_PROP), followed by the JOF_ELEM counterpart to op, eop:

xop, <24-bit-atom-index>, eop

An element op eop takes all operands from the stack, where they are [obj, id] for gets and [obj, id, val] for sets except for JSOP_ENUMELEM, which has [val, obj, id] (no JOF_NAME or JOF_PROP format converts to JSOP_ENUMELEM on 16-bit immediate overflow, note well).

But JSOP_BINDNAME (a JOF_NAME format, as its name implies) always precedes the right-hand-side evaluation, in order to compute the Reference base (the obj in [obj, id] where id comes from a 16-bit immediate) on the stack in advance of side-effects to the scope chain that might arise in the RHS evaluation.  Then a JSOP_SETNAME finds that obj stacked under the rvalue, and uses its own 16-bit immediate atom index to get the id to compute [obj, id] and actually set the property.

When the 16-bit immediate is not big enough, we don't want to turn JSOP_BINDNAME and JSOP_SETNAME into extended forms, because JSOP_FINDNAME *replaces* BINDNAME and pushes both [obj, id] at once.  The JSOP_SETNAME converts to JSOP_SETELEM after the RHS evaluation, so it needs no prefixing (no immediates to extend).

Hope this helps.

/be

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 207240 [details] [diff] [review]
fix

sr=shaver

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 207240 [details] [diff] [review]
fix

Checked into trunk (SpiderMonkey is still single-review), but I want mrbkap's r+ before nominating for the branches.

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 207240 [details] [diff] [review]
fix

r=mrbkap

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 207240 [details] [diff] [review]
fix

This is a local, safe fix for a bug that can result in garbage bytecodes being interpreted.  The number of bytecodes is limited to 2, and they can't have arbitrary effects, so I don't see a security hazard (yet).

Given the local and minimal (look at the diff -w) properties of the patch, and the argument in comment 3, I think we are better off taking this and not worrying about exploitability.

/be

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 207240 [details] [diff] [review]
fix

a=dveditz for drivers.
Please mark FIXED (it's on the trunk) and add the fixed1.8.1 and fixed1.8.0.1 keywords when it's checked into those branches

Comment 9

[Brendan Eich [:brendan]]

Fixed on branches.

/be

Comment 10

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-321971.js,v  <--  regress-321971.js

Comment 11

[Bob Clary [:bc] (inactive)]

v 20060113 win/linux/mac 1.8.0.1, 1.8, 1.9a1