Closed Bug 1457288 Opened 2 years ago Closed 2 years ago

heap-buffer-overflow in nsFloatManager::ShapeInfo::CreateCircleOrEllipse

Categories

(Core :: Layout: Floats, defect)

61 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 + fixed

People

(Reporter: nils, Assigned: bradwerth)

References

Details

(4 keywords)

Attachments

(6 files, 6 obsolete files)

289 bytes, text/html
Details
18.84 KB, text/plain
Details
2.00 KB, patch
mats
: review+
Details | Diff | Splinter Review
12.15 KB, patch
Details | Diff | Splinter Review
18.91 KB, patch
Details | Diff | Splinter Review
1.25 KB, patch
mats
: review+
Details | Diff | Splinter Review
The following testcase crashes the latest ASAN build of Firefox nightly (SourceStamp=ccfd7b716a91241ddbc084cb7116ec561e56d5d1).

crash.html:
<script>
function start() {
	o234=document.createElement('style');
	o235=document.createTextNode("*{ shape-outside: ellipse(1% 10%); shape-margin: 1048576rem; float: right}");
	o234.appendChild(o235);
	document.documentElement.appendChild(o234);
}
</script>
<body onload="start()"></body>

ASAN output:
=================================================================
==5705==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f67c6800810 at pc 0x7f67dead3585 bp 0x7ffe2a52aab0 sp 0x7ffe2a52aaa8
WRITE of size 2 at 0x7f67c6800810 thread T0 (file:// Content)
    #0 0x7f67dead3584 in nsFloatManager::EllipseShapeInfo::EllipseShapeInfo(nsPoint const&, nsSize const&, int, int) /builds/worker/workspace/build/src/layout/generic/nsFloatManager.cpp:892:19
    #1 0x7f67deade9c1 in MakeUnique<nsFloatManager::EllipseShapeInfo, nsPoint &, nsSize &, int &, int &> /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/UniquePtr.h:680:27
    #2 0x7f67deade9c1 in nsFloatManager::ShapeInfo::CreateCircleOrEllipse(mozilla::UniquePtr<mozilla::StyleBasicShape, mozilla::DefaultDelete<mozilla::StyleBasicShape> > const&, int, nsIFrame*, mozilla::LogicalRect const&, mozilla::WritingMode, nsSize const&) /builds/worker/workspace/build/src/layout/generic/nsFloatManager.cpp:2330
    #3 0x7f67deada40e in CreateBasicShape /builds/worker/workspace/build/src/layout/generic/nsFloatManager.cpp:2197:14
    #4 0x7f67deada40e in nsFloatManager::FloatInfo::FloatInfo(nsIFrame*, int, int, mozilla::LogicalRect const&, mozilla::WritingMode, nsSize const&) /builds/worker/workspace/build/src/layout/generic/nsFloatManager.cpp:2005
    #5 0x7f67dea717ff in nsFloatManager::AddFloat(nsIFrame*, mozilla::LogicalRect const&, mozilla::WritingMode, nsSize const&) /builds/worker/workspace/build/src/layout/generic/nsFloatManager.cpp:260:13
    #6 0x7f67de9d095b in mozilla::BlockReflowInput::FlowAndPlaceFloat(nsIFrame*) /builds/worker/workspace/build/src/layout/generic/BlockReflowInput.cpp:994:19
    #7 0x7f67de9cdcaf in mozilla::BlockReflowInput::AddFloat(nsLineLayout*, nsIFrame*, int) /builds/worker/workspace/build/src/layout/generic/BlockReflowInput.cpp:627:14
    #8 0x7f67dec29dd1 in AddFloat /builds/worker/workspace/build/src/layout/generic/nsLineLayout.h:182:22
    #9 0x7f67dec29dd1 in nsLineLayout::ReflowFrame(nsIFrame*, nsReflowStatus&, mozilla::ReflowOutput*, bool&) /builds/worker/workspace/build/src/layout/generic/nsLineLayout.cpp:966
    #10 0x7f67dea5f77d in nsBlockFrame::ReflowInlineFrame(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:4158:15
    #11 0x7f67dea5e127 in nsBlockFrame::DoReflowInlineFrames(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:3958:5
    #12 0x7f67dea54e49 in nsBlockFrame::ReflowInlineFrames(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:3832:9
    #13 0x7f67dea4d3a0 in nsBlockFrame::ReflowLine(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2816:5
    #14 0x7f67dea42c20 in nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2352:7
    #15 0x7f67dea3a434 in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:1225:3
    #16 0x7f67dea9a8f6 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:951:14
    #17 0x7f67dea99142 in nsCanvasFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /builds/worker/workspace/build/src/layout/generic/nsCanvasFrame.cpp:713:5
    #18 0x7f67dea9a8f6 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:951:14
    #19 0x7f67deb81358 in nsHTMLScrollFrame::ReflowScrolledFrame(mozilla::ScrollReflowInput*, bool, bool, mozilla::ReflowOutput*, bool) /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:555:3
    #20 0x7f67deb82779 in nsHTMLScrollFrame::ReflowContents(mozilla::ScrollReflowInput*, mozilla::ReflowOutput const&) /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:678:3
    #21 0x7f67deb86758 in nsHTMLScrollFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:1055:3
    #22 0x7f67dea1e78e in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, int, int, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:995:14
    #23 0x7f67dea1d30e in mozilla::ViewportFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /builds/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:335:7
    #24 0x7f67de802eb0 in mozilla::PresShell::DoReflow(nsIFrame*, bool) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:8962:11
    #25 0x7f67de818940 in mozilla::PresShell::ProcessReflowCommands(bool) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:9135:24
    #26 0x7f67de816d49 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:4343:11
    #27 0x7f67de7a671d in FlushPendingNotifications /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIPresShell.h:592:5
    #28 0x7f67de7a671d in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1937
    #29 0x7f67de7b5ff0 in TickDriver /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:337:13
    #30 0x7f67de7b5ff0 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:307
    #31 0x7f67de7b5bb6 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:329:5
    #32 0x7f67de7b892e in RunRefreshDrivers /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:770:5
    #33 0x7f67de7b892e in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:683
    #34 0x7f67de7b852e in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:584:9
    #35 0x7f67df06301f in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) /builds/worker/workspace/build/src/layout/ipc/VsyncChild.cpp:68:16
    #36 0x7f67d7d54b5d in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:156:20
    #37 0x7f67d7c12967 in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:1928:28
    #38 0x7f67d771bfee in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2141:25
    #39 0x7f67d7718fb6 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2071:17
    #40 0x7f67d771a76c in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1917:5
    #41 0x7f67d771adc8 in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1950:15
    #42 0x7f67d682aa69 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1090:14
    #43 0x7f67d68464a0 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:519:10
    #44 0x7f67d7723b5a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #45 0x7f67d76775c9 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #46 0x7f67d76775c9 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #47 0x7f67d76775c9 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #48 0x7f67de25332a in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:157:27
    #49 0x7f67e24cce0b in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:893:22
    #50 0x7f67d76775c9 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #51 0x7f67d76775c9 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #52 0x7f67d76775c9 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #53 0x7f67e24cc7d0 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:719:34
    #54 0x4f50dc in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #55 0x4f50dc in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:280
    #56 0x7f67f653882f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #57 0x42476c in _start (/fuzzer3/firefox/firefox+0x42476c)

0x7f67c6800810 is located 0 bytes to the right of 201326608-byte region [0x7f67ba800800,0x7f67c6800810)
allocated by thread T0 (file:// Content) here:
    #0 0x4c54b3 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:88:3
    #1 0x7f67dead2d22 in operator new[] /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:174:12
    #2 0x7f67dead2d22 in MakeUniqueFallible<unsigned short []> /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/UniquePtrExtensions.h:33
    #3 0x7f67dead2d22 in nsFloatManager::EllipseShapeInfo::EllipseShapeInfo(nsPoint const&, nsSize const&, int, int) /builds/worker/workspace/build/src/layout/generic/nsFloatManager.cpp:827
    #4 0x7f67deade9c1 in MakeUnique<nsFloatManager::EllipseShapeInfo, nsPoint &, nsSize &, int &, int &> /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/UniquePtr.h:680:27
    #5 0x7f67deade9c1 in nsFloatManager::ShapeInfo::CreateCircleOrEllipse(mozilla::UniquePtr<mozilla::StyleBasicShape, mozilla::DefaultDelete<mozilla::StyleBasicShape> > const&, int, nsIFrame*, mozilla::LogicalRect const&, mozilla::WritingMode, nsSize const&) /builds/worker/workspace/build/src/layout/generic/nsFloatManager.cpp:2330
    #6 0x7f67deada40e in CreateBasicShape /builds/worker/workspace/build/src/layout/generic/nsFloatManager.cpp:2197:14
    #7 0x7f67deada40e in nsFloatManager::FloatInfo::FloatInfo(nsIFrame*, int, int, mozilla::LogicalRect const&, mozilla::WritingMode, nsSize const&) /builds/worker/workspace/build/src/layout/generic/nsFloatManager.cpp:2005
    #8 0x7f67dea717ff in nsFloatManager::AddFloat(nsIFrame*, mozilla::LogicalRect const&, mozilla::WritingMode, nsSize const&) /builds/worker/workspace/build/src/layout/generic/nsFloatManager.cpp:260:13
    #9 0x7f67de9d095b in mozilla::BlockReflowInput::FlowAndPlaceFloat(nsIFrame*) /builds/worker/workspace/build/src/layout/generic/BlockReflowInput.cpp:994:19
    #10 0x7f67de9cdcaf in mozilla::BlockReflowInput::AddFloat(nsLineLayout*, nsIFrame*, int) /builds/worker/workspace/build/src/layout/generic/BlockReflowInput.cpp:627:14
    #11 0x7f67dec29dd1 in AddFloat /builds/worker/workspace/build/src/layout/generic/nsLineLayout.h:182:22
    #12 0x7f67dec29dd1 in nsLineLayout::ReflowFrame(nsIFrame*, nsReflowStatus&, mozilla::ReflowOutput*, bool&) /builds/worker/workspace/build/src/layout/generic/nsLineLayout.cpp:966
    #13 0x7f67dea5f77d in nsBlockFrame::ReflowInlineFrame(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:4158:15
    #14 0x7f67dea5e127 in nsBlockFrame::DoReflowInlineFrames(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:3958:5
    #15 0x7f67dea54e49 in nsBlockFrame::ReflowInlineFrames(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:3832:9
    #16 0x7f67dea4d3a0 in nsBlockFrame::ReflowLine(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2816:5
    #17 0x7f67dea42c20 in nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2352:7
    #18 0x7f67dea3a434 in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:1225:3
    #19 0x7f67dea9a8f6 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:951:14
    #20 0x7f67dea99142 in nsCanvasFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /builds/worker/workspace/build/src/layout/generic/nsCanvasFrame.cpp:713:5
    #21 0x7f67dea9a8f6 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:951:14
    #22 0x7f67deb81358 in nsHTMLScrollFrame::ReflowScrolledFrame(mozilla::ScrollReflowInput*, bool, bool, mozilla::ReflowOutput*, bool) /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:555:3
    #23 0x7f67deb82779 in nsHTMLScrollFrame::ReflowContents(mozilla::ScrollReflowInput*, mozilla::ReflowOutput const&) /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:678:3
    #24 0x7f67deb86758 in nsHTMLScrollFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:1055:3
    #25 0x7f67dea1e78e in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, int, int, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:995:14
    #26 0x7f67dea1d30e in mozilla::ViewportFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /builds/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:335:7
    #27 0x7f67de802eb0 in mozilla::PresShell::DoReflow(nsIFrame*, bool) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:8962:11
    #28 0x7f67de818940 in mozilla::PresShell::ProcessReflowCommands(bool) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:9135:24
    #29 0x7f67de816d49 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:4343:11
    #30 0x7f67de7a671d in FlushPendingNotifications /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIPresShell.h:592:5
    #31 0x7f67de7a671d in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1937
    #32 0x7f67de7b5ff0 in TickDriver /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:337:13
    #33 0x7f67de7b5ff0 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:307
    #34 0x7f67de7b5bb6 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:329:5
    #35 0x7f67de7b892e in RunRefreshDrivers /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:770:5
    #36 0x7f67de7b892e in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:683
    #37 0x7f67de7b852e in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:584:9

SUMMARY: AddressSanitizer: heap-buffer-overflow /builds/worker/workspace/build/src/layout/generic/nsFloatManager.cpp:892:19 in nsFloatManager::EllipseShapeInfo::EllipseShapeInfo(nsPoint const&, nsSize const&, int, int)
Shadow bytes around the buggy address:
  0x0fed78cf80b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fed78cf80c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fed78cf80d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fed78cf80e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fed78cf80f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0fed78cf8100: 00 00[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0fed78cf8110: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0fed78cf8120: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0fed78cf8130: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0fed78cf8140: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0fed78cf8150: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  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
==5705==ABORTING
Attached file ASAN output
Group: core-security → layout-core-security
I'm guessing this is a bug in our shape-outside / shape-margin layout code.
Brad / Ting-Yu, you're working on that, right?
It's me; this is my code. I'll see what's going on.
Assignee: nobody → bwerth
(In reply to Nils from comment #0)

> 0x7f67c6800810 is located 0 bytes to the right of 201326608-byte region
> [0x7f67ba800800,0x7f67c6800810)

Obviously this is a large allocation, but why is being "0 bytes to the right" of this large allocation a memory overrun? Is that address not within the allocation?
Flags: needinfo?(nils)
I think the relevant part is the "WRITE of size 2 at 0x7f67c6800810".
I think the "0 bytes to the right" thing is just ASAN providing 
additional info on the nearest allocated thing it knows about, 
in case that's the data we intended to use.  In this case I'd guess
it's not relevant since we don't really have that large layout
objects, so this is likely a use-after-free at 0x7f67c6800810.
Attachment #8971696 - Flags: review?(mats)
Can you describe exactly what the bug is please?
Flags: needinfo?(bwerth)
(In reply to Mats Palmgren (:mats) from comment #8)
> Can you describe exactly what the bug is please?

Several ShapeInfo classes calculate a distance field for shape-margin: ImageShapeInfo, EllipseShapeInfo, and soon PolygonShapeInfo. Since the float area is cropped to the margin rect, the distance field has to cover the entire margin rect. ImageShapeInfo and PolygonShapeInfo allocate their distance fields precisely to the margin rect and therefore a large distance field allocation is only possible with an extremely large DOM size. EllipseShapeInfo is different because it attempts to take advantage of symmetry and instead allocates a distance field to cover one quadrant of the ellipse plus shape-margin area. All the float calculations take place in that quadrant or on reflections into that quadrant. This is a reasonable optimization given the assumption that most EllipseShapeInfo floats will be used to show the curve of roughly half of the ellipse. However, with this optimization, a large shape-margin value results in a large distance field allocation. The existing code already limits the maximum shape-margin value that the distance field will calculate. The proposed patch also limits the size of the distance field allocation to be within that same limit.

That is the background on the feature, though not an explanation of the bug. Since the existing code uses a MakeUniqueFallible allocation, a too-large distance field should fail and the code should fail soft. I can't explain why a very large allocation should lead to an out-of-bounds memory access. In my local testing, I haven't been able to replicate. I'll keep trying and see if there is more to patch here.
Flags: needinfo?(bwerth)
(In reply to Brad Werth [:bradwerth] from comment #9)
 
> That is the background on the feature, though not an explanation of the bug.
> Since the existing code uses a MakeUniqueFallible allocation, a too-large
> distance field should fail and the code should fail soft. I can't explain
> why a very large allocation should lead to an out-of-bounds memory access.
> In my local testing, I haven't been able to replicate. I'll keep trying and
> see if there is more to patch here.

The behavior of MakeUniqueFallible surprises me. In local testing, in a debug build, I hit a MOZ_CRASH(). I can't get the fallible behavior. That may not be relevant to understanding this bug, because the log seems to indicate that a large allocation did occur, and then we attempted to address outside of it.
Thanks for the background, now I only need to know what the bug
is exactly to review :-)

> Since the existing code uses a MakeUniqueFallible allocation,
> a too-large distance field should fail and the code should fail soft

Well, we shouldn't allow single-line CSS declarations to allocate
arbitrary amounts of memory in the first place, b/c it's a DOS risk.

Reading the surrounding code, this stands out to me:

  const int32_t iSize = bounds.width + iExpand;
  const int32_t bSize = bounds.height + bExpand;
  auto df = MakeUniqueFallible<dfType[]>(iSize * bSize);

This is a classic error leading to out-of-bounds access of arrays.
An integer overflow above might result in allocating a too small buffer.
I don't see any clamping of the values involved to avoid overflowing,
but I haven't checked further up the stack ('bounds' comes from 'mRadii'
and 'aShapeMargin' which I don't know if they are clamped somewhere)

/me updates my inbound-tree and loads the testcase in 'rr' ...

So, yeah that seems to be exactly what happened...

Assertion failure: index >= 0 && index < (iSize * bSize) (Our distance field index should be in-bounds.), at layout/generic/nsFloatManager.cpp:861

(rr) info locals
index = 100663304
i = 16777214
bInAppUnits = 180
bIsInExpandedRegion = false
bIsMoreThanEllipseBEnd = true
iIntercept = -1073741823
iMax = 16777213
b = 5
shapeMarginDevPixels = 16777216
shapeMarginDevPixelsInt5X = 83886080
bSize = 16777220
df = {
  mTuple = {
    <mozilla::detail::PairHelper<unsigned short*, mozilla::DefaultDelete<unsigned short []>, mozilla::detail::StorageType::AsMember, mozilla::detail::StorageType::AsBase>> = {
      <mozilla::DefaultDelete<unsigned short []>> = {<No data fields>}, 
      members of mozilla::detail::PairHelper<unsigned short*, mozilla::DefaultDelete<unsigned short []>, mozilla::detail::StorageType::AsMember, mozilla::detail::StorageType::AsBase>: 
      mFirstA = 0x7fc611200000
    }, <No data fields>}
}
MAX_CHAMFER_VALUE = 11
MAX_MARGIN = 13104
MAX_MARGIN_5X = 65520
usedMargin5X = 65520
radiiPlusShapeMargin = {
  <mozilla::gfx::BaseSize<int, nsSize>> = {
    {
      {
        width = 1006632969, 
        height = 1006633056
      }, 
      components = {1006632969, 1006633056}
    }
  }, <No data fields>}
bounds = {
  <mozilla::gfx::BaseSize<int, mozilla::gfx::IntSizeTyped<mozilla::LayoutDevicePixel> >> = {
    {
      {
        width = 16777216, 
        height = 16777218
      }, 
      components = {16777216, 16777218}
    }
  }, 
  <mozilla::LayoutDevicePixel> = {<No data fields>}, <No data fields>}
iSize = 16777218
iExpand = 2
bExpand = 2
(rr) p mRadii
$5 = {
  <mozilla::gfx::BaseSize<int, nsSize>> = {
    {
      {
        width = 9, 
        height = 96
      }, 
      components = {9, 96}
    }
  }, <No data fields>}
(rr) p *this
$6 = {
  <nsFloatManager::ShapeInfo> = {
    _vptr$ShapeInfo = 0x7fc64f6a7428 <vtable for nsFloatManager::EllipseShapeInfo+16>
  }, 
  members of nsFloatManager::EllipseShapeInfo: 
  mCenter = {
    <mozilla::gfx::BasePoint<int, nsPoint, int>> = {
      {
        {
          x = 35280, 
          y = 480
        }, 
        components = {35280, 480}
      }
    }, <No data fields>}, 
  mRadii = {
    <mozilla::gfx::BaseSize<int, nsSize>> = {
      {
        {
          width = 9, 
          height = 96
        }, 
        components = {9, 96}
      }
    }, <No data fields>}, 
  mShapeMargin = 1006632960, 
  mIntervals = {
    <nsTArray_Impl<nsRect, nsTArrayInfallibleAllocator>> = {
      <nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>> = {
        mHdr = 0x7fc627549800
      }, 
      <nsTArray_TypedBase<nsRect, nsTArray_Impl<nsRect, nsTArrayInfallibleAllocator> >> = {
        <nsTArray_SafeElementAtHelper<nsRect, nsTArray_Impl<nsRect, nsTArrayInfallibleAllocator> >> = {<No data fields>}, <No data fields>}, 
      members of nsTArray_Impl<nsRect, nsTArrayInfallibleAllocator>: 
      static NoIndex = 18446744073709551615
    }, <No data fields>}
}
(rr) p (size_t)iSize * (size_t)bSize
$5 = 281475077373960
#0  mozilla::MakeUniqueFallible<unsigned short []> (aN=100663304) at dist/include/mozilla/UniquePtrExtensions.h:33
33        return UniquePtr<T>(new (fallible) ArrayType[aN]());
Comment on attachment 8971696 [details] [diff] [review]
Bug 1457288: Constrain EllipseShapeInfo distance field allocation to the size we can safely compute.

This might be a correct fix but I doubt it's complete.
Some sort of clamping of these values are needed to avoid DOS
and make integer overflows impossible.
Attachment #8971696 - Flags: review?(mats) → review-
(If you make calculations based on raw CSS values elsewhere
you should probably audit those places for similar issues.)
(In reply to Nils from comment #0)

> 0x7f67c6800810 is located 0 bytes to the right of 201326608-byte region
> [0x7f67ba800800,0x7f67c6800810)
> allocated by thread T0 (file:// Content) here:

I understand the bug now. Without the patch applied, the allocation that triggers the crash in this testcase is attempting to allocate 33554434 * 33554438 = 1125900175278092 elements. That multiplication is done on int32_t, so the multiplication is overflowing to (I believe) 100663304, which is large but succeeds. The code later on attempts to write to the element at that index (on its way to a much larger number based on the two large multiplicands) and goes beyond the allocation. I'll add a part to the patch that either clamps the multiplication or detects the overflow.
Comment on attachment 8971751 [details] [diff] [review]
Bug 1457288 Part 1: Clamp EllipseShapeInfo distance field operands to the maximum shape-margin we are willing to compute.

Please note that this change doesn't make much of a difference
from an integer overflow point of view.  The addition can
make 'bounds' become negative if 'mRadii' is close to MAX_INT
already.  Also, the value of
NSToIntRound(5.0f * shapeMarginDevPixels) is undefined if
the float value can't be represented as an int32_t.
Attachment #8971751 - Flags: review?(mats) → review+
Comment on attachment 8971752 [details] [diff] [review]
Bug 1457288 Part 2: Clamp shape-margin distance field operands to prevent int multiplication overflow when allocating.

> std::min(bounds.width + iExpand, DF_SIDE_MAX)

If 'bounds.width' is MAX_INT then the addition will
overflow and the result is UB.  In practice it might
become negative and std::min will choose that value.

Note that this isn't just a theoretical issue.
Optimizing compilers take advantage of UB by assuming that
the overflow never occurs and generates code that is
unsafe if it does occur.  See bug 1292443 comment 16
for example where it caused an exploitable security bug.

Your only two options for code like this is:
1. use CheckedInt<T> for all arithmetic operations
   (see bug 1348936 for an example how to use it)
2. clamp the operands upfront to guarantee that overflow
   can't possibly occur
(or a combination of those)

I'd also like to see changes that guarantees no overflow
occurred for the values stored in mRadii.
Attachment #8971752 - Flags: review?(mats) → review-
Unrelated drive-by comment:

>    for (int32_t i = 0; i < iSize; ++i) {
>      const int32_t index = i + b * iSize;

"b * iSize" is a loop-invariant expression.  We should probably move
that outside the loop in case the compiler isn't smart enough to
realize that.  Even better, add a variable iStart outside both loops
and increment it iStart += iSize after this loop and do iStart + i
here instead (reducing the multiplication to two additions).
Blocks: 1457307
(In reply to Mats Palmgren (:mats) from comment #18)
> Comment on attachment 8971751 [details] [diff] [review]
> Bug 1457288 Part 1: Clamp EllipseShapeInfo distance field operands to the
> maximum shape-margin we are willing to compute.
> 
> Please note that this change doesn't make much of a difference
> from an integer overflow point of view.  The addition can
> make 'bounds' become negative if 'mRadii' is close to MAX_INT
> already.  Also, the value of
> NSToIntRound(5.0f * shapeMarginDevPixels) is undefined if
> the float value can't be represented as an int32_t.

I'm going to solve these issues in Part 2, where I will also apply the fixes to ImageShapeInfo math.
(In reply to Mats Palmgren (:mats) from comment #19)
> Comment on attachment 8971752 [details] [diff] [review]
> Bug 1457288 Part 2: Clamp shape-margin distance field operands to prevent
> int multiplication overflow when allocating.
> 
> > std::min(bounds.width + iExpand, DF_SIDE_MAX)
> 
> If 'bounds.width' is MAX_INT then the addition will
> overflow and the result is UB.  In practice it might
> become negative and std::min will choose that value.
> 
> Note that this isn't just a theoretical issue.
> Optimizing compilers take advantage of UB by assuming that
> the overflow never occurs and generates code that is
> unsafe if it does occur.  See bug 1292443 comment 16
> for example where it caused an exploitable security bug.

I chose to address this in a new Part 3 where I switch all of the distance field sizes and indexes to unsigned ints. That is the solution used in Bug 1292443, I believe, and will result in overflow making unexpectedly small, but still safe values. For example, uint overflow to a value less than the iExpand value will just result in an empty float area.

> I'd also like to see changes that guarantees no overflow
> occurred for the values stored in mRadii.

I'm not addressing that in these patches because I don't see a way for a security errors to come from an overflow there. Amongst other things, a negative mRadii will cause ::IsEmpty() to return true, indicating an empty float area and preventing any of the other flow calculations from even being called.
(In reply to Mats Palmgren (:mats) from comment #20)
> Unrelated drive-by comment:
> 
> >    for (int32_t i = 0; i < iSize; ++i) {
> >      const int32_t index = i + b * iSize;
> 
> "b * iSize" is a loop-invariant expression.  We should probably move
> that outside the loop in case the compiler isn't smart enough to
> realize that.  Even better, add a variable iStart outside both loops
> and increment it iStart += iSize after this loop and do iStart + i
> here instead (reducing the multiplication to two additions).

I'm not making this change here since it's not a security issue, but I'll address this for the polygon implementation in Bug 1451499 and either roll it in for the other shapes in that bug, or open a new bug.
Attachment #8972177 - Flags: review?(mats)
Flags: needinfo?(nils)
Comment on attachment 8972177 [details] [diff] [review]
Bug 1457288 Part 2: Clamp shape-margin distance field operands to prevent int multiplication overflow when allocating.

>+  static const int32_t DF_SIDE_MAX =
>+    floor(sqrt((double)(std::numeric_limits<int32_t>::max())));
>+  const int32_t iSize = std::min(bounds.width + iExpand, DF_SIDE_MAX);
>+  const int32_t bSize = std::min(bounds.height + bExpand, DF_SIDE_MAX);
>   auto df = MakeUniqueFallible<dfType[]>(iSize * bSize);

Right, this should safe after we change the types to uint32_t in part 3.
Probably worth adding a code comment here that we clamp the values to
guarantee that the multiplication that follows doesn't overflow, to
make the intent explicit.

>+  const dfType kMaxMargin5X = MAX_MARGIN_5X;
>-        df[index] = MAX_MARGIN_5X;
>+        df[index] = kMaxMargin5X;

I don't see the need to introduce a local alias for the constant here.
Just leave this code as is please.
Attachment #8972177 - Flags: review?(mats) → review+
(In reply to Mats Palmgren (:mats) from comment #29)
> Comment on attachment 8972177 [details] [diff] [review]

> I don't see the need to introduce a local alias for the constant here.
> Just leave this code as is please.

Sorry, this was necessary to keep the compiler happen when trying to find MAX_MARGIN_5X in the std::min macro expansion. You can see this in several of the failed try runs. I'll make this a true alias (static const dfType&) and add a comment explaining why this alias is needed.
Comment on attachment 8972186 [details] [diff] [review]
Bug 1457288 Part 3: Change distance field sizes and indexes to be unsigned ints, avoiding undefined behavior with potential int overflows and underflows.

>+  const uint32_t w = aImageSize.width;
>+  const uint32_t h = aImageSize.height;

Please add a MOZ_ASSERT at the start of the method that
aImageSize.width,height >= 0.

>         const uint8_t alpha = aAlphaPixels[index];

It's a bit sad that we pass in aAlphaPixels as a raw C-array pointer,
because we have no way of knowing if 'index' is actually in bounds.
I don't really see a way around that though...

(Graphics/Media code does sort of thing quite a lot, and it has
proven to be notoriously error prone.  It's caused many stability
and security issues over the years.)
Attachment #8972186 - Flags: review?(mats) → review+
Attached patch fix? (obsolete) — Splinter Review
(In reply to Brad Werth [:bradwerth] from comment #30)
> Sorry, this was necessary to keep the compiler happen when trying to find
> MAX_MARGIN_5X in the std::min macro expansion.

Initializing them outside the class should work.
(In reply to Mats Palmgren (:mats) from comment #32)
> Created attachment 8972371 [details] [diff] [review]
> fix?
> 
> (In reply to Brad Werth [:bradwerth] from comment #30)
> > Sorry, this was necessary to keep the compiler happen when trying to find
> > MAX_MARGIN_5X in the std::min macro expansion.
> 
> Initializing them outside the class should work.

Thanks; I didn't think of that.
(In reply to Brad Werth [:bradwerth] from comment #25)
> (In reply to Mats Palmgren (:mats) from comment #20)
> > "b * iSize" is a loop-invariant expression.  We should probably move
> I'm not making this change here since it's not a security issue, but I'll
> address this for the polygon implementation in Bug 1451499 and either roll
> it in for the other shapes in that bug, or open a new bug.

OK, sounds good.  FYI, there are also a few of these:
      for (uint32_t i = 0; i < iSize; ++i) {
        const uint32_t col = aWM.IsVertical() ? b : i;

where aWM.IsVertical() is loop invariant.  The compiler might unroll
that for you automatically, but doing it explicitly might be better
just to be sure.  Also, it might be faster to use references to avoid
having that branch inside the loop:
      uint32_t i = 0;
      const uint32_t& col = aWM.IsVertical() ? b : i;
      for (; i < iSize; ++i) {

You'll have to declare i/b outside the loops, but it might be
worth it if this code is perf sensitive (and it probably is
if we might iterate this line a billion times).
Worth measuring which is faster though...
If this bug only affects Nightly, then we can land it without sec-approval.
In that case we can also land the testcase as a crashtest.
Attachment #8972398 - Flags: review?(mats)
Attachment #8972398 - Flags: review?(mats) → review+
Keywords: checkin-needed
Attachment #8972371 - Attachment is obsolete: true
Duplicate of this bug: 1457698
Duplicate of this bug: 1457307
Duplicate of this bug: 1459648
Flags: sec-bounty?
Group: core-security-release
Flags: sec-bounty? → sec-bounty+
Depends on: 1464113
You need to log in before you can comment on or make changes to this bug.