Last Comment Bug 327941 - CVE-2006-1723 JSXMLQName structure elements cause crash during gc
: CVE-2006-1723 JSXMLQName structure elements cause crash during gc
Status: VERIFIED FIXED
[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
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Mark the QName itself during GC (842 bytes, patch)
2006-02-20 15:05 PST, Michael Daumling
no flags Details | Diff | 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 | Review

Description 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 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 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 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.

/be
Comment 4 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 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 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 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 Brendan Eich [:brendan] 2006-02-21 18:08:31 PST
Created attachment 212676 [details] [diff] [review]
fix

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 :-/.

/be
Comment 9 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-02-21 18:10:10 PST
Comment on attachment 212676 [details] [diff] [review]
fix

r=mrbkap
Comment 10 Brendan Eich [:brendan] 2006-02-21 22:08:12 PST
Comment on attachment 212676 [details] [diff] [review]
fix

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

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

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

/be
Comment 13 Bob Clary [:bc:] 2006-03-13 10:06:58 PST
verified fixed 1.8.0.2 win/linux/mac from today's build.
Comment 14 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.