444608 - SM: jsxml.c assumes that Namespace and QName are read-only

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

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]

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]

Attachment: 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]

Nominating for branches.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 328963 [details] [diff] [review]
v1

Eek. Thanks,

/be

Comment 5

[Igor Bukanov]

Attachment: v1 as hg bundle

Comment 6

[Shawn Wilsher :sdwilsh]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/fc4af4573716

Comment 7

[Samuel Sidler (old account; do not CC)]

Can we get an automated test for this?

Comment 8

[Bob Clary [:bc] (inactive)]

Attachment: 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]

(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]

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] (inactive)]

Attachment: e4x/Namespace/regress-444608-02.js

Comment 12

[Bob Clary [:bc] (inactive)]

Attachment: e4x/QName/regress-444608.js

Comment 13

[Samuel Sidler (old account; do not CC)]

Igor, does the attached patch apply to both 1.9.0.x and 1.8.1.x?

Comment 14

[Igor Bukanov]

Comment on attachment 328963 [details] [diff] [review]
v1

The patch applies as-is to the 1.9.0 tree.

Comment 15

[Igor Bukanov]

Attachment: 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] (inactive)]

(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] (inactive)]

attachment 333936 [details] [diff] [review] passes 1.8.1 shell/browser opt/debug on linux.

Comment 18

[Igor Bukanov]

Comment on attachment 333936 [details] [diff] [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]

Attachment: diff between trunk/1.9 and 1.8 versions of the fix

Comment 20

[Igor Bukanov]

Attachment: diff -b version of fix for 1.8 branch

Comment 21

[Igor Bukanov]

(In reply to comment #17)
> attachment 333936 [details] [diff] [review] passes 1.8.1 shell/browser opt/debug on linux.
> 

Thanks!

Comment 22

[Bob Clary [:bc] (inactive)]

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]

(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)]

Comment on attachment 328963 [details] [diff] [review]
v1

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 25

[Bob Clary [:bc] (inactive)]

(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]

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 [:dveditz]]

Comment on attachment 333936 [details] [diff] [review]
fix for 1.8 branch

Approved for 1.8.1.17, a=dveditz for release-drivers.

Comment 28

[Igor Bukanov]

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] (inactive)]

verified 1.8.1, 1.9.0, 1.9.1 linux/mac/win browser/shell

Comment 30

[Alexander Sack]

Attachment: for 1.8.0 (just context changes in backport)

a=asac for 1.8.0.15

Comment 31

[Bob Clary [:bc] (inactive)]

/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 32

[:Gavin Sharp [email: [e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fc4af4573716

Comment 33

[Bob Clary [:bc] (inactive)]

v 1.9.1