Tyson Smith [:tsmith] Reporter
	Description • 6 years ago

==81806==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000408388 at pc 0x7f0f9a7ef6c9 bp 0x7ffe5a065370 sp 0x7ffe5a065368
READ of size 4 at 0x603000408388 thread T0 (file:// Content)
    #0 0x7f0f9a7ef6c8 in SkDashPath::InternalFilter(SkPath*, SkPath const&, SkStrokeRec*, SkRect const*, float const*, int, float, int, float, SkDashPath::StrokeRecApplication) src/gfx/skia/skia/src/utils/SkDashPath.cpp:378:31
    #1 0x7f0f9a1fee50 in SkDashImpl::filterPath(SkPath*, SkPath const&, SkStrokeRec*, SkRect const*) const src/gfx/skia/skia/src/effects/SkDashPathEffect.cpp:40:12
    #2 0x7f0f9a8b90a6 in SkPaint::getFillPath(SkPath const&, SkPath*, SkRect const*, float) const src/gfx/skia/skia/src/core/SkPaint.cpp:1498:37
    #3 0x7f0f9a4ceab3 in SkDraw::drawPath(SkPath const&, SkPaint const&, SkMatrix const*, bool, bool, SkBlitter*, SkInitOnceData*) const src/gfx/skia/skia/src/core/SkDraw.cpp:1120:25
    #4 0x7f0f9a4cc1dd in drawPath src/gfx/skia/skia/src/core/SkDraw.h:58:15
    #5 0x7f0f9a4cc1dd in draw_rect_as_path(SkDraw const&, SkRect const&, SkPaint const&, SkMatrix const*) src/gfx/skia/skia/src/core/SkDraw.cpp:735
    #6 0x7f0f9a4cb525 in SkDraw::drawRect(SkRect const&, SkPaint const&, SkMatrix const*, SkRect const*) const src/gfx/skia/skia/src/core/SkDraw.cpp:762:9
    #7 0x7f0f9a18bc4d in drawRect src/gfx/skia/skia/src/core/SkDraw.h:44:15
    #8 0x7f0f9a18bc4d in SkBitmapDevice::drawRect(SkRect const&, SkPaint const&) src/gfx/skia/skia/src/core/SkBitmapDevice.cpp:296
    #9 0x7f0f9a1bd641 in SkCanvas::onDrawRect(SkRect const&, SkPaint const&) src/gfx/skia/skia/src/core/SkCanvas.cpp:2041:27
    #10 0x7f0f9a1b4c2c in SkCanvas::drawRect(SkRect const&, SkPaint const&) src/gfx/skia/skia/src/core/SkCanvas.cpp:1712:11
    #11 0x7f0f907ff4f2 in mozilla::gfx::DrawTargetSkia::StrokeRect(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::Pattern const&, mozilla::gfx::StrokeOptions const&, mozilla::gfx::DrawOptions const&) src/gfx/2d/DrawTargetSkia.cpp:936:12
    #12 0x7f0f94f666ce in mozilla::dom::CanvasRenderingContext2D::StrokeRect(double, double, double, double) src/dom/canvas/CanvasRenderingContext2D.cpp:3151:5
    #13 0x7f0f9367320d in mozilla::dom::CanvasRenderingContext2D_Binding::strokeRect(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/CanvasRenderingContext2DBinding.cpp:5476:9
    #14 0x7f0f94e24759 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3311:13
    #15 0x7f0f9c0a1b0e in CallJSNative src/js/src/vm/Interpreter.cpp:445:15
    #16 0x7f0f9c0a1b0e in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:533
    #17 0x7f0f9c08c3f7 in CallFromStack src/js/src/vm/Interpreter.cpp:590:12
    #18 0x7f0f9c08c3f7 in Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3239
    #19 0x7f0f9c07280e in js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:425:12
    #20 0x7f0f9c0a23e4 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:557:15
    #21 0x7f0f9c0a3972 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) src/js/src/vm/Interpreter.cpp:603:10
    #22 0x7f0f9cb26b4a in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2915:12
    #23 0x7f0f944297fe in mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) src/obj-firefox/dom/bindings/EventListenerBinding.cpp:51:8
    #24 0x7f0f9569983e in HandleEvent<mozilla::dom::EventTarget *> src/obj-firefox/dist/include/mozilla/dom/EventListenerBinding.h:66:12
    #25 0x7f0f9569983e in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) src/dom/events/EventListenerManager.cpp:1108
    #26 0x7f0f9569b9c6 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) src/dom/events/EventListenerManager.cpp:1342:20
    #27 0x7f0f9567f5c9 in HandleEvent src/obj-firefox/dist/include/mozilla/EventListenerManager.h:390:5
    #28 0x7f0f9567f5c9 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:418
    #29 0x7f0f9567d883 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:635:16
    #30 0x7f0f956840de in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:1110:9
    #31 0x7f0f98284f7f in nsDocumentViewer::LoadComplete(nsresult) src/layout/base/nsDocumentViewer.cpp:1169:7
    #32 0x7f0f9aff7a4c in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) src/docshell/base/nsDocShell.cpp:7054:21
    #33 0x7f0f9aff262a in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp:6847:7
    #34 0x7f0f9affc207 in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp
    #35 0x7f0f905ca365 in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) src/uriloader/base/nsDocLoader.cpp:1309:3
    #36 0x7f0f905c8f8c in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp:852:14
    #37 0x7f0f905c4a91 in nsDocLoader::DocLoaderIsEmpty(bool) src/uriloader/base/nsDocLoader.cpp:741:9
    #38 0x7f0f905c7578 in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp:627:5
    #39 0x7f0f905c8ab4 in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp
    #40 0x7f0f8e0db467 in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) src/netwerk/base/nsLoadGroup.cpp:629:28
    #41 0x7f0f91e84c27 in DoUnblockOnload src/dom/base/nsDocument.cpp:8301:18
    #42 0x7f0f91e84c27 in nsDocument::UnblockOnload(bool) src/dom/base/nsDocument.cpp:8223
    #43 0x7f0f91e6168d in nsIDocument::DispatchContentLoadedEvents() src/dom/base/nsDocument.cpp:5095:3
    #44 0x7f0f91f232cb in applyImpl<nsIDocument, void (nsIDocument::*)()> src/obj-firefox/dist/include/nsThreadUtils.h:1168:12
    #45 0x7f0f91f232cb in apply<nsIDocument, void (nsIDocument::*)()> src/obj-firefox/dist/include/nsThreadUtils.h:1174
    #46 0x7f0f91f232cb in mozilla::detail::RunnableMethodImpl<nsIDocument*, void (nsIDocument::*)(), true, (mozilla::RunnableKind)0>::Run() src/obj-firefox/dist/include/nsThreadUtils.h:1219
    #47 0x7f0f8de37062 in mozilla::SchedulerGroup::Runnable::Run() src/xpcom/threads/SchedulerGroup.cpp:337:32
    #48 0x7f0f8de74420 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1235:14
    #49 0x7f0f8de7d185 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
    #50 0x7f0f8f052f1e in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
    #51 0x7f0f8ef5520c in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #52 0x7f0f8ef5520c in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #53 0x7f0f8ef5520c in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #54 0x7f0f97a191a6 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:158:27
    #55 0x7f0f9bd79e8e in XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:940:22
    #56 0x7f0f8ef5520c in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #57 0x7f0f8ef5520c in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #58 0x7f0f8ef5520c in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #59 0x7f0f9bd78f42 in XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:766:34
    #60 0x4f5b11 in content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #61 0x4f5b11 in main src/browser/app/nsBrowserApp.cpp:287
    #62 0x7f0fb38a282f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #63 0x424ee8 in _start (firefox+0x424ee8)

0x603000408388 is located 0 bytes to the right of 24-byte region [0x603000408370,0x603000408388)
allocated by thread T0 (file:// Content) here:
    #0 0x4c5633 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:88:3
    #1 0x51a34d in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:70:17
    #2 0x7f0f9a201473 in sk_malloc_throw src/gfx/skia/skia/include/private/SkMalloc.h:59:12
    #3 0x7f0f9a201473 in SkDashImpl src/gfx/skia/skia/src/effects/SkDashPathEffect.cpp:23
    #4 0x7f0f9a201473 in SkDashPathEffect::Make(float const*, int, float) src/gfx/skia/skia/src/effects/SkDashPathEffect.cpp:397
    #5 0x7f0f907fe3e5 in mozilla::gfx::StrokeOptionsToPaint(SkPaint&, mozilla::gfx::StrokeOptions const&) src/gfx/2d/HelpersSkia.h:156:32
    #6 0x7f0f907ff470 in mozilla::gfx::DrawTargetSkia::StrokeRect(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::Pattern const&, mozilla::gfx::StrokeOptions const&, mozilla::gfx::DrawOptions const&) src/gfx/2d/DrawTargetSkia.cpp:932:8
    #7 0x7f0f94f666ce in mozilla::dom::CanvasRenderingContext2D::StrokeRect(double, double, double, double) src/dom/canvas/CanvasRenderingContext2D.cpp:3151:5
    #8 0x7f0f9367320d in mozilla::dom::CanvasRenderingContext2D_Binding::strokeRect(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/CanvasRenderingContext2DBinding.cpp:5476:9
    #9 0x7f0f94e24759 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3311:13
    #10 0x7f0f9c0a1b0e in CallJSNative src/js/src/vm/Interpreter.cpp:445:15
    #11 0x7f0f9c0a1b0e in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:533
    #12 0x7f0f9c08c3f7 in CallFromStack src/js/src/vm/Interpreter.cpp:590:12
    #13 0x7f0f9c08c3f7 in Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3239
    #14 0x7f0f9c07280e in js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:425:12
    #15 0x7f0f9c0a23e4 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:557:15
    #16 0x7f0f9c0a3972 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) src/js/src/vm/Interpreter.cpp:603:10
    #17 0x7f0f9cb26b4a in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2915:12
    #18 0x7f0f944297fe in mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) src/obj-firefox/dom/bindings/EventListenerBinding.cpp:51:8
    #19 0x7f0f9569983e in HandleEvent<mozilla::dom::EventTarget *> src/obj-firefox/dist/include/mozilla/dom/EventListenerBinding.h:66:12
    #20 0x7f0f9569983e in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) src/dom/events/EventListenerManager.cpp:1108
    #21 0x7f0f9569b9c6 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) src/dom/events/EventListenerManager.cpp:1342:20
    #22 0x7f0f9567f5c9 in HandleEvent src/obj-firefox/dist/include/mozilla/EventListenerManager.h:390:5
    #23 0x7f0f9567f5c9 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:418
    #24 0x7f0f9567d883 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:635:16
    #25 0x7f0f956840de in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:1110:9

	Jason Kratzer [:jkratzer]
	Comment 1 • 6 years ago

Attached prefs are required to trigger the testcase (e10s disabled).

	Tyson Smith [:tsmith] Reporter
	Comment 2 • 6 years ago

Comment on attachment 9001365 [details]
prefs-default.js

This works with and without e10s.

	Lee Salzman [:lsalzman] Assignee
	Comment 3 • 6 years ago

Is there a Skia-specific testcase that you could report upstream? This looks like a Skia internal bug.

	Tyson Smith [:tsmith] Reporter
	Comment 4 • 6 years ago

This was found with a canvas fuzzer targeting the browser. So no skia specific test is available.

	Lee Salzman [:lsalzman] Assignee
	Comment 5 • 6 years ago

The problem here is that endPhase is a really big number, such that due to floating point imprecision subtracting a small non-zero number from it can still yield the exact same number, i.e. ReallyBigNumber - Epsilon == ReallyBigNumber.

We know that endPhase will start as a value less than intervalLength, but due to above-mentioned problem, it may never hit 0 via subtraction.

intervalLength is calculated as a sum of all the intervals here: https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/utils/SkDashPath.cpp?q=SkDashPath%3A%3ACalcDashParameters&redirect_type=direct#39

So to fix this, we need to sum the intervals until they exceed endPhase, which ensures that they are summed in the same order as intervalLength and will eventually get there given that endPhase is within bounds.

	Alex Gaynor [:Alex_Gaynor]
	Comment 6 • 6 years ago

Has this been fixed in upstream Skia, or do we need to notify them of this bug?

	Lee Salzman [:lsalzman] Assignee
	Comment 7 • 6 years ago

It's still broken upstream.

	Alex Gaynor [:Alex_Gaynor]
	Comment 8 • 6 years ago

Added awhalley from Chromium; not if there's anything else involved in notifying them?

	Andrew Whalley
	Comment 9 • 6 years ago

Thanks - I've filed crbug.com/875494 to track this upstream

	Tyson Smith [:tsmith] Reporter
	Comment 10 • 6 years ago

ni? test for abillings (please ignore)

	Lee Salzman [:lsalzman] Assignee
	Comment 11 • 6 years ago

Comment on attachment 9001742 [details] [diff] [review]
sum SkDashPath intervals instead of subtracting

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

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

> Which older supported branches are affected by this flaw?
60+

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

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Patch should already apply.

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

	Al Billings [:abillings - ex-MoCo]
	Comment 12 • 6 years ago

I'd love to take this but we only have a single beta left, building tomorrow. I need to ni? release management and get their approval.

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 6 years ago

Let's take this. If it lands on inbound today, we'll have time to uplift in time for tomorrow's build.

	Al Billings [:abillings - ex-MoCo]
	Comment 14 • 6 years ago

Comment on attachment 9001742 [details] [diff] [review]
sum SkDashPath intervals instead of subtracting

Sec-approval+ for trunk. We'll need a beta patch and an ESR60 patch nominated ASAP to try to get tomorrow's final beta for 62.

	Lee Salzman [:lsalzman] Assignee
	Comment 15 • 6 years ago

Comment on attachment 9001742 [details] [diff] [review]
sum SkDashPath intervals instead of subtracting

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1444506
[User impact if declined]: Heap overread
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Not very
[Why is the change risky/not risky?]: Just a small tweak to some Skia interval math to avoid floating-point precision issues.
[String changes made/needed]: None

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 6 years ago

Lee pushed this to inbound too:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54934de382c5

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 6 years ago

https://hg.mozilla.org/mozilla-central/rev/54934de382c5

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 6 years ago

Comment on attachment 9001742 [details] [diff] [review]
sum SkDashPath intervals instead of subtracting

Fixes a Skia sec issue, approved for 62.0b20 and ESR 60.2.

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/a6566a974207

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-esr60/rev/df3fb67f0972

	hcm
	Comment 21 • 6 years ago

Chromium/Skia bug fixed upstream

	Alex Gaynor [:Alex_Gaynor]
	Comment 22 • 6 years ago

For reference, which commit?

	Andrew Whalley
	Comment 23 • 6 years ago

https://skia.googlesource.com/skia/+/4c6514490e966198af427ec4df050470e55653a8

	Alex Gaynor [:Alex_Gaynor]
	Comment 24 • 6 years ago

Thanks!

	Alexandru Simonca [:asimonca], Release Desktop QA
	Comment 25 • 6 years ago

Hi,
I have verified the fix for this bug using the latest ASAN builds for Fx 62 and Fx 63 and Fx esr60 on Windows 10 x64, Ubuntu 16.04 LTS and Mac OS 10.13. The bug is Verified Fixed. Marking it accordingly.