Closed
Bug 312354
Opened 19 years ago
Closed 16 years ago
Assignment expressions have wrong type (ecma_3/Operators/11.13.1-002.js)
Categories
(Core :: JavaScript Engine, defect, P2)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
People
(Reporter: Seno.Aiko, Assigned: brendan)
References
Details
Attachments
(4 files, 3 obsolete files)
293 bytes,
text/html
|
Details | |
3.11 KB,
patch
|
brendan
:
review-
|
Details | Diff | Splinter Review |
1.18 KB,
text/plain
|
Details | |
9.48 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
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
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?
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".
Assignee | ||
Comment 4•19 years ago
|
||
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Updated•19 years ago
|
Summary: E4X: Assignment expressions have wrong type → Assignment expressions have wrong type
Assignee | ||
Comment 5•19 years ago
|
||
(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•19 years ago
|
||
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.
Flags: testcase+
Comment 7•19 years ago
|
||
search love.
Summary: Assignment expressions have wrong type → Assignment expressions have wrong type (ecma_3/Operators/11.13.1-002.js)
Assignee | ||
Comment 8•18 years ago
|
||
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
Assignee: general → brendan
Blocks: jsbcopt
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
OS: Windows XP → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
Assignee | ||
Comment 9•18 years ago
|
||
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•17 years ago
|
||
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.
Attachment #298287 -
Flags: review?(brendan)
Comment 14•17 years ago
|
||
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.
Attachment #298497 -
Flags: review?(brendan)
Comment 15•17 years ago
|
||
Attachment #298287 -
Attachment is obsolete: true
Attachment #298287 -
Flags: review?(brendan)
Assignee | ||
Comment 16•17 years ago
|
||
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
Attachment #298497 -
Flags: review?(brendan) → review-
Assignee | ||
Updated•17 years ago
|
Target Milestone: mozilla1.9alpha1 → ---
Comment 18•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
Adding this to the 1.9.1 triage queue by marking this wanted1.9.1?. If this should be a blocker, please mark accordingly.
Flags: wanted1.9.1?
Assignee | ||
Comment 22•17 years ago
|
||
(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
Assignee | ||
Comment 23•17 years ago
|
||
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•17 years ago
|
||
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.
Updated•16 years ago
|
Flags: wanted1.9.1? → wanted1.9.1+
Assignee | ||
Comment 26•16 years ago
|
||
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
Assignee | ||
Comment 27•16 years ago
|
||
** 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
Attachment #325710 -
Flags: review?(igor)
Comment 28•16 years ago
|
||
(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], ®s.sp[-1], vp)) goto error; }
Comment 29•16 years ago
|
||
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.
Attachment #325710 -
Flags: review?(igor)
Assignee | ||
Comment 30•16 years ago
|
||
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
Assignee | ||
Comment 31•16 years ago
|
||
** TOTAL **: 1.032x as fast 3119.1ms +/- 0.2% 3022.6ms +/- 0.1% significant /be
Attachment #325710 -
Attachment is obsolete: true
Attachment #325773 -
Flags: review?(igor)
Comment 32•16 years ago
|
||
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.
Attachment #325773 -
Flags: review?(igor) → review+
Assignee | ||
Comment 33•16 years ago
|
||
Attachment #325773 -
Attachment is obsolete: true
Attachment #325814 -
Flags: review+
Assignee | ||
Comment 34•16 years ago
|
||
Fixed: changeset: 15429:555c5f6a3804 tag: tip user: Brendan Eich <brendan@mozilla.org> 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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 35•16 years ago
|
||
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?)
Assignee | ||
Comment 36•16 years ago
|
||
Why should any of this go into a Firefox security/stability release, when we have 3.1 in sight? Stay on target! /be
Comment 38•16 years ago
|
||
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.)
Assignee | ||
Comment 39•16 years ago
|
||
(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•16 years ago
|
||
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•16 years ago
|
||
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.
Assignee | ||
Comment 42•16 years ago
|
||
(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•16 years ago
|
||
> ... 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?
Assignee | ||
Comment 44•16 years ago
|
||
(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•16 years ago
|
||
> 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?
Assignee | ||
Comment 46•16 years ago
|
||
(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
You need to log in
before you can comment on or make changes to this bug.
Description
•