312354 - Assignment expressions have wrong type (ecma_3/Operators/11.13.1-002.js)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Aiko]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051012 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051012 Firefox/1.6a1

If I place an E4X expression on the LHS of an assignment, the type of the result
of the assignment is always "string". It should be the same type as the RHS.

Reproducible: Always

Steps to Reproduce:
1. typeof ( <xml/>.x = 100 )
2. typeof ( <xml/>.x = false )

Actual Results:  
1. string
2. string

Expected Results:  
1. number
2. boolean

Comment 1

[Aiko]

Attachment: testcase

As described above, for browser or the JS shell.

Comment 2

[Aiko]

Another thing I've noticed: If I try the following line

XML.prettyPrinting = 1; XML.prettyPrinting

SpiderMonkey's result is 1, Rhino's result is true. I am not sure which one is
correct?

Comment 3

[Aiko]

Oops, the problem is not restricted to E4X. Consider the following example:

re = /x/g;
y = re.lastIndex = "7"

|typeof y| returns "number"; it should be "string".

Comment 4

[Brendan Eich [:brendan]]

I'm tempted to WONTFIX, but ECMA-262 Edition 3 11.13.1 is pretty clear.  Problem
is, we have allowed setters to normalize values and return them as the result of
the assignment expression for about 10 years now.  The DOM level 0 counts on
this, for certain setters such as window.location.

Suggestions?

/be

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> Suggestions?

I have one: we can analyze the setter, either before calling it, or based on its
actual vs. expected return value, and change the expression result to match the
normalized return value of the setter, only if the setter is flagged as in need
of this kind of backward compatibility.

Now where to find another JSFUN_* flag.

/be

Comment 6

[Bob Clary [:bc] (inactive)]

Checking in 11.13.1-002.js;
/cvsroot/mozilla/js/tests/ecma_3/Operators/11.13.1-002.js,v  <--  11.13.1-002.js
initial revision: 1.1
done

This is just Seno's re example. To be complete this test needs much more coverage, but I'm too lazy.

Comment 7

[Bob Clary [:bc] (inactive)]

search love.

Comment 8

[Brendan Eich [:brendan]]

Fixing this bug improves bytecode efficiency for the common case of a single assignment operator forming the top level of an expression in statement position.  See forthcoming attachments to bug 363529.

/be

Comment 9

[Brendan Eich [:brendan]]

In working through this, I am feeling pain from #if JS_HAS_BLOCK_SCOPE and #if JS_HAS_DESTRUCTURING (more the former than the latter).

What do SpiderMonkey hackers think of moving the compile-time supported version forward to JS1.7 and assuming these two, on the trunk only?

Pain can be tolerated for a while, so if there's a good reason to keep these #ifs I'm willing to do so.

/be

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Patch?

I don't know whether I hit every assigning opcode or not; I used the following to try to enumerate every case:

random-seventy-two:~/moz/js/mozilla/js/tests jwalden$ cat /tmp/test.js
var global, gprop = {};
function test(argvar)
{
  var local, localp = {}, lval;
  lval = this.p = local = localp.prop = global = gprop.foo = argvar = "5";
  return typeof lval == "string";
}
random-seventy-two:~/moz/js/mozilla/js/tests jwalden$ ~/moz/js/js -f /tmp/test.js -i
js> dis(test)
main:
00000:  getvar 0
00003:  pop
00004:  newinit 1
00006:  endinit
00007:  setvar 1
00010:  pop
00011:  getvar 2
00014:  pop
00015:  this
00016:  getvar 1
00019:  bindname "global"
00022:  name "gprop"
00025:  string "5"
00028:  setarg 0
00031:  setprop "foo"
00034:  setname "global"
00037:  setprop "prop"
00040:  setvar 0
00043:  setprop "p"
00046:  setvar 2
00049:  pop
00050:  getvar 2
00053:  typeof
00054:  string "string"
00057:  eq
00058:  return
00059:  stop

(Aside: aren't the globals supposed to be accessed with global-specific bytecodes like JSOP_SETGVAR?)

If this is to make 3, it should get in sooner rather than later so anyone accidentally relying on the old behavior can fix their code.

Comment 13

[Jason Orendorff [:jorendorff]]

I think JSOP_SETELEM (this[x] =) needs the same treatment.

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Fix more cases

Am I right in thinking maybe rtmp needs to be rooted?  Posing this just in case I'm wrong, but if I'm right I'd like to know what might be the best solution -- tvr seems a bit heavyweight but is the only way I know about now.

Also, my testcase was a bit bogus in some ways -- attaching a better one after this.

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Better testcase

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 298497 [details] [diff] [review]
Fix more cases

Waldo, thanks for taking a run at this -- you are welcome to steal this bug. Alas the fix is more involved. We can't afford expensive rooting machinery, and we do not want store ops that leave the value stacked. So the fix is to opcode stack budgets (nuses/ndefs) and to code generation, not merely to the interpreter.

We want the JOF_SET ops to pop by default. Chained assignments need to DUP the rval before emitting the inner assignment, so it'll be there for the outer (this requires lvalue for the outer to have been emitted already, too). JSOP_SETLOCALPOP and JSOP_SETLOCAL unify. Etc. Ask me anything, I'm happy to help.

/be

Comment 18

[Jason Orendorff [:jorendorff]]

Isn't the value in rtmp already rooted, since it's on the operand stack?

I agree about stack budgets, but it seems like a separate bug.

The patch is bitrotted, but the approach seems fine (brushing aside the compatibility issues in comment 4).

Comment 19

[Jeff Walden [:Waldo]]

Memory's hazy, but the problem is the assignment operator can change that value so that it's no longer rooted.  If we're not aiming for 3, there isn't a particularly good reason not to do it the right, hard way, in my opinion.

Comment 20

[Jason Orendorff [:jorendorff]]

Right, probably not.

Sorry I'm so dense today, but I still don't see the GC hazard.  What code do you mean by "the assignment operator"?  That is, specifically what line of code could potentially modify sp[-1] after the FETCH_OPND and before the STORE_OPND?

Comment 21

[Damon Sicore (:damons)]

Adding this to the 1.9.1 triage queue by marking this wanted1.9.1?.  If this should be a blocker, please mark accordingly.

Comment 22

[Brendan Eich [:brendan]]

(In reply to comment #12)
> (Aside: aren't the globals supposed to be accessed with global-specific
> bytecodes like JSOP_SETGVAR?)

There are missed opportunities here -- file a bug, please (or get shaver to do it; perf wins for Fx3.1).

(In reply to comment #18)
> Isn't the value in rtmp already rooted, since it's on the operand stack?

Yes, the problem is rval. See bug 397177.

/be

Comment 23

[Brendan Eich [:brendan]]

As bug 397177 shows, the problem is a pigeon-hole -- whether rtmp or rval is homed to a stack slot, and whichever stack slot is passed by reference for the out parameter to OBJ_SET_PROPERTY, we need another stack slot to hold the original right-hand side value (rtmp in Waldo's patch).

It's easy to dup the RHS before assigning for ops that do not use any stack slot for LHS evaluation, but JSOP_BINDNAME/JSOP_SETNAME, JSOP_SETPROP, and JSOP_SETELEM (among others) all use the stack for LHS evaluation, and order of evaluation requires LHS "reference base" to be computed before any RHS evaluation.

This suggests JSOP_DUP after LHS and RHS eval, to get two copies of the RHS value on the stack, and then some swapping stack op to bury the copy under the LHS slot or slots (JSOP_SETELEM, JSOP_SETXMLNAME). Painful. Still, the common case can be fast and avoid the JSOP_POP or JSOP_POPV.

/be

Comment 24

[Mike Shaver (:shaver emeritus)]

Do we want the dup and swap, or would we be better off with a general JSOP_PEEK taking an 8-bit immediate that specifies how far up sp to peek?  JSOP_DUP would become JSOP_PEEK(0), and it would also be useful if we start to cache common subexpression on the stack -- kind of like a poor man's copy propagation.

Comment 25

[Brendan Eich [:brendan]]

The value to peek at gets overwritten, so we need a copy.

/be

Comment 26

[Brendan Eich [:brendan]]

Easiest path to perf joy is to fuse the POP dispatch into the SET ops. That would give a pretty accurate measure of the win, since the dispatch cost dominates a well-predicted branch around inline popping code at the bottom of each SET case.

/be

Comment 27

[Brendan Eich [:brendan]]

Attachment: fix

** TOTAL **:           1.028x as fast    3119.1ms +/- 0.2%   3034.4ms +/- 0.1%     significant

The GC hazard will be dealt with in bug 397177.

/be

Comment 28

[Igor Bukanov]

(In reply to comment #27)
> Created an attachment (id=325710) [details]
> fix

I guess it make sense to extend the patch to skip POP after various VARINC bytecodes, that is, to add to the patch in js_Interpret(): 

          do_int_fast_incop:
            rval = *vp;
            if (JS_LIKELY(CAN_DO_FAST_INC_DEC(rval))) {
                *vp = rval + incr;
+               JS_ASSERT(JSOP_INCARG_LENGTH == js_CodeSpec[op].length);
+               SKIP_POP_AFTER_SET(JSOP_INCARG, 0);
                PUSH_OPND(rval + incr2);
            } else {
                PUSH_OPND(rval);
                if (!js_DoIncDec(cx, &js_CodeSpec[op], &regs.sp[-1], vp))
                    goto error;
            }

Comment 29

[Igor Bukanov]

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

>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
> 
>+#define SKIP_POP_AFTER_SET(OP,n)                                              \
>+            if (regs.pc[OP##_LENGTH] == JSOP_POP) {                           \
>+                regs.sp -= n;                                                 \
>+                regs.pc += OP##_LENGTH + JSOP_POP_LENGTH;                     \
>+                op = (JSOp) *regs.pc;                                         \
>+                DO_OP();                                                      \
>+            }

JS_BEGIN_MACRO/JS_END_MACRO is missing here. r+ with this fixed.

Comment 30

[Brendan Eich [:brendan]]

Great point about the inc/dec ops -- I will hit them in the next patch.

After looking at JamVM (http://jamvm.sourceforge.net/) I was not sure the JS_BEGIN/END_MACRO-logy was worth it for fragments of code that must end at a certain statement boundary -- where the macro call cannot possibly be treated as a function call (and so need to be safe against unbraced if (cond) FOO(); else ... and the like). But I can do SKIP_POP_AFTER_SET for sure.

/be

Comment 31

[Brendan Eich [:brendan]]

Attachment: fix, v2

** TOTAL **:           1.032x as fast    3119.1ms +/- 0.2%   3022.6ms +/- 0.1%     significant

/be

Comment 32

[Igor Bukanov]

Comment on attachment 325773 [details] [diff] [review]
fix, v2

>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>+#define SKIP_POP_AFTER_SET(OP,n)                                              \
>+            if (regs.pc[OP##_LENGTH] == JSOP_POP) {                           \
>+                regs.sp -= n;                                                 \
>+                regs.pc += OP##_LENGTH + JSOP_POP_LENGTH;                     \
>+                op = (JSOp) *regs.pc;                                         \
>+                DO_OP();                                                      \
>+            }
...

>           do_int_fast_incop:
>             rval = *vp;
>             if (JS_LIKELY(CAN_DO_FAST_INC_DEC(rval))) {
>                 *vp = rval + incr;
>+                JS_ASSERT(JSOP_INCARG_LENGTH == js_CodeSpec[op].length);
>+                SKIP_POP_AFTER_SET(JSOP_INCARG, 0);
>                 PUSH_OPND(rval + incr2);
>             } else {

Minor thing:

SKIP_POP_AFTER_SET depends only on the length of the bytecode. To emphasis that skipping pop does not depend on the bytecode flags etc. it would be better IMO to pass the length, not op there. Also with explicit opcode passing that assert in do_int_fast_incop started to look incomplete as a reader may wonder why only the length property of the bytecode is checked.

Comment 33

[Brendan Eich [:brendan]]

Attachment: patch to commit

Comment 34

[Brendan Eich [:brendan]]

Fixed:

changeset:   15429:555c5f6a3804
tag:         tip
user:        Brendan Eich <[e-mail]>
date:        Thu Jun 19 12:51:57 2008 -0700
summary:     Fix old assignment expression rval mutation via getter design, optimize setprop;pop and similar cliches (312354, r=igor).

/be

Comment 35

[Mike Shaver (:shaver emeritus)]

Should we look at backporting this to the 3.0.x branch?  It affects chaining of assignments for C++-implemented components, as well as increment patterns, obviously.  (Do we need 397177 as well?  That is, is a browser with this patch and without 397177 worse than a bug without this patch at all?)

Comment 36

[Brendan Eich [:brendan]]

Why should any of this go into a Firefox security/stability release, when we have 3.1 in sight? Stay on target!

/be

Comment 37

[Bob Clary [:bc] (inactive)]

v 1.9.1

Comment 38

[Daira Hopwood]

Does this bug also apply to compound assignments ('+=' etc.), i.e. is it ever possible for a compound assignment to return a non-number, or a non-(number or string) in the case of '+='?

(This question is relevant to the security of Jacaranda and Caja.)

Comment 39

[Brendan Eich [:brendan]]

(In reply to comment #38)
> Does this bug also apply to compound assignments ('+=' etc.), i.e. is it ever
> possible for a compound assignment to return a non-number, or a non-(number or
> string) in the case of '+='?

Yes, this bug applies to any assignment. Compound assignment is just a kind of assignment, and it expands in an obvious way (using a hidden temporary if needed to avoid re-evaluating any effects in the left-hand-side's Reference computation) to assignment.

There is no difference between + and += *other than* the issue in this bug with the assignment expression result. The same opcode is used to do the addition whether the source uses a += b or a = a + b. This opcode cannot produce other than a number or a string.

Prior to this bug being fixed, a host object could coerce the result type to object or another type. To see an example, enter:

javascript:alert(typeof (window.location="about:blank"))

in the location toolbar of Firefox 3 or older. To see the same effect with +=, enter:

javascript:alert(typeof(window.location += "#c38"))

while viewing this bug. You'll get "object" alerts both times.

/be

Comment 40

[Daira Hopwood]

Thanks for the information.

This does result in a potential security weakness in Jacaranda and other secure subsets. It would only be exploitable if subset code can get access to an object that does this kind of coercion, i.e. in combination with another security hole (unless there are any native objects that perform coercion on assignment?)

It doesn't result in a weakness in Caja as currently implemented, because the current implementation does not optimize compound assignments. It does optimize other numeric operators, and could optimize compound assignments if not for this bug. I have documented the resulting maintenance hazard at http://code.google.com/p/google-caja/wiki/CompoundAssignmentsCanReturnNonNumber?ts=1226715556&updated=CompoundAssignmentsCanReturnNonNumber

Can the fix be included in FF3.0.5, if and when you do a 3.0.5? It's not important enough for a security release on its own, but if you are doing a point release for any other reason, that would be appreciated.


Perhaps you can also explain the following odd behaviour:

"javascript:alert(window.location -= 0)" alerts with an object that displays as the current URL.

But "window.location - 0" is NaN (as I would expect), and after the alert, the loaded URL ends in "/NaN".

"const x = 1; alert(x -= 1);" alerts 0, not 1, so this behaviour can't be explained as the assignment evaluating to the final value of the left-hand-side (it shouldn't, anyway, but that would be a plausible theory).

So I'm confused as to why "javascript:alert(window.location -= 0)" does not alert "NaN".

Comment 41

[Daira Hopwood]

On further consideration, this issue is not exploitable in Jacaranda (even in combination with another security hole), because Jacaranda restricts assignable expressions to local variables and properties of 'this', which can never be a host object.

Comment 42

[Brendan Eich [:brendan]]

(In reply to comment #40)
> (unless there are any native objects that perform coercion on
> assignment?)

See comment 0, commen 3, and attachment 199452 [details].

> Can the fix be included in FF3.0.5, if and when you do a 3.0.5? It's not
> important enough for a security release on its own, but if you are doing a
> point release for any other reason, that would be appreciated.

Every change carries risk, and we have been burned by regressions from ride-along bugs in the recent past. I'm not going to push for this patch to be back-ported (that requires some small effort too), but if you did rebase the patch against cvs-mirror.mozilla.org and attach it, you could nominate it for consideration by the release drivers.

> Perhaps you can also explain the following odd behaviour:
> 
> "javascript:alert(window.location -= 0)" alerts with an object that displays as
> the current URL.

I'm not sure why, but the DOM's nsWindowSH::SetProperty hook does not return the updated value when processing an assignment to window.location. So you get the side effect but not the result -- the result is the previous value. One more get is required to see the update. Cc'ing jst, he or mrbkap may know why this is the way it is.

> But "window.location - 0" is NaN (as I would expect),

This follows from comment 39. There is no setter invocation.

> and after the alert, the loaded URL ends in "/NaN".

See above, and consider that "NaN" (string conversion of the NaN value) looks like a relative URL (a filename), so URL composition rules say to append it.

> "const x = 1; alert(x -= 1);" alerts 0, not 1, so this behaviour can't be
> explained as the assignment evaluating to the final value of the left-hand-side
> (it shouldn't, anyway, but that would be a plausible theory).

Again, it's not "the final value of the left-hand-side", it's the result of the setter called with the RHS value as "in" parameter. The setter can convert to any type, or return any value.

> So I'm confused as to why "javascript:alert(window.location -= 0)" does not
> alert "NaN".

That part was already answered in comment 39 -- the mystery is why you don't see the NaN appended to the URL.

/be

Comment 43

[Daira Hopwood]

> ... if you did rebase the
> patch against cvs-mirror.mozilla.org and attach it, you could nominate it for
> consideration by the release drivers.

I'm not the right person to do that, because I don't understand the code that is being patched, and it sounds like bug 397177 would also need to be fixed. I'm simply pointing out that it is a security-relevant bug for the JavaScript subsets (even though it happens not to be exploitable), which is pertinent to the argument in comment 36.

> > Perhaps you can also explain the following odd behaviour:
> > 
> > "javascript:alert(window.location -= 0)" alerts with an object that displays as
> > the current URL.
> 
> I'm not sure why, but the DOM's nsWindowSH::SetProperty hook does not return
> the updated value when processing an assignment to window.location. So you get
> the side effect but not the result -- the result is the previous value.

I see -- so the implementation of the object is relied on to return the updated value, and it might return anything.


Just to confirm: this bug never affects assignments to non-global variables, or to properties of objects created by object literals?

Comment 44

[Brendan Eich [:brendan]]

(In reply to comment #43)
> > ... if you did rebase the
> > patch against cvs-mirror.mozilla.org and attach it, you could nominate it for
> > consideration by the release drivers.
> 
> I'm not the right person to do that, because I don't understand the code that
> is being patched,

Ok.

> and it sounds like bug 397177 would also need to be fixed.

No, that is not related (note the lack of a "Depends on" relation). The patch for this bug uses operand stack space scanned by the GC to protect both RHS and setter result values, and defers reducing the stack pointer to unprotect the RHS value until the interpreter has copied the RHS value down over the setter result.

> I'm simply pointing out that it is a security-relevant bug for the JavaScript
> subsets (even though it happens not to be exploitable), which is pertinent to
> the argument in comment 36.

Sorry, no sale -- the subsets will have to cope, and Firefox 3.1 will be out soon enough (after six months, Firefox 3 support will end and users who haven't upgraded will be moved forward).

> Just to confirm: this bug never affects assignments to non-global variables, or
> to properties of objects created by object literals?

Never.

/be

Comment 45

[Daira Hopwood]

> Problem is, we have allowed setters to normalize values and return them as
> the result of the assignment expression for about 10 years now.  The DOM
> level 0 counts on this, for certain setters such as window.location.
[...]
> we can analyze the setter, either before calling it, or based on its
> actual vs. expected return value, and change the expression result to match
> the normalized return value of the setter, only if the setter is flagged
> as in need of this kind of backward compatibility.

Does this mean that the nonconformance with ES3 will not be fixed for window.location, and possibly other properties of host objects?

Comment 46

[Brendan Eich [:brendan]]

(In reply to comment #45)
> Does this mean that the nonconformance with ES3 will not be fixed for
> window.location, and possibly other properties of host objects?

The nonconformance dealt with by this bug has been fixed irrespective of host vs. native object status. The patch contains no special case exception for host objects.

This bug has served its purpose and it's dead -- let's let it RIP.

/be