Closed
Bug 444608
Opened 16 years ago
Closed 16 years ago
SM: jsxml.c assumes that Namespace and QName are read-only
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
People
(Reporter: igor, Assigned: igor)
Details
(Keywords: verified1.8.1.17, verified1.9.0.2, verified1.9.1, Whiteboard: [sg:critical?])
Attachments
(9 files)
11.57 KB,
patch
|
brendan
:
review+
samuel.sidler+old
:
approval1.9.0.2+
|
Details | Diff | Splinter Review |
1.70 KB,
application/octet-stream
|
Details | |
2.07 KB,
text/plain
|
Details | |
2.08 KB,
text/plain
|
Details | |
2.07 KB,
text/plain
|
Details | |
10.93 KB,
patch
|
brendan
:
review+
dveditz
:
approval1.8.1.17+
|
Details | Diff | Splinter Review |
11.39 KB,
text/plain
|
Details | |
10.41 KB,
patch
|
Details | Diff | Splinter Review | |
10.75 KB,
patch
|
asac
:
approval1.8.0.next+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•16 years ago
|
||
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.
Summary: SM: jsxml.c assumes that Namespace and QName is read-only → SM: jsxml.c assumes that Namespace and QName are read-only
Assignee | ||
Comment 2•16 years ago
|
||
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.
Attachment #328963 -
Flags: review?(brendan)
Assignee | ||
Comment 3•16 years ago
|
||
Nominating for branches.
Flags: blocking1.9.1?
Flags: blocking1.9.0.2?
Flags: blocking1.8.1.17?
Comment 4•16 years ago
|
||
Comment on attachment 328963 [details] [diff] [review] v1 Eek. Thanks, /be
Attachment #328963 -
Flags: review?(brendan) → review+
Updated•16 years ago
|
Flags: blocking1.8.1.17? → blocking1.8.1.17+
Whiteboard: [sg:critical?]
Assignee | ||
Comment 5•16 years ago
|
||
Comment 6•16 years ago
|
||
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/fc4af4573716
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 7•16 years ago
|
||
Can we get an automated test for this?
Flags: in-testsuite?
Flags: blocking1.9.0.2?
Flags: blocking1.9.0.2+
Comment 8•16 years ago
|
||
This is for redefining Namespace. I don't know how to get a crash with QName. Igor?
Updated•16 years ago
|
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus-
Assignee | ||
Comment 9•16 years ago
|
||
(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.
Assignee | ||
Comment 10•16 years ago
|
||
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
Updated•16 years ago
|
Flags: in-testsuite+ → in-testsuite?
Updated•16 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?][needs test before landing]
Comment 11•16 years ago
|
||
Comment 12•16 years ago
|
||
Updated•16 years ago
|
Flags: in-testsuite? → in-testsuite+
Comment 13•16 years ago
|
||
Igor, does the attached patch apply to both 1.9.0.x and 1.8.1.x?
Whiteboard: [sg:critical?][needs test before landing] → [sg:critical?]
Assignee | ||
Comment 14•16 years ago
|
||
Comment on attachment 328963 [details] [diff] [review] v1 The patch applies as-is to the 1.9.0 tree.
Attachment #328963 -
Flags: approval1.9.0.2?
Assignee | ||
Comment 15•16 years ago
|
||
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•16 years ago
|
||
(In reply to comment #15) > To Bob Clary: can you test this patch for 1.8.1 branch? coming right up.
Comment 17•16 years ago
|
||
attachment 333936 [details] [diff] [review] passes 1.8.1 shell/browser opt/debug on linux.
Assignee | ||
Comment 18•16 years ago
|
||
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.
Attachment #333936 -
Flags: review?(brendan)
Assignee | ||
Comment 19•16 years ago
|
||
Assignee | ||
Comment 20•16 years ago
|
||
Assignee | ||
Comment 21•16 years ago
|
||
(In reply to comment #17) > attachment 333936 [details] [diff] [review] passes 1.8.1 shell/browser opt/debug on linux. > Thanks!
Comment 22•16 years ago
|
||
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?
Updated•16 years ago
|
Attachment #333936 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 23•16 years ago
|
||
(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•16 years ago
|
||
Comment on attachment 328963 [details] [diff] [review] v1 Approved for 1.9.0.2. Please land in CVS. a=ss
Attachment #328963 -
Flags: approval1.9.0.2? → approval1.9.0.2+
Comment 25•16 years ago
|
||
(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.
Assignee | ||
Updated•16 years ago
|
Attachment #333936 -
Flags: approval1.8.1.17?
Assignee | ||
Comment 26•16 years ago
|
||
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
Keywords: fixed1.9.0.2
Comment 27•16 years ago
|
||
Comment on attachment 333936 [details] [diff] [review] fix for 1.8 branch Approved for 1.8.1.17, a=dveditz for release-drivers.
Attachment #333936 -
Flags: approval1.8.1.17? → approval1.8.1.17+
Assignee | ||
Comment 28•16 years ago
|
||
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
Keywords: fixed1.8.1.17
Comment 29•16 years ago
|
||
verified 1.8.1, 1.9.0, 1.9.1 linux/mac/win browser/shell
Status: RESOLVED → VERIFIED
Updated•16 years ago
|
Group: core-security
Comment 31•16 years ago
|
||
/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
Updated•15 years ago
|
Flags: blocking1.8.0.next+
Updated•15 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Comment 32•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fc4af4573716
Keywords: fixed1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•