Bugzilla@Mozilla – Bug 380359
Crash [@ nsEventStateManager::GetContentState] [@ nsNativeTheme::CheckBooleanAttr] with -moz-appearance and position: fixed
Last modified: 2009-07-01 13:17:12 PDT
Summon comment box
Created attachment 264443 [details] testcase (crashes Firefox when loaded) Loading the testcase in Firefox trunk on Mac (opt or debug) makes it crash.
Created attachment 264467 [details] Frame dump + stack The native theme code makes bad assumptions about the frame tree...
Created attachment 264528 [details] [review] Patch rev. 1 Add null-checks.
How can a null frame, or a frame with null content, get passed in to native theme APIs?
Comment on attachment 264528 [details] [review] Patch rev. 1 clearing request pending comments
*** Bug 429311 has been marked as a duplicate of this bug. ***
(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.
Any response to roc's question?
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?
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.
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.
We'll want the crashtest checked in of course...
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?
Fixed on trunk and 1.9.1.
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
If this is fixed by a simple null-check is it really sg:critical, or only sg:dos?
Comment on attachment 372441 [details] [review] patch Approved for 1.9.0.10, a=dveditz for release-drivers
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
checked in on 1.9.0.10
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.
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
(In reply to comment #20) > The testcase still crashes on Linux Forgot to mention, it's 1.9.0.11.
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)
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.
Does this crash on 1.9.1 on Linux too?
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
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.
As per comments 20-26, marking this as Linux only (has anyone tested it on w32?). Vlad: do you still think this blocks?
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)
My temptation is to unblock on this, but if all it needs is a nullcheck, can we get a patch whipped together?
Created attachment 380868 [details] [review] linux 1.9.0 patch This is on the try server now; linux 1.9.0 patch.
Created attachment 380869 [details] [review] linux 1.9.1 patch and the same patch ported to 1.9.1
Created attachment 380871 [details] [review] linux trunk patch and the same patch for the trunk, all slightly different!
All the patches had a compilation bug. That's what I get for not using emacs.
+ 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?
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.
1.9.0 builds -- can someone with linux test? https://build.mozilla.org/tryserver-builds/vladimir@mozilla.com-vlad190/
Vlad, I tested your build on Ubuntu 8.10. It doesn't crash anymore. Firefox 3.0.10 on the same machine does.
Can you attach the fixed patch?
Created attachment 380989 [details] [review] updated patch, 1.9.0 Updated patch, the 1.9.0 flavour; the others are similar.
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 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
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
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.
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
Crashtest added: http://hg.mozilla.org/mozilla-central/rev/bf3a4f5dd798
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.