Closed
Bug 449129
Opened 16 years ago
Closed 14 years ago
"ASSERTION: constructing frames in the middle of a paint" with flash slow script dialog [OOPP version]
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(blocking2.0 final+, blocking1.9.2 .13+, status1.9.2 .13-fixed, status1.9.1 unaffected)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
blocking1.9.2 | --- | .13+ |
status1.9.2 | --- | .13-fixed |
status1.9.1 | --- | unaffected |
People
(Reporter: jruderman, Assigned: benjamin)
References
Details
(Keywords: assertion, Whiteboard: [sg:critical?] don't un-hide until bug 591409 is fixed (non-OOPP part))
Attachments
(4 files, 1 obsolete file)
9.85 KB,
text/plain
|
Details | |
5.66 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
6.33 KB,
patch
|
christian
:
approval1.9.2.11-
christian
:
approval1.9.2.13+
|
Details | Diff | Splinter Review |
7.03 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a2pre) Gecko/2008080416 Minefield/3.1a2pre ###!!! ASSERTION: constructing frames in the middle of a paint: 'mPresContext->mLayoutPhaseCount[eLayoutPhase_Paint] == 0', file ../../dist/include/layout/nsPresContext.h, line 998 I don't have a reduced testcase like for bug 434894. Hopefully the stack trace makes it clear what the bug is.
Comment 1•16 years ago
|
||
Um, fun. We're telling flash to pain, and it decides to open a modal dialog while painting, which in turn lets us process other events so we run a JS timeout and who knows what'll happen. I don't know that there's all that much we can do about this. Anyone have thoughts?
Reporter | ||
Comment 2•15 years ago
|
||
Should we treat this is a bug in Flash? Could we just refuse to process some events if there's a paint in progress?
It's bad, probably exploitable. Maybe we could add something to block nsAppShell::ProcessGeckoEvents from handling Gecko events. Maybe it could just be a feature of the script-blocker, since any time we enter the Gecko event loop there is the possibility of running script.
Group: core-security
Updated•15 years ago
|
Whiteboard: [sg:critical?]
Comment 4•15 years ago
|
||
Jesse, is there a test case for this?
Reporter | ||
Comment 5•15 years ago
|
||
No. It might be possible to construct one using the Flash in bug 434894, but if I didn't do it in comment 0 that probably means I thought it would be hard.
Comment 6•15 years ago
|
||
But how did you get the stack trace?
Reporter | ||
Comment 7•15 years ago
|
||
Some kind of fuzz-testing + after-the-fact use of fix-macosx-stack.pl
What should we do with this bug? If we're not going to fix it, let's WONTFIX it and make sure Adobe's in the loop. If we're going to play with event suppression or whatever, let's get that plan in here. (I recommend the former, since we're going to have this much more rational once we get OOP for Flash.)
This sound very timing related when the slow script dialog is encountered. Difficult to time this and to reproduce if we even could. I am open to suggestions.
Reporter | ||
Comment 10•14 years ago
|
||
You can't figure out the bug based on the stack trace? :(
Reporter | ||
Comment 11•14 years ago
|
||
Maybe you could look at bug 434894? That might be the same bug and actually has a testcase.
Comment 12•14 years ago
|
||
Charles, were you able to take a look at bug 434894 to determine if it's the same bug (or related)?
Comment 13•14 years ago
|
||
Sorry I have not had time yet.
Comment 14•14 years ago
|
||
Charles, ping.
Comment 15•14 years ago
|
||
Benjamin, here's another plugin pain re-entrancy bug.
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → benjamin
Assignee | ||
Comment 16•14 years ago
|
||
In the OOPP case, I think we can just kill off the plugin if it starts to spin the event loop during painting. To fix this in-process, we could set a global "don't process events" flag, and simply throw away all Windows events which are dispatched during painting activities. Or we could abort/crash, which might be just as good here.
Comment 17•14 years ago
|
||
I bet this assertion fires due to the same problem that's described in bug 535651.
Comment 18•14 years ago
|
||
With our current tools I would need to know what version of the Player was running at the time the stack track was made and what the base address is. Its the only way I can stitch the symbols back. Michelle any thoughts on this bug?
Reporter | ||
Comment 19•14 years ago
|
||
Bug 434894 seems to not happen during painting so it's harder to blame the plugin there.
Reporter | ||
Comment 20•14 years ago
|
||
I'm not doing this kind of fuzzing any more, so I'm not sure I can reproduce with a known version of Flash :( But it was probably the version of Flash that Apple was shipping with Mac OS X 10.5 when I reported the bug.
Comment 21•14 years ago
|
||
I'll post this to another bug for checkin.
Attachment #438603 -
Flags: review?(jst)
Attachment #438603 -
Flags: feedback?(jruderman)
Attachment #438603 -
Flags: review?(roc)
Attachment #438603 -
Flags: feedback?(Olli.Pettay)
Reporter | ||
Updated•14 years ago
|
Attachment #438603 -
Flags: feedback?(jruderman)
Comment 22•14 years ago
|
||
Comment on attachment 438603 [details] [diff] [review] proposal Based on the stack trace I don't see how this helps with anything. Flash opens some dialog (native?) at evil time and that spins gecko event loop. nsAutoScriptBlocker doesn't prevent that.
Attachment #438603 -
Flags: feedback?(Olli.Pettay) → feedback-
Comment 23•14 years ago
|
||
my hope is that it would block the js from nsGlobalWindow::RunTimeout. It obviously fails if we go around and paint the window anyway. What we really need to do is have a paintblocker that works like the script blocker, but...
Comment 24•14 years ago
|
||
(In reply to comment #23) > my hope is that it would block the js from nsGlobalWindow::RunTimeout. Well, what if there is a synchronous XHR in the stack? nsAutoScriptBlocker wouldn't really help with handling it.
Comment 25•14 years ago
|
||
i filed bug 558938 against flash with what i think is a relatively correct list of things we don't want flash to do.
Assignee | ||
Comment 26•14 years ago
|
||
Per critsmash meeting today, I'm going to implement comment #16: starting with OOPP, we'll abort the plugin process if a plugin tries to spin an event loop during painting. If this goes smoothly, we will also abort the Firefox process if the plugin spins an event loop during painting. This will be a second step because we're concerned to figure out how common this problem is before actually deploying it. We'll get the OOPP crash reports first, which will help identify the frequency of the problem.
The concern here is more than spinning the event loop right? I.e. plugins that call back to the page to get properties which can potentially be implemented using web-page-js-getters. Can we abort there too?
Assignee | ||
Comment 28•14 years ago
|
||
That's a different bug, and can be solved more simply by simply making npruntime calls fail during painting. Once the plugin spins an event loop, however, there's little hope of recovery.
Assignee | ||
Updated•14 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?][ETA 7-May]
Updated•14 years ago
|
Attachment #438603 -
Flags: review?(roc)
Attachment #438603 -
Flags: review?(jst)
Comment 29•14 years ago
|
||
Is this a Mac-only problem (i.e., not Windows or Linux)? Also, could someone please grant me access to bug 535651, if you think that is still related? Thanks.
Comment 30•14 years ago
|
||
bug 535651 is windows but we can't reproduce that one now.
Updated•14 years ago
|
Whiteboard: [sg:critical?][ETA 7-May] → [sg:critical?][ETA 7-May][critsmash:investigating]
Reporter | ||
Updated•14 years ago
|
Whiteboard: [sg:critical?][ETA 7-May][critsmash:investigating] → [sg:critical?][ETA 7-May][critsmash:investigating][plan in comment 26]
Updated•14 years ago
|
blocking2.0: --- → ?
Comment 31•14 years ago
|
||
I can start analyzing this from the Flash Player perspective in a month or two. Sorry for the delay.
Updated•14 years ago
|
status1.9.1:
--- → wanted
status1.9.2:
--- → wanted
Comment 32•14 years ago
|
||
Can we get an update here?
Assignee | ||
Comment 33•14 years ago
|
||
I will get to this bug once there are no more 3.6.4 blockers.
Updated•14 years ago
|
blocking2.0: ? → final+
Comment 34•14 years ago
|
||
Benjamin, can you update the ETA (we're way past May 9th now)?
Comment 35•14 years ago
|
||
Poke, bsmedberg?
Comment 36•14 years ago
|
||
In looking through the Flash Player codebase, I've found at least one place where we are running script during a paint event. We are looking into plugging this hole.
Comment 37•14 years ago
|
||
Could someone clarify why calling JavaScript while processing a paint event is exploitable? Also, does anyone know of a test case that triggers this assert? Thanks so much for your help.
Assignee | ||
Comment 38•14 years ago
|
||
script can do things like change the position of the plugin or cause it to be destroyed. This in turn changes the painting structures which are on the stack for the paint.
Assignee | ||
Comment 39•14 years ago
|
||
Attachment #438603 -
Attachment is obsolete: true
Attachment #453149 -
Flags: review?(jmathies)
![]() |
||
Updated•14 years ago
|
Attachment #453149 -
Flags: review?(jmathies) → review+
Assignee | ||
Comment 40•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/de4b3d642996
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 41•14 years ago
|
||
This is the patch as checked in, I had to fix some includes and build goop.
Attachment #453374 -
Flags: approval1.9.2.7?
Attachment #453374 -
Flags: approval1.9.2.6?
Assignee | ||
Comment 42•14 years ago
|
||
Argh, my carelessness shows through: http://hg.mozilla.org/mozilla-central/rev/e3787b57b3ab This is pretty hard to test, but I'm working on it.
Assignee | ||
Comment 43•14 years ago
|
||
This is the more-scary bit: I'd like to go ahead and land this on trunk for 4.0 so we can get crash data from the betas, but probably not land this on branches for a while because of the scariness.
Attachment #453403 -
Flags: review?(roc)
Attachment #453403 -
Flags: review?(roc) → review+
Comment 44•14 years ago
|
||
I think we'll wait on this for after 3.6.6/3.5.11
Attachment #453374 -
Flags: approval1.9.2.6? → approval1.9.2.6-
Comment 45•14 years ago
|
||
This bug was resolved FIXED in comment 40, but then sprouted a new reviewed patch that you said you wanted in 4.0 in comment 43. Has that been forgotten because this bug is resolved? Should it be moved to a new bug?
Comment 46•14 years ago
|
||
Also, how does the crash data look on trunk? Do we even need in-process changes as this was written against flash and it is oop on trunk and the branches?
Assignee | ||
Comment 47•14 years ago
|
||
The reviewed patch doesn't pass tryserver on WinXP, and I didn't bother to figure out why (something about synchronous WM_GETICON messages in the event loop).
Comment 48•14 years ago
|
||
Is the extra patch _necessary_? Are we stuck with an incomplete fix, or is the extra patch something that can be put into a different bug?
Assignee | ||
Comment 49•14 years ago
|
||
That patch is necessary if we're running any plugins in-process, which we will on Mac 10.5 for its supported life.
Comment 50•14 years ago
|
||
Comment on attachment 453374 [details] [diff] [review] Patch with corrected build-goop, rev. 1.1 We can't take this on the branches until it passes try and the other patch is done. Clearing approval.
Attachment #453374 -
Flags: approval1.9.2.9?
Attachment #453374 -
Flags: approval1.9.2.7-
Assignee | ||
Comment 51•14 years ago
|
||
We should take the OOPP patch as soon as possible. It fixes Flash and Silverlight on Windows, which significantly reduces our attack surface. The other patch is necessary for in-process, but they don't have to land together.
Assignee | ||
Updated•14 years ago
|
Attachment #453374 -
Flags: approval1.9.2.9?
Attachment #453374 -
Flags: approval1.9.2.10?
Comment 52•14 years ago
|
||
With this fix, what will happen when a plug-in calls script during a paint event? Thanks.
Assignee | ||
Comment 53•14 years ago
|
||
It will be terminated and appear to the user as a plugin crash.
Assignee | ||
Comment 54•14 years ago
|
||
Note, however, that this only affects the case where the plugin either shows the slow-script dialog or spins a nested event loop. It doesn't cover the case where it just uses npruntime scripting and returns quickly.
Comment 55•14 years ago
|
||
Created bug 591409 to track the in-process patch from comment 43 so it doesn't get lost due to this bug being RESOLVED FIXED.
Updated•13 years ago
|
blocking1.9.1: --- → needed
blocking1.9.2: --- → needed
Comment 56•13 years ago
|
||
Comment on attachment 453374 [details] [diff] [review] Patch with corrected build-goop, rev. 1.1 Clearing old 1.9.2.9 approval. Unsure about .11 -- does landing this fix give away the problem of bug 591409, which we don't seem to be ready to land? Are we seeing these aborts in FF4 betas at all? Should be a trackable signature right?
Attachment #453374 -
Flags: approval1.9.2.9?
Comment 57•13 years ago
|
||
Comment on attachment 453374 [details] [diff] [review] Patch with corrected build-goop, rev. 1.1 I'm not up for taking this in 1.9.2.11, it'll have to wait for a future update.
Attachment #453374 -
Flags: approval1.9.2.11? → approval1.9.2.11-
Updated•13 years ago
|
Whiteboard: [sg:critical?][ETA 7-May][critsmash:investigating][plan in comment 26] → [sg:critical?][need answer from bsmedberg to 2 questions in comment 56]
Assignee | ||
Comment 58•13 years ago
|
||
Comment on attachment 453374 [details] [diff] [review] Patch with corrected build-goop, rev. 1.1 I don't know the answer to either of these questions. Aborts should show up in the crash stats, but I don't know what signature, and we can't search by the entire stack (RPCChannel::SpinInternalEventLoop would be near the top of the stack). And I certainly can't guess whether we'd be "giving away" anything that we haven't already given away on trunk. I guess the branch commits are watched more closely, though...
Attachment #453374 -
Flags: approval1.9.2.12?
Comment 59•13 years ago
|
||
I'm cobbling together a hacky way to search though a sample of stacks of the top 300 signatures of any given release. I'm looking for RPCChannel::SpinInternalEventLoop in 4.0b6 crashes. Nothing found yet. The closest to anything related to RPCChannel is Signature: StopRequest mozilla::ipc::RPCChannel::DispatchIncall mozilla::ipc::RPCChannel::Incall mozilla::ipc::RPCChannel::OnMaybeDequeueOne like in http://crash-stats.mozilla.com/report/index/84a3ad08-6078-4faa-a6cf-6b3ed2101011 and Signature: js::DeepBail(JSContext*) mozilla::ipc::RPCChannel::DispatchIncall(IPC::Message const mozilla::ipc::RPCChannel::Incall(IPC::Message const mozilla::ipc::RPCChannel::Call(IPC::Message *,IPC::Message *) like in http://crash-stats.mozilla.com/report/index/3b8e972a-b656-49e6-8ec6-7dc132101012 the search across 4.0b6 stacks is still running and I'll run against a sample from 3.6.10 and update if I find anything.
Comment 60•13 years ago
|
||
not much more in the search against 3.6.10 stacks In signature: mozilla::plugins::PPluginInstanceParent::CallPBrowserStreamConstructor(mozilla::plugins::PBrowserStreamParent*, nsCString const&, unsigned int const&, unsigned int const&, mozilla::plugins::PStreamNotifyParent*, nsCString const&, nsCString const&, bool... I found mozilla::ipc::RPCChannel::Call like in the hang http://crash-stats.mozilla.com/report/index/1b5383f7-d49b-419b-9d24-d347a2101013 and http://crash-stats.mozilla.com/report/index/27d520ca-8273-4163-bbfc-c2cc12101012 and # 129 Signature: mozilla::ipc::RPCChannel::EnteredCxxStack() mozilla::ipc::RPCChannel::EnteredCxxStack mozilla::ipc::RPCChannel::CxxStackFrame::CxxStackFrame mozilla::ipc::RPCChannel::Call maybe something in variation of crash [@ rasadhlp.dll@0x341f ] but I haven't found the particular report where I happend to stumble onto 0|5|xul.dll|mozilla::ipc::RPCChannel::WaitForNotify() 0|6|xul.dll|mozilla::ipc::RPCChannel::Call(IPC::Message *,IPC::Message *) 238.._PR_MD_RECV 0|5|xul.dll|mozilla::ipc::RPCChannel::WaitForNotify() 0|6|xul.dll|mozilla::ipc::RPCChannel::Call(IPC::Message *,IPC::Message *) many variations of the stack for this signature... need to change the script to to keep track of the reports that I'm using
Comment 61•13 years ago
|
||
I'm more interested in 4.0 crashes as this fix is there. I'm concerned that we will take this fix and introduce oopsies where previously it continued on (insecurely) but to the end-user appeared to work (not sure if that's even possible though). If there are really no crashes in 4.0 after this landed, does it mean that basically no one is hitting the insecure code to trigger and oopsie, and as such this is only really fuzzer-generated at this point and doesn't happen in the real world (yet)?
Assignee | ||
Comment 62•13 years ago
|
||
I'm really not concerned about introducing a new crash in 3.6.x... the user would *already* be experiencing the flash slow-script dialog, which makes the page already pretty useless and will probably crash very quickly anyway.
Comment 63•13 years ago
|
||
Is it correct that this bug fix is only needed in the case where there is no asynchronous painting? Which Firefox release will have that change?
Assignee | ||
Comment 64•13 years ago
|
||
Asynchronous painting works around the problem, yes. Asynchronous painting is currently being worked on and has landed on Linux for Firefox 4, but we're not sure when mac and windows will be ready.
Comment 65•13 years ago
|
||
Comment on attachment 453374 [details] [diff] [review] Patch with corrected build-goop, rev. 1.1 a=LegNeato for 1.9.2.11. Please land this on mozilla-1.9.2 default.
Attachment #453374 -
Flags: approval1.9.2.12? → approval1.9.2.12+
Updated•13 years ago
|
Whiteboard: [sg:critical?][need answer from bsmedberg to 2 questions in comment 56] → [sg:critical?]
Assignee | ||
Comment 67•13 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/52f20971aae1 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/32c1caae5b5c http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2b9eefffc926
Updated•13 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?] don't un-hide until bug 591409 is fixed (non-OOPP part)
Comment 68•13 years ago
|
||
If bug 591409 covers the in-process version of this bug then 1.9.1 is "unaffected" by the OOPP version, at least in a bookkeeping sense.
blocking1.9.1: needed → ---
Summary: "ASSERTION: constructing frames in the middle of a paint" with flash slow script dialog → "ASSERTION: constructing frames in the middle of a paint" with flash slow script dialog [OOPP version]
Updated•9 years ago
|
Group: core-security
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•