AddressSanitizer: SEGV /builds/worker/workspace/build/src/gfx/gl/../../mfbt/UniquePtr.h:326:32 in get

RESOLVED FIXED in Firefox 61
(NeedInfo from)

Status

()

defect
--
critical
RESOLVED FIXED
11 months ago
6 months ago

People

(Reporter: jkratzer, Assigned: kip, NeedInfo)

Tracking

(Blocks 1 bug, {crash, testcase})

59 Branch
mozilla62
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 wontfix, firefox60 wontfix, firefox61 fixed, firefox62 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

11 months ago
Posted file trigger.html
Testcase found while fuzzing mozilla-central rev 11ee70f24ea5.

Marking SS just in case.

==12124==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000a18 (pc 0x7fa20c7fbe09 bp 0x7ffd26bff710 sp 0x7ffd26bff700 T0)
==12124==The signal is caused by a READ memory access.
==12124==Hint: address points to the zero page.
    #0 0x7fa20c7fbe08 in get /builds/worker/workspace/build/src/gfx/gl/../../mfbt/UniquePtr.h:326:32
    #1 0x7fa20c7fbe08 in Screen /builds/worker/workspace/build/src/gfx/gl/GLContext.h:3578
    #2 0x7fa20c7fbe08 in mozilla::WebGLContext::GetVRFrame() /builds/worker/workspace/build/src/dom/canvas/WebGLContext.cpp:2387
    #3 0x7fa209a47762 in mozilla::gfx::VRLayerChild::SubmitFrame(unsigned long) /builds/worker/workspace/build/src/gfx/vr/ipc/VRLayerChild.cpp:67:39
    #4 0x7fa20e5eee01 in mozilla::dom::VRDisplay::SubmitFrame() /builds/worker/workspace/build/src/dom/vr/VRDisplay.cpp:667:20
    #5 0x7fa20b714887 in mozilla::dom::VRDisplayBinding::submitFrame(JSContext*, JS::Handle<JSObject*>, mozilla::dom::VRDisplay*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/VRDisplayBinding.cpp:1238:9
    #6 0x7fa20c606261 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:3279:13
    #7 0x7fa212ecec87 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/JSContext-inl.h:280:15
    #8 0x7fa212ecec87 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:467
    #9 0x7fa212eb9483 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:522:12
    #10 0x7fa212eb9483 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3086
    #11 0x7fa212e9fc43 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:417:12
    #12 0x7fa212ecea05 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:489:15
    #13 0x7fa212ecfc82 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:535:10
    #14 0x7fa213030558 in PromiseReactionJob(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/src/builtin/Promise.cpp:1237:14
    #15 0x7fa212ecec87 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/JSContext-inl.h:280:15
    #16 0x7fa212ecec87 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:467
    #17 0x7fa212ecfc82 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:535:10
    #18 0x7fa213a1291a 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:2981:12
    #19 0x7fa20ac893f2 in mozilla::dom::PromiseJobCallback::Call(JSContext*, JS::Handle<JS::Value>, mozilla::ErrorResult&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/PromiseBinding.cpp:25:8
    #20 0x7fa206da3665 in Call /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/PromiseBinding.h:91:12
    #21 0x7fa206da3665 in Call /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/PromiseBinding.h:104
    #22 0x7fa206da3665 in mozilla::PromiseJobRunnable::Run(mozilla::AutoSlowOperation&) /builds/worker/workspace/build/src/xpcom/base/CycleCollectedJSContext.cpp:205
    #23 0x7fa206d867d1 in mozilla::CycleCollectedJSContext::PerformMicroTaskCheckPoint() /builds/worker/workspace/build/src/xpcom/base/CycleCollectedJSContext.cpp:543:17
    #24 0x7fa206d8701d in mozilla::CycleCollectedJSContext::AfterProcessTask(unsigned int) /builds/worker/workspace/build/src/xpcom/base/CycleCollectedJSContext.cpp:374:3
    #25 0x7fa20881b1bd in XPCJSContext::AfterProcessTask(unsigned int) /builds/worker/workspace/build/src/js/xpconnect/src/XPCJSContext.cpp:1258:30
    #26 0x7fa206f2c19d in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1125:24
    #27 0x7fa206f47850 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:519:10
    #28 0x7fa207e27fca in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #29 0x7fa207d7b669 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #30 0x7fa207d7b669 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #31 0x7fa207d7b669 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #32 0x7fa20e99292a in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:157:27
    #33 0x7fa212be696b in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:893:22
    #34 0x7fa207d7b669 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #35 0x7fa207d7b669 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #36 0x7fa207d7b669 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #37 0x7fa212be6330 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:719:34
    #38 0x4f50dc in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #39 0x4f50dc in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:282
    #40 0x7fa226cab82f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
Flags: in-testsuite?
Group: core-security → gfx-core-security
Flags: needinfo?(kgilbert)
(Assignee)

Comment 1

11 months ago
It seems that we aren't handling GL context loss well.  WebVR will only work with discrete GPU's, which don't often lose context with DX11, but can still occur due to GPU driver restarts and other bugs.

I'm attempting to reproduce, and expect to make a patch to at least not crash, even if the VR session is interrupted by the context loss.
Assignee: nobody → kgilbert
Flags: needinfo?(kgilbert)
(Assignee)

Comment 2

11 months ago
I have reproduced.  Now WIP on patch
(Assignee)

Comment 3

11 months ago
Bug 1462695 - Prevent crashing when context lost during WebVR presentation
(Assignee)

Comment 4

11 months ago
+CC jgilbert for patch review
(Assignee)

Comment 5

11 months ago
Comment on attachment 8980774 [details] [diff] [review]
bug1462695.patch

Early exit WebGLContext::GetVRFrame() to avoid crashing when GL context is lost.  Avoids de-referencing "gl" when it is null after lost context.  Callers to function already have error handling when nullptr is returned from function.
Attachment #8980774 - Flags: review?(jgilbert)
Comment on attachment 8980774 [details] [diff] [review]
bug1462695.patch

Review of attachment 8980774 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLContext.cpp
@@ +2384,5 @@
>    BeginComposition();
>    EndComposition();
>  
> +  if (IsContextLost()) {
> +    return nullptr;

4-space, no {}
Attachment #8980774 - Flags: review?(jgilbert) → review+
Can you clear the sec bit here? This is a clean null-deref, sec:DoS at worst.
Flags: needinfo?(ryanvm)
Group: gfx-core-security
Flags: needinfo?(ryanvm)

Comment 9

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4fab7126c17f
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Please request Beta approval on this when you get a chance. Also, is it possible to land the attached testcase as a crashtest?
Flags: needinfo?(kgilbert)
(Assignee)

Comment 11

11 months ago
Comment on attachment 8980774 [details] [diff] [review]
bug1462695.patch

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1250244 - WebVR 1.0 API

[User impact if declined]:
The content process may crash if there is a GL context loss while the user is in an immersive VR presentation on a WebVR site.  This may happen due to GPU driver bugs, background updates of GPU drivers, or power management forcing GPU device changes.  The user will see an error message and need to refresh the browser tab.
[Is this code covered by automated tests?]:
Automated tests cover this code, but do not simulate GL context loss.  A crashtest will be created and landed separately based on the test case attached to the bug.
[Has the fix been verified in Nightly?]:
I have verified on local builds, have NI'ed for additional confirmation.
[Needs manual test from QE? If yes, steps to reproduce]: 
N/A - Repro may be difficult without relying on test harness to simulate context loss with precise timing.
[List of other uplifts needed for the feature/fix]:
N/A
[Is the change risky?]:
- Low risk.
[Why is the change risky/not risky?]:
It is a simple early-exit of a function, whose callers already include checks for the nullptr returned in this case.
[String changes made/needed]:
None
Attachment #8980774 - Flags: approval-mozilla-beta?
(Assignee)

Comment 12

11 months ago
I'll leave the NI open on myself as a reminder to implement a crashtest
Comment on attachment 8980774 [details] [diff] [review]
bug1462695.patch

Fixes a WebVR crash. Approved for 61.0b11.
Attachment #8980774 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 15

10 months ago
I am currently on the critical path for FxR release related work, but will likely have some time in the next week or two to add the crashtest.  I'll leave the NI open until then.
Any updates on the test here? :)
Flags: needinfo?(kgilbert)
Flags: needinfo?(kgilbert)
You need to log in before you can comment on or make changes to this bug.