Closed Bug 856106 Opened 11 years ago Closed 10 years ago

crash in mozilla::FrameLayerBuilder::RemoveFrameFromLayerManager

Categories

(Core :: Layout, defect)

defect
Not set
critical

Tracking

()

RESOLVED INCOMPLETE
Tracking Status
firefox20 --- wontfix
firefox21 - wontfix
firefox22 - wontfix
firefox23 - wontfix
firefox24 - wontfix
firefox25 --- wontfix
firefox-esr17 - wontfix
b2g18 --- wontfix

People

(Reporter: MatsPalmgren_bugz, Assigned: mattwoodrow)

Details

(Keywords: crash, sec-high)

Crash Data

This bug was filed from the Socorro interface and is 
report bp-4e5c0cbb-3bf5-4243-9283-e9daf2130329 .
============================================================= 

Crash occurs on all platforms and in all versions from 18.0 to 22.0a1,
1795 reported incidents in the last 4 weeks.
The "Crash Address" is all over the place so it might be a security problem.

mozilla::FrameLayerBuilder::RemoveFrameFromLayerManager	layout/base/FrameLayerBuilder.cpp:897
PresShell::NotifyDestroyingFrame	layout/base/nsPresShell.cpp:2039
nsFrame::DestroyFrom	layout/generic/nsFrame.cpp:652
nsBlockFrame::DestroyFrom	layout/generic/nsBlockFrame.cpp:302
nsBlockFrame::DestroyFrom	layout/generic/nsBlockFrame.cpp:281
nsBlockFrame::DestroyFrom	layout/generic/nsBlockFrame.cpp:281
nsContainerFrame::DestroyFrom	layout/generic/nsContainerFrame.cpp:228
nsHTMLScrollFrame::DestroyFrom	layout/generic/nsGfxScrollFrame.cpp:96
nsBlockFrame::DestroyFrom	layout/generic/nsBlockFrame.cpp:281
nsBlockFrame::DestroyFrom	layout/generic/nsBlockFrame.cpp:302
nsBlockFrame::DestroyFrom	layout/generic/nsBlockFrame.cpp:281
nsBlockFrame::DestroyFrom	layout/generic/nsBlockFrame.cpp:278
nsBlockFrame::DestroyFrom	layout/generic/nsBlockFrame.cpp:281
nsBlockFrame::DestroyFrom	layout/generic/nsBlockFrame.cpp:281
nsBlockFrame::DestroyFrom	layout/generic/nsBlockFrame.cpp:302
nsContainerFrame::DestroyFrom	layout/generic/nsContainerFrame.cpp:228
nsCanvasFrame::DestroyFrom	layout/generic/nsCanvasFrame.cpp:56
nsContainerFrame::DestroyFrom	layout/generic/nsContainerFrame.cpp:228
nsHTMLScrollFrame::DestroyFrom	layout/generic/nsGfxScrollFrame.cpp:96
nsContainerFrame::DestroyFrom	layout/generic/nsContainerFrame.cpp:228
nsFrameManager::Destroy	layout/base/nsFrameManager.cpp:218
PresShell::Destroy	layout/base/nsPresShell.cpp:1081
DocumentViewerImpl::DestroyPresShell	layout/base/nsDocumentViewer.cpp:4375
DocumentViewerImpl::DestroyPresShell	layout/base/nsDocumentViewer.cpp:4378
DocumentViewerImpl::Destroy	layout/base/nsDocumentViewer.cpp:1664
Assignee: nobody → matt.woodrow
Keywords: needURLs
"EXCEPTION_ACCESS_VIOLATION_WRITE" is even more evidence of exploitability.
Anyone: steps to reproduce outside of URLs in comment 2? Not much to go on.
I'll track this because of security level, and having urls to at least try reproducing with but since it's been present as far back as 18.0 we'll un-track if this doesn't get forward movement in the next cycle.
(In reply to Matt Wobensmith from comment #3)
> Anyone: steps to reproduce outside of URLs in comment 2? Not much to go on.

I think that's what we're hoping you can help out with given URLs, but they're pretty typical.

mattwoodrow - any tips for paths to reproduce?
Flags: needinfo?(matt.woodrow)
No, I've got no leads at all here.

I've tried to reproduce with the given URLs, and haven't been able to reproduce.
Flags: needinfo?(matt.woodrow)
We've got no leads so there's little reason to track this longstanding issue.
(In reply to Alex Keybl [:akeybl] from comment #7)
> We've got no leads so there's little reason to track this longstanding issue.

Right. We probably should tracking- on b2g18 too, right?
Actually, I'm pulling the tracking-b2g18 flag to get this off our radar.
tracking-b2g18: ? → ---
At this point, it's a zombie bug with no reproducible steps that continues to haunt our queries, but without anything actionable.  Closing for now.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → INCOMPLETE
The current crash rate is about the same as when I filed this bug so the
bug is still there and we need to fix it.

I don't think we should close bugs and give up on them just because
there's no activity or plan yet for how to address them.

There's lots of things we can do here; we could add assertions, logging,
poisoning, crash annotations etc in code areas of interest.  We could
also go back and review the code again in FrameLayerBuilder /
DisplayItemData that deals with frame pointers and see if we can make it
more robust.  (I think the crash is caused by some ownership problem in
that code.)

We could also try harder to find steps to reproduce the crash; with an
average of 400+ crashes per week there must be more interesting URLs
than the ones listed so far (in comment 2).  We could target those URLs
with fuzzing -- Bob Clary has a tool that does that, iirc.
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
This was closed because it is an open sec-critical that shows up every time we look at sec-criticals (several meetings and independent things a week) that had no actionable work since no one knows what to do (we thought on reading all the comments). Critsmash triage decided to try to move it off since it just sits there. :-)

If there are things to do, since this is an open sec-crit, is Matt going to do these things soon? Given the high security rating, this should be important.
Marking this as tracking - for 24 since we seem to be doing nothing with this issue.
I spelunked in crashstats for a bit.

So far I see only two crashes in 24.0a2 (aside: noticed both had testpilot installed - other pre-24 crashes do not though).

There are no consistent interesting URLs unfortunately.
(In reply to Mats Palmgren (on vacation) from comment #11)

...

> 
> There's lots of things we can do here; we could add assertions, logging,
> poisoning, crash annotations etc in code areas of interest.  We could
> also go back and review the code again in FrameLayerBuilder /
> DisplayItemData that deals with frame pointers and see if we can make it
> more robust.  (I think the crash is caused by some ownership problem in
> that code.)

We looked at this bug again today and considered lowering it to sec-high due to lack of testcase/in-hand-exploitability but it worries us. Regardless we definitely think the ideas you suggest are worthwhile. Mats do you want to drive doing that? Perhaps on separate bugs (or are they filed already)?

Would bug 860254 help this case? (Still looking for a taker on that bug)
Keywords: sec-criticalsec-high
Needinfo'ing to get answers to questions in comment 15.
Flags: needinfo?(matspal)
> Mats do you want to drive doing that?

Sorry, I'll focus on implementing overflow:fragments for the near future (rest of 2013
at least), so I don't think I'll have large enough chunks of free time to drive this.
I'll be happy to help out with reviews or advice if anyone else wants to take it.

> Would bug 860254 help this case?

Sure.  If it's a use-after-free then we'll see that in the crash addresses.
If it's not, then that's also useful information.

A smaller step than fixing bug 860254 might be to poison LayerManagerData.
Flags: needinfo?(matspal)
Hi Matt, any update? (Do you think the poison mitigation idea from Mats should be next?)
Flags: needinfo?(matt.woodrow)
Closing as incomplete because it isn't actionable at this point.
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → INCOMPLETE
I don't see any crashes with RemoveFrameFromLayerManager in the top frame on the top 50 crashes, and Al said he didn't see any crashes at all in the last 30 days.
Flags: needinfo?(matt.woodrow)
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.