478910 - e4x/Expressions/11.2.2.js FAIL

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Bob Clary [:bc] (inactive)]

Bug 460882 regressed this.

e4x/Expressions/11.2.2.js 

Section 14 of test - 11.2.2 - Function Calls reason: Expected value '<alpha>NL  <name>Foo</name>NL  <length>Bar</length>NL</alpha>'

I have a number of other regressions that may also be caused by this.

Comment 1

[Bob Clary [:bc] (inactive)]

e4x/Expressions/11.2.4.js

Section 1 of test - 11.2.4 - XML Filtering Predicate Operator reason: Expected value '<employee id="0">NL  <name>John</name>NL  <age>20</age>NL</employee>', Actual value ''
Section 2 of test - 11.2.4 - XML Filtering Predicate Operator reason: Expected value '<employee id="0">NL  <name>John</name>NL  <age>20</age>NL</employee>', Actual value ''

Comment 2

[Bob Clary [:bc] (inactive)]

I haven't check the regression range but these seem to be also caused by the same regression:

e4x/XML/13.4.4.29.js
e4x/XML/13.4.4.3.js
e4x/XML/13.4.4.33.js
e4x/XML/13.4.4.4.js
e4x/XML/13.4.4.7.js

Comment 3

[Daniel Veditz [:dveditz]]

Is there a security problem here, or just security-sensitive because it's regressed by a security bug?

Comment 4

[Bob Clary [:bc] (inactive)]

because it's regressed by a security bug? yes.

Comment 5

[Bob Clary [:bc] (inactive)]

e4x/Regress/regress-329257.js
js1_8/regress/regress-384412.js

Comment 6

[Bob Clary [:bc] (inactive)]

Also, <http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_5%2FRegress%2Fregress-68498-003.js;language=type;text/javascript> browser only.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Blake says he wants this!

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

And maybe the fix for this will also fix bug 420642.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 362830 [details] [diff] [review]
Proposed fix

>     indirectCall = (caller && caller->regs && *caller->regs->pc != JSOP_EVAL);
> 
>     /*
>+     * This call to js_GetWrappedObject is safe because of the security checks
>+     * we do below. However, the control flow below is confusing, so we double
>+     * check. There are two cases:
>+     * - Direct call: This object is never used. So unwrapping can't hurt.
>+     * - Indirect call: If this object isn't already the scope chain (which
>+     *   we're guaranteed to be allowed to access) then we do a security
>+     *   check.
>+     */
>+    obj = js_GetWrappedObject(cx, obj);

Why not test indirectCall and null obj if false? Safety first!

>+    /*
>      * Ban all indirect uses of eval (global.foo = eval; global.foo(...)) and
>      * calls that attempt to use a non-global object as the "with" object in
>      * the former indirect case.
>      */
>     scopeobj = OBJ_GET_PARENT(cx, obj);
>-    if (scopeobj) {
>-        scopeobj = js_GetWrappedObject(cx, obj);
>-        scopeobj = OBJ_GET_PARENT(cx, scopeobj);
>-    }

So the old code unwrapped only if parent was not null. Null parent means obj is a global object. What does OBJ_TO_INNER_OBJECT (called if (indirectCall below, not shown) do when invoked on the wrapped obj? Is it the same as what happens now with the unwrapped obj?

>@@ -1343,16 +1350,17 @@ obj_eval(JSContext *cx, JSObject *obj, u
>         if (caller) {
>             scopeobj = js_GetScopeChain(cx, caller);
>             if (!scopeobj) {
>                 ok = JS_FALSE;
>                 goto out;
>             }
>         }
>     } else {
>+        scopeobj = js_GetWrappedObject(cx, scopeobj);

Now that scopeobj wrapped if it is the parent of an unwrapped obj?

I'm so not the wrapper droid you are looking for (but who is?).

/be

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #10)
> So the old code unwrapped only if parent was not null. Null parent means obj is
> a global object. What does OBJ_TO_INNER_OBJECT (called if (indirectCall below,
> not shown) do when invoked on the wrapped obj? Is it the same as what happens
> now with the unwrapped obj?

No. This is the reason that the security checks below are so important in the indirect call case (and why I don't like this patch so much). In this case, we are specifically innerizing and shucking the wrapper.

> Now that scopeobj wrapped if it is the parent of an unwrapped obj?

The parent stuff is a red herring. If scopeobj is non-null here, then it means that we reloaded it from the second argument (js_ValueToObject, above) which *must* be a wrapper (if wrappers are required). But this brings up a good point. We need to innerize here, too (I thought that with objects use to do that for themselves, but no longer apparently.

> I'm so not the wrapper droid you are looking for (but who is?).

I think jst comes as close as anyone.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Really updated

Sorry, forgot to qrefresh.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With 100% less crashing

Setting obj to null early prevents us from testing and denying indirect eval on a non-global object.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 364224 [details] [diff] [review]
With 100% less crashing

>+    obj = js_GetWrappedObject(cx, obj);
>+
>+    /*
>      * Ban all indirect uses of eval (global.foo = eval; global.foo(...)) and
>      * calls that attempt to use a non-global object as the "with" object in
>      * the former indirect case.
>      */
>     scopeobj = OBJ_GET_PARENT(cx, obj);
...
>+        scopeobj = js_GetWrappedObject(cx, scopeobj);

So scopeobj may be set to js_GetWrappedObject(OBJ_GET_PARENT(js_GetWrappedObject(obj))) (cx params elided). But OBJ_GET_PARENT won't rewrap, so why the second js_GetWrappedObject?
 
>+        OBJ_TO_INNER_OBJECT(cx, scopeobj);

Good catch (didn't know I had it in my review comment :-P).

/be

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #15)
> So scopeobj may be set to
> js_GetWrappedObject(OBJ_GET_PARENT(js_GetWrappedObject(obj))) (cx params
> elided). But OBJ_GET_PARENT won't rewrap, so why the second
> js_GetWrappedObject?

This is overly confusing because we reuse scopeobj (I blame the C heritage of this code which prefers odd variable reuse to short lived double use variables). Here's the annotated and simplified control flow (with the current variable names):

scopeobj = OBJ_GET_PARENT(js_GetWrappedObject(obj)) // Note: you had this flipped in your comment
if (scopeobj) {
    throw "EvalError: eval must be called through ..."
}

// scopeobj == null
if (argc >= 2) {
  js_ValueToObject(argv[1], &scopeobj)
}

if (!scopeobj) // check indirectEval and use |obj| for the scope object
else // direct eval with a scope argument

Does that make things clearer?

Comment 17

[Bob Clary [:bc] (inactive)]

(In reply to comment #6)
> Also,
> <http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_5%2FRegress%2Fregress-68498-003.js;language=type;text/javascript>
> browser only.

this appears to have been fixed by http://hg.mozilla.org/mozilla-central/rev/839f915de914

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #16)
> scopeobj = OBJ_GET_PARENT(js_GetWrappedObject(obj)) // Note: you had this
> flipped in your comment

I didn't flip, see the cited code in comment 15 -- I just forward substituted. Of course if argc >= 2 then the early scopeobj set is killed, but that's not certain to happen and the else at 1355 is not conditioned on argc >= 2 (AFAICT).

/be

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Hopefully clearer

I changed some variable names to hopefully clarify this a little bit.

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 364406 [details] [diff] [review]
Hopefully clearer

I'd go with just parent, no "scope" -- avoids all confusion. It's just a short-lived temporary, which a pedant (sometimes I'm one) would wrap in a shrink-to-fit explicit block scope. Thanks for clarifying, I missed the way the first "scopeobj" gets killed by control flow. It really is a separate variable.

r=me with name fussing.

/be

Comment 21

[Robert Sayre]

is this ready to land?

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

I apparently landed it a while ago (March 4th according to the pushlog): http://hg.mozilla.org/mozilla-central/rev/a2f8cf347f2f

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8f53c4c8294d

Comment 24

[Bob Clary [:bc] (inactive)]

e4x/Expressions/11.2.2.js 
e4x/Expressions/11.2.4.js
e4x/XML/13.4.4.29.js
e4x/XML/13.4.4.3.js
e4x/XML/13.4.4.33.js
e4x/XML/13.4.4.4.js
e4x/XML/13.4.4.7.js

all still fail.

Comment 25

[Brendan Eich [:brendan]]

Never fixed, or fixed and then re-regressed?

/be

Comment 26

[Bob Clary [:bc] (inactive)]

I'm looking at my logs from March to find out if they ever passed.

Comment 27

[Bob Clary [:bc] (inactive)]

They appear to never have been fixed. The following fail on 1.9.1, 1.9.1 tracemonkey, 1.9.2

e4x/Expressions/11.2.2.js : 
Section 14 
Expected value '<alpha>NL  <name>Foo</name>NL  <length>Bar</length>NL</alpha>', 
Actual value ''

e4x/Expressions/11.2.4.js : 
Section 1 
Expected value '<employee id="0">NL  <name>John</name>NL  <age>20</age>NL</employee>', 
Actual value ''

e4x/Expressions/11.2.4.js : 
Section 2 Expected value '<employee id="0">NL  <name>John</name>NL  <age>20</age>NL</employee>', 
Actual value ''

e4x/XML/13.4.4.29.js : 
cannot call prependChild method on an XML list with 0 elements

e4x/XML/13.4.4.3.js  : 
cannot call appendChild method on an XML list with 0 elements

e4x/XML/13.4.4.33.js : 
cannot call setChildren method on an XML list with 0 elements

e4x/XML/13.4.4.4.js  : 
Section 3 of test - 13.4.4.4 - XML attribute() reason: Expected value '0', 
Actual value ''

e4x/XML/13.4.4.4.js  : Section 4 of test - 13.4.4.4 - XML attribute() reason: 
Expected value '0', 
Actual value ''

e4x/XML/13.4.4.7.js  : 
cannot call childIndex method on an XML list with 0 elements

Comment 28

[Damon Sicore (:damons)]

(In reply to comment #27)
> They appear to never have been fixed. The following fail on 1.9.1, 1.9.1
> tracemonkey, 1.9.2
> 

Wait, is one of those 1.9.0?  You have 1.9.1 listed twice.

Comment 29

[Damon Sicore (:damons)]

Unless you are saying "1.9.1 Tracemonkey" ...

Comment 30

[Bob Clary [:bc] (inactive)]

that's what I was intending to say.

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

Blake: can we expect a new patch?

Also: since this went unfixed (and unnoticed that it was unfixed) for several months, do we think it needs to block 3.5 hard, or can we have it block 3.5.1, instead?

Comment 32

[Brendan Eich [:brendan]]

IINM the fix is in bug 496113, which has a patch and is already marked blocking.

We can't generally use delays in hearing about a blocker to justify shipping with it unfixed. It was broken the whole time. If it's too late to fix, maybe. Here we have a patch in bug 496113.

I think that bug can relieve us of reopening this one, which is still restricted. Bob, what say you?

/be

Comment 33

[Bob Clary [:bc] (inactive)]

(In reply to comment #32)

> I think that bug can relieve us of reopening this one, which is still
> restricted. Bob, what say you?

Sounds good. I'll mark it fixed by|when bug 496113 lands which does fix all of e4x/Expressions/11.2.2.js e4x/Expressions/11.2.4.js e4x/XML/13.4.4.29.js e4x/XML/13.4.4.3.js e4x/XML/13.4.4.33.js e4x/XML/13.4.4.4.js e4x/XML/13.4.4.7.js e4x/Expressions/regress-496113.js.

Comment 34

[Robert Sayre]

Bug 496113 is fixed now

Comment 35

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2

Comment 36

[Al Billings [:abillings - ex-MoCo]]

Bob, is it possible for you to verify this for 1.9.0?

Comment 37

[Al Billings [:abillings - ex-MoCo]]

Note: This may not repro on 1.9.0 in the first place.

Comment 38

[Bob Clary [:bc] (inactive)]

Yea. None of the tests failed on 1.9.0. We can rubber stamp it, but that is all it would be.