Closed Bug 1744172 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect

Tracking

()

VERIFIED FIXED
98 Branch
Tracking Status
firefox-esr91 --- wontfix
firefox96 --- wontfix
firefox97 --- wontfix
firefox98 --- verified

People

(Reporter: tsmith, Assigned: aosmond)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase, Whiteboard: [bugmon:bisected,confirmed])

Crash Data

Attachments

(3 files)

Attached file 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>)
Flags: in-testsuite?
Attached file prefs.js

Requires pref webgl.enable-privileged-extensions=true

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

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)

Whiteboard: [bugmon:bisected,confirmed]

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.

Crash Signature: [@ mozilla::ClientWebGLContext::DeleteQuery ]

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

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.

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.

Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Attachment #9260932 - Attachment description: Bug 1744172 - Fix a crash in ClientWebGLContext::DeleteQuery. → Bug 1744172 - Fix a crash in ClientWebGLContext::DeleteQuery. r=jgilbert
Attachment #9260932 - Attachment description: Bug 1744172 - Fix a crash in ClientWebGLContext::DeleteQuery. r=jgilbert → Bug 1744172 - Fix a crash in ClientWebGLContext::DeleteQuery.
Pushed by aosmond@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a875b15a9a25
Fix a crash in ClientWebGLContext::DeleteQuery. r=jgilbert
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch
Flags: in-testsuite? → in-testsuite+

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.

Status: RESOLVED → VERIFIED
Keywords: bugmon

: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.

Flags: needinfo?(aosmond)

Sorry, bug in the bot.

Flags: needinfo?(aosmond)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: