824862 - Heap-use-after-free in nsCounterList::RecalcAll and "ASSERTION: Bit should never be set on generated content: '!frame || !frame->IsGeneratedContentFrame()", with display:flex, "overflow", and generated content

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Abhishek Arya]

Attachment: Testcase

Reproduces on trunk.

>==8706== ERROR: AddressSanitizer: heap-use-after-free on address 0x7f6a664f9448 at pc 0x7f6a8a256ea9 bp 0x7fff1550d350 sp 0x7fff1550d348
>READ of size 8 at 0x7f6a664f9448 thread T0
>    #0 0x7f6a8a256ea8 in nsGenConList::Next(nsGenConNode*) src/layout/base/nsGenConList.h:92
>    #1 0x7f6a8a24aedc in nsCounterList::Next(nsCounterNode*) src/layout/base/nsCounterManager.h:180
>    #2 0x7f6a8a24a794 in nsCounterList::RecalcAll() src/layout/base/nsCounterManager.cpp:178
>    #3 0x7f6a8a24ebd4 in RecalcDirtyLists(nsAString_internal const&, nsCounterList*, void*) src/layout/base/nsCounterManager.cpp:256
>    #4 0x7f6a8a24fbae in nsBaseHashtable<nsStringHashKey, nsAutoPtr<nsCounterList>, nsCounterList*>::s_EnumReadStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*) src/../../dist/include/nsBaseHashtable.h:400
>    #5 0x7f6a972d18f6 in PL_DHashTableEnumerate src/objdir-ff-asan-sym/xpcom/build/pldhash.cpp:717
>    #6 0x7f6a8a24e9a5 in nsBaseHashtable<nsStringHashKey, nsAutoPtr<nsCounterList>, nsCounterList*>::EnumerateRead(PLDHashOperator (*)(nsAString_internal const&, nsCounterList*, void*), void*) const src/../../dist/include/nsBaseHashtable.h:188
>    #7 0x7f6a8a24e6ae in nsCounterManager::RecalcAll() src/layout/base/nsCounterManager.cpp:263
>    #8 0x7f6a8a0b45ff in nsCSSFrameConstructor::RecalcQuotesAndCounters() src/layout/base/nsCSSFrameConstructor.cpp:8532
>    #9 0x7f6a8a0934d6 in nsCSSFrameConstructor::EndUpdate() src/layout/base/nsCSSFrameConstructor.cpp:8515
>    #10 0x7f6a89fe9a24 in mozilla::css::RestyleTracker::DoProcessRestyles() src/layout/base/RestyleTracker.cpp:254
>    #11 0x7f6a8a0da741 in mozilla::css::RestyleTracker::ProcessRestyles() src/layout/base/RestyleTracker.h:192
>    #12 0x7f6a8a0da079 in nsCSSFrameConstructor::ProcessPendingRestyles() src/layout/base/nsCSSFrameConstructor.cpp:12105
>    #13 0x7f6a8a5440e8 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/nsPresShell.cpp:3868
>    #14 0x7f6a8a5f1190 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:898
>    #15 0x7f6a8a60878b in mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver*, long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:164
>    #16 0x7f6a8a607da2 in mozilla::RefreshDriverTimer::Tick() src/layout/base/nsRefreshDriver.cpp:156
>    #17 0x7f6a8a60724b in mozilla::RefreshDriverTimer::TimerTick(nsITimer*, void*) src/layout/base/nsRefreshDriver.cpp:181
>    #18 0x7f6a976837eb in nsTimerImpl::Fire() src/xpcom/threads/nsTimerImpl.cpp:482
>    #19 0x7f6a97684c74 in nsTimerEvent::Run() src/xpcom/threads/nsTimerImpl.cpp:565
>    #20 0x7f6a9764705f in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:627
>    #21 0x7f6a972bb9e5 in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:237
>    #22 0x7f6a94f8d4bc in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82
>    #23 0x7f6a97938562 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:215
>    #24 0x7f6a97938399 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:208
>    #25 0x7f6a9793826e in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:182
>    #26 0x7f6a94385507 in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
>    #27 0x7f6a92eb2ed5 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:288
>    #28 0x7f6a88171554 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3823
>    #29 0x7f6a8817713a in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3890
>    #30 0x7f6a88179f00 in XRE_main src/toolkit/xre/nsAppRunner.cpp:4088
>    #31 0x41ec26 in do_main(int, char**) src/browser/app/nsBrowserApp.cpp:174
>    #32 0x41c440 in main src/browser/app/nsBrowserApp.cpp:279
>    #33 0x7f6aa9a4576c in
>0x7f6a664f9448 is located 8 bytes inside of 88-byte region [0x7f6a664f9440,0x7f6a664f9498)
>freed by thread T0 here:
>    #0 0x411232 in __interceptor_free
>    #1 0x7f6aa75d1409 in moz_free src/memory/mozalloc/mozalloc.cpp:48
>    #2 0x7f6a8a24f670 in operator delete(void*) src/../../dist/include/mozilla/mozalloc.h:224
>    #3 0x7f6a8a24f670 in nsCounterUseNode::~nsCounterUseNode() src/layout/base/nsCounterManager.h:77
>    #4 0x7f6a8a3e1910 in nsGenConList::DestroyNodesFor(nsIFrame*) src/layout/base/nsGenConList.cpp:42
>    #5 0x7f6a8a24f2b7 in DestroyNodesInList(nsAString_internal const&, nsCounterList*, void*) src/layout/base/nsCounterManager.cpp:281
>    #6 0x7f6a8a24fbae in nsBaseHashtable<nsStringHashKey, nsAutoPtr<nsCounterList>, nsCounterList*>::s_EnumReadStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*) src/../../dist/include/nsBaseHashtable.h:400
>    #7 0x7f6a972d18f6 in PL_DHashTableEnumerate src/objdir-ff-asan-sym/xpcom/build/pldhash.cpp:717
>    #8 0x7f6a8a24e9a5 in nsBaseHashtable<nsStringHashKey, nsAutoPtr<nsCounterList>, nsCounterList*>::EnumerateRead(PLDHashOperator (*)(nsAString_internal const&, nsCounterList*, void*), void*) const src/../../dist/include/nsBaseHashtable.h:188
>    #9 0x7f6a8a24ede2 in nsCounterManager::DestroyNodesFor(nsIFrame*) src/layout/base/nsCounterManager.cpp:292
>    #10 0x7f6a8a015526 in nsCSSFrameConstructor::NotifyDestroyingFrame(nsIFrame*) src/layout/base/nsCSSFrameConstructor.cpp:1476
>    #11 0x7f6a8a5179ed in PresShell::NotifyDestroyingFrame(nsIFrame*) src/layout/base/nsPresShell.cpp:1999
>    #12 0x7f6a8a8ee5c0 in nsFrame::DestroyFrom(nsIFrame*) src/layout/generic/nsFrame.cpp:652
>    #13 0x7f6a8ac8466a in nsSplittableFrame::DestroyFrom(nsIFrame*) src/layout/generic/nsSplittableFrame.cpp:43
>    #14 0x7f6a8a8a8cfd in nsContainerFrame::DestroyFrom(nsIFrame*) src/layout/generic/nsContainerFrame.cpp:249
>    #15 0x7f6a8ab38a0b in nsLineBox::DeleteLineList(nsPresContext*, nsLineList&, nsIFrame*, nsFrameList*) src/layout/generic/nsLineBox.cpp:372
>previously allocated by thread T0 here:
>    #0 0x411312 in malloc
>    #1 0x7f6aa75d1554 in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:54
>    #2 0x7f6a8a019771 in operator new(unsigned long) src/../../dist/include/mozilla/mozalloc.h:200
>    #3 0x7f6a8a019771 in nsCSSFrameConstructor::CreateGeneratedContent(nsFrameConstructorState&, nsIContent*, nsStyleContext*, unsigned int) src/layout/base/nsCSSFrameConstructor.cpp:1617
>    #4 0x7f6a8a01d710 in nsCSSFrameConstructor::CreateGeneratedContentItem(nsFrameConstructorState&, nsIFrame*, nsIContent*, nsStyleContext*, nsCSSPseudoElements::Type, nsCSSFrameConstructor::FrameConstructionItemList&) src/layout/base/nsCSSFrameConstructor.cpp:1744
>    #5 0x7f6a8a02b2b0 in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsIFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) src/layout/base/nsCSSFrameConstructor.cpp:9966
>    #6 0x7f6a8a040529 in nsCSSFrameConstructor::ConstructBlock(nsFrameConstructorState&, nsStyleDisplay const*, nsIContent*, nsIFrame*, nsIFrame*, nsStyleContext*, nsIFrame**, nsFrameItems&, bool, PendingBinding*) src/layout/base/nsCSSFrameConstructor.cpp:11032
>    #7 0x7f6a8a06461d in nsCSSFrameConstructor::ConstructNonScrollableBlock(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsIFrame*, nsStyleDisplay const*, nsFrameItems&, nsIFrame**) src/layout/base/nsCSSFrameConstructor.cpp:4488
>    #8 0x7f6a8a05725c in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsIFrame*, nsFrameItems&) src/layout/base/nsCSSFrameConstructor.cpp:3581
>    #9 0x7f6a8a06ff65 in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsIFrame*, nsFrameItems&) src/layout/base/nsCSSFrameConstructor.cpp:5511
>Shadow bytes around the buggy address:
>  0x1fed4cc9f230: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>  0x1fed4cc9f240: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
>  0x1fed4cc9f250: 00 00 00 00 00 00 00 fb fa fa fa fa fa fa fa fa
>  0x1fed4cc9f260: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
>  0x1fed4cc9f270: 00 00 00 00 00 00 fb fb fa fa fa fa fa fa fa fa
>=>0x1fed4cc9f280: fa fa fa fa fa fa fa fa fd[fd]fd fd fd fd fd fd
>  0x1fed4cc9f290: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
>  0x1fed4cc9f2a0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
>  0x1fed4cc9f2b0: 00 00 00 00 00 00 00 fb fa fa fa fa fa fa fa fa
>  0x1fed4cc9f2c0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
>  0x1fed4cc9f2d0: 00 00 00 00 00 00 00 fb fa fa fa fa fa fa fa fa
>Shadow byte legend (one shadow byte represents 8 application bytes):
>  Addressable:           00
>  Partially addressable: 01 02 03 04 05 06 07
>  Heap left redzone:     fa
>  Heap righ redzone:     fb
>  Freed Heap region:     fd
>  Stack left redzone:    f1
>  Stack mid redzone:     f2
>  Stack right redzone:   f3
>  Stack partial redzone: f4
>  Stack after return:    f5
>  Stack use after scope: f8
>  Global redzone:        f9
>  Global init order:     f6
>  Poisoned by user:      f7
>  ASan internal:         fe
>Stats: 76M malloced (83M for red zones) by 292454 calls
>Stats: 3M realloced by 17264 calls
>Stats: 48M freed by 165114 calls
>Stats: 15M really freed by 70859 calls
>Stats: 133M (133M-0M) mmaped; 262 maps, 0 unmaps
>  mmaps   by size class: 7:176085; 8:47081; 9:11253; 10:6132; 11:3825; 12:1664; 13:768; 14:352; 15:192; 16:624; 17:68; 18:14; 19:4; 20:4;
>  mallocs by size class: 7:212673; 8:52150; 9:11822; 10:7129; 11:4403; 12:1698; 13:949; 14:491; 15:210; 16:831; 17:75; 18:15; 19:4; 20:4;
>  frees   by size class: 7:126367; 8:24357; 9:5551; 10:3970; 11:2293; 12:729; 13:734; 14:349; 15:101; 16:585; 17:66; 18:8; 19:2; 20:2;
>  rfrees  by size class: 7:58735; 8:7685; 9:1619; 10:1158; 11:886; 12:121; 13:251; 14:141; 15:18; 16:237; 17:7; 18:1;
>Stats: malloc large: 1139 small slow: 2664
>==8706== ABORTING

Comment 1

[Mats Palmgren (inactive)]

Attachment: stack + frame dump at the assertion

Before the crash there is this assertion:
###!!! ASSERTION: Bit should never be set on generated content: '!frame || !frame->IsGeneratedContentFrame()', file layout/base/nsCSSFrameConstructor.cpp, line 7935

Comment 2

[Daniel Holbert [:dholbert]]

Ah, so this is a "display: flex; overflow: [non-visible];" situation, which doesn't work yet - bug 782441 is filed on that.  It's probably likely that fixing bug 782441 will fix this as well.  (Note that Jesse filed another assertion bug with "display:flex; overflow: [non-visible]" -- bug 812822)

(Luckily, the flexbox about:config pref (layout.css.flexbox.enabled) is only turned on by default on Nightly right now, so this only affects Nightly, in default configuration at least.)

Comment 3

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

If the pref was flipped, how far back would this go?

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Al Billings [:abillings] from comment #3)
> If the pref was flipped, how far back would this go?

Flexbox support has been built by default (from bug 797022) & controllable by a pref (from bug 796212) in builds since Firefox 18, which is currently on Beta.

So: with sophisticated user action (toggling the pref on), this should affect beta. (though it requires "diplay: -moz-flex" instead of "display: flex", since at that point our flexbox support was prefixed).

Local testing seems to confirm that Beta is affected -- at least, I've confirmed that the attached testcase (with that "-moz" modification) behaves the same in Beta-with-pref-flipped build as the original testcase behaves in a nightly build. (They both hangs with 100% CPU usage -- no crash -- probably because of opt vs debug build differences.) ( Adding "hang" keyword, since that's how this reproduces for me, in opt builds.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase for beta builds (requires layout.css.flexbox.enabled = true)

(For reference, here's a version of the original testcase with s/flex/-moz-flex/, to trigger the bug in Beta builds (w/ the flexbox pref enabled))

Comment 6

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

Daniel do you want this one? (Feel free to reassign)

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Ah, so this is a "display: flex; overflow: [non-visible];" situation

Sorry, I was wrong about that -- "overflow" is set on the flex container's parent, not the flex container -- so this isn't actually related to bug 782441, after all.

I think the real problem here is that we're incorrectly skipping over some generated whitespace inside of the generated flex container.  (not sure how overflow on the parent ends up interacting with that, but it apparently does)

I noticed that elsewhere in nsCSSFrameConstructor where we skip whitespace, we explicitly _don't_ skip if we're generated content.  If I add the same check during anonymous-flex-item formation, it appears to fix this bug.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> I noticed that elsewhere in nsCSSFrameConstructor where we skip whitespace,
> we explicitly _don't_ skip if we're generated content

For reference, here's that existing code (w/ the check for generated content):
> 9678         /* Walk an iterator past any whitespace that we might be able to drop from the list */
> 9679         FCItemIterator spaceEndIter(endIter);
> 9680         if (prevParentType != eTypeBlock &&
> 9681             !aParentFrame->IsGeneratedContentFrame() &&
> 9682             spaceEndIter.item().IsWhitespace(aState)) {
> 9683           bool trailingSpaces = spaceEndIter.SkipWhitespace(aState);

https://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=504d70c1ca9c&mark=9678-9683#9678

Comment 9

[Daniel Holbert [:dholbert]]

One other place where we check for generated content before skipping whitespace:
https://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=504d70c1ca9c#11441

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: fix v1

I don't fully understand why generated-content-whitespace is special, but this patch makes us consistent w/ other code (mxr links above) in honoring it where we'd otherwise skip it.

(As noted in an XXXdholbert comment in the patch, this makes us violate the flexbox spec for whitespace in generated-content flex containers, but at least we don't crash. :)  bz, if you know of a way we can_safely_ skip whitespace here, that'd be awesome -- but otherwise, it's not too evil if we honor it in this (restricted) generated-content case.)

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: fix v2

(er, forgot to qref after adding the XXXdholbert comment. fixed)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 698110 [details] [diff] [review]
fix v2

r=me.

The reason for these checks is that generated content starts off with empty (hence whitespace-only) textnodes and then sets them later.  But then if we dropped it we have to reframe, which destroys the generated content stuff... if we're lucky we also reframe the parent and then just create new generated content and go in a loop.  Not sure why we end up with memory corruption here instead...

Comment 13

[Daniel Holbert [:dholbert]]

Thanks for the quick review!

For good measure, I verified that an ASAN build reports no issues after this patch is applied.  (Previously I was just testing a debug build & monitoring for the assert/crash from comment 1)

I'll land this shortly.

Comment 14

[Daniel Holbert [:dholbert]]

Clarified the comment a bit, per comment 12, and landed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/becfdb6e3719

Comment 15

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/becfdb6e3719

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 698110 [details] [diff] [review]
fix v2

I think we should consider taking this for Beta (Firefox 19).

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 666041 (adding flexbox support), though it's preffed off by default

User impact if declined: for users who've enabled the flexbox pref: possibly-exploitable crash

Testing completed (on m-c, etc.): It's been on m-c for a week, and on Aurora 19 since this week's merge. 

Risk to taking this patch (and alternatives if risky): Extremely low. This code isn't used at all unless the flexbox about:config pref is turned on, and it's off-by-default on Beta. (currently on-by-default on Aurora & Trunk)

String or UUID changes made by this patch: none.

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 698110 [details] [diff] [review]
fix v2

Since this is in disabled code, we can uplift to FF19.

Comment 18

[Daniel Holbert [:dholbert]]

Pushed fix to beta:
  https://hg.mozilla.org/releases/mozilla-beta/rev/723811984f5d

(Updating status-firefox19 to "fixed", though it's really "disabled and also fixed", if our bugzilla instance supported that. :))

Comment 19

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

Attachment: Bounty Awarded $3000 [paid]

Comment 20

[Mats Palmgren (inactive)]

Landed the crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2c2032ea5ab

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d2c2032ea5ab