==13447==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6230000601a8 at pc 0x0000004bea3c bp 0x7ffeffd80c70 sp 0x7ffeffd80420 WRITE of size 4112 at 0x6230000601a8 thread T0 (Web Content) SCARINESS: 55 (multi-byte-write-heap-buffer-overflow-far-from-bounds) #0 0x4bea3b in __asan_memset _asan_rtl_ #1 0x7fd8de8e2b4c in rasterize_edges_1 gfx/cairo/libpixman/src/pixman-edge-imp.h:125:7 #2 0x7fd8de8e2b4c in pixman_rasterize_edges_no_accessors gfx/cairo/libpixman/src/pixman-edge.c:351 #3 0x7fd8de8e2b4c in _moz_pixman_rasterize_edges gfx/cairo/libpixman/src/pixman-edge.c:382 #4 0x7fd8de99d824 in _moz_pixman_rasterize_trapezoid gfx/cairo/libpixman/src/pixman-trap.c:386:2 #5 0x7fd8de7b1392 in _pixman_image_add_traps gfx/cairo/cairo/src/cairo-image-surface.c:2466:2 #6 0x7fd8de7b0701 in _composite_traps gfx/cairo/cairo/src/cairo-image-surface.c:2516:5 #7 0x7fd8de7b5b18 in _clip_and_composite gfx/cairo/cairo/src/cairo-image-surface.c:2359:15 #8 0x7fd8de7b891f in _clip_and_composite_trapezoids gfx/cairo/cairo/src/cairo-image-surface.c:3258:12 #9 0x7fd8de7b7d7d in _clip_and_composite_polygon gfx/cairo/cairo/src/cairo-image-surface.c:3625:15 #10 0x7fd8de7a3ceb in _cairo_image_surface_stroke gfx/cairo/cairo/src/cairo-image-surface.c:3717:15 #11 0x7fd8de81bce1 in _cairo_surface_stroke gfx/cairo/cairo/src/cairo-surface.c:2296:11 #12 0x7fd8de796646 in _cairo_gstate_stroke gfx/cairo/cairo/src/cairo-gstate.c:1166:14 #13 0x7fd8de83ea7d in _moz_cairo_stroke_preserve gfx/cairo/cairo/src/cairo.c:2430:14 #14 0x7fd8d75bfbcf in mozilla::gfx::DrawTargetCairo::DrawPattern(mozilla::gfx::Pattern const&, mozilla::gfx::StrokeOptions const&, mozilla::gfx::DrawOptions const&, mozilla::gfx::DrawTargetCairo::DrawPatternType, bool) gfx/2d/DrawTargetCairo.cpp:1024:7 #15 0x7fd8d75c4f81 in mozilla::gfx::DrawTargetCairo::Stroke(mozilla::gfx::Path const*, mozilla::gfx::Pattern const&, mozilla::gfx::StrokeOptions const&, mozilla::gfx::DrawOptions const&) gfx/2d/DrawTargetCairo.cpp:1255:3 #16 0x7fd8dcd8bca6 in nsSVGPathGeometryFrame::Render(gfxContext*, unsigned int, gfxMatrix const&) layout/svg/nsSVGPathGeometryFrame.cpp:867:21 #17 0x7fd8dcd8a20d in nsSVGPathGeometryFrame::PaintSVG(gfxContext&, gfxMatrix const&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*) layout/svg/nsSVGPathGeometryFrame.cpp:293:5 #18 0x7fd8dcd88726 in nsDisplaySVGPathGeometry::Paint(nsDisplayListBuilder*, nsRenderingContext*) layout/svg/nsSVGPathGeometryFrame.cpp:125:51 #19 0x7fd8dc5f5f98 in mozilla::FrameLayerBuilder::PaintItems(nsTArray<mozilla::FrameLayerBuilder::ClippedDisplayItem>&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, gfxContext*, nsRenderingContext*, nsDisplayListBuilder*, nsPresContext*, mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits> const&, float, float, int) layout/base/FrameLayerBuilder.cpp:5710:21 #20 0x7fd8dc5f9433 in mozilla::FrameLayerBuilder::DrawPaintedLayer(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*) layout/base/FrameLayerBuilder.cpp:5885:19 #21 0x7fd8d7940a51 in mozilla::layers::BasicPaintedLayer::PaintThebes(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*) gfx/layers/basic/BasicPaintedLayer.cpp:95:9 #22 0x7fd8d793b3ec in mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintLayerContext&, gfxContext*) gfx/layers/basic/BasicLayerManager.cpp:714:13 #23 0x7fd8d7939c49 in mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*) gfx/layers/basic/BasicLayerManager.cpp:883:5 #24 0x7fd8d793b0b2 in mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintLayerContext&, gfxContext*) gfx/layers/basic/BasicLayerManager.cpp:735:7 #25 0x7fd8d7939c49 in mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*) gfx/layers/basic/BasicLayerManager.cpp:883:5 #26 0x7fd8d7934f1d in mozilla::layers::BasicLayerManager::EndTransactionInternal(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) gfx/layers/basic/BasicLayerManager.cpp:623:5 #27 0x7fd8dc5f5fb6 in PaintInactiveLayer layout/base/FrameLayerBuilder.cpp:3605:12 #28 0x7fd8dc5f5fb6 in mozilla::FrameLayerBuilder::PaintItems(nsTArray<mozilla::FrameLayerBuilder::ClippedDisplayItem>&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, gfxContext*, nsRenderingContext*, nsDisplayListBuilder*, nsPresContext*, mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits> const&, float, float, int) layout/base/FrameLayerBuilder.cpp:5696 #29 0x7fd8dc5f9433 in mozilla::FrameLayerBuilder::DrawPaintedLayer(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*) layout/base/FrameLayerBuilder.cpp:5885:19 #30 0x7fd8d79536e2 in mozilla::layers::ClientPaintedLayer::PaintThebes() gfx/layers/client/ClientPaintedLayer.cpp:94:5 #31 0x7fd8d795437d in mozilla::layers::ClientPaintedLayer::RenderLayerWithReadback(mozilla::layers::ReadbackProcessor*) gfx/layers/client/ClientPaintedLayer.cpp:148:3 #32 0x7fd8d796dd8c in mozilla::layers::ClientContainerLayer::RenderLayer() gfx/layers/client/ClientContainerLayer.h:65:29 #33 0x7fd8d796dd8c in mozilla::layers::ClientContainerLayer::RenderLayer() gfx/layers/client/ClientContainerLayer.h:65:29 #34 0x7fd8d796dd8c in mozilla::layers::ClientContainerLayer::RenderLayer() gfx/layers/client/ClientContainerLayer.h:65:29 #35 0x7fd8d796dd8c in mozilla::layers::ClientContainerLayer::RenderLayer() gfx/layers/client/ClientContainerLayer.h:65:29 #36 0x7fd8d796dd8c in mozilla::layers::ClientContainerLayer::RenderLayer() gfx/layers/client/ClientContainerLayer.h:65:29 #37 0x7fd8d796dd8c in mozilla::layers::ClientContainerLayer::RenderLayer() gfx/layers/client/ClientContainerLayer.h:65:29 #38 0x7fd8d796dd8c in mozilla::layers::ClientContainerLayer::RenderLayer() gfx/layers/client/ClientContainerLayer.h:65:29 #39 0x7fd8d794e89e in mozilla::layers::ClientLayerManager::EndTransactionInternal(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) gfx/layers/client/ClientLayerManager.cpp:296:9 #40 0x7fd8d794ee7c in mozilla::layers::ClientLayerManager::EndTransaction(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) gfx/layers/client/ClientLayerManager.cpp:339:3 #41 0x7fd8dc74a3db in nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) layout/base/nsDisplayList.cpp:1927:17 #42 0x7fd8dc7fb0d0 in nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, nsDisplayListBuilderMode, nsLayoutUtils::PaintFrameFlags) layout/base/nsLayoutUtils.cpp:3606:10 #43 0x7fd8dc8835ad in PresShell::Paint(nsView*, nsRegion const&, unsigned int) layout/base/nsPresShell.cpp:6636:5 #44 0x7fd8dbe4bf5c in nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) view/nsViewManager.cpp:484:19 #45 0x7fd8dbe4ad31 in nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) view/nsViewManager.cpp:415:33 #46 0x7fd8dbe4ecc9 in nsViewManager::ProcessPendingUpdates() view/nsViewManager.cpp:1119:5 #47 0x7fd8dc57654f in nsRefreshDriver::Tick(long, mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:1911:9 #48 0x7fd8dc58109d in TickDriver layout/base/nsRefreshDriver.cpp:279:13 #49 0x7fd8dc58109d in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) layout/base/nsRefreshDriver.cpp:251 #50 0x7fd8dc580cf9 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:270:5 #51 0x7fd8dc582c94 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:430:9 #52 0x7fd8dcf0c2c4 in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) layout/ipc/VsyncChild.cpp:64:16 #53 0x7fd8d6823059 in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) objdir-ff-asan/ipc/ipdl/PVsyncChild.cpp:240:20 #54 0x7fd8d62a1814 in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) objdir-ff-asan/ipc/ipdl/PBackgroundChild.cpp:2133:28 #55 0x7fd8d61cd566 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) ipc/glue/MessageChannel.cpp:1661:25 #56 0x7fd8d61c9c87 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) ipc/glue/MessageChannel.cpp:1599:17 #57 0x7fd8d61b6dff in mozilla::ipc::MessageChannel::OnMaybeDequeueOne() ipc/glue/MessageChannel.cpp:1566:5 #58 0x7fd8d61e8152 in applyImpl<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()> objdir-ff-asan/dist/include/nsThreadUtils.h:729:12 #59 0x7fd8d61e8152 in apply<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()> objdir-ff-asan/dist/include/nsThreadUtils.h:735 #60 0x7fd8d61e8152 in mozilla::detail::RunnableMethodImpl<bool (mozilla::ipc::MessageChannel::*)(), false, true>::Run() objdir-ff-asan/dist/include/nsThreadUtils.h:764 #61 0x7fd8d61e773f in Run objdir-ff-asan/dist/include/mozilla/ipc/MessageChannel.h:550:29 #62 0x7fd8d61e773f in mozilla::ipc::MessageChannel::DequeueTask::Run() objdir-ff-asan/dist/include/mozilla/ipc/MessageChannel.h:569 #63 0x7fd8d53e8802 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1068:14 #64 0x7fd8d54684d8 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:290:10 #65 0x7fd8d61d4d51 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:100:21 #66 0x7fd8d61478f0 in RunInternal ipc/chromium/src/base/message_loop.cc:232:10 #67 0x7fd8d61478f0 in RunHandler ipc/chromium/src/base/message_loop.cc:225 #68 0x7fd8d61478f0 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:205 #69 0x7fd8dbec0e2f in nsBaseAppShell::Run() widget/nsBaseAppShell.cpp:156:27 #70 0x7fd8ddfe49c7 in XRE_RunAppShell toolkit/xre/nsEmbedFunctions.cpp:851:22 #71 0x7fd8d61478f0 in RunInternal ipc/chromium/src/base/message_loop.cc:232:10 #72 0x7fd8d61478f0 in RunHandler ipc/chromium/src/base/message_loop.cc:225 #73 0x7fd8d61478f0 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:205 #74 0x7fd8ddfe4088 in XRE_InitChildProcess toolkit/xre/nsEmbedFunctions.cpp:681:34 #75 0x510ae1 in content_process_main ipc/contentproc/plugin-container.cpp:224:19 #76 0x510ae1 in main browser/app/nsBrowserApp.cpp:357 #77 0x7fd8ef154f44 in __libc_start_main /build/eglibc-oGUzwX/eglibc-2.19/csu/libc-start.c:287 AddressSanitizer can not describe address in more detail (wild memory access suspected). SUMMARY: AddressSanitizer: heap-buffer-overflow (/mnt/scratch0/clusterfuzz/slave-bot/builds/linux_asan_firefox/custom/firefox/firefox-bin+0x4bea3b) Shadow bytes around the buggy address: 0x0c4680003fe0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c4680003ff0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c4680004000: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c4680004010: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c4680004020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa =>0x0c4680004030: fa fa fa fa fa[fa]fa fa fa fa fa fa fa fa fa fa 0x0c4680004040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c4680004050: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c4680004060: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c4680004070: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c4680004080: 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 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 ==13447==ABORTING

Daniel: please direct this bug to someone in the SVG team that can work on it.

I'll take a look to start out... Abishek, could you let me know what build/OS you're using? I can't reproduce any ASAN issues on linux, using the latest inbound ASAN build: http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-linux64-asan-debug/1470715521/ I just get this when loading the attached file: > JavaScript error: file:///...path-to-file.../test.svg, line 15: SyntaxError: expected expression, got '}' but no ASAN errors.

20160807213804 was previous build i used. i just tried with tip-of-tree trunk (6cf0089510fa) and easily reproduces with that svg file. you need to use release build. also my clang revision is r277962. and this was on linux.

(In reply to Abhishek Arya from comment #3) > you need to use release build. (I assume you mean an optimized build [not a build with release branding] -- gotcha.) > also my clang revision is r277962. and this was on linux. OK, thanks. Have you tested with a Mozilla-provided ASAN-enabled optimized Firefox build, like this one from today: https://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-linux64-asan/1470837316/firefox-51.0a1.en-US.linux-x86_64-asan.tar.bz2 ? I just tried that one and still can't reproduce. I'm building locally as well, but given that I've failed to repro with 2 different types of ASAN builds so far, I'm wondering if there's another factor that I'm missing. (And if you could test [or let me know if you've already tested] a build like the FTP one above, that would rule out the possibility of something special in your build environment.)

I just finished & tested my own local [enable-debug,enable-opt] ASAN Firefox build, as well -- couldn't repro any ASAN issues. I happen to be using clang from r278176 (~trunk), and mozilla-central c12bb83ad278 (also ~trunk). I'll try Abhishek's clang/m-c revision next, to rule out the possibility of any differences there. But I suspect there's some other factor that I'm missing here.

(In reply to Daniel Holbert [:dholbert] (recovering from vacation reviews/bugmail) from comment #5) > I'll try Abhishek's clang/m-c revision next, to rule out the possibility of > any differences there. OK, I've found that I can reproduce with those revisions.

Here's a reduced / cleaned-up testcase that still triggers the crash for me (using a debug+opt ASAN-enabled build with the mozilla-central/clang revisions noted in comment 3).

er, s/still triggers the crash/still triggers the ASAN report/abort.

Here's a debugging/diagnostic patch which demonstrates what the problem is, on my machine. It seems the compiler is assuming (incorrectly in this case) that signed integer overflow will never happen. IIRC, that is something that compilers are allowed to assume. We end up with an (overflowed) hugely-negative number, which the code *purportedly* clamps to be nonnegative -- BUT in actual fact, we skip the "lx < 0" check (and the clamping code) because the compiler is mistakenly assuming the value is still positive. Here's the tail end of the printf output from this patch, leading up to where we get killed by ASAN: { Initial lx: 2147183239 Adding 32767 to lx Do we think lx=2147216006 is negative & needs clamping? No. Initial lx: 2147254082 Adding 32767 to lx Do we think lx=2147286849 is negative & needs clamping? No. Initial lx: 2147324924 Adding 32767 to lx Do we think lx=2147357691 is negative & needs clamping? No. Initial lx: 2147395766 Adding 32767 to lx Do we think lx=2147428533 is negative & needs clamping? No. Initial lx: 2147466609 Adding 32767 to lx OH NO, we're about to wrap lx past INT32_MAX! Upcoming clamp might fail to catch this... Do we think lx=-2147467920 is negative & needs clamping? No. ***dholbert Calling WRITE with nmiddle=1029 (x=0, width=32964, startmask=0, endmask=15) (rx=12865573, lx=-2147467920) (rxi=196, lxi=-32768) ================================================================= ==12873==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x623000122378 at pc 0x7f5c6c3238c5 bp 0x7fffa9076e30 sp 0x7fffa9076e28 } Note in particular the final "Do we think lx=-2147467920 is negative...No" report there. :-/ That's the problem.

This particular chunk of code (the addition to & clamping of "lx") hasn't changed since a pixman update that happened in bug 562087 (back in 2010), BTW. And it looks like we had a version of this bug (potential for overflow & failure-to-clamp) before that, too. This bug exists in the latest version of the pixman code that I can find, too (i.e. it may trigger signed-overflow and then fail to clamp a negative |lx| value): > lx += X_FRAC_FIRST(1) - pixman_fixed_e; > rx += X_FRAC_FIRST(1) - pixman_fixed_e; > #endif > /* clip X */ > if (lx < 0) > lx = 0; https://cgit.freedesktop.org/pixman/tree/pixman/pixman-edge-imp.h#n58 Ideally this should get fixed in pixman, but that's perhaps pretty fragile... Perhaps we can also add some sanity-checking in the code that produces such large |lx| values, to clamp it such that it can never get this close to INT32_MAX.

Jeff, maybe this falls under your wheelhouse, as being a bug in cairo... do you have any ideas on what we should do here? Perhaps we should add a spot fix here to check that lx and rx aren't going to overflow *before* we do the addition? (since checking for after the overflow already happened isn't guaranteed to work, due to compiler optimizations) See my description of the bug in comment 10. As shown in the output there, this value |lx| is overflowing into negative territory, and it's used to determine where we write some memory, so we end up writing into hugely negative memory.

>so we end up writing into hugely negative memory. (By which I meant to say: we end up writing at a hugely negative offset from some pointer, i.e. at a bogus location.)

(In reply to Daniel Holbert [:dholbert] from comment #11) I'd like to understand how the compiler is dropping the check. Can you perhaps post the annotated assembly?

Here's a snippet of the annotated disassembly of "rasterize_edges_1". The source annotation is missing some source code -- it just shows us assigning |lx| and then checking if it's less than 0, and it's missing the lines of addition that I quoted in comment 11 (adding X_FRAC_FIRST(1) - pixman_fixed_e) But I think that addition is present in the annotation - I do see an "add" instruction in between the bunch of assembly between the assignment & the less-than-zero-check, at least.

I see different assembler from Daniel's. It appears this code essentially does this instead: if ("original lx before the increment" < -32767) lx = 0; which clearly ignores any overflow from the addition. (this is an Opt ASAN build on 64-bit Linux with clang 3.8.0)

If I use an unsigned type to do the addition then it seems to work, presumably because unsigned overflow is well-defined: - lx += X_FRAC_FIRST(1) - pixman_fixed_e; - rx += X_FRAC_FIRST(1) - pixman_fixed_e; + uint32_t lx_unsigned = lx; + lx = lx_unsigned + (X_FRAC_FIRST(1) - pixman_fixed_e); + uint32_t rx_unsigned = rx; + rx = rx_unsigned + (X_FRAC_FIRST(1) - pixman_fixed_e); I'm not sure how we typically fix stuff like this though. Any suggestions?

Reassigning to Graphics since the underlying bug is in Cairo.

Milan is there somebody who can look at this? This externally reported sec-critical bug is about a month old. Thanks!

Does this actually happen with non-ASAN builds? I couldn't quite tell from the different comments.

On Windows, the attached testcases show rendering issues with hardware acceleration disabled, but they don't crash. No asserts with debug builds. However, the switch to the Skia backend for unaccelerated layers (bug 1007702) has made this go away on m-c tip, i.e. rendering now matches the accelerated rendering. Per dholbert on IRC, the root pixman issue has probably been around since ever, so removing the regressionwindow-wanted keyword.

(In reply to Milan Sreckovic [:milan] from comment #20) > Does this actually happen with non-ASAN builds? I couldn't quite tell from > the different comments. I'm pretty sure. Since it depends on a compiler optimization, this does require optimizations to be enabled (i.e. not the sort of build we normally compile/test locally). I don't have an enable-optimize build handy, so I can't immediately check, but I'm rebuilding & can confirm soon. My attached diagnostic patch should make it easy to detect if/when this bug happens, in any build (particularly opt builds). The key "printf" output to look for is something like this: > OH NO, we're about to wrap lx past INT32_MAX! Upcoming clamp might fail to catch this... > Do we think lx=-2147467920 is negative & needs clamping? No. "lx" is used as an assumed-to-be-positive offset for a write shortly after that point (and that's the dangerous/exploitable part).

OK -- Milan, I can confirm that normal (non-ASAN) optimized builds **are indeed affected by this bug**, using my diagnostic patch. (I compiled my local opt build using Ubuntu 16.04's stock clang 3.8 version, if it matters.) With my diagnostic patch applied, I see this go by in my terminal output when I load & hover testcase 3 (after grepping for "OH NO" to narrow in on the bad part): > Adding 32767 to lx > OH NO, we're about to wrap lx past INT32_MAX! Upcoming clamp might fail to catch this... > Do we think lx=-2147467920 is negative & needs clamping? No. > ***dholbert Calling WRITE with nmiddle=1029 (x=0, width=32964, startmask=0, endmask=15) (rx=12865573, lx=-2147467920) (rxi=196, lxi=-32768) Note that I had to manually enable cairo as my rendering backend here (by setting gfx.content.azure.backends=cairo) in order to enable this codepath. We recently switched to prefer/mandate skia on trunk, in bug 1278957. But we're still shipping cairo in version 49 and 50, and as a fallback renderer on Windows, so we do still need to care about it I think.

I'm particularly enjoying this from comment 23: Do we think lx=-2147467920 is negative & needs clamping? No. Yikes.

Yeah, that's the problem. And then we use that as an (assumed-to-be-nonnegative) offset into a buffer, and we write data into that location. Mats' suggestion in comment 17 seems like a reasonable workaround to me, FWIW (just doing the arithmetic in unsigned space and then casting back to signed).

I don't mind changing the code, in order to fix this particular bug, but what are the chances of this being the only place where something like that happens? It'd be interesting to try if -fwrapv option changes the behaviour at all.

(In reply to Milan Sreckovic [:milan] from comment #26) > I don't mind changing the code, in order to fix this particular bug, but > what are the chances of this being the only place where something like that > happens? I'm worried about that, too. :/ I was hoping someone who knows the cairo code (Jeff?) might have a guess about (or might be able to quickly audit for) the presence of other instances of this pattern that we'd need to fix at the same time. We do probably want to report this bug to upstream cairo developers, too -- do we have a contact who works on cairo who we could CC here for thoughts/expertise?

On the upstream note - this code is still the same in the latest pixman from what I can see. Also, does this https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html apply here? We're not currently using it, and it could be GCC only.

Milan, what do you think should happen going forward? Looks like there is some ambiguity about how to best approach this. But it is a sec critical issue so we don't want to let it sit too long.

Yes, we're not completely sure what to do with this as a general issue, though perhaps could deal with it in this instance. Also, as of 52, we're moving to Skia where the problem goes away. Still, let me see if we can do something.

Not sure this is worth it (see my previous comment), but should take care of this particular instance of a problem. Don't know what kind of performance impact it may have.

I'd prefer this as this code is somewhat performance sensitive and this seems like it might be more palatable to upstream.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #32) > Created attachment 8800006 [details] [diff] [review] > Cast to unsigned for arithmetic > > I'd prefer this as this code is somewhat performance sensitive and this > seems like it might be more palatable to upstream. Pretty sure you attached a wrong patch.

Liz or Ritu, will this be able to make beta if we have a reviewed patch?

(In reply to Al Billings [:abillings] from comment #35) > Liz or Ritu, will this be able to make beta if we have a reviewed patch? Hi Al, given that this is a sec-crit, yes we can take it in Beta. I hope it lands today/tomm am PST so it gets included in 50.0b9 (gtb Thursday noon PST).

Comment on attachment 8800854 [details] [diff] [review] Cast to unsigned for arithmetic Review of attachment 8800854 [details] [diff] [review]: ----------------------------------------------------------------- Not sure if you meant to request review on this latest patch, but in the interests of getting this landed, I'll jump in & mark this r=me, assuming: - you've tested it locally & made sure it does fix the overflow. - you add a sane commit message that doesn't give away too much.

Comment on attachment 8798497 [details] [diff] [review] Check for overflow. (I think this earlier patch is obsolete, too --> marking as such.)

Comment on attachment 8800854 [details] [diff] [review] Cast to unsigned for arithmetic Looks like Jeff isn't online currently, so I'll jump-start the approvals... [Security approval request comment] How easily could an exploit be constructed based on the patch? Not really. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Nope. Which older supported branches are affected by this flaw? All, most likely. If not all supported branches, which bug introduced the flaw? N/A Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This patch should apply to all branches. How likely is this patch to cause regressions; how much testing does it need? Very unlikely to cause regressions. Approval Request Comment [Feature/regressing bug #]: - [User impact if declined]: sec-critical [Describe test coverage new/current, TreeHerder]: I suspect this code is covered by a lot of tests in general (although not with the extreme input values that the testcases on this bug creates). As usual, we'll wait with landing these tests until the bug is public. [Risks and why]: very low risc [String/UUID change made/needed]: none

I'll wait for a sec-approval+ before taking it in beta50 and aurora51. If someone can assess whether esr45 is affected or not, that would be great!

It's a really old issue in pixman, so I'm about 99% sure that esr45 is also affected.

Yup -- per comment 11, we've had this bug (via this chunk of pixman code) since 2010 or earlier, so esr45 is definitely affected.

Thanks Dan and Ryan. Hello Mats, could you please nominate a patch for uplift to ESR45 as well? Thanks!

(nominated. I would've done so sooner, in comment 42, but I misread the flags that RyanVM had tweaked & thought he'd already nominated it.)

Comment on attachment 8800854 [details] [diff] [review] Cast to unsigned for arithmetic sec-approval+ a=dveditz/ritu for aurora, beta, and esr45

https://hg.mozilla.org/releases/mozilla-aurora/rev/6abc8be2325d29eeabdd59de838907cf0c0dbd44 https://hg.mozilla.org/releases/mozilla-beta/rev/d4fe6876afeb14bfa005e24b9e47260d9a2b4e1c https://hg.mozilla.org/releases/mozilla-esr45/rev/5e39c1c2fded8a33099b0f0266a258b5673c79e0 Unclear to me if this needs to still land on trunk with the move to skia.

I pushed this: https://hg.mozilla.org/integration/mozilla-inbound/rev/fbd443ce8fb5b8a1897baac3bfe62f8c5c5b054f

I've managed to reproduce this bug with an inbound ASAN build (2016-08-04) and using testcase 3 on Ubuntu 16.04 x64 LTS. This issue is verified fixed on latest Nightly 52.0a1 (2016-11-07) and latest Aurora 51.a2 (2016-11-07) using the same platform mentioned above. I can no longer reproduce the crash and I don't see any rendering issues. -- On 45.5.0 esr (20161031153904) I cannot see the testcase 3 (interactive -- hover blue area to test), the page appears blank when I load the testcase. You can see here: https://goo.gl/zNp9xF -- On 50.0-build2 (20161104212021) there are some rendering issues but the tab is not crashing. See here: https://goo.gl/J6G0YX Jeff, Wes what do you think about this results in 45.5.0 esr and 50 RC2?

> Jeff, Wes what do you think about this results in 45.5.0 esr and 50 RC2? (comment 49)

I'm not worried about 45.5 and 50 results in the context of this bug. Looks like the crash got fixed. If those problems happen in those versions, we should probably open another bug.

Thank you, Milan. I will close this issue as verified fixed since I can no longer reproduce the crashes on 45.5.0 esr, 50, 51 beta 2 and 52 latest Aurora.