1583970 - addition of unsigned offset overflowed in dom/canvas/WebGLTexelConversions.cpp:209

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P1)

Description

[Tyson Smith [:tsmith]]

This is triggered with an UBSan build. To enable this check add the following to your mozconfig:

ac_add_options --enable-address-sanitizer
ac_add_options --enable-undefined-sanitizer="pointer-overflow"
ac_add_options --disable-jemalloc

INFO - TEST-START | dom/canvas/test/webgl-conf/generated/test_conformance__extensions__oes-texture-float-with-canvas.html
...
src/dom/canvas/WebGLTexelConversions.cpp:209:19: runtime error: addition of unsigned offset to 0x6030006b6c90 overflowed to 0x6030006b6c80
    #0 0x7fb52dac15d6 in void mozilla::(anonymous namespace)::WebGLImageConverter::run<(mozilla::WebGLTexelFormat)27>(mozilla::WebGLTexelFormat, mozilla::WebGLTexelPremultiplicationOp) src/dom/canvas/WebGLTexelConversions.cpp
    #1 0x7fb52da46583 in run src/dom/canvas/WebGLTexelConversions.cpp:309:7
    #2 0x7fb52da46583 in mozilla::ConvertImage(unsigned long, unsigned long, void const*, unsigned long, mozilla::gl::OriginPos, mozilla::WebGLTexelFormat, bool, void*, unsigned long, mozilla::gl::OriginPos, mozilla::WebGLTexelFormat, bool, bool*) src/dom/canvas/WebGLTexelConversions.cpp:407
    #3 0x7fb52d945ea9 in mozilla::webgl::TexUnpackBlob::ConvertIfNeeded(mozilla::WebGLContext*, unsigned int, unsigned int, mozilla::WebGLTexelFormat, unsigned char const*, long, mozilla::WebGLTexelFormat, long, unsigned char const**, mozilla::UniqueBuffer*) const src/dom/canvas/TexUnpackBlob.cpp:373:8
    #4 0x7fb52d94ae28 in mozilla::webgl::TexUnpackSurface::TexOrSubImage(bool, bool, mozilla::WebGLTexture*, StrongGLenum<TexImageTargetDetails>, int, mozilla::webgl::DriverUnpackInfo const*, int, int, int, mozilla::webgl::PackingInfo const&, unsigned int*) const src/dom/canvas/TexUnpackBlob.cpp:864:8
    #5 0x7fb52da5883b in mozilla::WebGLTexture::TexImage(StrongGLenum<TexImageTargetDetails>, int, unsigned int, mozilla::webgl::PackingInfo const&, mozilla::webgl::TexUnpackBlob const*) src/dom/canvas/WebGLTextureUpload.cpp:1239:14
    #6 0x7fb52da5778b in mozilla::WebGLTexture::TexImage(StrongGLenum<TexImageTargetDetails>, int, unsigned int, int, int, int, int, mozilla::webgl::PackingInfo const&, mozilla::TexImageSource const&) src/dom/canvas/WebGLTextureUpload.cpp:475:3
    #7 0x7fb52d9ccac2 in mozilla::WebGLContext::TexImage(unsigned char, unsigned int, int, unsigned int, int, int, int, int, unsigned int, unsigned int, mozilla::TexImageSource const&) src/dom/canvas/WebGLContextTextures.cpp:339:8
    #8 0x7fb52cefecfd in TexImage2D src/dom/canvas/WebGLContext.h:1366:5
    #9 0x7fb52cefecfd in TexImage2D<mozilla::dom::HTMLCanvasElement> src/dom/canvas/WebGLContext.h:1344
    #10 0x7fb52cefecfd in void mozilla::WebGLContext::TexImage2D<mozilla::dom::HTMLCanvasElement>(unsigned int, int, unsigned int, unsigned int, unsigned int, mozilla::dom::HTMLCanvasElement const&, mozilla::ErrorResult&) src/dom/canvas/WebGLContext.h:1322
    #11 0x7fb52cfc1bb7 in mozilla::dom::WebGLRenderingContext_Binding::texImage2D(JSContext*, JS::Handle<JSObject*>, mozilla::WebGLContext*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/WebGLRenderingContextBinding.cpp:13743:32
    #12 0x7fb52d81784c 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:3250:13
    #13 0x7fb533381554 in CallJSNative src/js/src/vm/Interpreter.cpp:458:13
    #14 0x7fb533381554 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) src/js/src/vm/Interpreter.cpp:551
    #15 0x7fb533383479 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) src/js/src/vm/Interpreter.cpp:620:10
    #16 0x7fb533368972 in CallFromStack src/js/src/vm/Interpreter.cpp:624:10
    #17 0x7fb533368972 in Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3113
    #18 0x7fb53334b3d5 in js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:424:10
    #19 0x7fb533381656 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) src/js/src/vm/Interpreter.cpp:592:13
    #20 0x7fb533383479 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) src/js/src/vm/Interpreter.cpp:620:10
    #21 0x7fb533383844 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) src/js/src/vm/Interpreter.cpp:637:8
    #22 0x7fb5334920a4 in js::PromiseObject::create(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, bool) src/js/src/builtin/Promise.cpp:2236:15
    #23 0x7fb5334d2adf in PromiseConstructor(JSContext*, unsigned int, JS::Value*) src/js/src/builtin/Promise.cpp:2157:7
    #24 0x7fb53338425a in CallJSNative src/js/src/vm/Interpreter.cpp:458:13
    #25 0x7fb53338425a in CallJSNativeConstructor src/js/src/vm/Interpreter.cpp:474
    #26 0x7fb53338425a in InternalConstruct(JSContext*, js::AnyConstructArgs const&) src/js/src/vm/Interpreter.cpp:664
    #27 0x7fb533383aef in js::ConstructFromStack(JSContext*, JS::CallArgs const&) src/js/src/vm/Interpreter.cpp:710:10
    #28 0x7fb53335e41a in Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3104:16
    #29 0x7fb53334b3d5 in js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:424:10
    #30 0x7fb533381656 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) src/js/src/vm/Interpreter.cpp:592:13
    #31 0x7fb533383479 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) src/js/src/vm/Interpreter.cpp:620:10
    #32 0x7fb533383844 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) src/js/src/vm/Interpreter.cpp:637:8
    #33 0x7fb533cc3f13 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2728:10
    #34 0x7fb52d307fac in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:267:37
    #35 0x7fb52de7285b in void mozilla::dom::EventHandlerNonNull::Call<nsCOMPtr<mozilla::dom::EventTarget> >(nsCOMPtr<mozilla::dom::EventTarget> const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:363:12
    #36 0x7fb52de70861 in mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) src/dom/events/JSEventHandler.cpp:205:12
    #37 0x7fb52de34e6f in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) src/dom/events/EventListenerManager.cpp:1039:22
    #38 0x7fb52de363a1 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) src/dom/events/EventListenerManager.cpp:1231:17
    #39 0x7fb52de23ebb in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:349:17
    #40 0x7fb52de227fa in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:551:16
    #41 0x7fb52de26e0b in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:1045:11
    #42 0x7fb53000a8b5 in nsDocumentViewer::LoadComplete(nsresult) src/layout/base/nsDocumentViewer.cpp:1170:7
    #43 0x7fb532745a1a in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) src/docshell/base/nsDocShell.cpp:6564:20
    #44 0x7fb532744c34 in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp:6342:7
    #45 0x7fb53274949f in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp
    #46 0x7fb52aaa1510 in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) src/uriloader/base/nsDocLoader.cpp:1346:3
    #47 0x7fb52aaa0656 in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp:905:14
    #48 0x7fb52aa9c872 in nsDocLoader::DocLoaderIsEmpty(bool) src/uriloader/base/nsDocLoader.cpp:731:9
    #49 0x7fb52aa9f13e in nsDocLoader::OnStopRequest(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp:619:5
    #50 0x7fb52aaa01dc in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp
    #51 0x7fb528f4753c in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) src/netwerk/base/nsLoadGroup.cpp:568:22

Comment 1

[Tyson Smith [:tsmith]]

Fixing this issue will allow us to remove the suppression list for the UBSan 'pointer-overflow' check. This is the last known issue of this type that can be found by the tests.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Which OS?

Comment 3

[Tyson Smith [:tsmith]]

This was found on Linux.

Comment 4

[Kelsey Gilbert [:jgilbert]]

That's weird:

    const ptrdiff_t dstStrideInElements = mDstStride / sizeof(DstType);
...
    DstType* dstRowStart = static_cast<DstType*>(mDstStart);
...
L209:      dstRowStart += dstStrideInElements;

Comment 5

[Kelsey Gilbert [:jgilbert]]

I can repro the STR, but I can't minimize it so far.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Oops: ptrdiff_t x / size_t y => size_t

mDstStride: fffffffffffffff0
sizeof(DstType): 4
0x6030008691e0 += 3ffffffffffffffc
0x6030008691e0 += 4611686018427387900

Comment 7

[Kelsey Gilbert [:jgilbert]]

    const ptrdiff_t stride = -16;
    const auto elem_size = sizeof(uint32_t);
    printf_stderr("%ti / %ti = %ti\n", stride, elem_size, stride / elem_size);

-16 / 4 = 4611686018427387900

Gosh darned new maths. :)

Comment 8

[Kelsey Gilbert [:jgilbert]]

A repl for this exciting integer promotion footgun:
https://repl.it/repls/OblongIdealisticPattern

Comment 9

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1583970 - Cast sizeof to ptrdiff_t to avoid `-16 / 4 = big`.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a628b702db6
Cast sizeof to ptrdiff_t to avoid `-16 / 4 = big`. r=tsmith

Comment 11

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/3a628b702db6

Comment 12

[Pascal Chevrel:pascalc (PTO until April 26)]

Jeff, this bug is marked as P1 and affecting 71, as don't understand what the problem that was fixed was, could you tell me if there is end-user value in uplifting to 71 beta or can it just ride the trains? Thanks

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 9103414 [details]
Bug 1583970 - Cast sizeof to ptrdiff_t to avoid -16 / 4 = big.

Beta/Release Uplift Approval Request

• User impact if declined: None that we know of, but potentially buggy/dangerous implementation on ARM?
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's a trivial type change, and it's clear to me now why this change is needed. (unfortunately!)
• String changes made/needed: none

Comment 14

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9103414 [details]
Bug 1583970 - Cast sizeof to ptrdiff_t to avoid -16 / 4 = big.

Potential defect fix on ARM, covered by tests and baked on nightly for a week, uplift approved for 71 beta 6, thanks.

Comment 15

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/98e2a38a4d78