Last Comment Bug 432068 - Crash [@ nsListBoxBodyFrame::GetNextItemBox] with <xul:listbox>, XBL
: Crash [@ nsListBoxBodyFrame::GetNextItemBox] with <xul:listbox>, XBL
Status: VERIFIED FIXED
[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]
:
Mentors:
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+
dveditz: blocking1.8.1.next+
stransky: wanted1.8.1.x+
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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]
no flags Details
Proposed fix (2.12 KB, patch)
2009-03-31 13:00 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review
diff -w (1.44 KB, patch)
2009-03-31 13:03 PDT, Boris Zbarsky [:bz]
roc: review+
neil: review+
roc: superreview+
Details | Diff | Review
roll-up patch suitable for 1.9.0 branch (4.44 KB, patch)
2009-04-05 21:08 PDT, Boris Zbarsky [:bz]
samuel.sidler+old: approval1.9.0.11+
Details | Diff | Review
1.8.0 patch (1.91 KB, patch)
2009-05-18 00:37 PDT, Martin Stránský
bzbarsky: review+
dveditz: approval1.8.1.next+
Details | Diff | Review

Description 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 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 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
HostMachine\HostUser
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:
xul!nsListBoxBodyFrame::GetNextItemBox+0x46
xul!nsListBoxBodyFrame::CreateRows+0x9f
xul!nsListBoxBodyFrame::ReflowFinished+0x19
xul!PresShell::HandlePostedReflowCallbacks+0x3d
xul!PresShell::ProcessReflowCommands+0x179
xul!PresShell::DoFlushPendingNotifications+0x12f
xul!PresShell::ReflowEvent::Run+0x37
xul!nsThread::ProcessNextEvent+0x213
xul!nsBaseAppShell::Run+0x4a
xul!nsAppStartup::Run+0x1e
xul!XRE_main+0xcb9
Unknown
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 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 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 Boris Zbarsky [:bz] 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 Boris Zbarsky [:bz] 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
    nsListBoxBodyFrame::OnContentInserted

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 Boris Zbarsky [:bz] 2009-03-31 13:00:09 PDT
Created attachment 370254 [details] [diff] [review]
Proposed fix
Comment 7 Boris Zbarsky [:bz] 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 neil@parkwaycc.co.uk 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 Boris Zbarsky [:bz] 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 Boris Zbarsky [:bz] 2009-04-03 12:49:06 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/41b09fdb40d8

No test pushed yet, as usual.  :(
Comment 11 Boris Zbarsky [:bz] 2009-04-03 13:12:54 PDT
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dbb9e2e53ea6
Comment 12 Boris Zbarsky [:bz] 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 Boris Zbarsky [:bz] 2009-04-05 21:08:45 PDT
Created attachment 371189 [details] [diff] [review]
roll-up patch suitable for 1.9.0 branch
Comment 14 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 1.9.0.10. a=ss
Comment 15 Boris Zbarsky [:bz] 2009-04-17 10:10:15 PDT
Fixed in CVS.
Comment 16 Bob Clary [:bc:] 2009-04-18 21:17:12 PDT
v 1.9.0, 1.9.1, 1.9.2 mac
Comment 17 Martin Stránský 2009-05-12 02:38:29 PDT
Affects 1.8.1 too.
Comment 18 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 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 Boris Zbarsky [:bz] 2009-05-29 14:07:23 PDT
Comment on attachment 378005 [details] [diff] [review]
1.8.0 patch

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

Approved for 1.8.1.22, a=dveditz for release-drivers
Comment 22 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: 1.50.12.5; previous revision: 1.50.12.4
done
Comment 23 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:1.8.1.22pre) Gecko/20090604 SeaMonkey/1.1.17pre). Last shipped version crashes on testcase but the new nightly does not.
Comment 24 Boris Zbarsky [:bz] 2009-06-12 19:10:35 PDT
Checked in the two tests on m-c, 1.9.1, and 1.9.0.
Comment 25 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:1.8.1.22) Gecko/20090606 SeaMonkey/1.1.17

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