Closed
Bug 356238
Opened 18 years ago
Closed 18 years ago
Avoid null, dups and bad indexes in jsxml.c
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
People
(Reporter: igor, Assigned: igor)
References
Details
(Keywords: verified1.8.0.9, verified1.8.1.1, Whiteboard: [sg:critical])
Attachments
(4 files, 1 obsolete file)
3.17 KB,
patch
|
brendan
:
review+
mconnor
:
approval1.8.0.9+
mconnor
:
approval1.8.1.1+
|
Details | Diff | Splinter Review |
1.97 KB,
text/plain
|
Details | |
2.23 KB,
text/plain
|
Details | |
2.28 KB,
text/plain
|
Details |
[ This is a spin-off of security-sensitive bug 354145 comment 71. ] With the bug 354145 fixed xml_child_helper may set rval to JSVAL_NULL, but xml_child does not check for this leading to potential null pointer access.
Assignee | ||
Comment 1•18 years ago
|
||
I am renaming the bug to reflect its generic nature for extra fixes after landing bug 354145.
Summary: xml_child should check for null rval set by xml_child_helper → Avoid null, dups and bad indexes in jsxml.c
Assignee | ||
Comment 2•18 years ago
|
||
Replace method in jsxml.c contains an assert that checks that the passed index is not exceeding the length. But this is a wrong assert as Replace is called directly from xml_replace so it can contain an arbitrary index. This assert is harmless since the code resets the index in any case later. The following one-liner shows the problem in a debug build: js> <xml/>.replace(1); Assertion failure: i <= n, at jsxml.c:3834 Trace/breakpoint trap (core dumped)
Assignee | ||
Comment 3•18 years ago
|
||
Here is a test case that show that Insert can create duplicates: var xml = <xml><child><a/><b/></child></xml>; var child = xml.child[0]; try { child.insertChildAfter(null, xml.child); throw "insertChildAfter succeded when it should throw an exception"; } catch (e if e instanceof Error) { } if (child.a[0] === child.a[1]) throw "Duplicate detected!" Currently it throws "Duplicate detected!" since Insert first reserve the room for list elements to insert and then checks for loops there.
Assignee | ||
Updated•18 years ago
|
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
Assignee | ||
Comment 4•18 years ago
|
||
Attachment #241938 -
Flags: review?(brendan)
Attachment #241938 -
Flags: approval1.8.1.1?
Comment 5•18 years ago
|
||
Comment on attachment 241938 [details] [diff] [review] Fix v1 Looks good, some questions: * Does Rhino have this bug? * Is this a spec bug? * Why undefined instead of null from XML.prototype.child? Spec doesn't help here, so suspect a spec bug in part. /be
Assignee | ||
Comment 6•18 years ago
|
||
(In reply to comment #5) > (From update of attachment 241938 [details] [diff] [review] [edit]) > Looks good, some questions: > > * Does Rhino have this bug? No. In Rhino E4X implementation is effectively a bridge from XMLBeans Apache library and JS. Thus all the unwrapping of object into corresponding native classes is done very early or not done at all. That is, in Rhino in many places the code just returns when it see NON-xml object. > * Is this a spec bug? [[Insert]] and [[Replace]] definitions, 9.1.1.11 and 9.1.1.12 do not require to detect cycles. In particular [[Insert]] states: 1. If x.[[Class]] ∈ {"text", "comment", "processing-instruction", "attribute"}, return 2. Let i = ToUint32(P) 3. If (ToString(i) is not equal to P), throw a TypeError exception 4. Let n = 1 5. If Type(V) is XMLList, let n = V.[[Length]] 6. If n == 0, Return 7. For j = x.[[Length]]-1 downto i, rename property ToString(j) of x to ToString(j + n) 8. Let x.[[Length]] = x.[[Length]] + n 9. If Type(V) is XMLList a. For j = 0 to V.[[Length-1]] i. V[j].[[Parent]] = x ii. x[i + j] = V[j] NOTE: the E4X data model does not enforce the constraint: ∀ x ∈ XML : x.[[InScopeNamespaces]] ⊇ x.[[Parent]].[[InScopeNamespaces]]. However, implementations may at this point add namespaces from V[j].[[InScopeNamespaces]] to x or any ancestors of x. Likewise, implementations may at this point add namespaces from x to V[j] or any descendents of V[j]. 10. Else a. Call the [[Replace]] method of x with arguments i and V 11. Return > * Why undefined instead of null from XML.prototype.child? Spec doesn't help > here, so suspect a spec bug in part. Because JSVAL_VOID is already used by xml_child_helper to indicate that child does not exist and I wanted a minimal patch. Note that rval in xml_child_helper represent local jsval in xml_child which is never exposed to scripts.
Comment 7•18 years ago
|
||
(In reply to comment #6) > > * Is this a spec bug? > > [[Insert]] and [[Replace]] definitions, 9.1.1.11 and 9.1.1.12 do not require to > detect cycles. Actually we added cycle checking to Edition 2 of ECMA-357 (what went to ISO), IIRC. I'll see if I can find the .pdf. Ok, patch looks good, thanks for answers. Stamping in a minute. /be
Comment 8•18 years ago
|
||
Comment on attachment 241938 [details] [diff] [review] Fix v1 r=me, thanks again. /be
Attachment #241938 -
Flags: review?(brendan) → review+
Comment 9•18 years ago
|
||
Comment on attachment 241938 [details] [diff] [review] Fix v1 Should this follow the patch for bug 354145 into 1.8.0.x? /be
Assignee | ||
Comment 10•18 years ago
|
||
I committed the patch from comment 4 to the trunk: Checking in jsxml.c; /cvsroot/mozilla/js/src/jsxml.c,v <-- jsxml.c new revision: 3.132; previous revision: 3.131 done
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 11•18 years ago
|
||
Comment on attachment 241938 [details] [diff] [review] Fix v1 The patch applies to 1.8.0 branch as is.
Attachment #241938 -
Flags: approval1.8.0.9?
Comment 12•18 years ago
|
||
Comment 13•18 years ago
|
||
Updated•18 years ago
|
Flags: in-testsuite+
Comment 14•18 years ago
|
||
fix the initial value of the actual variable.
Attachment #242052 -
Attachment is obsolete: true
Assignee | ||
Comment 16•18 years ago
|
||
This is not a cleanup or null-pointer-only bug. The problem is that Insert in jsxml.c uses XMLArrayInsert. That in turn calls XMLArraySetCapacity which calls realloc without clearing the extra allocated tail. Then insert in case of tail insert simply increases the array length so junk elements become a part of the array. Since prior this bug fix a script can prevent overwriting them, this allows for the script to reinterpret as JSXML* a memory that realloc brought, like in the following example: @callisto~/m/1.8.1/mozilla/js/src> cat ~/m/files/y2.js var xml = <xml><child><a/></child></xml>; var child = xml.child[0]; try { child.insertChildBefore(null, xml.child); throw "insertChildBefore succeded when it should throw an exception"; } catch (e if e instanceof Error) { } var list = child.*; var text = uneval(list[1]); if (text !== "undefined") throw "child got unecxpected second element: "+text; @callisto~/m/1.8.1/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/m/files/y2.js Assertion failure: (uint32)2 < JS_MIN(((obj)->map)->freeslot, ((obj)->map)->nslots), at jsapi.c:2358 Trace/breakpoint trap (core dumped) @callisto~/m/1.8.1/mozilla/js/src> On the trunk with the patch fixed the inserted elements are always cleared.
Assignee | ||
Comment 17•18 years ago
|
||
(In reply to comment #16) > On the trunk with the patch fixed the inserted elements are always cleared. I meant "with the patch landed". That is, the committed fix solved this problem as well.
Comment 18•18 years ago
|
||
additional Igor test from comment 16.
Updated•18 years ago
|
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+
Comment 19•18 years ago
|
||
Comment on attachment 241938 [details] [diff] [review] Fix v1 a=mconnor on behalf of drivers for 1.8.0.9 and 1.8.1.1 checkin
Attachment #241938 -
Flags: approval1.8.1.1?
Attachment #241938 -
Flags: approval1.8.1.1+
Attachment #241938 -
Flags: approval1.8.0.9?
Attachment #241938 -
Flags: approval1.8.0.9+
Assignee | ||
Comment 20•18 years ago
|
||
I committed the patch from comment 4 to MOZILLA_1_8_BRANCH: Checking in jsxml.c; /cvsroot/mozilla/js/src/jsxml.c,v <-- jsxml.c new revision: 3.50.2.53; previous revision: 3.50.2.52 done
Assignee | ||
Updated•18 years ago
|
Keywords: fixed1.8.1.1
Assignee | ||
Comment 21•18 years ago
|
||
I committed the patch from comment 4 to MOZILLA_1_8_0_BRANCH: Checking in jsxml.c; /cvsroot/mozilla/js/src/jsxml.c,v <-- jsxml.c new revision: 3.50.2.15.2.24; previous revision: 3.50.2.15.2.23 done
Keywords: fixed1.8.0.9
Updated•18 years ago
|
Whiteboard: [sg:moderate] (mem snooping?)
Assignee | ||
Comment 23•18 years ago
|
||
> Status Whiteboard: [sg:moderate] (memsnooping?)
See coment 16 where the example crashes due to a reinterpretation of the garbge that fills the tail of realloced memory as JSXML.
Comment 24•18 years ago
|
||
Thanks, missed that comment (was triaging a bunch of bugs taking best guess)
Whiteboard: [sg:moderate] (mem snooping?) → [sg:critical]
Updated•18 years ago
|
Group: security
Comment 25•17 years ago
|
||
/cvsroot/mozilla/js/tests/e4x/Regress/regress-356238-01.js,v <-- regress-356238-01.js /cvsroot/mozilla/js/tests/e4x/Regress/regress-356238-02.js,v <-- regress-356238-02.js /cvsroot/mozilla/js/tests/e4x/Regress/regress-356238-03.js,v <-- regress-356238-03.js modified to remove conditional catch clause to remove SpiderMonkey only extension.
Comment 26•17 years ago
|
||
bug 375801 changed uneval behavior. /cvsroot/mozilla/js/tests/e4x/Regress/regress-356238-03.js,v <-- regress-356238-03.js new revision: 1.3; previous revision: 1.2
You need to log in
before you can comment on or make changes to this bug.
Description
•