325236 - memory corruption probably caused by chrome alert()

Status: RESOLVED FIXED

(Firefox :: Page Info Window, defect, P1)

Description

[georgi - hopefully not receiving bugspam]

there is strange memory corruption probably caused by alert() involving chrome.

valgrind does not detect illegal writes, but detects several illegal reads.

testcase to follow.

#6  0x080ab66e in ah_crap_handler (signum=11) at nsSigHandlers.cpp:131
#7  0x080abccb in nsProfileLock::FatalSignalHandler (signo=11)
    at nsProfileLock.cpp:210
#8  <signal handler called>
#9  0xb7a61b39 in gdk_x11_drawable_get_xid () from /usr/lib/libgdk-x11-2.0.so.0
#10 0x097e08d0 in ?? ()
#11 0x097d7bd0 in ?? ()
#12 0xbfffdc80 in ?? ()
#13 0xb7a4b800 in gdk_rgb_get_visual () from /usr/lib/libgdk-x11-2.0.so.0
#14 0x097e08d0 in ?? ()
#15 0x097d7bd0 in ?? ()
#16 0xbfffdc80 in ?? ()
#17 0x0828246f in nsDrawingSurfaceGTK::GetXftDraw (this=0xb7aa63f4)
    at /opt/firefox-cvs/mozilla/gfx/src/gtk/nsDrawingSurfaceGTK.cpp:334
#18 0x08278823 in nsFontMetricsXft::PrepareToDraw (this=0x97a8670, 
    aContext=0x97d7bd0, aSurface=0x97e08d0, aDraw=0xbfffdc94, 
    aColor=@0xbfffdc98)
#19 0x08276d36 in nsFontMetricsXft::DrawString (this=0x97a8670, 
    aString=0xbfffdda8, aLength=9, aX=149464408, aY=149464408, aFontID=825, 
    aSpacing=0x8e8a558, aContext=0x97d7bd0, aSurface=0x97e08d0)
    at /opt/firefox-cvs/mozilla/gfx/src/gtk/nsFontMetricsXft.cpp:636
#20 0x082758a4 in nsRenderingContextGTK::DrawString (this=0x8e8a078, 
    aString=0xbfffdda8, aLength=9, aX=5281, aY=4415, aFontID=825, aSpacing=0x0)
    at /opt/firefox-cvs/mozilla/gfx/src/gtk/nsRenderingContextGTK.cpp:1331
#21 0x0834222c in nsBidiPresUtils::RenderText (this=0x9815220, 
    aText=0xbfffe098, aLength=9, aBaseDirection=NSBIDI_LTR, 
    aPresContext=0x96e9ef0, aRenderingContext=@0x97d7bd0, aX=5281, aY=4415, 
    aPosResolve=0x0, aPosResolveCount=0)
    at /opt/firefox-cvs/mozilla/layout/base/nsBidiPresUtils.cpp:1129
#22 0x08602766 in nsTreeBodyFrame::PaintText (this=0x9758b94, aRowIndex=0, 
    aColumn=0x964a330, aTextRect=@0xbfffe1c0, aPresContext=0x96e9ef0, 
    aRenderingContext=@0x97d7bd0, aDirtyRect=@0xbfffe480, aCurrX=@0xbfffe194)
    at /opt/firefox-cvs/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:3080
#23 0x08601750 in nsTreeBodyFrame::PaintCell (this=0x9758b94, aRowIndex=0, 
    aColumn=0x964a330, aCellRect=@0xbfffe290, aPresContext=0x96e9ef0, 
    aRenderingContext=@0x97d7bd0, aDirtyRect=@0xbfffe480, aCurrX=@0x8e8a558, 
    aPt={x = 0, y = -536862720})
    at /opt/firefox-cvs/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp

#24 0x08600f4c in nsTreeBodyFrame::PaintRow (this=0x9758b94, aRowIndex=0, 
    aRowRect=@0xbfffe350, aPresContext=0x96e9ef0, 
    aRenderingContext=@0x97d7bd0, aDirtyRect=@0xbfffe480, aPt=
      {x = 355, y = 4205})
    at /opt/firefox-cvs/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:2461
#25 0x0860081f in nsTreeBodyFrame::PaintTreeBody (this=0x9758b94, 
    aRenderingContext=@0x97d7bd0, aDirtyRect=@0xbfffe480, aPt=
      {x = 355, y = 4205})
    at /opt/firefox-cvs/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:2291
#26 0x08600453 in PaintTreeBody (aFrame=0x9758b94, aCtx=0x97d7bd0, 
    aDirtyRect=@0xbfffe480)
    at /opt/firefox-cvs/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:2212
#27 0x0836c72e in nsDisplayGeneric::Paint (this=0x97f63c8, 
    aBuilder=0xbfffe550, aCtx=0x97d7bd0, aDirtyRect=@0xbfffe480)
    at nsDisplayList.h:691
#28 0x086ee29b in nsDisplayList::Paint (this=0x97f6408, aBuilder=0xbfffe550, 

(gdb) frame 9
#9  0xb7a61b39 in gdk_x11_drawable_get_xid () from /usr/lib/libgdk-x11-2.0.so.0
(gdb) x/i $eip
0xb7a61b39 <gdk_x11_drawable_get_xid+41>:       mov    (%esi),%edx
(gdb) p/x $esi
$1 = 0x1
(gdb) p/x $edi
$2 = 0x1

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: crash when selecting "page info" "forms"

Comment 2

[georgi - hopefully not receiving bugspam]

a problem may be if this can be triggered outside of chrome

Comment 3

[georgi - hopefully not receiving bugspam]

on second thought the script in the testcase is not the corporate type, so it may be also involved.

Comment 4

[georgi - hopefully not receiving bugspam]

in some cases "info stack" in gdb terminates in gdk*

Comment 5

[georgi - hopefully not receiving bugspam]

bz: this is related to bug 324630

do you have an idea about the codepath - there are signs of memory corruption and  i am trying to trigger it without chrome?

Comment 6

[Boris Zbarsky [:bzbarsky]]

In a debug build I see:

###!!! ASSERTION: recursive painting not permitted: '!IsPainting()', file ../../../mozilla/view/src/nsViewManager.cpp, line 547

as the first assertion I hit.  That assert comes from more or less the following stack:

#3  0xb570293c in nsViewManager::Refresh (this=0x8898058, aView=0x8e0c5c0, 
    aContext=0x8d294e0, aRegion=0x8d230e8, aUpdateFlags=1)
    at ../../../mozilla/view/src/nsViewManager.cpp:547
....
#8  0xb68b3ff3 in expose_event_cb (widget=0x8abc8d8, event=0xbfffbf8c)
    at ../../../../mozilla/widget/src/gtk2/nsWindow.cpp:3679
....
#18 0x001e08d7 in gdk_window_process_all_updates () from /usr/lib/libgdk-x11-2.0.so.0
#19 0x00c94b5a in g_child_watch_add () from /usr/lib/libglib-2.0.so.0
....
#23 0xb68bc02c in nsAppShell::DispatchNativeEvent (this=0x8e79a58, aRealEvent=0, 
    aEvent=0x0) at ../../../../mozilla/widget/src/gtk2/nsAppShell.cpp:274
#24 0xb5b963d5 in nsXULWindow::ShowModal (this=0x8cb7510)
    at ../../../../mozilla/xpfe/appshell/src/nsXULWindow.cpp:403
....
#31 0xb5725c53 in nsGlobalWindow::Alert (this=0x87fbdd0, aString=@0x8e761a0)
    at ../../../../mozilla/dom/src/base/nsGlobalWindow.cpp:3293
...
Call into pageinfo.js through getCellText, which tries to convert a DOM node (?) to text and gets screwed by the XPCNativeWrapper toString bug.
...
#47 0xb579a82d in nsTreeBodyFrame::PaintText (this=0x8e1e174, aRowIndex=0, 
    aColumn=0x8e92fb8, aTextRect=@0xbfffdfa0, aPresContext=0x83e8798, 
    aRenderingContext=@0x8e338e8, aDirtyRect=@0xbfffe2f0, aCurrX=@0xbfffe01c)
    at ../../../../../../../mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:2876

If I continue, I get that assert a couple more times, then crash when I get back to PaintText().  At the crash point, we have a dead drawing surface:

#1  0xb5b39747 in nsDrawingSurfaceGTK::GetXftDraw (this=0x8e7e238)
    at ../../../../mozilla/gfx/src/gtk/nsDrawingSurfaceGTK.cpp:334
334         mXftDraw = XftDrawCreate(GDK_DISPLAY(), GDK_WINDOW_XWINDOW(mPixmap),
(gdb) p *this
$3 = {<nsIDrawingSurface> = {<nsISupports> = {
      _vptr.nsISupports = 0x8f71150}, <No data fields>}, mRefCnt = {mValue = 150566856}, 
  _mOwningThread = {mThread = 0x0}, mPixmap = 0x1, mGC = 0x804a548, mDepth = 0, 
  mPixFormat = {mRedZeroMask = 0, mGreenZeroMask = 3047470580, mBlueZeroMask = 40, 
    mAlphaZeroMask = 48, mRedMask = 148018696, mGreenMask = 150006528, mBlueMask = 0, 
    mAlphaMask = 1, mRedCount = 72 'H', mGreenCount = 165 '&#65533;', mBlueCount = 4 '\004', 
    mAlphaCount = 8 '\b', mRedShift = 1 '\001', mGreenShift = 0 '\0', 
    mBlueShift = 0 '\0', mAlphaShift = 0 '\0'}, mWidth = 149925312, 
  mHeight = 3047561432, mFlags = 0, mIsOffscreen = 137821264, mImage = 0x58, 
  mLockX = 32, mLockY = 148419328, mLockWidth = 779249011, mLockHeight = 1769235297, 
  mLockFlags = 1667196278, mLocked = 1919904879, mXftDraw = 0x0, mLastXftClip = {
    mRawPtr = 0x78}}

I bet we destroyed it at some point and never dropped our pointer to it....  :(

I really wonder why the GTK2/XFT Seamonkey build I have is not being hit by this (doesn't even alert).  In any case, the XPCNativeWrapper toString thing might (ought to?) fix this...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Also... can unprivileged code implement tree views?  If so, we have a major problem, since we're trusting the tree views to not screw us over too badly by calling them inside Paint()...

Comment 8

[:Gavin Sharp [email: [e-mail]]]

So the treeview is being poisoned at:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/pageInfo.js&rev=1.29&mark=654#613
where we end up calling the bogus getter on .type. The reason this isn't happening in SeaMonkey is that it uses the implicit deep wrappers (fixed in bug 295582), while Firefox's page info uses an explicit shallow wrapper on the form ( http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/pageInfo.js&rev=1.29&mark=627#613 ).

I guess the fix for 295582 could just be ported over (it might even apply directly).

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: use implicit deep wrappers

Comment 10

[Boris Zbarsky [:bzbarsky]]

So I filed bug 326501 on the fact that content _can_ implement tree views.  so the answer to "i am trying to trigger it without chrome?" from comment 5 is "yeah, you can do that".

Comment 11

[georgi - hopefully not receiving bugspam]

(In reply to comment #6)
> In a debug build I see:
> 
> ###!!! ASSERTION: recursive painting not permitted: '!IsPainting()', file
> ../../../mozilla/view/src/nsViewManager.cpp, line 547
> 

are there known dangerous assertions that are a sign of exploitability?

Comment 12

[[e-mail]]

Comment on attachment 211228 [details] [diff] [review]
use implicit deep wrappers

Kill the XPCNativeWrapper comment too?

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #12)
> (From update of attachment 211228 [details] [diff] [review] [edit])
> Kill the XPCNativeWrapper comment too?

You're right, made this change locally.

Comment 14

[Boris Zbarsky [:bzbarsky]]

There probably are some asserts that indicate that something has gone awry badly enough that we're guaranteed to end up accessing deleted memory...  I can't think of any off the top of my head, though.

Comment 15

[georgi - hopefully not receiving bugspam]

wouldn't it be better asserts that show screwed state to be NS_ENSURE_TRUE?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Real asserts, imo, should happen when:

1)  The situation should never arise.
2)  There is no sane way to recover if it does.

In other words, if you're hitting such an assert, the NS_ENSURE_TRUE won't help.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Checked in on the trunk.
mozilla/browser/base/content/pageInfo.js; new revision: 1.30;

Comment 18

[georgi - hopefully not receiving bugspam]

this fix seems to fix Bug 324630

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Fixed on the 1.8 branch as well (b=mconnor over irc).
mozilla/browser/base/content/pageInfo.js; new revision: 1.25.2.4;

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 211228 [details] [diff] [review]
use implicit deep wrappers

approved for 1.8.0 branch, a=dveditz for drivers

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Checked in on the 1.8.0 branch.
mozilla/browser/base/content/pageInfo.js; new revision: 1.25.2.2.2.1;

Comment 22

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060224 Firefox/1.5.0.1, no crash or other bad stuff with testcase.

Comment 23

[Tanner M. Young [:tmyoung]]

Resetting QA contact to default.