1181011 - Heap-use-after-free in nsFrame::DidSetStyleContext

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Abhishek Arya]

Attachment: test.html

==7493==ERROR: AddressSanitizer: heap-use-after-free on address 0x608000000028 at pc 0x7f3f5ceeea8a bp 0x7fffa54df280 sp 0x7fffa54df278
READ of size 1 at 0x608000000028 thread T0 (Web Content)
    #0 0x7f3f5ceeea89 in nsFrame::DidSetStyleContext(nsStyleContext*) layout/style/nsStyleStruct.h:627:9
    #1 0x7f3f5cb7c39a in mozilla::RestyleManager::ReparentStyleContext(nsIFrame*) layout/generic/nsIFrame.h:543:7
    #2 0x7f3f5cff157a in nsFirstLineFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsInlineFrame.cpp:364:5
    #3 0x7f3f5ce01b86 in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) layout/generic/nsLineLayout.cpp:956:5
    #4 0x7f3f5ce6f000 in nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) layout/generic/nsBlockFrame.cpp:3937:3
    #5 0x7f3f5ce6d4a2 in nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) layout/generic/nsBlockFrame.cpp:3739:5
    #6 0x7f3f5ce644e6 in nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:3605:9
    #7 0x7f3f5ce54908 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) layout/generic/nsBlockFrame.cpp:2712:5
    #8 0x7f3f5ce4db07 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsBlockFrame.cpp:1160:3
    #9 0x7f3f5ce6a9ea in nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) layout/generic/nsBlockReflowContext.cpp:297:3
    #10 0x7f3f5ce6128f in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:3332:5
    #11 0x7f3f5ce548a3 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) layout/generic/nsBlockFrame.cpp:2709:5
    #12 0x7f3f5ce4db07 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsBlockFrame.cpp:1160:3
    #13 0x7f3f5ce6a9ea in nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) layout/generic/nsBlockReflowContext.cpp:297:3
    #14 0x7f3f5ce6128f in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:3332:5
    #15 0x7f3f5ce548a3 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) layout/generic/nsBlockFrame.cpp:2709:5
    #16 0x7f3f5ce4db07 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsBlockFrame.cpp:1160:3
    #17 0x7f3f5ce6a9ea in nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) layout/generic/nsBlockReflowContext.cpp:297:3
    #18 0x7f3f5ce6128f in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:3332:5
    #19 0x7f3f5ce548a3 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) layout/generic/nsBlockFrame.cpp:2709:5
    #20 0x7f3f5ce4db07 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsBlockFrame.cpp:1160:3
    #21 0x7f3f5ceb9758 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) layout/generic/nsContainerFrame.cpp:976:3
    #22 0x7f3f5ce9da29 in nsCanvasFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsCanvasFrame.cpp:676:5
    #23 0x7f3f5cf4f676 in nsHTMLScrollFrame::ReflowScrolledFrame(ScrollReflowState*, bool, bool, nsHTMLReflowMetrics*, bool) layout/generic/nsContainerFrame.cpp:976:3
    #24 0x7f3f5cf510d9 in nsHTMLScrollFrame::ReflowContents(ScrollReflowState*, nsHTMLReflowMetrics const&) layout/generic/nsGfxScrollFrame.cpp:631:3
    #25 0x7f3f5cf535bb in nsHTMLScrollFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsGfxScrollFrame.cpp:866:3
    #26 0x7f3f5ceb9b54 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) layout/generic/nsContainerFrame.cpp:1018:3
    #27 0x7f3f5d0ba61c in ViewportFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsViewportFrame.cpp:217:7
    #28 0x7f3f5cda41df in PresShell::DoReflow(nsIFrame*, bool) layout/base/nsPresShell.cpp:9030:3
    #29 0x7f3f5cdb985f in PresShell::ProcessReflowCommands(bool) layout/base/nsPresShell.cpp:9189:24
    #30 0x7f3f5cdb89b5 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) layout/base/nsPresShell.cpp:4092:11
    #31 0x7f3f5caf9920 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:1731:11
    #32 0x7f3f5cb0465a in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:195:5
    #33 0x7f3f5cb064e4 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:437:5
    #34 0x7f3f5d3e9564 in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) layout/ipc/VsyncChild.cpp:63:5
    #35 0x7f3f578c6e5b in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) objdir-ff-asan/ipc/ipdl/PVsyncChild.cpp:220:20
    #36 0x7f3f5741d6f7 in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) objdir-ff-asan/ipc/ipdl/PBackgroundChild.cpp:1236:16
    #37 0x7f3f573a118b in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) ipc/glue/MessageChannel.cpp:1279:14
    #38 0x7f3f5739e64a in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&) ipc/glue/MessageChannel.cpp:1198:9
    #39 0x7f3f573911d0 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne() ipc/glue/MessageChannel.cpp:1182:9
    #40 0x7f3f57333e3d in MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) ipc/chromium/src/base/message_loop.cc:364:3
    #41 0x7f3f57334b0a in MessageLoop::DoWork() ipc/chromium/src/base/message_loop.cc:459:13
    #42 0x7f3f573a8792 in mozilla::ipc::DoWorkRunnable::Run() ipc/glue/MessagePump.cpp:220:3
    #43 0x7f3f56a80a06 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:848:7
    #44 0x7f3f56af964c in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:265:10
    #45 0x7f3f573a7f3e in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:95:21
    #46 0x7f3f57332aa1 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:234:3
    #47 0x7f3f5c45154f in nsBaseAppShell::Run() widget/nsBaseAppShell.cpp:165:3
    #48 0x7f3f5e394943 in XRE_RunAppShell toolkit/xre/nsEmbedFunctions.cpp:778:12
    #49 0x7f3f57332aa1 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:234:3
    #50 0x7f3f5e393e67 in XRE_InitChildProcess toolkit/xre/nsEmbedFunctions.cpp:614:7
    #51 0x4dbbb2 in content_process_main(int, char**) ipc/contentproc/plugin-container.cpp:236:19
    #52 0x7f3f53e63ec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287
0x608000000028 is located 8 bytes inside of 96-byte region [0x608000000020,0x608000000080)
freed by thread T0 (Web Content) here:
    #0 0x4b6690 in realloc _asan_rtl_
    #1 0x7f3f56949aa9 in nsACString_internal::MutatePrep(unsigned int, char**, unsigned int*) xpcom/string/nsSubstring.cpp:246:27
    #2 0x7f3f56953847 in nsACString_internal::ReplacePrepInternal(unsigned int, unsigned int, unsigned int, unsigned int) xpcom/string/nsTSubstring.cpp:169:8
    #3 0x7f3f56955fc0 in nsACString_internal::Replace(unsigned int, unsigned int, char) xpcom/string/nsTSubstring.h:1010:12
    #4 0x7f3f569e70cf in nsLocalFile::AppendRelativeNativePath(nsACString_internal const&) objdir-ff-asan/dist/include/nsTSubstring.h:524:5
    #5 0x7f3f56abe7b8 in mozilla::FileLocation::FileLocation(mozilla::FileLocation const&, char const*) xpcom/build/FileLocation.cpp:69:7
    #6 0x7f3f56a5260a in nsComponentManagerImpl::ManifestBinaryComponent(nsComponentManagerImpl::ManifestProcessingContext&, int, char* const*) xpcom/components/nsComponentManager.cpp:675:16
    #7 0x7f3f56a6132c in ParseManifest(NSLocationType, mozilla::FileLocation&, char*, bool, bool) xpcom/components/ManifestParser.cpp:807:9
    #8 0x7f3f56a520e6 in nsComponentManagerImpl::RegisterManifest(NSLocationType, mozilla::FileLocation&, bool) xpcom/components/nsComponentManager.cpp:638:5
    #9 0x7f3f56a52463 in nsComponentManagerImpl::ManifestManifest(nsComponentManagerImpl::ManifestProcessingContext&, int, char* const*) xpcom/components/nsComponentManager.cpp:660:3
    #10 0x7f3f56a6132c in ParseManifest(NSLocationType, mozilla::FileLocation&, char*, bool, bool) xpcom/components/ManifestParser.cpp:807:9
    #11 0x7f3f56a520e6 in nsComponentManagerImpl::RegisterManifest(NSLocationType, mozilla::FileLocation&, bool) xpcom/components/nsComponentManager.cpp:638:5
    #12 0x7f3f56a4fbe4 in nsComponentManagerImpl::Init() xpcom/components/nsComponentManager.cpp:846:5
    #13 0x7f3f56acef3e in NS_InitXPCOM2 xpcom/build/XPCOMInit.cpp:698:8
    #14 0x7f3f5e393388 in XRE_InitEmbedding2 toolkit/xre/nsEmbedFunctions.cpp:172:8
    #15 0x7f3f573ab366 in mozilla::ipc::ScopedXREEmbed::Start() ipc/glue/ScopedXREEmbed.cpp:104:10
    #16 0x7f3f5be3dc0e in mozilla::dom::ContentProcess::Init() dom/ipc/ContentProcess.cpp:81:5
    #17 0x7f3f5e393e5b in XRE_InitChildProcess toolkit/xre/nsEmbedFunctions.cpp:601:12
    #18 0x4dbbb2 in content_process_main(int, char**) ipc/contentproc/plugin-container.cpp:236:19
    #19 0x7f3f53e63ec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287

previously allocated by thread T0 (Web Content) here:
    #0 0x4b62f8 in __interceptor_malloc _asan_rtl_
    #1 0x7f3f569499ba in nsACString_internal::MutatePrep(unsigned int, char**, unsigned int*) xpcom/string/nsSubstring.cpp:217:22
    #2 0x7f3f56953847 in nsACString_internal::ReplacePrepInternal(unsigned int, unsigned int, unsigned int, unsigned int) xpcom/string/nsTSubstring.cpp:169:8
    #3 0x7f3f569542ba in nsACString_internal::Assign(char const*, unsigned int, mozilla::fallible_t const&) xpcom/string/nsTSubstring.h:1010:12
    #4 0x7f3f569548a9 in nsACString_internal::Assign(nsACString_internal const&, mozilla::fallible_t const&) xpcom/string/nsTSubstring.cpp:408:10
    #5 0x7f3f56936a24 in nsACString_internal::Assign(nsACString_internal const&) xpcom/string/nsTSubstring.cpp:364:8
    #6 0x7f3f569e5f99 in nsLocalFile::InitWithNativePath(nsACString_internal const&) objdir-ff-asan/dist/include/nsTString.h:98:5
    #7 0x7f3f569f0254 in NS_NewNativeLocalFile xpcom/io/nsLocalFileUnix.cpp:2070:19
    #8 0x7f3f569e8346 in nsLocalFile::GetParent(nsIFile**) xpcom/io/nsLocalFileUnix.cpp:1473:17
    #9 0x7f3f56abe6f6 in mozilla::FileLocation::FileLocation(mozilla::FileLocation const&, char const*) xpcom/build/FileLocation.cpp:56:7
    #10 0x7f3f56a5260a in nsComponentManagerImpl::ManifestBinaryComponent(nsComponentManagerImpl::ManifestProcessingContext&, int, char* const*) xpcom/components/nsComponentManager.cpp:675:16
    #11 0x7f3f56a6132c in ParseManifest(NSLocationType, mozilla::FileLocation&, char*, bool, bool) xpcom/components/ManifestParser.cpp:807:9
    #12 0x7f3f56a520e6 in nsComponentManagerImpl::RegisterManifest(NSLocationType, mozilla::FileLocation&, bool) xpcom/components/nsComponentManager.cpp:638:5
    #13 0x7f3f56a52463 in nsComponentManagerImpl::ManifestManifest(nsComponentManagerImpl::ManifestProcessingContext&, int, char* const*) xpcom/components/nsComponentManager.cpp:660:3
    #14 0x7f3f56a6132c in ParseManifest(NSLocationType, mozilla::FileLocation&, char*, bool, bool) xpcom/components/ManifestParser.cpp:807:9
    #15 0x7f3f56a520e6 in nsComponentManagerImpl::RegisterManifest(NSLocationType, mozilla::FileLocation&, bool) xpcom/components/nsComponentManager.cpp:638:5
    #16 0x7f3f56a4fbe4 in nsComponentManagerImpl::Init() xpcom/components/nsComponentManager.cpp:846:5
    #17 0x7f3f56acef3e in NS_InitXPCOM2 xpcom/build/XPCOMInit.cpp:698:8
    #18 0x7f3f5e393388 in XRE_InitEmbedding2 toolkit/xre/nsEmbedFunctions.cpp:172:8
    #19 0x7f3f573ab366 in mozilla::ipc::ScopedXREEmbed::Start() ipc/glue/ScopedXREEmbed.cpp:104:10
    #20 0x7f3f5be3dc0e in mozilla::dom::ContentProcess::Init() dom/ipc/ContentProcess.cpp:81:5
    #21 0x7f3f5e393e5b in XRE_InitChildProcess toolkit/xre/nsEmbedFunctions.cpp:601:12
    #22 0x4dbbb2 in content_process_main(int, char**) ipc/contentproc/plugin-container.cpp:236:19
    #23 0x7f3f53e63ec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287

Shadow bytes around the buggy address:
  0x0c107fff7fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c107fff7fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c107fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c107fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c107fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c107fff8000: fa fa fa fa fd[fd]fd fd fd fd fd fd fd fd fd fd
  0x0c107fff8010: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fa
  0x0c107fff8020: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c107fff8030: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fa
  0x0c107fff8040: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 01
  0x0c107fff8050: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 01 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 right 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
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==7493==ABORTING

Comment 1

[Andrew McCreight [:mccr8]]

The UAF is in layout, but the free is in some string code, which is weird.

Comment 2

[Daniel Holbert [:dholbert]]

I can't reproduce, using the following (latest) ASAN build:
https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1436263326/firefox-42.0a1.en-US.linux-x86_64-asan.tar.bz2

Abhishek, can you reproduce reliably? (and can you reproduce reliably with an ASAN build from FTP?)

Comment 3

[Daniel Holbert [:dholbert]]

(I tried loading the bugzilla-hosted testcase as well as a local copy, over and over. Not hitting any crashes or ASAN issues. My OS is 64-bit Ubuntu 15.04.)

Comment 4

[Andrew McCreight [:mccr8]]

The use and free are so unrelated it seems more like a wild pointer issue.

Comment 5

[Abhishek Arya]

I can easily reproduce this in tip-of-tree trunk. I think you are viewing this in a full screen window, just shrink your window to something so that the first line wraps and then it easily crashes.

Screen { availWidth: 1338, availHeight: 819, width: 1403, height: 843, colorDepth: 24, pixelDepth: 24, top: 0, left: 0, availTop: 24, availLeft: 65 }

Regarding UAF, it is not, looks like wildptr. Sometimes crashes with heap-buffer-overflow.
==34547==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x625000000028 at pc 0x7fce1fc456aa bp 0x7ffde2e8e460 sp 0x7ffde2e8e458
READ of size 1 at 0x625000000028 thread T0 (Web Content)

Comment 6

[Daniel Holbert [:dholbert]]

Thanks, I can reproduce with a smaller window size (small enough to trigger text-wrapping).

Though, so far I've only crashes with:
==8000==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (reduced) - WARNING, crashes firefox nightly/aurora on load

Here's a reduced testcase. Unlike the first testcase, this one crashes Nightly regardless of window size. Here's the backtrace from a non-ASAN debug build:

#0  0x00007f8b68c6f37e in nsConditionalResetStyleData::SetStyleData (
    this=0x7f8b4d32a958, aSID=eStyleStruct_Margin, 
    aStyleStruct=0x7f8b4d32aba0)
    at /scratch/work/builds/mozilla-inbound/mozilla/layout/style/nsRuleNode.h:178
#1  0x00007f8b68c30dfd in nsRuleNode::ComputeMarginData (this=0x7f8b4d32a860, 
    aStartStruct=0x7f8b4d2d7ba8, aRuleData=0x7ffc99507270, 
    aContext=0x7f8b4d2d6940, aHighestNode=0x7f8b4d32a860, 
    aRuleDetail=nsRuleNode::eRulePartialReset, aConditions=...)
    at /scratch/work/builds/mozilla-inbound/mozilla/layout/style/nsRuleNode.cpp:6623
#2  0x00007f8b68c1df3c in nsRuleNode::WalkRuleTree (this=0x7f8b4d32a860, 
    aSID=eStyleStruct_Margin, aContext=0x7f8b4d2d6940)
    at /scratch/work/builds/mozilla-inbound/mozilla/layout/style/nsRuleNode.cpp:2397
#3  0x00007f8b68c1628b in nsRuleNode::GetStyleMargin<true> (
    this=0x7f8b4d32a860, aContext=0x7f8b4d2d6940) at ./nsStyleStructList.h:117
#4  0x00007f8b68c161c6 in nsStyleContext::DoGetStyleMargin<true> (
    this=0x7f8b4d2d6940) at ./nsStyleStructList.h:117
#5  0x00007f8b68c16145 in nsStyleContext::StyleMargin (this=0x7f8b4d2d6940)
    at ./nsStyleStructList.h:117
#6  0x00007f8b68c47faa in nsStyleContext::CalcStyleDifference (
    this=0x7f8b4d32a8a8, aOther=0x7f8b4d2d6940, 
    aParentHintsNotHandledForDescendants=(unknown: 0), 
    aEqualStructs=0x7ffc99507a74)
    at /scratch/work/builds/mozilla-inbound/mozilla/layout/style/nsStyleContext.cpp:908
#7  0x00007f8b68ccbe87 in mozilla::RestyleManager::ReparentStyleContext (
    this=0x7f8b4cff1190, aFrame=0x7f8b4d2d75b0)
    at /scratch/work/builds/mozilla-inbound/mozilla/layout/base/RestyleManager.cpp:2351
#8  0x00007f8b68ee9fb2 in ReparentChildListStyle (aPresContext=
    0x7f8b4cc66800, aFrames=..., aParentFrame=0x7f8b4d2d7770)
    at /scratch/work/builds/mozilla-inbound/mozilla/layout/generic/nsInlineFrame.cpp:364
#9  0x00007f8b68eebcf8 in nsFirstLineFrame::Reflow (this=0x7f8b4d2d7770, 
    aPresContext=0x7f8b4cc66800, aMetrics=..., aReflowState=..., 
    aStatus=@0x7ffc99508224: 32651)
[...]


m-c regression range (for both testcases' crashes):
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=be81b8d6fae9&tochange=4cdc1a95a672

Looks like a regression from bug 804975.

Comment 8

[Daniel Holbert [:dholbert]]

[Tracking Requested - why for this release]: new crash in Firefox 41, with potential security implications per comment 0.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: backtrace of crash with testcase 2 (non-ASAN debug build)

Comment 10

[Cameron McCormack (:heycam)]

Thanks Abhishek for the bug report (as always), and Daniel for bisecting and the reduced test case.  I'll look at this tomorrow.

Comment 11

[Cameron McCormack (:heycam)]

In bug 804975 we allow reset style structs to be cached on an nsRuleNode conditionally, based on font-size and/or writing mode dependencies.  So say you have a document like:

  <style>
    body { font-size: 24px; }
    div { margin-right: 1em; }
  </style>
  <div></div>

Before bug 804975, during nsRuleNode::ComputeMarginData, we would encounter the 1em value and set canStoreInRuleTree to false, since caching the nsStyleMargin on the rule node wouldn't be valid if we had another div in the document that inherited a different font-size.  But now, we can cache that struct, along with an annotation that says it's only valid for style contexts where font-size is 24px.

We record whether it is safe to conditionally cache a struct on a rule node in the same places where we previously would have set canStoreInRuleTree to false; now, we have a RuleNodeCacheConditions object that we pass around during nsRuleNode::Compute*Data.  This all happens after we've walked the rule tree and filled in nsRuleData with the highest-specificity values for each property in the struct we're computing.

In the reduced test case, we have a div which has a ::first-line rule and we also have some transitions running.  At some point we have a frame tree that looks like this:

  Block (for the div)
    Line ::first-line
      Text "\n    a"
      BR
      Text "\n    b"
      BR
      (a couple more white space Text frames)

and then we set about reflowing the div, which results in frames being moved from the ::first-line frame into an nsFirstLineFrame that contains the non-first-line content.  That is done in nsFirstLineFrame::Reflow, which calls RestyleManager::ReparentStyleContext for each of the frames that is moving.  The frame where we run into trouble is the first BR frame.  Here is the BR frame's style context, and its parent and grandparent, before the ReparentStyleContext:

2aaadb6fe0c8(5) parent=2aaadb6640d0 {
  [anim values] { margin-right: 10px; font-size: 10px; }
  .class1 { list-style-position: inside;} /* file:///tmp/crash.html:24 */
  * { font-size: 20px;} /* file:///tmp/crash.html:25 */
  * { font-size: 10px; transition-duration: 5s; margin-right: 1em;} /* file:///tmp/crash.html:5 */
  ..., div, ... { display: block;} /* resource://gre-resources
  ..., div, ... { unicode-bidi: -moz-isolate;} /* resource://gre-resources/html.css:33 */
}
  2aaadb67c020(4) parent=2aaadb6fe0c8 :first-line {
    .class1::first-line { } /* file:///tmp/crash.html:10 */
    [empty style rule] {}
  }
    2aaadb6fe6b8(1) parent=2aaadb67c020 {
      [anim values] { margin-right: 10px; font-size: 10px; }
      * { font-size: 20px;} /* file:///tmp/crash.html:25 */
      * { font-size: 10px; transition-duration: 5s; margin-right: 1em;} /* file:///tmp/crash.html:5 */
    }

So the transition is running and has set margin-right to 10px.  Just looking at the rules, it seems like the nsStyleMargin should be cached unconditionally on the rule node, since the margin-right:10px will win over the margin-right:1em.  However, this is a frame that is a child of a ::first-line, and transitions don't apply inside ::first-lines.  That's enforced by AnimValuesStyleRule::MapRuleInfoInto, which skips copying its values into the nsRuleData if the parent style context HasPseudoElementData().  Thus, we end up copying the 1em into the nsRuleData for margin-right, finding that em-united value under nsRuleNode::ComputeMarginData, and caching the struct conditionally on the rule node based on its dependent 20px font-size.

Now, what happens in the ReparentStyleContext, to account for the BR frame having moved out of the ::first-line frame and into the rest-of-the-lines frame, is that we produce a new style context whose parent is 0x2aaadb6fe0c8, i.e. the style context for the div.  But this new style context has the same rule node as the old style context.  This makes sense, as the br element can't have changed which sequence of style rules it matched.

In ReparentStyleContext, we will look up the Margin struct for the new style context:

  https://hg.mozilla.org/mozilla-central/file/f34a7120f46b/layout/base/RestyleManager.cpp#l2346

The rule node already has the conditionally cached Margin struct, but we don't use it, instead computing a new one.  And this is because the style context is now not a child of a ::first-line, and thus the animation values are used, and thus the style context's font-size value is different from the one we cached (it's 10px rather than 20px).  It's correct that we don't use the conditionally cached struct, but what we then try to do is unconditionally cache the newly computed Margin struct.  We think it's OK to store unconditionally because we have the 10px margin-right value from the transition and don't encounter any em-united values.

We then hit the assertion that checks we don't try to store both conditional and unconditional structs on the rule node.  We then proceed to overwrite the Entry* value for the conditional structs with the nsStyleMargin* value for the unconditional struct, but our mConditionalBits looks like we still have an Entry* there, and later we'll poke at the nsStyleMargin thinking it's an Entry.

So ultimately the bug is that the set of specified property values you can get from WalkRuleTree can depend on the style context's HasPseudoElementData(), and I was assuming that a rule node unambiguously represented a single set of specified property values.  I think we should just never conditionally cache a struct on a rule node that is for an animation values rule when we're inside a pseudo-element.

Comment 12

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e53b62432b7

Comment 13

[Cameron McCormack (:heycam)]

Attachment: patch

Daniel, I'm having trouble reproducing the ASAN assertion on the original test case.  Can you check that this patch fixes that test?

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8631409 [details] [diff] [review]
patch

Well, for a start, it's commented out, but that's not really the problem.  (And "pseduo" typo.)

The bigger problem is that if we need this, we need to make it uncacheable for *either* condition, not just for one of them, since we can cross conditions in either direction to get incorrect data.

I'm still a little stuck on what to suggest to fix that, though.

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #13)
> Daniel, I'm having trouble reproducing the ASAN assertion on the original
> test case.  Can you check that this patch fixes that test?

(I did, too, at first -- did you try with a window that's small enough to trigger wrapping, per comment 5?)

Comment 16

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (busy until July 20) from comment #14)
> The bigger problem is that if we need this, we need to make it uncacheable
> for *either* condition, not just for one of them, since we can cross
> conditions in either direction to get incorrect data.

Hmm, yes I think you're right, although that situation shouldn't result in the memory safety errors that the test here exhibits, as we'll look up and return the unconditionally cached struct.

> I'm still a little stuck on what to suggest to fix that, though.

What about allocating two bits on RuleNodeCacheConditions to represent "dependent on the style context's HasPseudoElementData() value" and its actual value, and set that in AnimValuesStyleRule::MapRuleInfoInto?

Comment 17

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #16)
> What about allocating two bits on RuleNodeCacheConditions to represent
> "dependent on the style context's HasPseudoElementData() value" and its
> actual value, and set that in AnimValuesStyleRule::MapRuleInfoInto?

"Dependent on the parent style context's HasPseudoElementData() value", rather.

Comment 18

[Cameron McCormack (:heycam)]

Attachment: patch v2

Like this.  I haven't tried writing a test for the non-pseudo -> pseudo condition switch yet.

Comment 19

[Cameron McCormack (:heycam)]

+  //   bit 3:      do we have a "parent has pseudo-element data" flag value?
+  //   bits 5-7:   unused

I've fixed my mis-numbered comments locally.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8631432 [details] [diff] [review]
patch v2

I guess so.  This makes us fall into conditions in a bunch of cases where we didn't before, but I guess it's better than the alternative.

Comment 21

[Cameron McCormack (:heycam)]

Another option would be to have nsRuleNode::GetStyle#name_ check whether its mRule is an AnimStyleValuesRule and call aContext->HasPseudoElementData(), and if both are true, just skip looking up a cached struct.  Plus do what my v1 patch did.  This would let non-pseudo-targetting animations still have unconditional struct caching.  Let me know if you'd prefer that.

Comment 22

[Cameron McCormack (:heycam)]

Unfortunately the v2 patch reveals another problem (or is defective itself).  I get:

Assertion failure: aRuleData->mVariables (shouldn't be in ComputeVariablesData if there were no variable declarations specified), at /z/moz2/b/layout/style/nsRuleNode.cpp:9303

when hovering over the address bar now.

Comment 23

[Cameron McCormack (:heycam)]

This is because here:

  https://hg.mozilla.org/mozilla-central/file/f34a7120f46b/layout/style/nsRuleNode.cpp#l2326

we now force recomputation of the Variables struct even though we got detail == eRuleNone.  Bug 1176794 would fix that, I guess.  In the meantime I'm inclined to do comment 21, actually.  I had thought the only overhead would be the setting and getting of conditional structs on the rule node, but since we're computing a bunch more structs when we're animating I'm thinking the cost might be too high.

(Yet another alternative to solving this bug could be to have distinct rule nodes for animations inside and outside of pseudo-elements.  Then we could keep the current unconditional struct caching.)

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Cameron McCormack (:heycam) from comment #23)
> (Yet another alternative to solving this bug could be to have distinct rule
> nodes for animations inside and outside of pseudo-elements.  Then we could
> keep the current unconditional struct caching.)

I think that would be the way that fits better with the architecture we have; it's not immediately clear to me right now how hard it would be.

Comment 25

[Cameron McCormack (:heycam)]

Attachment: patch v3 WIP

(In reply to David Baron [:dbaron] ⏰UTC+2 (busy until July 20) from comment #24)
> I think that would be the way that fits better with the architecture we
> have; it's not immediately clear to me right now how hard it would be.

I tried doing this (see the attachment).  One problem that I'm not sure how best to solve is that when reflowing, we get frames that move between the first line and non-first line.  We call RestyleManager::ReparentStyleContext on those frames (in nsInlineFrame.cpp), but this keeps the same rule node, which is not what we want, as we'll want to include or not include the animation rules depending on which way we're moving.  Maybe we could add a method analogous to RestyleManager::ReparentStyleContext that does ResolveStyleWithReplacement calls instead?

(In my comment 21 suggestion I obviously wasn't considering that animation rules might not be the rule pointed to by the rule node.)

Comment 26

[Cameron McCormack (:heycam)]

Attachment: patch v4 (following comment 21)

And here is what a solution using the approach in comment 21 would look like.

My feeling is that we should do this to get the bug fixed and then later work out how to get the v3 patch approach working.  Do you agree David?

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Cameron McCormack (:heycam) from comment #25)
> I tried doing this (see the attachment).  One problem that I'm not sure how
> best to solve is that when reflowing, we get frames that move between the
> first line and non-first line.  We call RestyleManager::ReparentStyleContext
> on those frames (in nsInlineFrame.cpp), but this keeps the same rule node,
> which is not what we want, as we'll want to include or not include the
> animation rules depending on which way we're moving.  Maybe we could add a
> method analogous to RestyleManager::ReparentStyleContext that does
> ResolveStyleWithReplacement calls instead?

I don't see how the other patches avoid this problem.  How do they?

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Cameron McCormack (:heycam) from comment #26)
> Created attachment 8632620 [details] [diff] [review]
> patch v4 (following comment 21)
> 
> And here is what a solution using the approach in comment 21 would look like.
> 
> My feeling is that we should do this to get the bug fixed and then later
> work out how to get the v3 patch approach working.  Do you agree David?

Hmmm.  I'd like to know the answer to the question in comment 27, at least.

Another thought on this is that the additional virtual method on nsIStyleRule and parameter to the nsRuleNode constructor and Transition method seems like a lot of overhead.  Given that this is hopefully temporary, I think it would be better to add the bit to the rule node in CommonAnimationManager::RulesMatching (two versions of it!) right after the call to aData->mRuleWalker->Forward(rule), to the rule node aData->mRuleWalker->CurrentNode().

Otherwise, I'm a bit concerned about modifying GetStyle##name_ given how performance-sensitive it is.  (It also seems like an annoying piece of complexity there.)  But I guess I'm ok with it temporarily if it doesn't show up as a performance regression.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] ⏰UTC+2 (busy until July 20) from comment #28)
> Hmmm.  I'd like to know the answer to the question in comment 27, at least.

Er, but I guess I do understand for at least the most recent patch -- which is by changing GetStyle##name_ itself.

One other thought, though, which is that I'd think you'd need to change nsRuleNode::GetStyleData to match nsRuleNode::GetStyle##name_.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

comment 27 still applies to patch 2, though

Comment 31

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] ⏰UTC+2 (busy until July 20) from comment #28)
> Another thought on this is that the additional virtual method on
> nsIStyleRule and parameter to the nsRuleNode constructor and Transition
> method seems like a lot of overhead.  Given that this is hopefully
> temporary, I think it would be better to add the bit to the rule node in
> CommonAnimationManager::RulesMatching (two versions of it!) right after the
> call to aData->mRuleWalker->Forward(rule), to the rule node
> aData->mRuleWalker->CurrentNode().

That's much better.

> Otherwise, I'm a bit concerned about modifying GetStyle##name_ given how
> performance-sensitive it is.  (It also seems like an annoying piece of
> complexity there.)  But I guess I'm ok with it temporarily if it doesn't
> show up as a performance regression.

I agree that making GetStyle##name_ more complex isn't great.

(In reply to David Baron [:dbaron] ⏰UTC+2 (busy until July 20) from comment #30)
> comment 27 still applies to patch 2, though

Patch v2 doesn't create distinct animated and non-animated rule nodes, so it is fine to continue using ReparentStyleContext.  The conditions will be used when looking up cached structs to ensure we get the right style data.

(In reply to David Baron [:dbaron] ⏰UTC+2 (busy until July 20) from comment #29)
> One other thought, though, which is that I'd think you'd need to change
> nsRuleNode::GetStyleData to match nsRuleNode::GetStyle##name_.

Yes, good catch.

Comment 32

[Cameron McCormack (:heycam)]

Attachment: patch v4.1 (following comment 21)

I unfortunately had to store two bits on the rule node (spilling one of them into mNoneBits), since in RuleNodeWithReplacement we need to know whether the rule on a given node is an animation rule, and not just whether we had an animation rule as an ancestor.  This is because we might end up removing an existing animation rule and we don't want to keep the bits set on the new rule nodes corresponding to the old animation rule's descendants.

I looked at the callers of ResolveStyleForRules and ResolveStyleByAddingRules and I don't think any of them need SetIsAnimationRule calls.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47b95d57eb65

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8633299 [details] [diff] [review]
patch v4.1 (following comment 21)

NS_RULE_NODE_IS_ANIMATION_RULE duplicates the level information that we already have, but I guess if this is (hopefully) temporary that's not a big deal, and we could always clean it up later.

r=dbaron

Comment 34

[Cameron McCormack (:heycam)]

Comment on attachment 8633299 [details] [diff] [review]
patch v4.1 (following comment 21)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily.

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?

41

If not all supported branches, which bug introduced the flaw?

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

The patch applies cleanly to 41.

How likely is this patch to cause regressions; how much testing does it need?

Not that likely.  But I've only locally tested that the patch fixes the bug, and done a try run.

Comment 35

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

Sec-approval+ for trunk. Please nominate for Aurora and I'll approve it there too.

Comment 36

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/443625267e00

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/443625267e00

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/13c68fd25c0c

Comment 40

[Cornel Ionce [:noni] [Hubs QA]]

Reproduced the crash on Windows 10 64-bit using the attached test cases with 2015-07-08 Aurora, build ID: 20150708004005.

Verified fixed using:
- Latest 42.0a2 Aurora, build ID: 20150907004030
- Firefox 41 beta 7, build ID: 20150903133607.