Last Comment Bug 444608 - SM: jsxml.c assumes that Namespace and QName are read-only
: SM: jsxml.c assumes that Namespace and QName are read-only
Status: VERIFIED FIXED
: [sg:critical?]
: verified1.8.1.17, verified1.9.0.2, verified1.9.1
Product: Core
Classification: Components
Component: JavaScript Engine
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
: general
:
:
:
  Show dependency treegraph
 
Reported: 2008-07-10 09:47 PDT by Igor Bukanov
Modified: 2009-03-02 00:44 PST (History)
7 users (show)
sayrer: blocking1.9.1+
samuel.sidler+old: blocking1.9.0.2+
dveditz: blocking1.8.1.17+
asac: blocking1.8.0.next+
bclary: in‑testsuite+
bclary: in‑litmus-
See Also:
Crash Signature:


Attachments
v1 (11.57 KB, patch)
2008-07-10 13:06 PDT, Igor Bukanov
brendan: review+
samuel.sidler+old: approval1.9.0.2+
Details | Diff | Splinter Review
v1 as hg bundle (1.70 KB, application/octet-stream)
2008-07-17 07:21 PDT, Igor Bukanov
no flags Details
e4x/Namespace/regress-444608.js (2.07 KB, text/plain)
2008-08-08 01:04 PDT, Bob Clary [:bc:]
no flags Details
e4x/Namespace/regress-444608-02.js (2.08 KB, text/plain)
2008-08-13 16:41 PDT, Bob Clary [:bc:]
no flags Details
e4x/QName/regress-444608.js (2.07 KB, text/plain)
2008-08-13 16:41 PDT, Bob Clary [:bc:]
no flags Details
fix for 1.8 branch (10.93 KB, patch)
2008-08-15 07:42 PDT, Igor Bukanov
brendan: review+
dveditz: approval1.8.1.17+
Details | Diff | Splinter Review
diff between trunk/1.9 and 1.8 versions of the fix (11.39 KB, text/plain)
2008-08-15 08:57 PDT, Igor Bukanov
no flags Details
diff -b version of fix for 1.8 branch (10.41 KB, patch)
2008-08-15 08:59 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
for 1.8.0 (just context changes in backport) (10.75 KB, patch)
2008-08-31 05:54 PDT, Alexander Sack
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Summon comment box

Description Igor Bukanov 2008-07-10 09:47:46 PDT
jsxml.c contains 3 fragments like:

    nsobj = CallConstructorFunction(cx, obj, &js_NamespaceClass.base, 1,
                                    vp + 2);
...
    nameobj = CallConstructorFunction(cx, obj, &js_QNameClass.base, 1, &name);
    if (!nameobj)
        return JS_FALSE;

with CallConstructorFunction defined as:

static JSObject *
CallConstructorFunction(JSContext *cx, JSObject *obj, JSClass *clasp,
                        uintN argc, jsval *argv)
{
...
    if (!JS_CallFunctionName(cx, obj, clasp->name, argc, argv, &rval))
        return NULL;
    JS_ASSERT(!JSVAL_IS_PRIMITIVE(rval));
    return JSVAL_TO_OBJECT(rval);
}

Here the code assumes that Namespace and QName at the global scope point to the default native constructors. Since both Namespace and QName are not read-only, this can be trivially used to crush the engine on a de-reference of a script-supplied address:

~/m/20-ff/js/src> cat ~/s/x.js
var x = <xml/>;
Namespace = function() { return 10; };
x.addNamespace("x");
~/m/20-ff/js/src> ./Linux_All_OPT.OBJ/js ~/s/x.js
Segmentation fault
Comment 1 Igor Bukanov 2008-07-10 09:49:48 PDT
To fix this I suggest to call Namespace and QName constructors directly in the same way as the code directly calls the Array and Object constructors for [] and {} literals.
Comment 2 Igor Bukanov 2008-07-10 13:06:03 PDT
Created attachment 328963 [details] [review]
v1

The patch adds QNameHelper and NamespaceHelper that essentially represent constructor code but can also be called directly. The the patch replaces CallConstructorFunction calls by the direct calls to the helpers while ensuring the rooting of all involved GC things.
Comment 3 Igor Bukanov 2008-07-10 13:08:53 PDT
Nominating for branches.
Comment 4 Brendan Eich [:brendan] 2008-07-16 14:43:43 PDT
Comment on attachment 328963 [details] [review]
v1

Eek. Thanks,

/be
Comment 5 Igor Bukanov 2008-07-17 07:21:01 PDT
Created attachment 330033 [details]
v1 as hg bundle
Comment 6 Shawn Wilsher :sdwilsh 2008-07-17 08:28:10 PDT
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/fc4af4573716
Comment 7 Samuel Sidler (old account; do not CC) 2008-07-21 17:45:10 PDT
Can we get an automated test for this?
Comment 8 Bob Clary [:bc:] 2008-08-08 01:04:40 PDT
Created attachment 332903 [details]
e4x/Namespace/regress-444608.js

This is for redefining Namespace. I don't know how to get a crash with QName. Igor?
Comment 9 Igor Bukanov 2008-08-08 01:30:15 PDT
(In reply to comment #8)
> This is for redefining Namespace. I don't know how to get a crash with QName.
> Igor?

I will make a test later today.
Comment 10 Igor Bukanov 2008-08-08 08:09:55 PDT
Here is a 3-line test for QName:

~/m/31-ff/js/src $ cat ~/s/y.js
var x = <a><b/></a>;
QName = function() { return 10; };
x.replace("b", 10);
~/m/31-ff/js/src $ ~/m/31-ff/js/src/Linux_All_OPT.OBJ/js ~/s/x.js
segmentation fault
~/m/

For completeness we should also have another test for Namespace that trigger the bug via removeNamespace call:

~/m/31-ff/js/src $ cat ~/s/y.js
var x = <xml/>;
Namespace = function() { return 10; };
x.removeNamespace("x");
~/m/31-ff/js/src $ ~/m/31-ff/js/src/Linux_All_OPT.OBJ/js ~/s/x.js
segmentation fault

Comment 11 Bob Clary [:bc:] 2008-08-13 16:41:09 PDT
Created attachment 333649 [details]
e4x/Namespace/regress-444608-02.js
Comment 12 Bob Clary [:bc:] 2008-08-13 16:41:41 PDT
Created attachment 333650 [details]
e4x/QName/regress-444608.js
Comment 13 Samuel Sidler (old account; do not CC) 2008-08-13 16:45:41 PDT
Igor, does the attached patch apply to both 1.9.0.x and 1.8.1.x?
Comment 14 Igor Bukanov 2008-08-15 06:37:20 PDT
Comment on attachment 328963 [details] [review]
v1

The patch applies as-is to the 1.9.0 tree.
Comment 15 Igor Bukanov 2008-08-15 07:42:32 PDT
Created attachment 333936 [details] [review]
fix for 1.8 branch

To Bob Clary: can you test this patch for 1.8.1 branch? I am having some troubles running the tests for js shell.
Comment 16 Bob Clary [:bc:] 2008-08-15 07:55:21 PDT
(In reply to comment #15)

> To Bob Clary: can you test this patch for 1.8.1 branch?

coming right up.

Comment 17 Bob Clary [:bc:] 2008-08-15 08:44:51 PDT
attachment 333936 [details] [review] passes 1.8.1 shell/browser opt/debug on linux.
Comment 18 Igor Bukanov 2008-08-15 08:56:04 PDT
Comment on attachment 333936 [details] [review]
fix for 1.8 branch

The patch required hand-merge for the 1.8.1 branch mostly due to fast method changes.
Comment 19 Igor Bukanov 2008-08-15 08:57:37 PDT
Created attachment 333954 [details]
diff between trunk/1.9 and 1.8 versions of the fix
Comment 20 Igor Bukanov 2008-08-15 08:59:23 PDT
Created attachment 333955 [details] [review]
diff -b version of fix for 1.8 branch
Comment 21 Igor Bukanov 2008-08-15 08:59:50 PDT
(In reply to comment #17)
> attachment 333936 [details] [review] passes 1.8.1 shell/browser opt/debug on linux.
> 

Thanks!
Comment 22 Bob Clary [:bc:] 2008-08-15 09:10:51 PDT
Note I only ran the tests included in this bug, not a full test run. Igor, it will take a few hours to do a full run. Do you want it?
Comment 23 Igor Bukanov 2008-08-15 14:19:18 PDT
(In reply to comment #22)
> it will take a few hours to do a full run.

Oh, now I understand why I had troubles with that. Is that on that 8GB box of yours?

> Do you want it?

the full test is not necessary, just anything that involves E4X would be sufficient.
Comment 24 Samuel Sidler (old account; do not CC) 2008-08-15 14:20:56 PDT
Comment on attachment 328963 [details] [review]
v1

Approved for 1.9.0.2. Please land in CVS. a=ss
Comment 25 Bob Clary [:bc:] 2008-08-15 20:40:41 PDT
(In reply to comment #23)
> (In reply to comment #22)
> > it will take a few hours to do a full run.
> 
> Oh, now I understand why I had troubles with that. Is that on that 8GB box of
> yours?

Doesn't matter really. I limit the memory and virtual address space tests can use on the host anyway. VMs are worse of course but I'm moving that box to ESXi tonight if possible (/me crosses fingers). 1.8.1 has lots of failures including tests which timeout (currently 8 minutes each). 2300+ test files * 1 branch * 2 buildtypes (opt/debug) * 2 testtypes (shell/browser) can take a while. 
> 
> the full test is not necessary, just anything that involves E4X would be
> sufficient.

1.8.1 w/patch passed opt/debug shell/browser centos5-64 on all e4x without regressions modulo normally excluded files 

Note: only e4x/Namespace/regress-444608.js failed on the 1.8.1 branch previous to the patch.
Comment 26 Igor Bukanov 2008-08-16 16:38:08 PDT
landed on 1.9.0 branch:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.214; previous revision: 3.213
done
Comment 27 Daniel Veditz 2008-08-18 11:24:41 PDT
Comment on attachment 333936 [details] [review]
fix for 1.8 branch

Approved for 1.8.1.17, a=dveditz for release-drivers.
Comment 28 Igor Bukanov 2008-08-19 07:42:43 PDT
landed on MOZILLA1_8_BRANCH:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.71; previous revision: 3.50.2.70
done
Comment 29 Bob Clary [:bc:] 2008-08-21 18:22:08 PDT
verified 1.8.1, 1.9.0, 1.9.1 linux/mac/win browser/shell
Comment 30 Alexander Sack 2008-08-31 05:54:11 PDT
Created attachment 336240 [details] [review]
for 1.8.0 (just context changes in backport)

a=asac for 1.8.0.15
Comment 31 Bob Clary [:bc:] 2008-09-24 00:46:53 PDT
/cvsroot/mozilla/js/tests/e4x/Namespace/regress-444608.js,v  <--  regress-444608.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/e4x/Namespace/regress-444608-02.js,v  <--  regress-444608-02.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/e4x/QName/regress-444608.js,v  <--  regress-444608.js
initial revision: 1.1
done

http://hg.mozilla.org/mozilla-central/rev/42f1309d549f
Comment 33 Bob Clary [:bc:] 2009-03-02 00:44:38 PST
v 1.9.1

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