Bugzilla@Mozilla – Bug 444608
SM: jsxml.c assumes that Namespace and QName are read-only
Last modified: 2009-03-02 00:44:38 PST
Summon comment box
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
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.
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.
Nominating for branches.
Comment on attachment 328963 [details] [review] v1 Eek. Thanks, /be
Created attachment 330033 [details] v1 as hg bundle
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/fc4af4573716
Can we get an automated test for this?
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?
(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.
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
Created attachment 333649 [details] e4x/Namespace/regress-444608-02.js
Created attachment 333650 [details] e4x/QName/regress-444608.js
Igor, does the attached patch apply to both 1.9.0.x and 1.8.1.x?
Comment on attachment 328963 [details] [review] v1 The patch applies as-is to the 1.9.0 tree.
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.
(In reply to comment #15) > To Bob Clary: can you test this patch for 1.8.1 branch? coming right up.
attachment 333936 [details] [review] passes 1.8.1 shell/browser opt/debug on linux.
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.
Created attachment 333954 [details] diff between trunk/1.9 and 1.8 versions of the fix
Created attachment 333955 [details] [review] diff -b version of fix for 1.8 branch
(In reply to comment #17) > attachment 333936 [details] [review] passes 1.8.1 shell/browser opt/debug on linux. > Thanks!
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?
(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 on attachment 328963 [details] [review] v1 Approved for 1.9.0.2. Please land in CVS. a=ss
(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.
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 on attachment 333936 [details] [review] fix for 1.8 branch Approved for 1.8.1.17, a=dveditz for release-drivers.
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
verified 1.8.1, 1.9.0, 1.9.1 linux/mac/win browser/shell
Created attachment 336240 [details] [review] for 1.8.0 (just context changes in backport) a=asac for 1.8.0.15
/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
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fc4af4573716
v 1.9.1