1744172 - crash at null in [@ mozilla::ClientWebGLContext::DeleteQuery]

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found while fuzzing m-c 20211202-7af79a49bf5f (--enable-address-sanitizer --enable-fuzzing)

==15084==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f573167b8a9 bp 0x7ffec194f3b0 sp 0x7ffec194f2a0 T0)
==15084==The signal is caused by a READ memory access.
==15084==Hint: address points to the zero page.
    #0 0x7f573167b8a9 in operator==<mozilla::WebGLQueryJS, mozilla::WebGLQueryJS> /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:505:44
    #1 0x7f573167b8a9 in mozilla::ClientWebGLContext::DeleteQuery(mozilla::WebGLQueryJS*) /gecko/dom/canvas/ClientWebGLContext.cpp:1332:22
    #2 0x7f5730bd072f in mozilla::ClientWebGLExtensionDisjointTimerQuery::DeleteQueryEXT(mozilla::WebGLQueryJS*) const /gecko/dom/canvas/ClientWebGLExtensions.h:263:15
    #3 0x7f5730bd0232 in mozilla::dom::EXT_disjoint_timer_query_Binding::deleteQueryEXT(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/WebGLRenderingContextBinding.cpp:2321:24
    #4 0x7f57314f138d in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /gecko/dom/bindings/BindingUtils.cpp:3306:13
    #5 0x7f5738db7174 in CallJSNative /gecko/js/src/vm/Interpreter.cpp:388:13
    #6 0x7f5738db7174 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /gecko/js/src/vm/Interpreter.cpp:475:12
    #7 0x7f5739c9e7d1 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /gecko/js/src/jit/BaselineIC.cpp:1595:10
    #8 0x7f56a16a6db7  (<unknown module>)

Comment 1

[Tyson Smith [:tsmith]]

Attachment: prefs.js

Requires pref webgl.enable-privileged-extensions=true

Comment 2

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/5LCBaawZR8DdSPLSY7lnug/index.html

Comment 3

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Verified bug as reproducible on mozilla-central 20211202215058-260e2362bbc3.
Failed to bisect testcase (Testcase reproduces on start build!):

Start: ee7cd95a414c7307a137c07e0c367c336a74a737 (20201204033450)
End: 7af79a49bf5f7d22deb1e81b5ffd7d45034b4bd2 (20211202094249)
BuildFlags: BuildFlags(asan=True, tsan=False, debug=False, fuzzing=True, coverage=False, valgrind=False, no_opt=False, fuzzilli=False)

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

This looks like we returned a nullptr here:

https://searchfox.org/mozilla-central/rev/2e0ceee0158293a37466aaf9495dbeffef744b7c/dom/canvas/ClientWebGLContext.cpp#1405

but we derefed it anyways and we crashed on the next line in the comparison check.

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

It looks like queryCounterEXT sets mTarget:
https://searchfox.org/mozilla-central/rev/2e0ceee0158293a37466aaf9495dbeffef744b7c/dom/canvas/ClientWebGLContext.cpp#5006

where as beginQueryEXT (not called) would have also set the slot we store the query in which would allow it to be deleted without issue:
https://searchfox.org/mozilla-central/rev/2e0ceee0158293a37466aaf9495dbeffef744b7c/dom/canvas/ClientWebGLContext.cpp#4957

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

It looks to me the whole point of the check is to make sure we call EndQuery on things we called BeginQuery for. Hence in the case where we called QueryCounter on it with BeginQuery, we should just skip this step.

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1744172 - Fix a crash in ClientWebGLContext::DeleteQuery.

When DeleteQuery is called, it uses mTarget to determine whether or not
BeginQuery has been called. This is an insufficient condition because
QueryCounter also sets mTarget without calling BeginQuery and occupying
a slot in current query map. This patch fixes the crash by checking if
the slot is empty first.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a875b15a9a25
Fix a crash in ClientWebGLContext::DeleteQuery. r=jgilbert

Comment 9

[Atila Butkovits]

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

Comment 10

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Verified bug as fixed on rev mozilla-central 20220127213627-b9121c1a4330.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

:aosmond, since this bug contains a bisection range, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Comment 12

[Marco Castelluccio [:marco]]

Sorry, bug in the bot.