Last Comment Bug 325425 - jsxml.c: Bad assumptions about js_ConstructObject
: jsxml.c: Bad assumptions about js_ConstructObject
: verified1.8.0.5, verified1.8.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: mozilla1.9alpha1
Assigned To: Blake Kaplan (:mrbkap)
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2006-02-01 01:31 PST by Igor Bukanov
Modified: 2007-02-08 13:29 PST (History)
6 users (show)
dveditz: blocking1.7.14-
dveditz: blocking‑aviary1.0.9-
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.5+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Fix both cases (3.51 KB, patch)
2006-02-02 19:17 PST, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Attempt 2 (2.96 KB, patch)
2006-02-06 11:16 PST, Blake Kaplan (:mrbkap)
igor: review-
Details | Diff | Splinter Review
Not forgetting js.msg this time (4.30 KB, patch)
2006-02-06 23:28 PST, Blake Kaplan (:mrbkap)
igor: review+
brendan: superreview+
dveditz: approval‑branch‑1.8.1+
dveditz: approval1.8.0.5+
Details | Diff | Splinter Review
e4x/Regress/regress-325425.js (2.04 KB, text/plain)
2006-02-11 21:25 PST, Bob Clary [:bc:]
no flags Details
e4x/Regress/regress-325425.js (2.10 KB, text/plain)
2006-02-15 09:50 PST, Bob Clary [:bc:]
no flags Details

Description Igor Bukanov 2006-02-01 01:31:08 PST
1. In several places when constructing QName and Namespace instances the code form jsxml.c assumes that the result of js_ConstructObject is proper QName/Namespace\
 object. But since QName and Namespace constructors can be replaced or hidden via "with" statement, that assumption is wrong. Here is a trivial example that demon\
strates the bug:

js> QName = function() { }; <xml/>.elements("")
Segmentation fault

The bug allows, for example, for a script for cast as a pointer to JSXMLNamespace or JSXMLQName a pointer to JSScript. This would lead to reading of pointer to in\
terpreter bytecode as a pointer to JSObject. Given the rather dense nature of the bytecode, this provides a wide range of possible value.

2. ToXMLName calls js_ConstructObject with a pointer to jsval as argument array. Since jsval is constructed from ValueToString, it can be rooted only through last\
InternalResult. With prototype getter defined that can be made unrooted and GC-ed after which it would be passed to the constructor.
Comment 1 Daniel Veditz [:dveditz] 2006-02-01 09:21:38 PST
Assuming this can be abused
Comment 2 Daniel Veditz [:dveditz] 2006-02-01 09:22:59 PST
"(I am) assuming..." to be clear, not doubting.

Comment 3 Blake Kaplan (:mrbkap) 2006-02-01 19:17:38 PST
I'll take this.
Comment 4 Igor Bukanov 2006-02-02 04:31:37 PST
Here are more problems with js_ConstructObject usage when code from jsxml.c calls it passing unrooted arguments:

1. xml_setNamespace calls js_ConstructObject with unrooted nsobj obtained from the previous call to js_ConstructObject.

2. The interpreter calls js_SetDefaultXMLNamespace with jsval popped up from the stack and cut from its roots. The value is then passed to js_ConstructObject.
Comment 5 Brendan Eich [:brendan] 2006-02-02 12:05:18 PST
(In reply to comment #4)
> Here are more problems with js_ConstructObject usage when code from jsxml.c
> calls it passing unrooted arguments:

js_ConstructObject should use a JSTempValueRooter to protect its argv.  I think that would fix these cases.  Any other hazards would have to exist in the callers of js_ConstructObject, independent of its hazards.

Blake and I talked about this bug, it bites more than jsxml.c.  It's an old API bug: JS_ConstructObject should throw a TypeError if the constructor substitutes an object of a different class from the clasp passed into it.  Otherwise many users of {js,JS}_ConstructObject are hacked.  Changing the API to throw changes it in an incompatible way, but fixes bad exploitable bugs, so it's a good and necessary compatibility break.

Thanks for finding this.

Comment 6 Blake Kaplan (:mrbkap) 2006-02-02 19:17:22 PST
Created attachment 210552 [details] [diff] [review]
Fix both cases

Brendan discussed rooting argv in js_ConstructObject (freeing callers from having to do the same) but I think that an argv parameter is implicity rooted by name elsewhere, so it should be up to the caller in this case to ensure that argv is in fact rooted.
Comment 7 Brendan Eich [:brendan] 2006-02-02 19:41:29 PST
Comment on attachment 210552 [details] [diff] [review]
Fix both cases

>@@ -2158,31 +2158,50 @@ js_ConstructObject(JSContext *cx, JSClas
>             goto out;
>         }
>         if (JSVAL_IS_OBJECT(rval))
>             proto = JSVAL_TO_OBJECT(rval);
>     }
>     obj = js_NewObject(cx, clasp, proto, parent);
>     if (!obj)
>         goto out;
>-    if (!js_InternalConstruct(cx, obj, cval, argc, argv, &rval)) {
>-        cx->newborn[GCX_OBJECT] = NULL;
>-        obj = NULL;
>+    if (!js_InternalConstruct(cx, obj, cval, argc, argv, &rval))
>+        goto bad;
>+    if (JSVAL_IS_PRIMITIVE(rval))
>         goto out;
>+    obj = JSVAL_TO_OBJECT(rval);

Note that obj is by definition non-null here.

>+    /*
>+     * If the given class has both the JSCLASS_HAS_PRIVATE and the
>+     * JSCLASS_CONSTRUCT_PROTOTYPE flags, then the class should have its private
>+     * data set. If it doesn't, then it means the constructor was replaced, and
>+     * we should throw a typerr.
>+     */
>+    if ((obj && OBJ_GET_CLASS(cx, obj) != clasp) ||

So don't null check here.

>+        (!(~clasp->flags & (JSCLASS_HAS_PRIVATE |
>+                            JSCLASS_CONSTRUCT_PROTOTYPE)) &&
>+         !JS_GetPrivate(cx, obj))) {
>+        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>+                             JSMSG_WRONG_CONSTRUCTOR, clasp->name);
>+        goto bad;
>     }
> js_SetDefaultXMLNamespace(JSContext *cx, jsval v)
> {
>     jsval argv[2];
>     JSObject *nsobj, *varobj;
>     JSStackFrame *fp;
>+    JSTempValueRooter tvr;

Nit: declare after argv, to match use order.

>+    JSBool ok;
>     argv[0] = STRING_TO_JSVAL(cx->runtime->emptyString);
>     argv[1] = v;
>+    JS_PUSH_TEMP_ROOT(cx, sizeof argv / sizeof argv[0], argv, &tvr);

Nit: squish argv init and JS_PUSH_TEMP_ROOT here.

>     nsobj = js_ConstructObject(cx, &js_NamespaceClass.base, NULL, NULL,
>                                2, argv);
>-    if (!nsobj)
>-        return JS_FALSE;
>+    if (!nsobj) {
>+        ok = JS_FALSE;
>+        goto out;
>+    }
>     v = OBJECT_TO_JSVAL(nsobj);
>     fp = cx->fp;
>     varobj = fp->varobj;
>     if (varobj) {
>                                  JS_PropertyStub, JS_PropertyStub,
>                                  JSPROP_PERMANENT, NULL)) {
>-            return JS_FALSE;
>+            ok = JS_FALSE;

Nit: set ok = OBJ_DEFINE_PROPERTY(...) to save setting it false on failure.

>+            goto out;
>         }
>     } else {
>         JS_ASSERT(fp->fun && !(fp->fun->flags & JSFUN_HEAVYWEIGHT));

Move the ok = JS_TRUE below to this else clause.

>     }
>     fp->xmlNamespace = JSVAL_TO_OBJECT(v);
>-    return JS_TRUE;
>+    ok = JS_TRUE;
>+    JS_POP_TEMP_ROOT(cx, &tvr);
>+    return ok;
> }

Are there other js_ConstructObject calls in jsxml.c or elsewhere that need similar love?

Comment 8 Igor Bukanov 2006-02-02 23:00:51 PST
(In reply to comment #6)
> Created an attachment (id=210552) [edit]
> Fix both cases

This does not fix xml_setNamespace or ToXMLName cases.

> so it should be up to the caller in this case to ensure that
> argv is in fact rooted.

But then by the same logic it should be interpreter, not js_SetDefaultXMLNamespace, who roots the value. AFAICS popping the value after it is passed to js_SetDefaultXMLNamespace would require less code besides ensuring that the value never looses its roots.


Comment 9 Blake Kaplan (:mrbkap) 2006-02-06 11:16:00 PST
Created attachment 210902 [details] [diff] [review]
Attempt 2

How's this?
Comment 10 Brendan Eich [:brendan] 2006-02-06 17:38:52 PST
Comment on attachment 210902 [details] [diff] [review]
Attempt 2

Much as I love tvr as a name ( -- Travolta's character had one in "Swordfish"), argvtvr might rather be argvr or argtvr -- hey, the last keeps the "tvr" magic and avoids the doubled "v"!

Comment 11 Igor Bukanov 2006-02-06 23:21:51 PST
Comment on attachment 210902 [details] [diff] [review]
Attempt 2

The patch does not compile:
jsobj.c: In function ‘js_ConstructObject’:
jsobj.c:2191: error: ‘JSMSG_WRONG_CONSTRUCTOR’ undeclared (first use in this function)

Plus that obj = NULL after JS_PUSH_SINGLE_TEMP_ROOT can be moved after failed OBJ_GET_PROPERTY
Comment 12 Blake Kaplan (:mrbkap) 2006-02-06 23:28:15 PST
Created attachment 210985 [details] [diff] [review]
Not forgetting js.msg this time
Comment 13 Igor Bukanov 2006-02-07 01:20:01 PST
Comment on attachment 210985 [details] [diff] [review]
Not forgetting js.msg this time

The last attachment only incorporates Brendan-induced renames, it does not include js.msg!
Comment 14 Igor Bukanov 2006-02-07 01:27:21 PST
Comment on attachment 210985 [details] [diff] [review]
Not forgetting js.msg this time

Forget my last comment, everything was OK!
Comment 15 Brendan Eich [:brendan] 2006-02-07 11:44:33 PST
Comment on attachment 210985 [details] [diff] [review]
Not forgetting js.msg this time

> js_ConstructObject(JSContext *cx, JSClass *clasp, JSObject *proto,
>                    JSObject *parent, uintN argc, jsval *argv)
> {
>     jsval cval, rval;
>-    JSTempValueRooter tvr;
>+    JSTempValueRooter tvr, argtvr;

Nit: argtvr, tvr should come first to match init order below.

>     JSObject *obj, *ctor;
>-    if (!js_FindConstructor(cx, parent, clasp->name, &cval))
>+    JS_PUSH_TEMP_ROOT(cx, argc, argv, &argtvr);

sr=me with that nit picked -- thanks!

Comment 16 Blake Kaplan (:mrbkap) 2006-02-07 12:10:13 PST
Fix checked into trunk.
Comment 17 Bob Clary [:bc:] 2006-02-11 21:25:50 PST
Created attachment 211556 [details]
Comment 18 Bob Clary [:bc:] 2006-02-15 09:48:59 PST
Comment on attachment 211556 [details]

This test has an error but there is a spelling issue as well:
e4x/Regress/regress-325425.js:50: TypeError: wrong construtor called for QName
Comment 19 Bob Clary [:bc:] 2006-02-15 09:50:18 PST
Created attachment 211999 [details]
Comment 20 Bob Clary [:bc:] 2006-02-15 09:51:35 PST
(In reply to comment #19)
this has the wrong bug number in the test. fixed locally. 
Comment 21 Brendan Eich [:brendan] 2006-02-15 10:43:24 PST
(In reply to comment #18)
> (From update of attachment 211556 [details] [edit])
> This test has an error but there is a spelling issue as well:
> e4x/Regress/regress-325425.js:50: TypeError: wrong construtor called for QName
>                                                    ^^^^^^^^^^

Fixed by work-in-progress patches for bug 326466.

Comment 22 Bob Clary [:bc:] 2006-03-29 09:53:41 PST
verified fixed trunk 20060328 win/mac/linux
Comment 23 Daniel Veditz [:dveditz] 2006-06-09 13:37:23 PDT
This is marked fixed based on bug 326466, but don't we want this on the 1.8/1.8.0 branches as well? I doubt we the full Pythonic generators patch.

Please ask for approval on the patch if this is branch-appropriate
Comment 24 Daniel Veditz [:dveditz] 2006-06-15 14:16:48 PDT
Comment on attachment 210985 [details] [diff] [review]
Not forgetting js.msg this time

approved for 1.8.0 branch, a=dveditz for drivers
Comment 25 Blake Kaplan (:mrbkap) 2006-06-15 17:52:49 PDT
Fix checked into the 1.8 branches.
Comment 26 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-06-16 06:50:28 PDT
We are intending to take the Pythonic generators on 1.8.1, last I heard.  Given that, I don't _think_ we need this patch on 1.8.1 separately.
Comment 27 Bob Clary [:bc:] 2006-06-26 17:36:31 PDT
verified fixed no crash, 1.8.1, 1.9a1 all platforms
Comment 28 Bob Clary [:bc:] 2007-02-08 13:29:09 PST
/cvsroot/mozilla/js/tests/e4x/Regress/regress-325425.js,v  <--  regress-325425.js
initial revision: 1.1

Note You need to log in before you can comment on or make changes to this bug.