816359 - Heap-use-after-free in nsFrameSelection::cycleCollection::TraverseImpl

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Abhishek Arya]

Attachment: Testcase

==994== ERROR: AddressSanitizer: heap-use-after-free on address 0x7f7ed81c7e48 at pc 0x7f7f012e2e65 bp 0x7f7ef02d7a90 sp 0x7f7ef02d7a88
READ of size 8 at 0x7f7ed81c7e48 thread T4
    #0 0x7f7f012e2e64 in nsIPresShell::GetDocument() const src/../../../dist/include/nsIPresShell.h:271
    #1 0x7f7f01e7fc21 in nsFrameSelection::cycleCollection::TraverseImpl(nsFrameSelection::cycleCollection*, void*, nsCycleCollectionTraversalCallback&) src/layout/generic/nsSelection.cpp:513
    #2 0x7f7f0de2c27f in nsCycleCollectionParticipant::Traverse(void*, nsCycleCollectionTraversalCallback&) src/../../dist/include/nsCycleCollectionParticipant.h:201
    #3 0x7f7f0de2bb44 in GCGraphBuilder::Traverse(PtrInfo*) src/xpcom/base/nsCycleCollector.cpp:1856
    #4 0x7f7f0de330d7 in nsCycleCollector::MarkRoots(GCGraphBuilder&) src/xpcom/base/nsCycleCollector.cpp:2171
    #5 0x7f7f0de3da84 in nsCycleCollector::BeginCollection(bool, nsICycleCollectorListener*) src/xpcom/base/nsCycleCollector.cpp:2850
    #6 0x7f7f0de5b6d5 in nsCycleCollectorRunner::Run() src/xpcom/base/nsCycleCollector.cpp:3208
    #7 0x7f7f0dd8332e in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:627
    #8 0x7f7f0d9f8f7f in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:221
    #9 0x7f7f0dd7afc4 in nsThread::ThreadFunc(void*) src/xpcom/threads/nsThread.cpp:265
    #10 0x7f7f1f58c306 in _pt_root src/nsprpub/pr/src/pthreads/ptthread.c:156
    #11 0x4c672a in __asan::AsanThread::ThreadStart()
0x7f7ed81c7e48 is located 8 bytes inside of 416-byte region [0x7f7ed81c7e40,0x7f7ed81c7fe0)
freed by thread T0 here:
    #0 0x4c3750 in __interceptor_free
    #1 0x7f7f1d39b405 in moz_free src/memory/mozalloc/mozalloc.cpp:48
    #2 0x7f7f017bf9eb in operator delete(void*) src/../../dist/include/mozilla/mozalloc.h:224
    #3 0x7f7f017bf9eb in PresShell::operator delete(void*) src/layout/base/nsPresShell.h:65
    #4 0x7f7f017bf85b in PresShell::~PresShell() src/layout/base/nsPresShell.cpp:755
    #5 0x7f7f017be4f2 in PresShell::Release() src/layout/base/nsPresShell.cpp:749
    #6 0x7f7eff6b59ce in nsCOMPtr_base::assign_assuming_AddRef(nsISupports*) src/../../dist/include/nsCOMPtr.h:442
    #7 0x7f7f0d9b082a in nsCOMPtr_base::assign_with_AddRef(nsISupports*) src/objdir-ff-asan-sym/xpcom/build/nsCOMPtr.cpp:49
    #8 0x7f7f0162e098 in nsCOMPtr<nsIPresShell>::operator=(nsIPresShell*) src/../../dist/include/nsCOMPtr.h:624
    #9 0x7f7f0161c1a2 in DocumentViewerImpl::DestroyPresShell() src/layout/base/nsDocumentViewer.cpp:4367
    #10 0x7f7f01619e84 in DocumentViewerImpl::Destroy() src/layout/base/nsDocumentViewer.cpp:1664
    #11 0x7f7f01622c16 in DocumentViewerImpl::Show() src/layout/base/nsDocumentViewer.cpp:1938
    #12 0x7f7f01781ebb in nsPresContext::EnsureVisible() src/layout/base/nsPresContext.cpp:1825
    #13 0x7f7f01808372 in PresShell::UnsuppressAndInvalidate() src/layout/base/nsPresShell.cpp:3575
    #14 0x7f7f0180900c in PresShell::UnsuppressPainting() src/layout/base/nsPresShell.cpp:3626
    #15 0x7f7f0160ba08 in DocumentViewerImpl::LoadComplete(tag_nsresult) src/layout/base/nsDocumentViewer.cpp:1069
previously allocated by thread T0 here:
    #0 0x4c3810 in malloc
    #1 0x7f7f1d39b541 in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:54
    #2 0x7f7f017bc811 in operator new(unsigned long) src/../../dist/include/mozilla/mozalloc.h:200
    #3 0x7f7f017bc811 in PresShell::operator new(unsigned long) src/layout/base/nsPresShell.h:65
    #4 0x7f7f017bc439 in NS_NewPresShell(nsIPresShell**) src/layout/base/nsPresShell.cpp:695
    #5 0x7f7f0310821f in nsDocument::doCreateShell(nsPresContext*, nsIViewManager*, nsStyleSet*, nsCompatibility, nsIPresShell**) src/content/base/src/nsDocument.cpp:3075
    #6 0x7f7f050ae21d in nsHTMLDocument::CreateShell(nsPresContext*, nsIViewManager*, nsStyleSet*, nsIPresShell**) src/content/html/document/src/nsHTMLDocument.cpp:312
    #7 0x7f7f016011e1 in DocumentViewerImpl::InitPresentationStuff(bool) src/layout/base/nsDocumentViewer.cpp:705
    #8 0x7f7f01600282 in DocumentViewerImpl::InitInternal(nsIWidget*, nsISupports*, nsIntRect const&, bool, bool, bool) src/layout/base/nsDocumentViewer.cpp:952
    #9 0x7f7f015fd86f in DocumentViewerImpl::Init(nsIWidget*, nsIntRect const&) src/layout/base/nsDocumentViewer.cpp:686
Thread T4 created by T0 here:
    #0 0x4bfa14 in __interceptor_pthread_create
    #1 0x7f7f1f57d6b5 in _PR_CreateThread src/nsprpub/pr/src/pthreads/ptthread.c:393
    #2 0x7f7f1f57b4a6 in PR_CreateThread src/nsprpub/pr/src/pthreads/ptthread.c:476
    #3 0x7f7f0dd7e050 in nsThread::Init() src/xpcom/threads/nsThread.cpp:331
    #4 0x7f7f0dd95918 in nsThreadManager::NewThread(unsigned int, unsigned int, nsIThread**) src/xpcom/threads/nsThreadManager.cpp:215
    #5 0x7f7f0d9f66f1 in NS_NewThread_P(nsIThread**, nsIRunnable*, unsigned int) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:50
    #6 0x7f7f0de41c3b in nsCycleCollector_startup() src/xpcom/base/nsCycleCollector.cpp:3311
    #7 0x7f7f0da36d30 in NS_InitXPCOM2_P src/xpcom/build/nsXPComInit.cpp:443
    #8 0x7f7eff5686a0 in ScopedXPCOMStartup::Initialize() src/toolkit/xre/nsAppRunner.cpp:1183
    #9 0x7f7eff597629 in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3886
    #10 0x7f7eff59a620 in XRE_main src/toolkit/xre/nsAppRunner.cpp:4084
    #11 0x40c0c6 in do_main(int, char**) src/browser/app/nsBrowserApp.cpp:174
    #12 0x409900 in main src/browser/app/nsBrowserApp.cpp:279
    #13 0x7f7f1f80d76c in
Shadow byte and word:
  0x1fefdb038fc9: fd
  0x1fefdb038fc8: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1fefdb038fa8: fa fa fa fa fa fa fa fa
  0x1fefdb038fb0: fa fa fa fa fa fa fa fa
  0x1fefdb038fb8: fa fa fa fa fa fa fa fa
  0x1fefdb038fc0: fa fa fa fa fa fa fa fa
=>0x1fefdb038fc8: fd fd fd fd fd fd fd fd
  0x1fefdb038fd0: fd fd fd fd fd fd fd fd
  0x1fefdb038fd8: fd fd fd fd fd fd fd fd
  0x1fefdb038fe0: fd fd fd fd fd fd fd fd
  0x1fefdb038fe8: fd fd fd fd fd fd fd fd
Stats: 276M malloced (258M for red zones) by 458491 calls
Stats: 49M realloced by 26124 calls
Stats: 250M freed by 347463 calls
Stats: 218M really freed by 215147 calls
Stats: 256M (65635 full pages) mmaped in 489 calls
  mmaps   by size class: 7:167895; 8:49128; 9:15345; 10:10220; 11:8670; 12:1280; 13:1600; 14:512; 15:224; 16:792; 17:468; 18:36; 19:35; 20:21;
  mallocs by size class: 7:278568; 8:100263; 9:30963; 10:15158; 11:22702; 12:2799; 13:2980; 14:1618; 15:418; 16:1508; 17:1376; 18:72; 19:42; 20:24;
  frees   by size class: 7:207134; 8:72641; 9:25000; 10:12209; 11:20982; 12:2073; 13:2812; 14:1425; 15:294; 16:1415; 17:1361; 18:58; 19:39; 20:20;
  rfrees  by size class: 7:118307; 8:52096; 9:16065; 10:5384; 11:15900; 12:1641; 13:1448; 14:1348; 15:268; 16:1233; 17:1345; 18:55; 19:38; 20:19;
Stats: malloc large: 3440 small slow: 6210
==994== ABORTING

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

I assume mShell points to a deleted shell.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Mats, could you perhaps look at this? I have plenty of ss bugs in my todo list.
If not, I'll take this.

Comment 3

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

Andrew said he'd take a look (during CRITSMASH).

Comment 4

[Andrew McCreight [:mccr8]]

When I run this in a debug build, I get the following console spew:

++DOMWINDOW == 11 (0x11c3b0c88) [serial = 13] [outer = 0x122ab5800]
###!!! ASSERTION: Think of the children!: '!HasAbsolutelyPositionedChildren()', file /Users/amccreight/mz/cent/layout/generic/nsFrame.cpp, line 266
###!!! ASSERTION: asked to create frame construction item for a node that already has a frame: 'Error', file /Users/amccreight/mz/cent/layout/base/nsCSSFrameConstructor.cpp, line 5051
###!!! ASSERTION: no placeholder frame for out-of-flow frame: 'Not Reached', file /Users/amccreight/mz/cent/layout/generic/nsFrame.cpp, line 7374
frame: nsTextControlFrame (0x1292d9390) style: 0x11bda21b0 {}
###!!! ASSERTION: Wrong parent style context: 'Error', file /Users/amccreight/mz/cent/layout/base/nsFrameManager.cpp, line 596
Wrong parent style context:  style: 0x12574fee0 {}
should be using:  style: 0x1292f6610 {}

Then I get this:

###!!! ASSERTION: Why did this not get handled while processing mRestyleRoots?: '!element->HasFlag(collector->tracker->RootBit()) || (element->GetFlattenedTreeParent() && (!element->GetFlattenedTreeParent()->GetPrimaryFrame()|| element->GetFlattenedTreeParent()->GetPrimaryFrame()->IsLeaf())) || (aData.mChangeHint & nsChangeHint_ReconstructFrame)', file /Users/amccreight/mz/cent/layout/base/RestyleTracker.cpp, line 88
###!!! ASSERTION: no placeholder frame for out-of-flow frame: 'Not Reached', file /Users/amccreight/mz/cent/layout/generic/nsFrame.cpp, line 7374
###!!! ASSERTION: no placeholder frame for out-of-flow frame: 'Not Reached', file /Users/amccreight/mz/cent/layout/generic/nsFrame.cpp, line 7374
WARNING: NS_ENSURE_SUCCESS(rv, false) failed with result 0x8000FFFF: file /Users/amccreight/mz/cent/content/base/src/nsContentUtils.cpp, line 2956
WARNING: NS_ENSURE_TRUE(pusher.Push(aBoundElement)) failed: file /Users/amccreight/mz/cent/content/xbl/src/nsXBLProtoImplMethod.cpp, line 321

Then I get those three assertions over and over again.

Eventually, we get:

###!!! ASSERTION: Think of the children!: '!HasAbsolutelyPositionedChildren()', file /Users/amccreight/mz/cent/layout/generic/nsFrame.cpp, line 266
###!!! ASSERTION: asked to create frame construction item for a node that already has a frame: 'Error', file /Users/amccreight/mz/cent/layout/base/nsCSSFrameConstructor.cpp, line 5051
###!!! ASSERTION: no placeholder frame for out-of-flow frame: 'Not Reached', file /Users/amccreight/mz/cent/layout/generic/nsFrame.cpp, line 7374
frame: nsTextControlFrame (0x119944390) style: 0x129d6f1b0 {}
###!!! ASSERTION: Wrong parent style context: 'Error', file /Users/amccreight/mz/cent/layout/base/nsFrameManager.cpp, line 596
Wrong parent style context:  style: 0x119321ee0 {}
should be using:  style: 0x129d71610 {}

Assertion failure: mPresArenaAllocCount == 0 (Some pres arena objects were not freed), at /Users/amccreight/mz/cent/layout/base/nsPresShell.cpp:769

The top of the assertion stack:

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   PresShell::~PresShell() + 1390 (nsPresShell.cpp:768)
1   PresShell::~PresShell() + 14 (mozalloc.h:224)
2   PresShell::Release() + 206 (nsPresShell.cpp:749)
3   nsDocumentViewer::DestroyPresShell() + 316 (nsCOMPtr.h:625)
4   nsDocumentViewer::Destroy() + 1093 (nsAutoPtr.h:1005)
5   nsDocumentViewer::Show() + 192 (nsCOMPtr.h:764)
6   nsPresContext::EnsureVisible() + 296 (nsPresContext.cpp:1825)
7   PresShell::UnsuppressAndInvalidate() + 46 (nsPresShell.cpp:3575)

I'll look at this some more tomorrow.

Comment 5

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

That is some fine spew. CC+ bz,roc

Comment 6

[Andrew McCreight [:mccr8]]

I can keep looking at this, but I'm not familiar with PresShell ownership etc., so it will be slow going.

Comment 7

[Andrew McCreight [:mccr8]]

If anybody else wants to grab this, feel free.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Fixes the crash, but not the right patch

So this fixes the crash, but leaks.
As the assertions hints, the real problem seems to be that frames aren't deleted
properly, so nsTextEditorState::UnbindFromFrame doesn't get called.
I assume nsTextControlFrame::DestroyFrom doesn't get called.

I'm moving this bug to layout.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: When constructing inline frames, we should never treat transforms as making the inline an abs-pos containing block, since transforms don't apply to inlines

Some frame constructor code still thinks transforms apply to inlines...

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Make nsStyleDisplay::HasTransform take a frame parameter and check that the frame supports transforms

This is just for extra robustness, to ensure callers of HasTransforms check whether the frame actually supports transforms as well.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 693865 [details] [diff] [review]
Part 1: When constructing inline frames, we should never treat transforms as making the inline an abs-pos containing block, since transforms don't apply to inlines

r=me

Comment 12

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

How far back does this affect things? Should we be tracking this for branch releases?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The underlying buggy code has been around for a long time (not sure how long). Abishek's testcase relies on the patch from bug 691591, which is in FF19 (not 18). I suspect the buggy code could not cause these sorts of problems before 691591 ... an nsInlineFrame with a CSS transform would incorrectly be treated as an abs-pos container, but from then on things would probably work fine without crashing.

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 693866 [details] [diff] [review]
Part 2: Make nsStyleDisplay::HasTransform take a frame parameter and check that the frame supports transforms

r=mats  (sorry for the delay)

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 693865 [details] [diff] [review]
Part 1: When constructing inline frames, we should never treat transforms as making the inline an abs-pos containing block, since transforms don't apply to inlines

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Probably isn't easy. Just triggering the bug in this code isn't enough to do anything dangerous.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

Which older supported branches are affected by this flaw?
None. Probably only FF19 and later are dangerous. See bug 691591.

If not all supported branches, which bug introduced the flaw?
Bug 691591 didn't introduce the flaw but it did make it more dangerous.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Easy as necessary.

How likely is this patch to cause regressions; how much testing does it need?
This particular patch is very low risk.

Comment 16

[Mats Palmgren (inactive)]

> Bug 691591 didn't introduce the flaw but it did make it more dangerous.

Wrong bug number?

Comment 17

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

(In reply to Mats Palmgren [:mats] from comment #16)
> > Bug 691591 didn't introduce the flaw but it did make it more dangerous.
> 
> Wrong bug number?

I had the same question.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry yes. Bug 691651.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/96d7b0bfb19e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7eb5d695818

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 693865 [details] [diff] [review]
Part 1: When constructing inline frames, we should never treat transforms as making the inline an abs-pos containing block, since transforms don't apply to inlines

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none, but bug 691651 
User impact if declined: security bug
Testing completed (on m-c, etc.): landed on inbound
Risk to taking this patch (and alternatives if risky): very low risk
String or UUID changes made by this patch: none

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/96d7b0bfb19e
https://hg.mozilla.org/mozilla-central/rev/e7eb5d695818

Comment 22

[bhavana bajaj [:bajaj]]

Comment on attachment 693865 [details] [diff] [review]
Part 1: When constructing inline frames, we should never treat transforms as making the inline an abs-pos containing block, since transforms don't apply to inlines

low risk patch for a FF19 regression. Approving on aurora

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/96c0c0c5d0b7
https://hg.mozilla.org/releases/mozilla-aurora/rev/f2caa500afe5