Last Comment Bug 432068 - Crash [@ nsListBoxBodyFrame::GetNextItemBox] with <xul:listbox>, XBL
: Crash [@ nsListBoxBodyFrame::GetNextItemBox] with <xul:listbox>, XBL
[sg:critical?] needs r=bzbarsky
: crash, testcase, verified1.8.1.22, verified1.9.0.11, verified1.9.1
Product: Core
Classification: Components
Component: XP Toolkit/Widgets: XUL (show other bugs)
: Trunk
: All All
P2 critical (vote)
: ---
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
: Neil Deakin
Depends on: 486842 486848 488210
Blocks: 348483
  Show dependency treegraph
Reported: 2008-05-03 17:02 PDT by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
14 users (show)
roc: blocking1.9.1+
stransky: wanted1.8.1.x+
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (crashes Firefox when loaded) (798 bytes, application/vnd.mozilla.xul+xml)
2008-05-03 17:02 PDT, Jesse Ruderman
no flags Details
Testcase not involving XBL (647 bytes, application/vnd.mozilla.xul+xml)
2009-03-31 12:38 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Proposed fix (2.12 KB, patch)
2009-03-31 13:00 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
diff -w (1.44 KB, patch)
2009-03-31 13:03 PDT, Boris Zbarsky [:bz] (still a bit busy)
roc: review+
neil: review+
roc: superreview+
Details | Diff | Splinter Review
roll-up patch suitable for 1.9.0 branch (4.44 KB, patch)
2009-04-05 21:08 PDT, Boris Zbarsky [:bz] (still a bit busy)
samuel.sidler+old: approval1.9.0.11+
Details | Diff | Splinter Review
1.8.0 patch (1.91 KB, patch)
2009-05-18 00:37 PDT, Martin Stránský
bzbarsky: review+
Details | Diff | Splinter Review

Description User image Jesse Ruderman 2008-05-03 17:02:13 PDT
Created attachment 319229 [details]
testcase (crashes Firefox when loaded)

Loading the testcase makes Firefox crash [@ nsListBoxBodyFrame::GetNextItemBox].
Comment 1 User image Théophile Helleboid 2008-05-04 01:58:30 PDT
I can reproduc it with Linux /Firefox 30b5 :
Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9b5) Gecko/2008032619 Firefox/3.0b5
Comment 2 User image Gary Kwong [:gkw] [:nth10sd] 2009-03-29 02:58:30 PDT
(I tested this on mozilla-central latest-trunk nightly build on WinXP SP3)

!exploitable shows this is probably exploitable, turning security-sensitive and nominating blocking1.9.1?

0:000> !exploitable -v
Executing Processor Architecture is x86
Debuggee is in User Mode
Debuggee is a live user mode debugging session on the local machine
Event Type: Exception
Exception Faulting Address: 0x0
First Chance Exception Type: STATUS_ACCESS_VIOLATION (0xC0000005)
Exception Sub-Type: Read Access Violation

Faulting Instruction:1062c568 mov eax,dword ptr [esi]

Basic Block:
    1062c568 mov eax,dword ptr [esi]
       Tainted Input Operands: esi
    1062c56a mov ecx,esi
       Tainted Input Operands: esi
    1062c56c call dword ptr [eax+4ch]
       Tainted Input Operands: eax, ecx

Exception Hash (Major/Minor): 0x6224767a.0x6444e23

Stack Trace:
Instruction Address: 0x1062c568

Description: Data from Faulting Address controls Code Flow
Short Description: TaintedDataControlsCodeFlow
Exploitability Classification: PROBABLY_EXPLOITABLE
Recommended Bug Title: Probably Exploitable - Data from Faulting Address controls Code Flow starting at xul!nsListBoxBodyFrame::GetNextItemBox+0x46 (Hash=0x6224767a.0x6444e23)

The data from the faulting address is later used as the target for a branch.
Comment 3 User image David Baron :dbaron: ⌚️UTC-8 2009-03-31 10:03:31 PDT
I'm seeing a null-dereference; I think that's because we've created a content tree where a particular node has no parent, although I don't completely follow what's going on.
Comment 4 User image Boris Zbarsky [:bz] (still a bit busy) 2009-03-31 12:22:40 PDT
If I breakpoint in the nsListBoxBodyFrame::OnContentRemoved call from that removeChild call, here's what the relevant part of the frametree looks like:

                Box(listboxbody)(0)@0x14ef838 [view=0x1d718510] {0,0,71100,11880} [state=80802000] [content=0x1d710f50] [sc=0x14f94e0] pst=:-moz-scrolled-content<
                  nsGridRowLeaf(listitem)(0)@0x14f9828 next=0x14e9c14 {0,0,71100,840} [state=80c00010] [content=0x1c7bb7e0] [sc=0x14f97cc]<
                    Box(listcell)(-1)@0x14e986c {60,60,70980,720} [state=80600000] [content=0x1d71a410] [sc=0x14f96b8]<
                      XULLabel(label)(-1)@0x14e9b98 {0,330,70980,60} [state=00d00000] sc=0x14e9aa8(i=0,b=0)<
                  nsGridRowLeaf(listitem)(-1)@0x14e9c14 next=0x14e9d3c {0,840,71100,840} [state=80c00000] [content=0x1c7bb870] [sc=0x14f97cc]<
                    Box(listcell)(-1)@0x14e9c7c {60,60,70980,720} [state=80600000] [content=0x1d71dee0] [sc=0x14f96b8]<
                      XULLabel(label)(-1)@0x14e9ce4 {0,330,70980,60} [state=00d00000] sc=0x14e9aa8(i=0,b=0)<
                  nsGridRowLeaf(listitem)(-1)@0x14e9d3c {0,1680,71100,840} [state=80c00000] [content=0x1c7bb870] [sc=0x14f97cc]<
                    Box(listcell)(-1)@0x14e9da4 {60,60,70980,720} [state=80600000] [content=0x1d71dee0] [sc=0x14f96b8]<
                      XULLabel(label)(-1)@0x14e9e0c {0,330,70980,60} [state=00d00000] sc=0x14e9aa8(i=0,b=0)<

Note that the second and third nsGridRowLeaf frames have the same content node; in fact the node we're removing.  So we remove one of them, and then have a frame in the tree which points to a content node whose parent is null.  When we dereference that null later, we crash.
Comment 5 User image Boris Zbarsky [:bz] (still a bit busy) 2009-03-31 12:38:00 PDT
Created attachment 370251 [details]
Testcase not involving XBL

The only reason XBL was needed was to make sure that:

1)  A frame reconstruct (and in particular a ContentInserted) happened
2)  No frame was constructed for the content passed to

This testcase just uses display:none on the listitem for similar effect.  We call nsListBoxBodyFrame::OnContentInserted, which calls nsListBoxBodyFrame::CreateRows, which does the whole GetNextItemBox thing.  Then GetNextItemBox calls CreatListBoxContent, which returns null.  This triggers this lovely bit of code:

      if (result) {
        if (aCreated)
           *aCreated = PR_TRUE;
      } else
        return GetNextItemBox(aBox, ++aOffset, aCreated);

which then tries to construct a frame for the next content... but we have a frame for it already.
Comment 6 User image Boris Zbarsky [:bz] (still a bit busy) 2009-03-31 13:00:09 PDT
Created attachment 370254 [details] [diff] [review]
Proposed fix
Comment 7 User image Boris Zbarsky [:bz] (still a bit busy) 2009-03-31 13:03:10 PDT
Created attachment 370256 [details] [diff] [review]
diff -w

I think this is the right thing to do; just skipping the create if GetPrimaryFrameFor() returns something different from mLinkupFrame might not work well if mRowsToPrepend > 0.  But then again, I'm not quite sure exactly what this code should be doing when mRowsToPrepend > 0....
Comment 8 User image 2009-04-01 05:13:12 PDT
Comment on attachment 370256 [details] [diff] [review]
diff -w

>@@ -1203,30 +1203,34 @@ nsListBoxBodyFrame::GetNextItemBox(nsIBo
>     PRInt32 i = parentContent->IndexOf(prevContent);
I didn't try the first test case, but the second test case crashes here; is that supposed to happen? Note that this is a debug build, so I'm assuming it's not doing any optimisation tricks that would result in a confusing stack.
Comment 9 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-01 05:57:22 PDT
Yes, the second testcase is expected to crash on that line (as does the first).  parentContent is null, since prevContent is no longer in the document.
Comment 10 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-03 12:49:06 PDT

No test pushed yet, as usual.  :(
Comment 11 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-03 13:12:54 PDT
Comment 12 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-03 13:13:34 PDT
Comment on attachment 370254 [details] [diff] [review]
Proposed fix

Presumably need this on 1.9.0 as well, right?  Not sure which is the right milestone to request approval for there.
Comment 13 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-05 21:08:45 PDT
Created attachment 371189 [details] [diff] [review]
roll-up patch suitable for 1.9.0 branch
Comment 14 User image Samuel Sidler (old account; do not CC) 2009-04-10 16:04:13 PDT
Comment on attachment 371189 [details] [diff] [review]
roll-up patch suitable for 1.9.0 branch

Approved for a=ss
Comment 15 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-17 10:10:15 PDT
Fixed in CVS.
Comment 16 User image Bob Clary [:bc:] 2009-04-18 21:17:12 PDT
v 1.9.0, 1.9.1, 1.9.2 mac
Comment 17 User image Martin Stránský 2009-05-12 02:38:29 PDT
Affects 1.8.1 too.
Comment 18 User image Martin Stránský 2009-05-18 00:37:17 PDT
Created attachment 378005 [details] [diff] [review]
1.8.0 patch

There's a patch for 1.8.0 there, works fine for the second testcase but cycles in an endless loop for the first one.
Comment 19 User image Samuel Sidler (old account; do not CC) 2009-05-29 10:47:57 PDT
Comment on attachment 378005 [details] [diff] [review]
1.8.0 patch

Boris, can you spot check this for 1.8.0/1.8.1?
Comment 20 User image Boris Zbarsky [:bz] (still a bit busy) 2009-05-29 14:07:23 PDT
Comment on attachment 378005 [details] [diff] [review]
1.8.0 patch

Looks ok
Comment 21 User image Daniel Veditz [:dveditz] 2009-06-01 14:12:54 PDT
Comment on attachment 378005 [details] [diff] [review]
1.8.0 patch

Approved for, a=dveditz for release-drivers
Comment 22 User image Daniel Veditz [:dveditz] 2009-06-02 14:08:51 PDT
Fix checked into the 1.8(.1) branch
Checking in layout/xul/base/src/nsListBoxBodyFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsListBoxBodyFrame.cpp,v  <--  nsListBoxBodyFrame.cpp
new revision:; previous revision:
Comment 23 User image Al Billings [:abillings] 2009-06-04 10:01:13 PDT
Verified using Seamonkey nightly (Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv: Gecko/20090604 SeaMonkey/1.1.17pre). Last shipped version crashes on testcase but the new nightly does not.
Comment 24 User image Boris Zbarsky [:bz] (still a bit busy) 2009-06-12 19:10:35 PDT
Checked in the two tests on m-c, 1.9.1, and 1.9.0.
Comment 25 User image joris huizer 2009-06-23 17:05:02 PDT
Crashing still on the first test case (not the second one), using Mozilla/5.0 (X11; U; Linux i686; en-US; rv: Gecko/20090606 SeaMonkey/1.1.17

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