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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
12 years ago
5 years ago

People

(Reporter: Michael Daumling, Assigned: Michael Daumling)

Tracking

({fixed1.8.1, verified1.8.0.2})

Trunk
x86
Windows XP
fixed1.8.1, verified1.8.0.2
Points:
---
Bug Flags:
blocking1.9a1 +
blocking1.8.1 +
blocking1.8.0.2 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical][rft-dl] doesn't affect ff1.0.x/moz1.7)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

12 years ago
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

12 years ago
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.
(Assignee)

Comment 2

12 years ago
My bad - During the JSGC_MARK_END GC callback, I called JS_RemoveRoot(), which messed up the garbage collector (sometimes).
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → INVALID
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
(Assignee)

Comment 4

12 years ago
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.
(Assignee)

Comment 5

12 years ago
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!

Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Assignee)

Comment 6

12 years ago
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.
Attachment #212530 - Flags: review?(brendan)
(Assignee)

Updated

12 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Updated

12 years ago
Assignee: general → daumling
Status: ASSIGNED → NEW
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
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.
Blocks: 327691
Group: security
Flags: blocking1.9a1+
Flags: blocking1.8.1+
Flags: blocking1.8.0.2?
Whiteboard: [sg:critical]
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
Attachment #212530 - Attachment is obsolete: true
Attachment #212676 - Flags: review?(mrbkap)
Attachment #212530 - Flags: review?(brendan)
Comment on attachment 212676 [details] [diff] [review]
fix

r=mrbkap
Attachment #212676 - Flags: review?(mrbkap) → review+
Flags: blocking1.8.0.2? → blocking1.8.0.2+
Comment on attachment 212676 [details] [diff] [review]
fix

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

/be
Attachment #212676 - Flags: approval1.8.0.2?
Attachment #212676 - Flags: approval-branch-1.8.1+

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
Comment on attachment 212676 [details] [diff] [review]
fix

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #212676 - Flags: approval1.8.0.2? → approval1.8.0.2+
Fixed on branches.

/be
Keywords: fixed1.8.0.2, fixed1.8.1

Updated

12 years ago
Whiteboard: [sg:critical] → [sg:critical][rft-dl]

Updated

12 years ago
Flags: testcase-
Whiteboard: [sg:critical][rft-dl] → [sg:critical][rft-dl] doesn't affect ff1.0.x/moz1.7

Comment 13

11 years ago
verified fixed 1.8.0.2 win/linux/mac from today's build.
Keywords: fixed1.8.0.2 → verified1.8.0.2

Updated

11 years ago
Summary: JSXMLQName structure elements cause crash during gc → CVE-2006-1723 JSXMLQName structure elements cause crash during gc
Group: security
Verified fixed by BC six years ago.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.