Last Comment Bug 380359 - Crash [@ nsEventStateManager::GetContentState] [@ nsNativeTheme::CheckBooleanAttr] with -moz-appearance and position: fixed
: Crash [@ nsEventStateManager::GetContentState] [@ nsNativeTheme::CheckBoolean...
Status: VERIFIED FIXED
: [sg:critical?]
: crash, regression, testcase, verified1.9.0.11, verified1.9.0.12, verified1.9.1
Product: Core
Classification: Components
Component: Widget
: Trunk
: All All
: P2 critical (vote)
: mozilla1.9.2a1
Assigned To: Vladimir Vukicevic (:vlad)
: general
:
:
: 306939 312455 348996
  Show dependency treegraph
 
Reported: 2007-05-10 21:33 PDT by Jesse Ruderman
Modified: 2009-07-01 13:17 PDT (History)
16 users (show)
vladimir: blocking1.9.1+
dveditz: blocking1.9.0.11+
dveditz: blocking1.9.0.12+
samuel.sidler+old: wanted1.9.0.x+
dveditz: wanted1.8.1.x-
jruderman: in‑testsuite+
See Also:
Crash Signature:
[@ nsEventStateManager::GetContentState] [@ nsNativeTheme::CheckBooleanAttr]


Attachments
testcase (crashes Firefox when loaded) (214 bytes, application/xhtml+xml)
2007-05-10 21:33 PDT, Jesse Ruderman
no flags Details
Frame dump + stack (10.62 KB, text/html)
2007-05-11 07:45 PDT, Mats Palmgren [:mats]
no flags Details
Patch rev. 1 (37.69 KB, patch)
2007-05-11 14:43 PDT, Mats Palmgren [:mats]
no flags Details | Diff | Splinter Review
patch (671 bytes, patch)
2009-04-13 12:01 PDT, Vladimir Vukicevic (:vlad)
roc: review+
dveditz: approval1.9.0.11+
Details | Diff | Splinter Review
linux 1.9.0 patch (4.00 KB, patch)
2009-06-01 11:30 PDT, Vladimir Vukicevic (:vlad)
no flags Details | Diff | Splinter Review
linux 1.9.1 patch (3.83 KB, patch)
2009-06-01 11:30 PDT, Vladimir Vukicevic (:vlad)
no flags Details | Diff | Splinter Review
linux trunk patch (3.80 KB, patch)
2009-06-01 11:33 PDT, Vladimir Vukicevic (:vlad)
no flags Details | Diff | Splinter Review
updated patch, 1.9.0 (3.99 KB, patch)
2009-06-01 20:03 PDT, Vladimir Vukicevic (:vlad)
roc: review+
samuel.sidler+old: approval1.9.0.12+
Details | Diff | Splinter Review

Summon comment box

Description Jesse Ruderman 2007-05-10 21:33:41 PDT
Created attachment 264443 [details]
testcase (crashes Firefox when loaded)

Loading the testcase in Firefox trunk on Mac (opt or debug) makes it crash.
Comment 1 Mats Palmgren [:mats] 2007-05-11 07:45:12 PDT
Created attachment 264467 [details]
Frame dump + stack

The native theme code makes bad assumptions about the frame tree...
Comment 2 Mats Palmgren [:mats] 2007-05-11 14:43:03 PDT
Created attachment 264528 [details] [review]
Patch rev. 1

Add null-checks.
Comment 3 Robert O'Callahan (:roc) (Mozilla Corporation) 2007-05-15 03:08:52 PDT
How can a null frame, or a frame with null content, get passed in to native theme APIs?
Comment 4 Robert O'Callahan (:roc) (Mozilla Corporation) 2007-09-17 16:11:39 PDT
Comment on attachment 264528 [details] [review]
Patch rev. 1

clearing request pending comments
Comment 5 Ryan Flint [:rflint] (ping via IRC for reviews) 2008-04-16 13:55:54 PDT
*** Bug 429311 has been marked as a duplicate of this bug. ***
Comment 6 Gary Kwong [:nth10sd] 2009-03-29 03:28:34 PDT
(I tested this on latest-trunk mozilla-central nightly build on WinXP SP3)

Turning security-sensitive and nominating blocking1.9.1? just-in-case, due to !exploitable PROBABLY_EXPLOITABLE result.


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:1019945f mov eax,dword ptr [esi]

Basic Block:
    1019945f mov eax,dword ptr [esi]
       Tainted Input Operands: esi
    10199461 mov edx,dword ptr [eax+3ch]
       Tainted Input Operands: eax
    10199464 push 80h
    10199469 mov ecx,esi
       Tainted Input Operands: esi
    1019946b call edx
       Tainted Input Operands: ecx, edx

Exception Hash (Major/Minor): 0x6c0b5600.0xb3e226e

Stack Trace:
xul!nsNativeTheme::CheckBooleanAttr+0xf
xul!nsNativeTheme::GetCheckedOrSelected+0x33c0dc
xul!nsNativeThemeWin::GetThemePartAndState+0xf0
xul!nsNativeThemeWin::GetMinimumWidgetSize+0x23cf14
xul!nsCSSOffsetState::InitOffsets+0x184
xul!nsCOMPtr_base::assign_from_qi+0x19
xul!nsAbsoluteContainingBlock::Reflow+0x132
xul!ViewportFrame::Reflow+0x1a0
xul!PresShell::DoReflow+0x249
xul!PresShell::ProcessReflowCommands+0x11e
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: 0x1019945f

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!nsNativeTheme::CheckBooleanAttr+0xf (Hash=0x6c0b5600.0xb3e226e)

The data from the faulting address is later used as the target for a branch.
Comment 7 Vladimir Vukicevic (:vlad) 2009-03-30 14:29:15 PDT
Any response to roc's question?
Comment 8 Vladimir Vukicevic (:vlad) 2009-04-07 11:09:52 PDT
So the aFrame passed in to nsNativeTheme::GetContentState has a mContent, but there's this chunk of code:

77	  PRBool isXULCheckboxRadio = 
78	    (aWidgetType == NS_THEME_CHECKBOX ||
79	     aWidgetType == NS_THEME_RADIO) &&
80	    aFrame->GetContent()->IsNodeOfType(nsINode::eXUL);
81	  if (isXULCheckboxRadio)
82	    aFrame = aFrame->GetParent();

isXULCheckboxRadio is TRUE, so we grab the frame's parent.  The frame's parent doesn't have mContent, and has a mRect of 0,0,65820,59880 (which I think is 2194x1996 pixels?)  No idea what type it is.

We grab this parent frame for checkbox/radio in a few different places.

roc, any ideas as to what we should do here?
Comment 9 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-04-07 13:01:13 PDT
oof, it's the viewport. that makes sense I guess.

Where we do
  if (isXULCheckboxRadio)
    aFrame = aFrame->GetParent();
we should check that the new aFrame's content is non-null and bail if it is.
Comment 10 Vladimir Vukicevic (:vlad) 2009-04-13 12:01:22 PDT
Created attachment 372441 [details] [review]
patch

This fixes the testcase for me; just protects the places where we grab a parent and then grab its content.
Comment 11 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-04-14 03:10:59 PDT
We'll want the crashtest checked in of course...
Comment 12 Vladimir Vukicevic (:vlad) 2009-04-14 10:15:21 PDT
Hmm.. I'll hold off on checking in the crashtest actually, since this bug is old enough that it might be in 3.0.x.  Sam, do you know if that's the case?
Comment 13 Vladimir Vukicevic (:vlad) 2009-04-14 10:26:11 PDT
Fixed on trunk and 1.9.1.
Comment 14 Samuel Sidler (old account; do not CC) 2009-04-14 10:51:11 PDT
This, indeed, crashes 1.9.0. I tested with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.10pre) Gecko/2009041404 GranParadiso/3.0.10pre
Comment 15 Daniel Veditz 2009-04-15 15:29:44 PDT
If this is fixed by a simple null-check is it really sg:critical, or only sg:dos?
Comment 16 Daniel Veditz 2009-04-17 11:15:57 PDT
Comment on attachment 372441 [details] [review]
patch

Approved for 1.9.0.10, a=dveditz for release-drivers
Comment 17 Henrik Skupin (:whimboo) 2009-04-22 16:16:36 PDT
Verfied fixed on trunk and 1.9.1 with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090421 Minefield/3.6a1pre ID:20090421032809

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090421 Shiretoko/3.5b4pre ID:20090421030848
Comment 18 Vladimir Vukicevic (:vlad) 2009-04-22 21:40:18 PDT
checked in on 1.9.0.10
Comment 19 Al Billings [:abillings] 2009-05-11 15:38:20 PDT
Verified for 1.9.0.11 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.11pre) Gecko/2009051111 GranParadiso/3.0.11pre. No crash with testcase.
Comment 20 Martin Stránský 2009-05-27 03:07:37 PDT
The testcase still crashes on Linux, there's a missing null pointer check at nsNativeThemeGTK::GetGtkWidgetAndState():

#0  0x012a244e in nsNativeThemeGTK::GetGtkWidgetAndState (this=0xb5619000, aWidgetType=3 '\003', aFrame=0xaf5284ac, aGtkWidgetType=@0xbfced668,	aState=0xbfced66c, aWidgetFlags=0xbfced664) at nsNativeThemeGTK.cpp:247

246
247  if (aFrame && aFrame->GetContent()->IsNodeOfType(nsINode::eXUL)) { 
248    // For these widget types, some element (either a child or parent)
249    // actually has element focus, so we check the focused attribute
250    // to see whether to draw in the focused state.
251    if (aWidgetType == NS_THEME_TEXTFIELD || 

(gdb) p	aFrame->mContent
$5 = (class nsIContent *) 0x0
Comment 21 Martin Stránský 2009-05-27 03:39:15 PDT
(In reply to comment #20)
> The testcase still crashes on Linux

Forgot to mention, it's 1.9.0.11.
Comment 22 Daniel Veditz 2009-05-29 10:40:20 PDT
qawanted: Al, please check out Martin's claim in comment 21. If this is not fixed completely we need to pull it out of the advisories and finish in 1.9.0.12

Does not affect the 1.8 branch (tested on Mac 1.8.1.22pre)
Comment 23 Al Billings [:abillings] 2009-05-29 13:42:12 PDT
Ack. It does crash on Linux with 1.9.0.11. 

This was listed as a Mac crash in comments so I had no reason to specifically look on Linux before now once it was fixed on Mac. Weird.
Comment 24 Samuel Sidler (old account; do not CC) 2009-05-29 14:17:27 PDT
Does this crash on 1.9.1 on Linux too?
Comment 25 Al Billings [:abillings] 2009-05-29 14:28:27 PDT
Yes, it crashes last night's 1.9.1.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1pre) Gecko/20090529 Shiretoko/3.5pre
Comment 26 Samuel Sidler (old account; do not CC) 2009-05-29 14:42:04 PDT
I'm going to assume trunk is still crashing too then and reopen this. "yay"

We need to be sure this bug isn't listed in our advisories as well.
Comment 27 Mike Beltzner [:beltzner] 2009-05-29 16:09:40 PDT
As per comments 20-26, marking this as Linux only (has anyone tested it on w32?). Vlad: do you still think this blocks?
Comment 28 Martijn Wargers [:mw22] (QA - IRC nick: mw22) 2009-05-29 16:21:24 PDT
This doesn't crash for me on windowsxp, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.12pre) Gecko/2009052005 GranParadiso/3.0.12pre (.NET CLR 3.5.30729)
Comment 29 Mike Beltzner [:beltzner] 2009-05-30 05:20:23 PDT
My temptation is to unblock on this, but if all it needs is a nullcheck, can we get a patch whipped together?
Comment 30 Vladimir Vukicevic (:vlad) 2009-06-01 11:30:06 PDT
Created attachment 380868 [details] [review]
linux 1.9.0 patch

This is on the try server now; linux 1.9.0 patch.
Comment 31 Vladimir Vukicevic (:vlad) 2009-06-01 11:30:51 PDT
Created attachment 380869 [details] [review]
linux 1.9.1 patch

and the same patch ported to 1.9.1
Comment 32 Vladimir Vukicevic (:vlad) 2009-06-01 11:33:36 PDT
Created attachment 380871 [details] [review]
linux trunk patch

and the same patch for the trunk, all slightly different!
Comment 33 Vladimir Vukicevic (:vlad) 2009-06-01 13:21:12 PDT
All the patches had a compilation bug.  That's what I get for not using emacs.
Comment 34 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-06-01 13:52:39 PDT
+  nsIContent *content = aFrame ? nsnull : aFrame->GetContent();

So if aFrame is non-null, we set content to null, otherwise aFrame is null and we dereference it?
Comment 35 Vladimir Vukicevic (:vlad) 2009-06-01 16:10:51 PDT
There's a tryserver build going of 190 with the fixed patch; I don't have linux easily accessible here, I'll post a link here when it's done.   Then I'll kick off 191 and 192 builds, given that the patches are slightly different for all three.
Comment 36 Vladimir Vukicevic (:vlad) 2009-06-01 18:31:13 PDT
1.9.0 builds -- can someone with linux test?  https://build.mozilla.org/tryserver-builds/vladimir@mozilla.com-vlad190/
Comment 37 Al Billings [:abillings] 2009-06-01 18:52:08 PDT
Vlad, I tested your build on Ubuntu 8.10. It doesn't crash anymore. Firefox 3.0.10 on the same machine does.
Comment 38 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-06-01 20:00:26 PDT
Can you attach the fixed patch?
Comment 39 Vladimir Vukicevic (:vlad) 2009-06-01 20:03:31 PDT
Created attachment 380989 [details] [review]
updated patch, 1.9.0

Updated patch, the 1.9.0 flavour; the others are similar.
Comment 40 Vladimir Vukicevic (:vlad) 2009-06-01 21:27:01 PDT
Checking in nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.161; previous revision: 1.160
done

http://hg.mozilla.org/mozilla-central/rev/78c6f6a6ecb5
http://hg.mozilla.org/mozilla-central/rev/2b3ff1a9ad49


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/60e46f62c924

marking fixed 1.9.0.12, not sure if 11 or 12 is right.
Comment 41 Samuel Sidler (old account; do not CC) 2009-06-01 21:36:14 PDT
Comment on attachment 380989 [details] [review]
updated patch, 1.9.0

/me subtly notes that patches on 1.9.0 need explicit approval before landing and retroactively approves.

Approved for 1.9.0.12. a=ss
Comment 42 Daniel Veditz 2009-06-02 10:11:32 PDT
Fix checked into 1.9.0.11 relbranch

Checking in nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.160.14.1; previous revision: 1.160
Comment 43 Al Billings [:abillings] 2009-06-03 14:07:19 PDT
Verified for 1.9.0.11 with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.11) Gecko/2009060214 Firefox/3.0.11.
Comment 44 Henrik Skupin (:whimboo) 2009-06-03 14:43:23 PDT
Sorry that I missed to verify the fix on Linux. But now I can also verify that it is fixed on trunk and 1.9.1 with

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090602 Minefield/3.6a1pre ID:20090602031649

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1pre) Gecko/20090603 Shiretoko/3.5pre ID:20090603031326
Comment 45 Jesse Ruderman 2009-06-13 13:02:42 PDT
Crashtest added:

http://hg.mozilla.org/mozilla-central/rev/bf3a4f5dd798
Comment 46 Al Billings [:abillings] 2009-07-01 13:17:12 PDT
Verified again for 1.9.0.12 with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.12pre) Gecko/2009070104 GranParadiso/3.0.12pre.

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