1515052 - AddressSanitizer: heap-use-after-free /builds/worker/workspace/build/src/dom/canvas/WebGLContext.cpp:2259:44 in mozilla::WebGLContext::FuncScope::FuncScope(mozilla::WebGLContext const&, char const*)

Status: VERIFIED FIXED

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

Description

[Jason Kratzer [:jkratzer]]

Found while fuzzing mozilla-central rev 7ce7e9407a75.  I'm currently reducing the testcase and will update once complete.

==5201==ERROR: AddressSanitizer: heap-use-after-free on address 0x61b0000a5e08 at pc 0x7fef142a65b2 bp 0x7ffea02e4560 sp 0x7ffea02e4558
READ of size 8 at 0x61b0000a5e08 thread T0 (file:// Content)
    #0 0x7fef142a65b1 in mozilla::WebGLContext::FuncScope::FuncScope(mozilla::WebGLContext const&, char const*) /builds/worker/workspace/build/src/dom/canvas/WebGLContext.cpp:2259:44
    #1 0x7fef1430552a in mozilla::WebGLExtensionDebugShaders::GetTranslatedShaderSource(mozilla::WebGLShader const&, nsTSubstring<char16_t>&) const /builds/worker/workspace/build/src/dom/canvas/WebGLExtensionDebugShaders.cpp:27:33
    #2 0x7fef12f76c37 in mozilla::dom::WEBGL_debug_shaders_Binding::getTranslatedShaderSource(JSContext*, JS::Handle<JSObject*>, mozilla::WebGLExtensionDebugShaders*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/WebGLRenderingContextBinding.cpp:8861:9
    #3 0x7fef1403d584 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3064:13
    #4 0x7fef1bb5366d in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:443:13
    #5 0x7fef1bb5366d in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:535
    #6 0x7fef1cc89afe in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jit/BaselineIC.cpp:3909:10
    #7 0x2f87b23b3d47  (<unknown module>)

0x61b0000a5e08 is located 136 bytes inside of 1472-byte region [0x61b0000a5d80,0x61b0000a6340)
freed by thread T0 (file:// Content) here:
    #0 0x55ac1f7bba12 in __interceptor_free /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:124:3
    #1 0x7fef0c8bacd8 in MaybeKillObject /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2428:29
    #2 0x7fef0c8bacd8 in SnowWhiteKiller::Visit(nsPurpleBuffer&, nsPurpleBufferEntry*) /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2457
    #3 0x7fef0c88de05 in void nsPurpleBuffer::VisitEntries<SnowWhiteKiller>(SnowWhiteKiller&) /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:941:23
    #4 0x7fef0c88f5e8 in nsCycleCollector::FreeSnowWhiteWithBudget(js::SliceBudget&) /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2622:14
    #5 0x7fef0ee15a59 in AsyncFreeSnowWhite::Run() /builds/worker/workspace/build/src/js/xpconnect/src/XPCJSRuntime.cpp:141:9
    #6 0x7fef0cab14ae in Run /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:317:22
    #7 0x7fef0cab14ae in IdleRunnableWrapper::TimedOut(nsITimer*, void*) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:323
    #8 0x7fef0cac0d9a in nsTimerImpl::Fire(int) /builds/worker/workspace/build/src/xpcom/threads/nsTimerImpl.cpp:559:7
    #9 0x7fef0ca79987 in nsTimerEvent::Run() /builds/worker/workspace/build/src/xpcom/threads/TimerThread.cpp:259:11
    #10 0x7fef0ca90d28 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1157:14
    #11 0x7fef0ca99add in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:468:10
    #12 0x7fef16937b34 in SpinEventLoopUntil<mozilla::ProcessFailureBehavior::ReportToCaller, (lambda at /builds/worker/workspace/build/src/dom/xhr/XMLHttpRequestMainThread.cpp:2888:31)> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:335:25
    #13 0x7fef16937b34 in mozilla::dom::XMLHttpRequestMainThread::SendInternal(mozilla::dom::BodyExtractorBase const*, bool) /builds/worker/workspace/build/src/dom/xhr/XMLHttpRequestMainThread.cpp:2888
    #14 0x7fef169355ad in mozilla::dom::XMLHttpRequestMainThread::Send(JSContext*, mozilla::dom::Nullable<mozilla::dom::DocumentOrBlobOrArrayBufferViewOrArrayBufferOrFormDataOrURLSearchParamsOrUSVString> const&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/xhr/XMLHttpRequestMainThread.cpp:2665:11
    #15 0x7fef132821e3 in mozilla::dom::XMLHttpRequest_Binding::send(JSContext*, JS::Handle<JSObject*>, mozilla::dom::XMLHttpRequest*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/XMLHttpRequestBinding.cpp:1338:9
    #16 0x7fef1403d584 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3064:13
    #17 0x7fef1bb5366d in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:443:13
    #18 0x7fef1bb5366d in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:535
    #19 0x7fef1cc89afe in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jit/BaselineIC.cpp:3909:10
    #20 0x2f87b23b3d47  (<unknown module>)
    #21 0x62100097974f  (<unknown module>)
    #22 0x2f87b23af4de  (<unknown module>)
    #23 0x7fef1ce5850d in EnterBaseline /builds/worker/workspace/build/src/js/src/jit/BaselineJIT.cpp:124:5
    #24 0x7fef1ce5850d in js::jit::EnterBaselineAtBranch(JSContext*, js::InterpreterFrame*, unsigned char*) /builds/worker/workspace/build/src/js/src/jit/BaselineJIT.cpp:202

previously allocated by thread T0 (file:// Content) here:
    #0 0x55ac1f7bbd93 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:146:3
    #1 0x55ac1f7ef79d in moz_xmalloc /builds/worker/workspace/build/src/memory/mozalloc/mozalloc.cpp:68:15
    #2 0x7fef141b19c8 in operator new /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:131:10
    #3 0x7fef141b19c8 in Create /builds/worker/workspace/build/src/dom/canvas/WebGL2Context.cpp:38
    #4 0x7fef141b19c8 in mozilla::dom::CanvasRenderingContextHelper::CreateContextHelper(mozilla::dom::CanvasContextType, mozilla::layers::LayersBackend) /builds/worker/workspace/build/src/dom/canvas/CanvasRenderingContextHelper.cpp:130
    #5 0x7fef14b307cc in mozilla::dom::HTMLCanvasElement::CreateContext(mozilla::dom::CanvasContextType) /builds/worker/workspace/build/src/dom/html/HTMLCanvasElement.cpp:398:7
    #6 0x7fef141b20c0 in mozilla::dom::CanvasRenderingContextHelper::GetContext(JSContext*, nsTSubstring<char16_t> const&, JS::Handle<JS::Value>, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/canvas/CanvasRenderingContextHelper.cpp:155:15
    #7 0x7fef14b38616 in mozilla::dom::HTMLCanvasElement::GetContext(JSContext*, nsTSubstring<char16_t> const&, JS::Handle<JS::Value>, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/html/HTMLCanvasElement.cpp:907:40
    #8 0x7fef13a797eb in mozilla::dom::HTMLCanvasElement_Binding::getContext(JSContext*, JS::Handle<JSObject*>, mozilla::dom::HTMLCanvasElement*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/HTMLCanvasElementBinding.cpp:288:49
    #9 0x7fef1403d584 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3064:13
    #10 0x7fef1bb5366d in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:443:13
    #11 0x7fef1bb5366d in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:535
    #12 0x7fef1bb3cdf2 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:594:10
    #13 0x7fef1bb3cdf2 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3320
    #14 0x7fef1bb1fd16 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:423:10
    #15 0x7fef1bb54011 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:563:13
    #16 0x7fef1bb55c92 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:606:8
    #17 0x7fef1c7436e6 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jsapi.cpp:2649:10
    #18 0x7fef1364a549 in mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/EventListenerBinding.cpp:52:8
    #19 0x7fef1488c642 in HandleEvent<mozilla::dom::EventTarget *> /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/EventListenerBinding.h:66:12
    #20 0x7fef1488c642 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1039
    #21 0x7fef1488ec73 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1238:17
    #22 0x7fef1486f686 in HandleEvent /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/EventListenerManager.h:350:5
    #23 0x7fef1486f686 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:346
    #24 0x7fef1486d908 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:548:16
    #25 0x7fef14874360 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:1038:11

SUMMARY: AddressSanitizer: heap-use-after-free /builds/worker/workspace/build/src/dom/canvas/WebGLContext.cpp:2259:44 in mozilla::WebGLContext::FuncScope::FuncScope(mozilla::WebGLContext const&, char const*)
Shadow bytes around the buggy address:
  0x0c368000cb70: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c368000cb80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c368000cb90: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c368000cba0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c368000cbb0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c368000cbc0: fd[fd]fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c368000cbd0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c368000cbe0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c368000cbf0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c368000cc00: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c368000cc10: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  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
  Shadow gap:              cc
==5201==ABORTING

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: testcase.html

Comment 2

[Jason Kratzer [:jkratzer]]

Attachment: index.html

In order to improve reliability, the testcase must be accessed via index.html.  This will open that actual testcase in multiple tabs.

Comment 3

[Jason Kratzer [:jkratzer]]

Attachment: testcase.html

The previously attached testcase.html is for the wrong bug.  Fixed here.

Comment 4

[Andrew McCreight [:mccr8]]

Looks like some sync XHR spinning a nested event loop shenanigans interacting badly with WebGL stuff.

Comment 5

[Jason Kratzer [:jkratzer]]

Lee, is there a reason this is P3 considering it's a sec-high with a testcase?

Comment 6

[Lee Salzman [:lsalzman]]

(In reply to Jason Kratzer [:jkratzer] from comment #5)
> Lee, is there a reason this is P3 considering it's a sec-high with a
> testcase?

P3 means the graphics team has nobody to immediately allocate to looking at this, since right now the priority is on WebRender. Jeff Gilbert would be best to reassign the priority if he thinks he can look into this more immediately, given that it is a WebGL bug.

Comment 7

[Kelsey Gilbert [:jgilbert]]

I think I know what this is.

Comment 8

[Kelsey Gilbert [:jgilbert]]

I don't see how we can get here?

We mark all extensions as lost in DestroyResourcesAndContext, and we call that from the WebGLContext dtor, so unless CC is bypassing the dtor (or there's a race condition somehow??), I don't see how we could reach the FuncScope ctor.

Comment 9

[Kelsey Gilbert [:jgilbert]]

I reproduced this one! \o/

Comment 10

[Kelsey Gilbert [:jgilbert]]

Ooooh crap, is WebGLContext::Unlink not calling ~WebGLContext, thus skipping DestroyResourcesAndContext??

Well that could certainly help explain our intermittant CI OOMs also!

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Jeff Gilbert [:jgilbert] from comment #10)

Ooooh crap, is WebGLContext::Unlink not calling ~WebGLContext, thus skipping DestroyResourcesAndContext??

Yeah, Unlink never calls the dtor. Having some kind of "destroy resources" method that gets called in both Unlink and the dtor is a fairly common pattern. (An Unlink method should also be able to deal with getting called on the same object multiple times, though if you are already making the dtor do the same cleanup as Unlink that shouldn't be an issue.)

Comment 12

[Kelsey Gilbert [:jgilbert]]

Looks like Unlink is clearing mExtensions, so our later pseudo-weak-ptr logic in DestroyResourcesAndContext sees a big ol' array of null! Oops!

Comment 14

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1515052 - Use WeakPtr for extension parent pointer.

Comment 15

[Kelsey Gilbert [:jgilbert]]

Please test this build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=909092cb3dc0496fe8aa0641db16f6fe22b5e514

I believe I have it fixed locally with that change.

Comment 16

[Jason Kratzer [:jkratzer]]

(In reply to Jeff Gilbert [:jgilbert] from comment #15)

Please test this build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=909092cb3dc0496fe8aa0641db16f6fe22b5e514

I believe I have it fixed locally with that change.

I can no longer trigger this issue using the patch in comment 14. Please note that I had to make an additional try build with --fuzzing enabled due to the calls to FuzzingFunctions.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 9066294 [details]
Bug 1515052 - Use WeakPtr for extension parent pointer.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Medium: Patching a raw pointer to a WeakPtr is a big hint that there's a UAF somewhere, but it's not necessarily clear why or how to incur it.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Easy to backport.
• How likely is this patch to cause regressions; how much testing does it need?: Very unlikely to cause regressions.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for checkin on June 4, two weeks into the new cycle.

Comment 19

[Kelsey Gilbert [:jgilbert]]

Attachment: esr60

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
• User impact if declined: UAF crash, sec-high
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Replaces a raw pointer with a weak_ptr, which is pretty safe.
  Pretty trivial rebase conflicts resolved for esr60 backport. Try run:
  https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=248812592&revision=83fdb6da850d28e2d0a27abd49dcb7fca650b13a
• String or UUID changes made by this patch: none

Comment 20

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 9066294 [details]
Bug 1515052 - Use WeakPtr for extension parent pointer.

Beta/Release Uplift Approval Request

• User impact if declined: sec-high UAF, possible crashes
• 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): No rebase conflicts, and we're largely replacing a raw pointer with a weak_ptr and adding null checks.
• String changes made/needed: none

Comment 21

[Kelsey Gilbert [:jgilbert]]

I forgot to ask for uplifts. :(

Comment 22

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/integration/autoland/rev/efb03ccd9427

Comment 23

[Julien Cristau [:jcristau]]

Comment on attachment 9066294 [details]
Bug 1515052 - Use WeakPtr for extension parent pointer.

webgl sec fix, approved for 68.0b8

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/efb03ccd9427

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/88114c48b1ee

Comment 26

[Rares Doghi, Desktop QA]

Hi, I managed to reproduce this issue in an older version of Firefox but this issue no longer occurs in Firefox beta 68.0b12 or our latest Nightly build 69.0a1 (2019-06-20) on Ubuntu 18.04. I will mark this issue accordingly.

Comment 27

[Kelsey Gilbert [:jgilbert]]

Great, thanks!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9069792 [details] [diff] [review]
esr60

Fixes a WebGL use-after-free. Approved for 60.8esr.

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/59cff574fac4

Comment 30

[Rares Doghi, Desktop QA]

I have marked this issue verified as fixed since it's a Won't fix for esr60.

Comment 31

[Rares Doghi, Desktop QA]

This issue is also verified as fixed in 60.7.3esr.I will mark this issue accordingly.
This is the build I used to verify this issue : https://tools.taskcluster.net/index/gecko.v2.mozilla-esr60.latest.firefox/linux64-fuzzing-asan-opt