Last Comment Bug 850931 - (CVE-2013-1680) Heap-use-after-free in nsFrameList::FirstChild
(CVE-2013-1680)
: Heap-use-after-free in nsFrameList::FirstChild
Status: RESOLVED FIXED
[asan][adv-main21+][adv-esr1706+]
: crash, csectype-uaf, sec-critical, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla22
Assigned To: Mats Palmgren (:mats)
: Matt Wobensmith [:mwobensmith][:matt:]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-13 17:15 PDT by Abhishek Arya
Modified: 2014-07-24 16:08 PDT (History)
9 users (show)
dveditz: sec‑bounty+
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
-
wontfix
+
verified
+
verified
21+
verified
21+
fixed
wontfix
affected


Attachments
Testcase (1.28 KB, text/html)
2013-03-13 17:15 PDT, Abhishek Arya
no flags Details
stack + frame tree (33.62 KB, text/html)
2013-03-17 11:27 PDT, Mats Palmgren (:mats)
no flags Details
stack + frame dump for the assertion (71.38 KB, text/html)
2013-03-17 11:56 PDT, Mats Palmgren (:mats)
no flags Details
stack + frame dump when the out-of-order overflow list occurs (16.88 KB, text/html)
2013-03-17 18:00 PDT, Mats Palmgren (:mats)
no flags Details
stack + frame dump for "ASSERTION: overflow containers out of order" (58.54 KB, text/html)
2013-03-18 07:01 PDT, Mats Palmgren (:mats)
no flags Details
stack #2 + frame dump for "ASSERTION: overflow containers out of order" (14.83 KB, text/html)
2013-03-18 07:24 PDT, Mats Palmgren (:mats)
no flags Details
fix (4.46 KB, patch)
2013-03-18 13:52 PDT, Mats Palmgren (:mats)
roc: review+
fantasai.bugs: feedback-
bajaj.bhavana: approval‑mozilla‑aurora+
bajaj.bhavana: approval‑mozilla‑esr17+
bajaj.bhavana: approval‑mozilla‑b2g18+
abillings: sec‑approval+
Details | Diff | Review

Description Abhishek Arya 2013-03-13 17:15:32 PDT
Created attachment 724696 [details]
Testcase

>==16813== ERROR: AddressSanitizer: heap-use-after-free on address 0x6004001e15b0 at pc 0x7f805347f8ae bp 0x7fffc90887b0 sp 0x7fffc90887a8
>READ of size 8 at 0x6004001e15b0 thread T0
>    #0 0x7f805347f8ad in nsFrameList::FirstChild() const src/../../../dist/include/nsFrameList.h:229
>    #1 0x7f8053cbba5e in nsOverflowContinuationTracker::Finish(nsIFrame*) src/layout/generic/nsContainerFrame.cpp:1745
>    #2 0x7f8053c61fec in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) src/layout/generic/nsBlockReflowContext.cpp:304
>    #3 0x7f8053c0aba2 in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) src/layout/generic/nsBlockFrame.cpp:3090
>    #4 0x7f8053c004b7 in nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, bool*) src/layout/generic/nsBlockFrame.cpp:2501
>    #5 0x7f8053be9ce6 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) src/layout/generic/nsBlockFrame.cpp:2023
>    #6 0x7f8053bdd3fa in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsBlockFrame.cpp:1067
>    #7 0x7f8053cbb198 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:968
>    #8 0x7f8053c9ee3d in nsColumnSetFrame::ReflowChildren(nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&, nsColumnSetFrame::ReflowConfig const&, bool, nsCollapsingMargin*, nsColumnSetFrame::ColumnBalanceData&) src/layout/generic/nsColumnSetFrame.cpp:671
>    #9 0x7f8053ca613f in nsColumnSetFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsColumnSetFrame.cpp:1045
>    #10 0x7f8053cbb198 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:968
>    #11 0x7f8053cbe4da in nsContainerFrame::ReflowOverflowContainerChildren(nsPresContext*, nsHTMLReflowState const&, nsOverflowAreas&, unsigned int, unsigned int&) src/layout/generic/nsContainerFrame.cpp:1148
>    #12 0x7f8053bdced2 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsBlockFrame.cpp:1042
>    #13 0x7f8053cbb198 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:968
>    #14 0x7f8053c9ee3d in nsColumnSetFrame::ReflowChildren(nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&, nsColumnSetFrame::ReflowConfig const&, bool, nsCollapsingMargin*, nsColumnSetFrame::ColumnBalanceData&) src/layout/generic/nsColumnSetFrame.cpp:671
>    #15 0x7f8053ca46ff in nsColumnSetFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsColumnSetFrame.cpp:941
>    #16 0x7f8053cbb198 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:968
>    #17 0x7f8053e70018 in nsCanvasFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsCanvasFrame.cpp:487
>    #18 0x7f8053cbb198 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:968
>    #19 0x7f8053de6161 in nsHTMLScrollFrame::ReflowScrolledFrame(ScrollReflowState*, bool, bool, nsHTMLReflowMetrics*, bool) src/layout/generic/nsGfxScrollFrame.cpp:430
>    #20 0x7f8053de9df0 in nsHTMLScrollFrame::ReflowContents(ScrollReflowState*, nsHTMLReflowMetrics const&) src/layout/generic/nsGfxScrollFrame.cpp:530
>    #21 0x7f8053dee2b3 in nsHTMLScrollFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsGfxScrollFrame.cpp:771
>    #22 0x7f8053cbb198 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:968
>    #23 0x7f80541a0ebd in ViewportFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsViewportFrame.cpp:202
>    #24 0x7f80539480b2 in PresShell::DoReflow(nsIFrame*, bool) src/layout/base/nsPresShell.cpp:7801
>    #25 0x7f8053975911 in PresShell::ProcessReflowCommands(bool) src/layout/base/nsPresShell.cpp:7942
>    #26 0x7f80539740cd in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/nsPresShell.cpp:3932
>    #27 0x7f8053972727 in PresShell::FlushPendingNotifications(mozFlushType) src/layout/base/nsPresShell.cpp:3778
>    #28 0x7f805378449f in nsDocumentViewer::LoadComplete(tag_nsresult) src/layout/base/nsDocumentViewer.cpp:987
>    #29 0x7f805b4c91dd in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, tag_nsresult) src/docshell/base/nsDocShell.cpp:6710
>    #30 0x7f805b4c11db in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, tag_nsresult) src/docshell/base/nsDocShell.cpp:6538
>    #31 0x7f805b4c2109 in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, tag_nsresult) src/docshell/base/nsDocShell.cpp:6545
>    #32 0x7f805b5c4cdb in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, tag_nsresult) src/uriloader/base/nsDocLoader.cpp:1290
>    #33 0x7f805b5c257c in nsDocLoader::doStopDocumentLoad(nsIRequest*, tag_nsresult) src/uriloader/base/nsDocLoader.cpp:870
>    #34 0x7f805b5bb8a4 in nsDocLoader::DocLoaderIsEmpty(bool) src/uriloader/base/nsDocLoader.cpp:760
>    #35 0x7f805b5bfb5b in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) src/uriloader/base/nsDocLoader.cpp:644
>    #36 0x7f805b5c121a in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) src/uriloader/base/nsDocLoader.cpp:648
>    #37 0x7f80519d4c03 in nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, tag_nsresult) src/netwerk/base/src/nsLoadGroup.cpp:676
>    #38 0x7f80551f3ec5 in nsDocument::DoUnblockOnload() src/content/base/src/nsDocument.cpp:7881
>    #39 0x7f80551f391b in nsDocument::UnblockOnload(bool) src/content/base/src/nsDocument.cpp:7809
>    #40 0x7f805519499b in nsDocument::DispatchContentLoadedEvents() src/content/base/src/nsDocument.cpp:4443
>    #41 0x7f8055297a1a in nsRunnableMethodImpl<void (nsDocument::*)(), true>::Run() src/../../../dist/include/nsThreadUtils.h:367
>    #42 0x7f80611ff057 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:627
>    #43 0x7f8060eac572 in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:238
>    #44 0x7f805d9ca369 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82
>    #45 0x7f80614c54eb in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:216
>    #46 0x7f80614c533e in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:209
>    #47 0x7f80614c5229 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:183
>    #48 0x7f805ce4ff4e in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
>    #49 0x7f805ba8efa0 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:288
>    #50 0x7f805176d11c in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3880
>    #51 0x7f80517727c9 in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3947
>    #52 0x7f8051775285 in XRE_main src/toolkit/xre/nsAppRunner.cpp:4150
>    #53 0x427f37 in do_main(int, char**, nsIFile*) src/browser/app/nsBrowserApp.cpp:228
>    #54 0x4250f1 in main src/browser/app/nsBrowserApp.cpp:529
>    #55 0x7f807264476c in
>    #56 0x424864 in
>0x6004001e15b0 is located 0 bytes inside of 16-byte region [0x6004001e15b0,0x6004001e15c0)
>freed by thread T0 here:
>    #0 0x4186d2 in __interceptor_free
>    #1 0x7f807366077e in moz_free src/memory/mozalloc/mozalloc.cpp:48
>    #2 0x7f8053cc356d in TryRemoveFrame(nsIFrame*, mozilla::FramePropertyTable*, mozilla::FramePropertyDescriptor const*, nsIFrame*, bool (nsFrameList::*)(nsIFrame*)) src/../../dist/include/mozilla/mozalloc.h:225
>    #3 0x7f8053cc2799 in nsContainerFrame::StealFrame(nsPresContext*, nsIFrame*, bool) src/layout/generic/nsContainerFrame.cpp:1259
>    #4 0x7f8053c41d6f in nsBlockFrame::StealFrame(nsPresContext*, nsIFrame*, bool) src/layout/generic/nsBlockFrame.cpp:5616
>    #5 0x7f8053cc4e4c in nsContainerFrame::DeleteNextInFlowChild(nsPresContext*, nsIFrame*, bool) src/layout/generic/nsContainerFrame.cpp:1408
>    #6 0x7f8053c4390b in nsBlockFrame::DeleteNextInFlowChild(nsPresContext*, nsIFrame*, bool) src/layout/generic/nsBlockFrame.cpp:5701
>    #7 0x7f8053c6210a in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) src/layout/generic/nsBlockReflowContext.cpp:305
>    #8 0x7f8053c0aba2 in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) src/layout/generic/nsBlockFrame.cpp:3090
>    #9 0x7f8053c004b7 in nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, bool*) src/layout/generic/nsBlockFrame.cpp:2501
>    #10 0x7f8053be9ce6 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) src/layout/generic/nsBlockFrame.cpp:2023
>    #11 0x7f8053bdd3fa in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsBlockFrame.cpp:1067
>    #12 0x7f8053cbb198 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:968
>    #13 0x7f8053c9ee3d in nsColumnSetFrame::ReflowChildren(nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&, nsColumnSetFrame::ReflowConfig const&, bool, nsCollapsingMargin*, nsColumnSetFrame::ColumnBalanceData&) src/layout/generic/nsColumnSetFrame.cpp:671
>    #14 0x7f8053ca613f in nsColumnSetFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsColumnSetFrame.cpp:1045
>    #15 0x7f8053cbb198 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:968
>    #16 0x7f8053cbe4da in nsContainerFrame::ReflowOverflowContainerChildren(nsPresContext*, nsHTMLReflowState const&, nsOverflowAreas&, unsigned int, unsigned int&) src/layout/generic/nsContainerFrame.cpp:1148
>    #17 0x7f8053bdced2 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsBlockFrame.cpp:1042
>    #18 0x7f8053cbb198 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:968
>    #19 0x7f8053c9ee3d in nsColumnSetFrame::ReflowChildren(nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&, nsColumnSetFrame::ReflowConfig const&, bool, nsCollapsingMargin*, nsColumnSetFrame::ColumnBalanceData&) src/layout/generic/nsColumnSetFrame.cpp:671
>    #20 0x7f8053ca46ff in nsColumnSetFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsColumnSetFrame.cpp:941
>    #21 0x7f8053cbb198 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:968
>    #22 0x7f8053e70018 in nsCanvasFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsCanvasFrame.cpp:487
>    #23 0x7f8053cbb198 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:968
>    #24 0x7f8053de6161 in nsHTMLScrollFrame::ReflowScrolledFrame(ScrollReflowState*, bool, bool, nsHTMLReflowMetrics*, bool) src/layout/generic/nsGfxScrollFrame.cpp:430
>    #25 0x7f8053de9df0 in nsHTMLScrollFrame::ReflowContents(ScrollReflowState*, nsHTMLReflowMetrics const&) src/layout/generic/nsGfxScrollFrame.cpp:530
>    #26 0x7f8053dee2b3 in nsHTMLScrollFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsGfxScrollFrame.cpp:771
>    #27 0x7f8053cbb198 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:968
>    #28 0x7f80541a0ebd in ViewportFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsViewportFrame.cpp:202
>    #29 0x7f80539480b2 in PresShell::DoReflow(nsIFrame*, bool) src/layout/base/nsPresShell.cpp:7801
>previously allocated by thread T0 here:
>    #0 0x4187b2 in __interceptor_malloc
>    #1 0x7f80736608c5 in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:54
>    #2 0x7f8053cc0ae6 in nsOverflowContinuationTracker::Insert(nsIFrame*, unsigned int&) src/../../dist/include/mozilla/mozalloc.h:201
>    #3 0x7f8053cbf46e in nsContainerFrame::ReflowOverflowContainerChildren(nsPresContext*, nsHTMLReflowState const&, nsOverflowAreas&, unsigned int, unsigned int&) src/layout/generic/nsContainerFrame.cpp:1185
>    #4 0x7f8053bdced2 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsBlockFrame.cpp:1042
>    #5 0x7f8053cbb198 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:968
>    #6 0x7f8053c9ee3d in nsColumnSetFrame::ReflowChildren(nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&, nsColumnSetFrame::ReflowConfig const&, bool, nsCollapsingMargin*, nsColumnSetFrame::ColumnBalanceData&) src/layout/generic/nsColumnSetFrame.cpp:671
>    #7 0x7f8053ca46ff in nsColumnSetFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsColumnSetFrame.cpp:941
>    #8 0x7f8053cbb198 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:968
>    #9 0x7f8053cbe4da in nsContainerFrame::ReflowOverflowContainerChildren(nsPresContext*, nsHTMLReflowState const&, nsOverflowAreas&, unsigned int, unsigned int&) src/layout/generic/nsContainerFrame.cpp:1148
>    #10 0x7f8053bdced2 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsBlockFrame.cpp:1042
>    #11 0x7f8053cbb198 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:968
>    #12 0x7f8053c9ee3d in nsColumnSetFrame::ReflowChildren(nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&, nsColumnSetFrame::ReflowConfig const&, bool, nsCollapsingMargin*, nsColumnSetFrame::ColumnBalanceData&) src/layout/generic/nsColumnSetFrame.cpp:671
>    #13 0x7f8053ca613f in nsColumnSetFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsColumnSetFrame.cpp:1045
>    #14 0x7f8053cbb198 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:968
>    #15 0x7f8053e70018 in nsCanvasFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsCanvasFrame.cpp:487
>    #16 0x7f8053cbb198 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:968
>    #17 0x7f8053de6161 in nsHTMLScrollFrame::ReflowScrolledFrame(ScrollReflowState*, bool, bool, nsHTMLReflowMetrics*, bool) src/layout/generic/nsGfxScrollFrame.cpp:430
>    #18 0x7f8053de9df0 in nsHTMLScrollFrame::ReflowContents(ScrollReflowState*, nsHTMLReflowMetrics const&) src/layout/generic/nsGfxScrollFrame.cpp:530
>    #19 0x7f8053dee2b3 in nsHTMLScrollFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsGfxScrollFrame.cpp:771
>    #20 0x7f8053cbb198 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:968
>    #21 0x7f80541a0ebd in ViewportFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsViewportFrame.cpp:202
>    #22 0x7f80539480b2 in PresShell::DoReflow(nsIFrame*, bool) src/layout/base/nsPresShell.cpp:7801
>    #23 0x7f8053975911 in PresShell::ProcessReflowCommands(bool) src/layout/base/nsPresShell.cpp:7942
>    #24 0x7f80539740cd in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/nsPresShell.cpp:3932
>    #25 0x7f8053972727 in PresShell::FlushPendingNotifications(mozFlushType) src/layout/base/nsPresShell.cpp:3778
>    #26 0x7f80551df86e in nsDocument::FlushPendingNotifications(mozFlushType) src/content/base/src/nsDocument.cpp:6986
>    #27 0x7f80553978f8 in mozilla::dom::Element::GetPrimaryFrame(mozFlushType) src/content/base/src/Element.cpp:1626
>    #28 0x7f8055397507 in mozilla::dom::Element::GetStyledFrame() src/content/base/src/Element.cpp:486
>    #29 0x7f805624e20f in nsGenericHTMLElement::GetOffsetRect(nsRect&) src/content/html/content/src/nsGenericHTMLElement.cpp:397
>Shadow bytes around the buggy address:
>  0x0c0100034260: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
>  0x0c0100034270: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
>  0x0c0100034280: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
>  0x0c0100034290: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
>  0x0c01000342a0: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
>=>0x0c01000342b0: fa fa fd fd fa fa[fd]fd fa fa fd fd fa fa fd fd
>  0x0c01000342c0: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
>  0x0c01000342d0: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
>  0x0c01000342e0: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
>  0x0c01000342f0: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
>  0x0c0100034300: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
>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
>==16813== ABORTING
>
>
Comment 1 Mats Palmgren (:mats) 2013-03-17 11:27:43 PDT
Created attachment 725907 [details]
stack + frame tree

The OverflowContainersList containing the red and blue frames will be
destroyed (because it becomes empty) when the green frame calls
DeleteNextInFlowChild.

We crash because there's a nsOverflowContinuationTracker with
mOverflowContList pointing to that deleted nsFrameList.
We do call nsOverflowContinuationTracker::Finish() though, which
is supposed to take care of nulling out the pointer:
http://hg.mozilla.org/mozilla-central/annotate/c444f2bfadc4/layout/generic/nsContainerFrame.cpp#l1747

The problem is "nif->GetNextSibling() == nif->GetNextInFlow()"
doesn't catch it because the list is not ordered properly. (There's plenty
of "ASSERTION: overflow containers out of order" leading up to the crash.)
Comment 2 Mats Palmgren (:mats) 2013-03-17 11:56:04 PDT
Created attachment 725913 [details]
stack + frame dump for the assertion
Comment 3 Mats Palmgren (:mats) 2013-03-17 12:01:45 PDT
Probably exploitable since nsFrameList is allocated from the general heap.
I should go and fix bug 729519 now so that frame poisoning will protect us
from bugs like this.
Comment 4 Mats Palmgren (:mats) 2013-03-17 18:00:13 PDT
Created attachment 725956 [details]
stack + frame dump when the out-of-order overflow list occurs

The first frame dump is at the start of nsBlockFrame::ReflowBlockFrame.

While reflowing the green block, the ColumnSet child 0x7fffdfc36810 (pink)
decides it's complete, but not fully complete, so we insert its next-in-flow
(red) into the tracker.  The tracker tracks the OverflowContainersList of the
next-in-flow block (cyan) and it already contains the next-in-flow (blue)
of the red frame from a previous reflow.  mPrevOverflowCont is that nif, and
nsOverflowContinuationTracker::Insert happily inserts the red frame *after* it,
resulting in the frame tree at the end.

This looks like a perfectly valid reflow scenario to me, and something that
nsOverflowContinuationTracker::Insert must be able to deal with.
Comment 5 Mats Palmgren (:mats) 2013-03-18 07:01:01 PDT
Created attachment 726149 [details]
stack + frame dump for "ASSERTION: overflow containers out of order"

So, after fixing Insert to maintain order amongst the next-in-flows
on the same list, the next problem is an assertion:
###!!! ASSERTION: overflow containers out of order: '!(mOverflowContList && mOverflowContList->ContainsFrame(aOverflowCont))', file layout/generic/nsContainerFrame.cpp, line 1788
http://hg.mozilla.org/mozilla-central/annotate/c444f2bfadc4/layout/generic/nsContainerFrame.cpp#l1677

In this case we're in ReflowOverflowContainerChildren and the tracker list
is the ExcessOverflowContainersList of the block we're reflowing (lime).
After reflowing the pink child frame (complete but not fully complete)
we Insert its next-in-flow (red) into the tracker.  But it's already there so
it triggers the assert.  We StealFrame it though, so the "only" problem is that
we would normally insert after the blue frame which seems misplaced.

Still, what if it was the *only* frame on the excess list?  then StealFrame
would remove it and *delete* the property, leaving mOverflowContList pointing
at freed memory.
Comment 6 Mats Palmgren (:mats) 2013-03-18 07:24:46 PDT
Created attachment 726159 [details]
stack #2 + frame dump for "ASSERTION: overflow containers out of order"

For completeness, that assertion can also occur when the tracker inserts
to the next-in-flow's OverflowContainersList.
Comment 7 Mats Palmgren (:mats) 2013-03-18 13:52:59 PDT
Created attachment 726324 [details] [diff] [review]
fix

Description of changes in the order they appear:

In ReflowOverflowContainerChildren, if the frame has its own
ExcessOverflowContainersProperty, then append those frames to the list of
frames to reflow.  This avoids the problem of Inserting a next-in-flow
that is already in the tracked list.  It's rare that this occurs.
See comment 5 and its attachment.

In nsOverflowContinuationTracker::Insert, check if the frame to insert
is already in the mOverflowContList.  The ContainsFrame() call there is
unfortunately O(n).  See comment 6.  Given the first change above, it might
be safe to restrict this check to OverflowContainers insertions only.
I think we should fix bug 729519 first though, before making optimizations
that could lead to exploitable bugs.

Last hunk checks if mOverflowContList already has a prev/next-in-flow
for the inserted frame and if so adjusts the insertion point so that the
next-in-flow chain within the list remains ordered.

Fwiw, reftest/crashtests pass in a local ASan debug build on Linux64.
Comment 8 Mats Palmgren (:mats) 2013-03-18 13:58:32 PDT
Comment on attachment 726324 [details] [diff] [review]
fix

fantasai, I'd appreciate if you could take a look at this patch and
the bug comments / attachments as well.  Thanks.
Comment 10 Daniel Veditz [:dveditz] 2013-03-21 13:22:56 PDT
Is this a new bug (regression) or has it been around for a while?
Comment 11 Mats Palmgren (:mats) 2013-03-21 14:37:26 PDT
(In reply to Daniel Veditz [:dveditz] from comment #10)
> Is this a new bug (regression) or has it been around for a while?

I suspect all supported branches are affected.
Comment 12 Mats Palmgren (:mats) 2013-03-22 15:34:14 PDT
Comment on attachment 726324 [details] [diff] [review]
fix

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

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I expect the same patch should apply. If not, backporting should be easy.

How likely is this patch to cause regressions; how much testing does it need?
Medium risk.
Comment 13 Lukas Blakk [:lsblakk] use ?needinfo 2013-03-25 10:02:35 PDT
Given that this is extremely hard to create an exploit for, it's been in the product for a while (though unclear for how long) and risk of regression from taking this in our final beta (going to build today) I'd like to hold off on landing this until FF21 is in beta.
Comment 15 Mats Palmgren (:mats) 2013-03-25 14:01:02 PDT
(In reply to lsblakk@mozilla.com from comment #13)
> Given that this is extremely hard to create an exploit for

Maybe we mean the same thing, but I want to clarify that I didn't say
that the crash condition is hard to exploit (it's not).  I said it
would be extremely hard to figure out an exploit *based on the
information revealed by the patch*.

Anyway, I agree that the regression risk is too high to take this
for beta without baking.
Comment 16 Ryan VanderMeulen [:RyanVM] 2013-03-26 07:30:27 PDT
https://hg.mozilla.org/mozilla-central/rev/4992c8267847
Comment 17 Alex Keybl [:akeybl] 2013-03-27 10:12:25 PDT
(In reply to Mats Palmgren [:mats] from comment #12)
> Do you have backports for the affected branches? If not, how different, hard
> to create, and risky will they be?
> I expect the same patch should apply. If not, backporting should be easy.

ESR/B2G:21+ in that case.
Comment 18 Mats Palmgren (:mats) 2013-03-28 13:41:06 PDT
Comment on attachment 726324 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: sec-critical crash
Testing completed (on m-c, etc.): on m-c since 2013-03-26
Risk to taking this patch (and alternatives if risky): medium risk
String or IDL/UUID changes made by this patch: none
Comment 19 bhavana bajaj [:bajaj] 2013-03-28 14:52:31 PDT
(In reply to Mats Palmgren [:mats] from comment #18)
> Comment on attachment 726324 [details] [diff] [review]
> fix
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
> User impact if declined: sec-critical crash
> Testing completed (on m-c, etc.): on m-c since 2013-03-26

Did we verify with the testcase once the patch landed on m-c or we need to pull in QA here for verification  ? 

> Risk to taking this patch (and alternatives if risky): medium risk

Any obvious areas of regression to lookout for or additional testing we need qa help here based on the risk evaluation ?

> String or IDL/UUID changes made by this patch: none
Comment 20 Mats Palmgren (:mats) 2013-03-28 15:17:37 PDT
(In reply to bhavana bajaj [:bajaj] from comment #19)
> Did we verify with the testcase once the patch landed on m-c or we need to
> pull in QA here for verification  ? 

The crash was quite hard to reproduce (on Linux64).  I could only do it in an
ASan build, loading 20 or so tabs with the testcase file from the command line
then resizing the window and doing an occasional "Reload All Tabs" from the
tab context menu.  That STR should crash an unfixed ASan build in less than
a minute of trying.  I haven't tried regular builds on other platforms though
so maybe it's easier there.

> Any obvious areas of regression to lookout for or additional testing we need
> qa help here based on the risk evaluation ?

Any regression is probably going to be a crash and most likely in pagination
contexts, such as content styled with -moz-columns or Print/Print Preview.
Comment 21 bhavana bajaj [:bajaj] 2013-03-29 12:00:57 PDT
(In reply to Mats Palmgren [:mats] from comment #20)
> (In reply to bhavana bajaj [:bajaj] from comment #19)
> > Did we verify with the testcase once the patch landed on m-c or we need to
> > pull in QA here for verification  ? 
> 
> The crash was quite hard to reproduce (on Linux64).  I could only do it in an
> ASan build, loading 20 or so tabs with the testcase file from the command
> line
> then resizing the window and doing an occasional "Reload All Tabs" from the
> tab context menu.  That STR should crash an unfixed ASan build in less than
> a minute of trying.  I haven't tried regular builds on other platforms though
> so maybe it's easier there.

Adding Matt as QA contact here to see he if can help verify this on other platforms based on above 

> 
> > Any obvious areas of regression to lookout for or additional testing we need
> > qa help here based on the risk evaluation ?
> 
> Any regression is probably going to be a crash and most likely in pagination
> contexts, such as content styled with -moz-columns or Print/Print Preview.
Comment 23 Mats Palmgren (:mats) 2013-04-01 17:23:33 PDT
Comment on attachment 726324 [details] [diff] [review]
fix

See comment 12 and forward.
Comment 25 Matt Wobensmith [:mwobensmith][:matt:] 2013-04-08 11:36:58 PDT
Since we don't have ASan builds available for every date/branch/platform, I'm not able to run a 100% consistent verification. However, I can reproduce the crash in non-ASan builds from the affected period, so I feel it's safe to assume that we can verify this issue despite lack of ASan build availability.


Confirmed crash in 2013-03-13 FF21, both ASan and release
Confirmed no crash in 2013-04-01 FF20, release
Confirmed no crash in 2013-04-01 FF21, release
Confirmed no crash in 2013-04-06 FF21, ASan
Confirmed no crash in 2013-04-06 17esr, release
Comment 26 fantasai 2013-04-25 06:43:07 PDT
Comment on attachment 726324 [details] [diff] [review]
fix

Sorry for not responding earlier. I kept trying to understand the patch and failing, so I kindof gave up... trying again tonight, and I think maybe it's just wrong.

This bit --
+  // Our own excess overflow containers from a previous reflow can still be
+  // present if our next-in-flow hasn't been reflown yet.
+  nsFrameList* selfExcessOCFrames =
+    RemovePropTableFrames(aPresContext, ExcessOverflowContainersProperty());
+  if (selfExcessOCFrames) {
+    if (overflowContainers) {
+      overflowContainers->AppendFrames(nullptr, *selfExcessOCFrames);
+      delete selfExcessOCFrames;
+    } else {
+      overflowContainers = selfExcessOCFrames;
+      SetPropTableFrames(aPresContext, overflowContainers,
+                         OverflowContainersProperty());
+    }
+  }

This does not make sense. Our excessOC list is a list of frames that are *not ours*. Trying to merge it with our OC list is like trying to grab our next-in-flow's OC list and merge it with ours. Why are we doing this? Or if this is the right thing to do (for some reason), why are we not grabbing our NIF's OC list when our excess list is missing?

I can't think of a reason why ReflowOverflowContainerChildren should be reflowing an overflow continuation *and* its continuation both as children of the current nsContainerFrame. But that seems to be will happen with this patch in place, because the excess list, in theory, consists of continuations of this frame's children (either normal children or OC children).

+    // If aOverflowCont has a prev/next-in-flow that might be in
+    // mOverflowContList we need to find it and insert after/before it to
+    // maintain the order amongst next-in-flows in this list.

My expectation was that aOverflowCont's prev-in-flow would never be in this list, because if we're calling Insert() on aOverflowCont, its prev-in-flow must be the child of this nsContainerFrame that we just reflowed, not a frame that we plan to pass on to our NIF. So this looks wrong as well.

It is possible for aOverflowCont's next-in-flow to be in this list. But we shouldn't have encountered it yet. It should just sit at the end of this list until our NIF reflows this list, at which point it should be stolen from this list and put into our NIF's excessOC list for reflow by *its* NIF.

So this code also looks wrong.

+  // If we have a list and aOverflowCont is already in it then don't try to
+  // add it again.
+  if (addToList && aOverflowCont->GetParent() == mParent &&
+      (aOverflowCont->GetStateBits() & NS_FRAME_IS_OVERFLOW_CONTAINER) &&
+      mOverflowContList && mOverflowContList->ContainsFrame(aOverflowCont)) {
+    addToList = false;
+    mPrevOverflowCont = aOverflowCont->GetPrevSibling();
+  }

If aOverflowCont is already in the list, it should be the very next frame in it. In which case we will step over it further down in this function. If it's not the very next frame in the list, then this code steps over some arbitrary number of frames. What are these frames, and why are we stepping over them? The overflow container lists are supposed to strictly maintain the frame order of their prev-in-flows, so if we are reflowing those frames in the correct order, we should not encounter an overflow container until we have encountered each of its previous-siblings.

----------------

In Comment 4 you wrote that mPrevOverflowCont is the NIF of the aOverflowCont we're trying to insert. That's clearly broken, and is what should be fixed.

It looks like mPrevOveflowCont became that NIF when we were setting up the list walker (walking over the OC continuations, one of which was aOverflowCont's NIF). What we probably should be doing is checking that the OC continuations that we're skipping are the ones we want to keep in this list, and we can check that by checking that their prev-in-flow's parent is our prev-in-flow.

Any OC continuations whose prev-in-flow is not parented by our prev-in-flow don't belong on this list. They have to be kept somewhere, but that should be at the end of the list (because that maintains continuation order) and should not be encountered by our walker or reflowed by ReflowOverflowContainerChildren. (They'll eventually be pushed out to another frame further down the continuation chain, and should be placed correctly once their immediate prev-in-flow is reflowed. We just need to own them until that happens.)

So, r- fantasai. I think the patch is totally wrong. Even though it apparently works fine. :/

I'll look at 863935 next.
Comment 27 Mats Palmgren (:mats) 2013-05-11 08:41:39 PDT
(In reply to fantasai from comment #26)
> This does not make sense. Our excessOC list is a list of frames that are
> *not ours*. 

If by "ours" you mean a parent/child relationship then you're mistaken.
I guess you mean they were destined to be picked up by our NIF and
thus should not contribute to our overflow areas.

> Why are we doing this?

The overflow tracker needs to deal with an EOC list from a *previous*
reflow somehow.  As the comment says, this happens when a frame is
reflowed for the second time before its NIF is reflowed so the EOC list
is obsolete anyway.

But you do have a point that reflowing the EOC children here is
unnecessary.  I suspect that it doesn't really matter in practice
since if the reflow conditions are the same they will have been moved
into a new EOC list again before we reach them.

> why are we not grabbing our
> NIF's OC list when our excess list is missing?

Yeah, we should probably drain those for the same reason.

> It is possible for aOverflowCont's next-in-flow to be in this list. But we
> shouldn't have encountered it yet. It should just sit at the end of this
> list until our NIF reflows this list, at which point it should be stolen
> from this list and put into our NIF's excessOC list for reflow by *its* NIF.

You seem to assume that given a NIF chain, A B C, that first A is reflowed
then B, then C.  That's a bogus assumption.  These are all valid reflows:

A B C A ...
A A B C ...
A B A B C ...

> In Comment 4 you wrote that mPrevOverflowCont is the NIF of the
> aOverflowCont we're trying to insert. That's clearly broken, and is what
> should be fixed.

Filed bug 871163.
Comment 28 Mats Palmgren (:mats) 2014-07-18 03:52:13 PDT
Landed the crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ae89578a470
Comment 29 Ryan VanderMeulen [:RyanVM] 2014-07-18 12:08:28 PDT
https://hg.mozilla.org/mozilla-central/rev/5ae89578a470

Note You need to log in before you can comment on or make changes to this bug.