327941 - CVE-2006-1723 JSXMLQName structure elements cause crash during gc

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Michael Daumling]

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] (inactive)]

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]

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]]

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]

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]

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]

Attachment: 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]]

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]]

Attachment: 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) (inactive)]

Comment on attachment 212676 [details] [diff] [review]
fix

r=mrbkap

Comment 10

[Brendan Eich [:brendan]]

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]]

Comment on attachment 212676 [details] [diff] [review]
fix

approved for 1.8.0 branch, a=dveditz for drivers

Comment 12

[Brendan Eich [:brendan]]

Fixed on branches.

/be

Comment 13

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.0.2 win/linux/mac from today's build.

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Verified fixed by BC six years ago.