856106 - crash in mozilla::FrameLayerBuilder::RemoveFrameFromLayerManager

Status: RESOLVED INCOMPLETE

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

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

Comment 1

[Daniel Veditz [:dveditz]]

"EXCEPTION_ACCESS_VIOLATION_WRITE" is even more evidence of exploitability.

Comment 2

[Tracy Walker [:tracy]]

9 	https://www.facebook.com/
7 	http://www.facebook.com/
6 	http://www.facebook.com/logout.php
5 	https://www.facebook.com/logout.php
4 	about:blank
4 	about:newtab
3 	https://www.facebook.com/?ref=tn_tnmn
2 	https://www.facebook.com/index.php?l_s=r
2 	https://apps.facebook.com/playhappyfarm/?fb_source=bookmark_apps&ref=bookmarks&count=0&fb_bmpos=2_0

Comment 3

[Matt Wobensmith [:mwobensmith][:matt:]]

Anyone: steps to reproduce outside of URLs in comment 2? Not much to go on.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 5

[Alex Keybl [:akeybl]]

(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?

Comment 6

[Matt Woodrow (:mattwoodrow)]

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.

Comment 7

[Alex Keybl [:akeybl]]

We've got no leads so there's little reason to track this longstanding issue.

Comment 8

[Jason Smith [:jsmith]]

(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?

Comment 9

[Jason Smith [:jsmith]]

Actually, I'm pulling the tracking-b2g18 flag to get this off our radar.

Comment 10

[Matt Wobensmith [:mwobensmith][:matt:]]

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.

Comment 11

[Mats Palmgren (inactive)]

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.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Marking this as tracking - for 24 since we seem to be doing nothing with this issue.

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

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.

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

(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)

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Needinfo'ing to get answers to questions in comment 15.

Comment 17

[Mats Palmgren (inactive)]

> 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.

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

Hi Matt, any update? (Do you think the poison mitigation idea from Mats should be next?)

Comment 19

[Al Billings [:abillings - ex-MoCo]]

Closing as incomplete because it isn't actionable at this point.

Comment 20

[Andrew McCreight [:mccr8]]

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.