Last Comment Bug 327941 - CVE-2006-1723 JSXMLQName structure elements cause crash during gc
: CVE-2006-1723 JSXMLQName structure elements cause crash during gc
[sg:critical][rft-dl] doesn't affect ...
: fixed1.8.1, verified1.8.0.2
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows XP
-- normal (vote)
: ---
Assigned To: Michael Daumling
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 327691
  Show dependency treegraph
Reported: 2006-02-20 09:58 PST by Michael Daumling
Modified: 2012-03-14 15:25 PDT (History)
6 users (show)
dveditz: blocking1.9a1+
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.2+
bob: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Mark the QName itself during GC (842 bytes, patch)
2006-02-20 15:05 PST, Michael Daumling
no flags Details | Diff | Splinter Review
fix (2.85 KB, patch)
2006-02-21 18:08 PST, Brendan Eich [:brendan]
mrbkap: review+
brendan: approval‑branch‑1.8.1+
dveditz: approval1.8.0.2+
Details | Diff | Splinter Review

Description User image Michael Daumling 2006-02-20 09:58:16 PST
In my test environment, I execute all e4x tests using a Javascript that loads and executes each test, giving each test it unique global object.

The test e4x/Regress/regress-290056.js crashes. I call gc() after each test using the global object that runs the tests, and JSXMLQName->uri is gone when js_MarkXMLQName() is called.

It could be a problem of my test environment - please comment.
Comment 1 User image Bob Clary [:bc:] 2006-02-20 10:28:56 PST
Is this a new failure? I run the browser based tests where gc is performed after the each test completes and didn't see this in my 2-17 runs. Are you running the tests on WinXP really? I've been running on Win2k3, RHEL4 and Mac OS 10.4.4 but haven't on WinXP that recently.
Comment 2 User image Michael Daumling 2006-02-20 13:05:49 PST
My bad - During the JSGC_MARK_END GC callback, I called JS_RemoveRoot(), which messed up the garbage collector (sometimes).
Comment 3 User image Brendan Eich [:brendan] 2006-02-20 13:17:49 PST
Michael: how did removing a root after the mark phase completed mess up the GC, exactly?  It would poke the GC, causing it to restart itself after the sweep phase completed, but that shouldn't be problematic.  I hate a mystery.

Comment 4 User image Michael Daumling 2006-02-20 13:46:15 PST
This is extremely hard to debug. 

The JSXMLQName object was created during an XML parsing operation. During the sweep phase, the JSXMLQName affected would be freed, and restarting all of a sudden caused the JSObject that embedded the XML tree to mark its tree.

I'll spend more time on the problem and comment later.
Comment 5 User image Michael Daumling 2006-02-20 15:03:42 PST
OK, I've found the problem.

In xml_mark_tail(), if the XML is a list, this code marks the JSXMLQName:

        if (xml->xml_targetprop)
            js_MarkXMLQName(cx, xml->xml_targetprop, arg);

Only if js_MarkXMLQName() is called from within this context, the JSXMLQName itself is never marked!

Comment 6 User image Michael Daumling 2006-02-20 15:05:08 PST
Created attachment 212530 [details] [diff] [review]
Mark the QName itself during GC

OK, here is the fix. This seems to be a very rare crasher. I could repro it reliably in my test suite, but it was impossible to narrow down.
Comment 7 User image Daniel Veditz [:dveditz] 2006-02-21 17:10:48 PST
This is the same problem described by the crashMe(false) testcase in bug 327691. Easier to split that case here (especially given the patch) than to track multiple problems in the same bug.
Comment 8 User image Brendan Eich [:brendan] 2006-02-21 18:08:31 PST
Created attachment 212676 [details] [diff] [review]

The mark implementation for a GC-thing type should not have to call mark on the thing, only on its "children".  What's really wrong is the lack of JS_MarkGCThing usage elsewhere.

While here, I fixed another bug that Blake pointed out.  Someone take this from my plate please!  Too much food there already :-/.

Comment 9 User image Blake Kaplan (:mrbkap) 2006-02-21 18:10:10 PST
Comment on attachment 212676 [details] [diff] [review]

Comment 10 User image Brendan Eich [:brendan] 2006-02-21 22:08:12 PST
Comment on attachment 212676 [details] [diff] [review]

This is branch-worthy, both for the next patch release and for Firefox 2.

Comment 11 User image Daniel Veditz [:dveditz] 2006-02-22 11:43:15 PST
Comment on attachment 212676 [details] [diff] [review]

approved for 1.8.0 branch, a=dveditz for drivers
Comment 12 User image Brendan Eich [:brendan] 2006-02-22 15:46:15 PST
Fixed on branches.

Comment 13 User image Bob Clary [:bc:] 2006-03-13 10:06:58 PST
verified fixed win/linux/mac from today's build.
Comment 14 User image Al Billings [:abillings] 2012-03-14 15:25:42 PDT
Verified fixed by BC six years ago.

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