Closed Bug 479931 Opened 12 years ago Closed 11 years ago

Crash [@ nsTreeContentView::InsertRow] with <xul:tree> and <option>

Categories

(Core :: XUL, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2
Tracking Status
status1.9.2 --- beta2-fixed
blocking1.9.1 --- .6+
status1.9.1 --- .6-fixed

People

(Reporter: jruderman, Assigned: tnikkel)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [sg:critical?])

Crash Data

Attachments

(5 files, 1 obsolete file)

Firefox crashes trying to do something with memory at 0x1c6ffffc.
Attached file stack trace
Attached patch Like so? (obsolete) — Splinter Review
Attachment #364001 - Flags: superreview?(neil)
Attachment #364001 - Flags: review?(neil)
Comment on attachment 364001 [details] [diff] [review]
Like so?

It's not the parent index that's wrong, it's the child index, and that's wrong because we're not allowing for the <div>. Mind you, I think the code for <optgroup> is wrong too, but that just fails "safe".
Attachment #364001 - Flags: superreview?(neil)
Attachment #364001 - Flags: superreview-
Attachment #364001 - Flags: review?(neil)
Attachment #364001 - Flags: review-
This is all just xbl form controls stuff. We should just remove it from the tree code.
Attachment #364001 - Attachment is obsolete: true
Whiteboard: [sg:critical?]
Blocks: 414170
Attached patch patchSplinter Review
Ah, there is already a function that takes into account content nodes that the tree ignores in order to determine proper indices, so use it here.
Assignee: nobody → tnikkel
Attachment #406901 - Flags: review?(neil)
Comment on attachment 406901 [details] [diff] [review]
patch

>-    PRInt32 count = InsertRow(parentIndex, aIndexInContainer, aChild);
>+    PRInt32 index = 0;
>+    GetIndexInSubtree(aContainer, aChild, &index);
>+    PRInt32 count = InsertRow(parentIndex, index, aChild);
>     if (mBoxObject)
>       mBoxObject->RowCountChanged(parentIndex + aIndexInContainer + 1, count);
You meant to use index here too, right? r=me with that fixed.
Attachment #406901 - Flags: review?(neil) → review+
Whoops, thanks for catching that!
Attached patch patch v2Splinter Review
Fixed Neil's comment.
Even though 1.9.0 and 1.9.1 don't crash on the testcase, the same vulnerable code is on all branches, so I think we should take this on all branches.
blocking1.9.1: --- → ?
Flags: blocking1.9.0.16?
Attachment #406942 - Flags: approval1.9.2?
Attachment #406942 - Flags: approval1.9.1.5?
Attachment #406942 - Flags: approval1.9.0.16?
Duplicate of this bug: 414170
blocking1.9.1: ? → .5+
Flags: wanted1.9.0.x+
Flags: blocking1.9.2?
Flags: blocking1.9.0.16?
Flags: blocking1.9.0.16+
Is there any reason this hasn't landed on mozilla-central for testing yet?
http://hg.mozilla.org/mozilla-central/rev/afd3d529bc14
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9.3a1
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?] → [sg:critical?][needs 1.9.2 approval or blocking]
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P1
Whiteboard: [sg:critical?][needs 1.9.2 approval or blocking] → [sg:critical?][needs 1.9.2 landing]
Comment on attachment 406942 [details] [diff] [review]
patch v2

This is blocking 1.9.2 now, so it doesn't need approval to land there.
Attachment #406942 - Flags: approval1.9.2?
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/52713e37cfc6
Whiteboard: [sg:critical?][needs 1.9.2 landing] → [sg:critical?]
Target Milestone: mozilla1.9.3a1 → mozilla1.9.2
Is it possible to write a testcase that exercises the bug on the 1.9.1/1.9.0 branches? Otherwise QA won't be able to verify this.
I think the reason is doesn't crash on 1.9.0 and 1.9.1 is the switch from a nsVoidArray to a nsTArray. I haven't looked any more closely.
If you just need something that has observable differences before/after this patch on 1.9.1/1.9.0 this testcase will do.

Clicking the button should insert an x between the a and b. Without the patch the x is inserted between the b and c.
Comment on attachment 406942 [details] [diff] [review]
patch v2

Approved for 1.9.1.5 and 1.9.0.16, a=dveditz for release-drivers
Attachment #406942 - Flags: approval1.9.1.6?
Attachment #406942 - Flags: approval1.9.1.6+
Attachment #406942 - Flags: approval1.9.0.16?
Attachment #406942 - Flags: approval1.9.0.16+
Checking in nsTreeContentView.cpp;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeContentView.cpp,v  <--  nsTreeContentView.cpp
new revision: 1.71; previous revision: 1.70
Keywords: fixed1.9.0.16
(In reply to comment #17)
> Created an attachment (id=410163) [details]
> testcase with observable difference on 1.9.1/1.9.0
> 
> If you just need something that has observable differences before/after this
> patch on 1.9.1/1.9.0 this testcase will do.
> 
> Clicking the button should insert an x between the a and b. Without the patch
> the x is inserted between the b and c.

Verified using this testcase on 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5. I see the difference in behavior between 1.9.1.5 and 1.9.1.6.
Keywords: verified1.9.1
Verified using the same testcase on 1.9.0 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.16pre) Gecko/2009111921 GranParadiso/3.0.16pre (.NET CLR 3.5.30729) and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.0.16pre) Gecko/2009111916 GranParadiso/3.0.16pre.
Group: core-security
Crash Signature: [@ nsTreeContentView::InsertRow]
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
You need to log in before you can comment on or make changes to this bug.