1810261 - crash at null in [@ mozilla::dom::BodyStream::CancelCallback]

Status: VERIFIED FIXED

(Core :: DOM: Streams, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.zip

Found while fuzzing m-c 20230112-e5ed23660819 (--enable-address-sanitizer --enable-fuzzing)

To reproduce via Grizzly Replay:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch -a --fuzzing -n firefox
$ python -m grizzly.replay ./firefox/firefox testcase.html

==657244==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000068 (pc 0x7fe7c211616f bp 0x7fe720e60360 sp 0x7fe720e60360 T18)
==657244==The signal is caused by a READ memory access.
==657244==Hint: address points to the zero page.
    #0 0x7fe7c211616f in get /builds/worker/workspace/obj-build/dist/include/nsCOMPtr.h:851:48
    #1 0x7fe7c211616f in operator nsIGlobalObject * /builds/worker/workspace/obj-build/dist/include/nsCOMPtr.h:859:33
    #2 0x7fe7c211616f in mozilla::dom::BodyStream::CancelCallback(JSContext*, mozilla::dom::Optional<JS::Handle<JS::Value>> const&, mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/base/BodyStream.cpp:286:47
    #3 0x7fe7c72a9f2f in mozilla::dom::BodyStreamUnderlyingSourceAlgorithms::CancelCallback(JSContext*, mozilla::dom::Optional<JS::Handle<JS::Value>> const&, mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/streams/ByteStreamHelpers.cpp:134:24
    #4 0x7fe7c7277fd3 in mozilla::dom::ReadableByteStreamController::CancelSteps(JSContext*, JS::Handle<JS::Value>, mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/streams/ReadableByteStreamController.cpp:991:40
    #5 0x7fe7c727ff8c in mozilla::dom::ReadableStreamCancel(JSContext*, mozilla::dom::ReadableStream*, JS::Handle<JS::Value>, mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/streams/ReadableStream.cpp:382:19
    #6 0x7fe7c7280940 in mozilla::dom::ReadableStream::Cancel(JSContext*, JS::Handle<JS::Value>, mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/streams/ReadableStream.cpp:426:10
    #7 0x7fe7c321e635 in cancel /builds/worker/workspace/obj-build/dom/bindings/ReadableStreamBinding.cpp:620:60
    #8 0x7fe7c321e635 in mozilla::dom::ReadableStream_Binding::cancel_promiseWrapper(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/ReadableStreamBinding.cpp:636:13
    #9 0x7fe7c42e6002 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ConvertExceptionsToPromises>(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/dom/bindings/BindingUtils.cpp:3308:13
    #10 0x7fe7ccdd1334 in CallJSNative /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:459:13
    #11 0x7fe7ccdd1334 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:547:12
    #12 0x7fe7ccdc031a in InternalCall /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:614:10
    #13 0x7fe7ccdc031a in CallFromStack /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:619:10
    #14 0x7fe7ccdc031a in Interpret(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:3362:16
    #15 0x7fe7ccda443c in js::RunScript(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:431:13
    #16 0x7fe7ccdd1450 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:579:13
    #17 0x7fe7ccdd310f in InternalCall /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:614:10
    #18 0x7fe7ccdd310f in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:646:8
    #19 0x7fe7cd237f44 in js::CallSelfHostedFunction(JSContext*, JS::Handle<js::PropertyName*>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/vm/SelfHosting.cpp:1488:10
    #20 0x7fe7ccea1da0 in AsyncFunctionResume(JSContext*, JS::Handle<js::AsyncFunctionGeneratorObject*>, ResumeKind, JS::Handle<JS::Value>) /builds/worker/checkouts/gecko/js/src/vm/AsyncFunction.cpp:149:8
    #21 0x7fe7cd174a5e in AsyncFunctionPromiseReactionJob /builds/worker/checkouts/gecko/js/src/builtin/Promise.cpp:2111:12
    #22 0x7fe7cd174a5e in PromiseReactionJob(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/js/src/builtin/Promise.cpp:2174:12
    #23 0x7fe7ccdd1334 in CallJSNative /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:459:13
    #24 0x7fe7ccdd1334 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:547:12
    #25 0x7fe7ccdd310f in InternalCall /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:614:10
    #26 0x7fe7ccdd310f in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:646:8
    #27 0x7fe7ccedc52d in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/vm/CallAndConstruct.cpp:117:10
    #28 0x7fe7c304c2bc in mozilla::dom::PromiseJobCallback::Call(mozilla::dom::BindingCallContext&, JS::Handle<JS::Value>, mozilla::ErrorResult&) /builds/worker/workspace/obj-build/dom/bindings/PromiseBinding.cpp:83:8
    #29 0x7fe7beeb74fa in Call /builds/worker/workspace/obj-build/dist/include/mozilla/dom/PromiseBinding.h:198:12
    #30 0x7fe7beeb74fa in Call /builds/worker/workspace/obj-build/dist/include/mozilla/dom/PromiseBinding.h:211:12
    #31 0x7fe7beeb74fa in mozilla::PromiseJobRunnable::Run(mozilla::AutoSlowOperation&) /builds/worker/checkouts/gecko/xpcom/base/CycleCollectedJSContext.cpp:213:18
    #32 0x7fe7bee972a7 in mozilla::CycleCollectedJSContext::PerformMicroTaskCheckPoint(bool) /builds/worker/checkouts/gecko/xpcom/base/CycleCollectedJSContext.cpp:676:17
    #33 0x7fe7bee982cf in mozilla::CycleCollectedJSContext::AfterProcessTask(unsigned int) /builds/worker/checkouts/gecko/xpcom/base/CycleCollectedJSContext.cpp:463:3
    #34 0x7fe7bf0c8a17 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1234:24
    #35 0x7fe7bf0d28b4 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:473:10
    #36 0x7fe7c71444dc in mozilla::dom::WorkerPrivate::DoRunLoop(JSContext*) /builds/worker/checkouts/gecko/dom/workers/WorkerPrivate.cpp:3239:7
    #37 0x7fe7c711afb2 in mozilla::dom::workerinternals::(anonymous namespace)::WorkerThreadPrimaryRunnable::Run() /builds/worker/checkouts/gecko/dom/workers/RuntimeService.cpp:2044:42
    #38 0x7fe7bf0c8ceb in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1191:16
    #39 0x7fe7bf0d28b4 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:473:10
    #40 0x7fe7c0844c04 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:300:20
    #41 0x7fe7c06c2af7 in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:381:10
    #42 0x7fe7c06c2af7 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:374:3
    #43 0x7fe7c06c2af7 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:356:3
    #44 0x7fe7bf0c07c5 in nsThread::ThreadFunc(void*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:383:10
    #45 0x7fe7e11b7628 in _pt_root /builds/worker/checkouts/gecko/nsprpub/pr/src/pthreads/ptthread.c:201:5
    #46 0x7fe7e161ab42 in start_thread nptl/pthread_create.c:442:8
    #47 0x7fe7e16ac9ff  misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

Comment 1

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20230113234514-9af5d0877b6b.
The bug appears to have been introduced in the following build range:

Start: 1ec69d3d2954307d23dc2b7a722e9ddbd687e31f (20230112144356)
End: c03fafca6fb4f895eb2fa9d6235a700c8c43ae82 (20230112173018)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=1ec69d3d2954307d23dc2b7a722e9ddbd687e31f&tochange=c03fafca6fb4f895eb2fa9d6235a700c8c43ae82

Comment 2

[Kagami Rosylight [:saschanaz] (they/them)]

The fix here is trivial, but I wonder this is also a spec issue.

Bug 1809673 replaced ReadableStreamClose with CloseNative in BodyStream , which per the spec waits for the internal queue to be emptied before closing. As such the [[state]] of the stream doesn't become "closed" and thus ReadableStreamCancel proceeds to call the [[CancelSteps]].

Then it does not check whether the close request exists and just calls [[cancelAlgorithm]] when the underlying source is already cleaned up, while the pull steps do check it. Maybe the cancel steps should also check it too?

I'd like to double check with you before filing a spec issue. What do you think?

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1810261 - Move the CancelCallback impl to BodyStreamUnderlyingSourceAlgorithms r=evilpie

It simply just returns a promise, no need to define another method for that.

Comment 4

[Tom S [:evilpie]]

I am going to have to think about your question a bit more before answering.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51a7aba4ed32
Move the CancelCallback impl to BodyStreamUnderlyingSourceAlgorithms r=evilpie

Comment 6

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/37998 for changes under testing/web-platform/tests

Comment 7

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/51a7aba4ed32

Comment 8

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 9

[Kagami Rosylight [:saschanaz] (they/them)]

This was for a question in comment #2 which is still to be answered.

Comment 10

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20230117161302-455aa95a34de.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 11

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

Should we uplit to beta or can it ride the 111 train? Thanks

Comment 12

[Kagami Rosylight [:saschanaz] (they/them)]

Comment on attachment 9312431 [details]
Bug 1810261 - Move the CancelCallback impl to BodyStreamUnderlyingSourceAlgorithms r=evilpie

Beta/Release Uplift Approval Request

• User impact if declined: User may experience crash by accessing nullptr when accessing streams generated from Blob/Response.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• 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 web-visible behavior change and the fix is small.
• String changes made/needed:
• Is Android affected?: Yes

Comment 13

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

Comment on attachment 9312431 [details]
Bug 1810261 - Move the CancelCallback impl to BodyStreamUnderlyingSourceAlgorithms r=evilpie

Approved for 110 beta 3, thanks

Comment 14

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

https://hg.mozilla.org/releases/mozilla-beta/rev/b615cc23f141

Comment 15

[Tom S [:evilpie]]

I don't follow your explanation. So we are looking at "To close a ReadableStream", which calls ReadableByteStreamControllerClose for a byte stream and that sets the [[closeRequested]] when the queue isn't empty. I don't see a call to ReadableStreamCancel. I guess Step 1.2 with the call to ReadableByteStreamControllerRespond will empty the queue somehow and actually close the stream?

Comment 16

[Kagami Rosylight [:saschanaz] (they/them)]

(In reply to Tom S [:evilpie] from comment #15)

I don't follow your explanation. So we are looking at "To close a ReadableStream", which calls ReadableByteStreamControllerClose for a byte stream and that sets the [[closeRequested]] when the queue isn't empty. I don't see a call to ReadableStreamCancel.

Ah, ReadableStreamCancel may come from JavaScript by calling .cancel on a close-requested stream. So my question is whether .cancel should call cancel algorithm when there's a close request. (Because a close request means the source is done, why would it need to care about cancellation?)

I guess Step 1.2 with the call to ReadableByteStreamControllerRespond will empty the queue somehow and actually close the stream?

My understading is that the actual close happens when the queue is fully read and thus Respond may call Close if it can empty the queue.

Comment 17

[Tom S [:evilpie]]

So my question is whether .cancel should call cancel algorithm when there's a close request.

Isn't that the fundamental difference between cancel and close? For cancel we want to immediately abort all read requests and not wait for the queue to be empty.

My understading is that the actual close happens when the queue is fully read and thus Respond may call Close if it can empty the queue.

I guess you are right, ReadableByteStreamControllerHandleQueueDrain will call ReadableStreamClose when the the queue is empty. So we basically have to wait for everything to be read.

Comment 18

[Kagami Rosylight [:saschanaz] (they/them)]

For cancel we want to immediately abort all read requests and not wait for the queue to be empty.

Yes, but why ping the source? The source is already done and don't really care about anything anymore, isn't it? The read request processing happens in ReadableStream and that's not what the underlying source should care IMO.

Comment 19

[Tom S [:evilpie]]

I don't know. I suggest you open an issue in the streams repo and ask.

Comment 20

[Kagami Rosylight [:saschanaz] (they/them)]

Filed https://github.com/whatwg/streams/issues/1252, thanks for thinking about this!