Closed Bug 1181011 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla42
Tracking Status
firefox39 --- unaffected
firefox40 --- unaffected
firefox41 + verified
firefox42 + verified
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.2r --- unaffected
b2g-master --- fixed

People

(Reporter: inferno, Assigned: heycam)

References

Details

(5 keywords, Whiteboard: [b2g-adv-main2.5-])

Attachments

(6 files, 2 obsolete files)

Attached file 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
The UAF is in layout, but the free is in some string code, which is weird.
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?)
Flags: needinfo?(inferno)
(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.)
The use and free are so unrelated it seems more like a wild pointer issue.
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)
Flags: needinfo?(inferno)
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
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.
Flags: needinfo?(cam)
[Tracking Requested - why for this release]: new crash in Firefox 41, with potential security implications per comment 0.
Thanks Abhishek for the bug report (as always), and Daniel for bisecting and the reduced test case.  I'll look at this tomorrow.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Flags: needinfo?(cam)
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.
Attached patch patch (obsolete) — Splinter Review
Daniel, I'm having trouble reproducing the ASAN assertion on the original test case.  Can you check that this patch fixes that test?
Attachment #8631409 - Flags: review?(dbaron)
Attachment #8631409 - Flags: feedback?(dholbert)
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.
Attachment #8631409 - Flags: review?(dbaron) → review-
(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?)
(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?
(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.
Attached patch patch v2Splinter Review
Like this.  I haven't tried writing a test for the non-pseudo -> pseudo condition switch yet.
Attachment #8631409 - Attachment is obsolete: true
Attachment #8631409 - Flags: feedback?(dholbert)
Attachment #8631432 - Flags: review?(dbaron)
+  //   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 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.
Attachment #8631432 - Flags: review?(dbaron) → review+
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.
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.
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.)
(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.
Attached patch patch v3 WIPSplinter Review
(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.)
Attached patch patch v4 (following comment 21) (obsolete) — Splinter Review
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?
Flags: needinfo?(dbaron)
(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?
Flags: needinfo?(cam)
(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.
Flags: needinfo?(dbaron)
(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 27 still applies to patch 2, though
(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.
Flags: needinfo?(cam)
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
Attachment #8632620 - Attachment is obsolete: true
Attachment #8633299 - Flags: review?(dbaron)
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
Attachment #8633299 - Flags: review?(dbaron) → review+
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.
Attachment #8633299 - Flags: sec-approval?
Sec-approval+ for trunk. Please nominate for Aurora and I'll approve it there too.
Attachment #8633299 - Flags: sec-approval? → sec-approval+
Attachment #8633299 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/443625267e00
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Attachment #8633299 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1186508
Group: core-security → core-security-release
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
QA Contact: cornel.ionce
Group: core-security-release
Whiteboard: [b2g-adv-main2.5-]
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Depends on: CVE-2016-5274
You need to log in before you can comment on or make changes to this bug.