diff --git a/src/runtime/api/bun/subprocess.rs b/src/runtime/api/bun/subprocess.rs index 1e17989a5791..0232cf3e6ce3 100644 --- a/src/runtime/api/bun/subprocess.rs +++ b/src/runtime/api/bun/subprocess.rs @@ -1119,34 +1119,35 @@ impl Subprocess<'_> { did_update_has_pending_activity = true; } - match status { - Status::Exited(exited) => { - let _ = promise - .as_any_promise() - .unwrap() - .resolve(global_this, JSValue::js_number(exited.code as f64)); - // TODO: properly propagate exception upwards - } + let settled = match status { + Status::Exited(exited) => promise + .as_any_promise() + .unwrap() + .resolve(global_this, JSValue::js_number(exited.code as f64)), Status::Err(err) => { let js_err = err.to_js(global_this); - let _ = promise + promise .as_any_promise() .unwrap() - .reject_with_async_stack(global_this, js_err); - // TODO: properly propagate exception upwards - } - Status::Signaled(signaled) => { - let _ = promise.as_any_promise().unwrap().resolve( - global_this, - JSValue::js_number(128u8.wrapping_add(*signaled) as f64), - ); - // TODO: properly propagate exception upwards + .reject_with_async_stack(global_this, js_err) } + Status::Signaled(signaled) => promise.as_any_promise().unwrap().resolve( + global_this, + JSValue::js_number(128u8.wrapping_add(*signaled) as f64), + ), _ => { // crash in debug mode #[cfg(debug_assertions)] unreachable!(); + #[cfg(not(debug_assertions))] + Ok(()) } + }; + if settled.is_err() { + // A failed settle leaves an exception pending on the VM; + // the exit callback below must not run with it (the + // error label erases Thrown, so probe the VM instead). + global_this.report_active_exception_as_unhandled(jsc::JsError::Thrown); } } diff --git a/src/runtime/valkey_jsc/js_valkey.rs b/src/runtime/valkey_jsc/js_valkey.rs index 8c68f4c82ed3..c9bfab23525c 100644 --- a/src/runtime/valkey_jsc/js_valkey.rs +++ b/src/runtime/valkey_jsc/js_valkey.rs @@ -1165,8 +1165,11 @@ impl JSValkeyClient { .expect("unreachable"); let len = start - cur.len(); let msg = &buf[..len]; - let _ = self.client_fail(msg, protocol::RedisError::IdleTimeout); - // TODO: properly propagate exception upwards + if let Err(e) = self.client_fail(msg, protocol::RedisError::IdleTimeout) { + // A failed settle inside `fail` leaves an exception pending + // on the VM; this timer callback has nowhere to bubble it. + self.global_object.report_active_exception_as_unhandled(e); + } } valkey::Status::Disconnected | valkey::Status::Connecting => { use std::io::Write; @@ -1180,8 +1183,10 @@ impl JSValkeyClient { .expect("unreachable"); let len = start - cur.len(); let msg = &buf[..len]; - let _ = self.client_fail(msg, protocol::RedisError::ConnectionTimeout); - // TODO: properly propagate exception upwards + if let Err(e) = self.client_fail(msg, protocol::RedisError::ConnectionTimeout) { + // See the idle-timeout arm above. + self.global_object.report_active_exception_as_unhandled(e); + } } } } diff --git a/src/runtime/webcore/ByteStream.rs b/src/runtime/webcore/ByteStream.rs index a696aace9e38..d010f8110bc3 100644 --- a/src/runtime/webcore/ByteStream.rs +++ b/src/runtime/webcore/ByteStream.rs @@ -620,11 +620,17 @@ impl ByteStream { if let Some(mut action) = self.buffer_action.replace(None) { let global = self.parent_const().global_this(); - // TODO: properly propagate exception upwards - let _ = action.reject( - global, - &streams::StreamError::AbortReason(jsc::CommonAbortReason::UserAbort), - ); + if action + .reject( + global, + &streams::StreamError::AbortReason(jsc::CommonAbortReason::UserAbort), + ) + .is_err() + { + // A failed settle leaves an exception pending on the VM (the + // error label erases Thrown, so probe the VM instead). + global.report_active_exception_as_unhandled(jsc::JsError::Thrown); + } self.buffer_action.set(None); } } diff --git a/src/runtime/webcore/blob/copy_file.rs b/src/runtime/webcore/blob/copy_file.rs index cf4aae0668e7..a7a32d11405f 100644 --- a/src/runtime/webcore/blob/copy_file.rs +++ b/src/runtime/webcore/blob/copy_file.rs @@ -137,7 +137,14 @@ impl<'a> CopyFile<'a> { if let Some(store) = self.store.take() { drop(store); // deref() } - promise.reject(global_this, Ok(instance)) + if promise.reject(global_this, Ok(instance)).is_err() { + // The settle error label erases Thrown, so probe the VM: report a + // pending non-termination exception instead of letting the task + // channel treat it as termination, and keep real termination as + // the `Err` that unwinds the tick loop. + return jsc::task::report_error_or_terminate(global_this, jsc::JsError::Thrown); + } + Ok(()) } pub(crate) fn then(&mut self, promise: &mut JSPromise) -> Result<(), jsc::JsTerminated> { @@ -147,10 +154,17 @@ impl<'a> CopyFile<'a> { return self.reject(promise); } - promise.resolve( - self.global_this, - JSValue::js_number_from_uint64(self.read_len as u64), - ) + if promise + .resolve( + self.global_this, + JSValue::js_number_from_uint64(self.read_len as u64), + ) + .is_err() + { + // See `reject` above. + return jsc::task::report_error_or_terminate(self.global_this, jsc::JsError::Thrown); + } + Ok(()) } #[cfg(not(windows))] @@ -1642,7 +1656,12 @@ impl<'a> CopyFileWindows<'a> { // SAFETY: self was heap-allocated in init(); destroy reclaims and drops it. self is not accessed afterward. unsafe { Self::destroy(core::ptr::from_mut(self)) }; // `promise` points to a GC-owned `JSPromise` cell, not into `self`; valid after `destroy`. - let _ = promise.reject(global_this, err_instance); // TODO: properly propagate exception upwards + if promise.reject(global_this, err_instance).is_err() { + // A failed settle leaves an exception pending on the VM; report it + // so it cannot ride the tick into unrelated JS (the error label + // erases Thrown, so probe the VM instead). + global_this.report_active_exception_as_unhandled(jsc::JsError::Thrown); + } } pub(crate) fn on_complete(&mut self, written_actual: usize) { @@ -1729,7 +1748,13 @@ impl<'a> CopyFileWindows<'a> { // SAFETY: self was heap-allocated in init(); destroy reclaims and drops it. self is not accessed afterward. unsafe { Self::destroy(core::ptr::from_mut(self)) }; // `promise` points to a GC-owned `JSPromise` cell, not into `self`; valid after `destroy`. - let _ = promise.resolve(global_this, JSValue::js_number_from_uint64(written as u64)); // TODO: properly propagate exception upwards + if promise + .resolve(global_this, JSValue::js_number_from_uint64(written as u64)) + .is_err() + { + // See `throw` above: keep a failed settle's exception off the tick. + global_this.report_active_exception_as_unhandled(jsc::JsError::Thrown); + } } #[cold] diff --git a/src/runtime/webcore/blob/write_file.rs b/src/runtime/webcore/blob/write_file.rs index 32a0e2762a8c..b4ea1ccec8a2 100644 --- a/src/runtime/webcore/blob/write_file.rs +++ b/src/runtime/webcore/blob/write_file.rs @@ -927,7 +927,9 @@ mod windows_impl { ) } { WriteFileWindowsError::WriteFileWindowsDeinitialized => {} - WriteFileWindowsError::JSTerminated => {} // TODO: properly propagate exception upwards + // The completion callback reported any failed settle; + // JSTerminated is real termination, left pending. + WriteFileWindowsError::JSTerminated => {} } return; } @@ -940,7 +942,9 @@ mod windows_impl { if let Err(e) = unsafe { Self::do_write_loop(this, (*this).loop_()) } { match e { WriteFileWindowsError::WriteFileWindowsDeinitialized => {} - WriteFileWindowsError::JSTerminated => {} // TODO: properly propagate exception upwards + // The completion callback reported any failed settle; + // JSTerminated is real termination, left pending. + WriteFileWindowsError::JSTerminated => {} } } } @@ -989,7 +993,9 @@ mod windows_impl { // SAFETY: caller contract — `this` is live; `throw` consumes it. match unsafe { Self::throw(this, err_) } { WriteFileWindowsError::WriteFileWindowsDeinitialized => {} - WriteFileWindowsError::JSTerminated => {} // TODO: properly propagate exception upwards + // The completion callback reported any failed settle; + // JSTerminated is real termination, left pending. + WriteFileWindowsError::JSTerminated => {} } return; } @@ -998,14 +1004,16 @@ mod windows_impl { if let Err(e) = unsafe { Self::open(this) } { match e { WriteFileWindowsError::WriteFileWindowsDeinitialized => {} - WriteFileWindowsError::JSTerminated => {} // TODO: properly propagate exception upwards + // The completion callback reported any failed settle; + // JSTerminated is real termination, left pending. + WriteFileWindowsError::JSTerminated => {} } } } /// `ManagedTask`-shaped trampoline for [`on_mkdirp_complete`]: takes /// `*mut Self` and returns the event-loop `JsResult<()>` (always `Ok`; - /// the inner body already swallows `JSTerminated`). + /// a failed settle is reported inside the completion callback). fn on_mkdirp_complete_task(this: *mut WriteFileWindows) -> bun_event_loop::JsResult<()> { // SAFETY: `this` is the live Box-allocated `WriteFileWindows` whose // pointer was stashed in `on_mkdirp_complete_concurrent` below; @@ -1059,7 +1067,9 @@ mod windows_impl { ) } { WriteFileWindowsError::WriteFileWindowsDeinitialized => {} - WriteFileWindowsError::JSTerminated => {} // TODO: properly propagate exception upwards + // The completion callback reported any failed settle; + // JSTerminated is real termination, left pending. + WriteFileWindowsError::JSTerminated => {} } return; } @@ -1070,7 +1080,9 @@ mod windows_impl { if let Err(e) = unsafe { Self::do_write_loop(this, (*this).loop_()) } { match e { WriteFileWindowsError::WriteFileWindowsDeinitialized => {} - WriteFileWindowsError::JSTerminated => {} // TODO: properly propagate exception upwards + // The completion callback reported any failed settle; + // JSTerminated is real termination, left pending. + WriteFileWindowsError::JSTerminated => {} } } } @@ -1303,22 +1315,28 @@ impl WriteFilePromise { // SAFETY: GC-owned cell (kept alive below); scoped shared access. let value = unsafe { (*promise).to_js() }; value.ensure_still_alive(); - match count { + let settled = match count { WriteFileResultType::Err(err) => { // SAFETY: GC-owned cell; the error build's shared borrow ends before the // scoped exclusive `reject` borrow. unsafe { let err_js = err.to_error_instance_with_async_stack(global_this, &*promise); - (*promise).reject(global_this, Ok(err_js))?; + (*promise).reject(global_this, Ok(err_js)) } } WriteFileResultType::Result(wrote) => { // SAFETY: GC-owned cell; exclusive borrow scoped to the call. unsafe { - (*promise) - .resolve(global_this, JSValue::js_number_from_uint64(wrote as u64))?; + (*promise).resolve(global_this, JSValue::js_number_from_uint64(wrote as u64)) } } + }; + if settled.is_err() { + // The settle error label erases Thrown, so probe the VM: report a + // pending non-termination exception instead of letting the task + // channel treat it as termination, and keep real termination as + // the `Err` that unwinds the tick loop. + return jsc::task::report_error_or_terminate(global_this, jsc::JsError::Thrown); } Ok(()) } @@ -1343,8 +1361,16 @@ impl WriteFileWaitFromLockedValueTask { let this = unsafe { bun_core::heap::take(this.cast::().as_ptr()) }; - let _ = Self::then(this, value); - // TODO: properly propagate exception upwards + // `BackRef` is `Copy`; keep the global past the consuming call. + let global_ref = this.global_this; + if Self::then(this, value).is_err() { + // A failed settle leaves an exception pending on the VM; report it + // so it cannot ride the tick into unrelated JS (the error label + // erases Thrown, so probe the VM instead). + global_ref + .get() + .report_active_exception_as_unhandled(jsc::JsError::Thrown); + } } pub(crate) fn then( diff --git a/src/runtime/webcore/s3/multipart.rs b/src/runtime/webcore/s3/multipart.rs index 429fac114065..3e4466945aa9 100644 --- a/src/runtime/webcore/s3/multipart.rs +++ b/src/runtime/webcore/s3/multipart.rs @@ -990,7 +990,7 @@ impl MultiPartUpload { self.state.set(State::SinglefileStarted); // we can do only 1 request self.ref_(); - let _ = execute_simple_s3_request( + let started = execute_simple_s3_request( &self.credentials, s3_simple_request::S3RequestOptions { path: &self.path, @@ -1007,10 +1007,21 @@ impl MultiPartUpload { }, s3_simple_request::S3Callback::Upload(Self::single_send_upload_response), self.as_ctx_ptr(), - ); // TODO: properly propagate exception upwards + ); + if started.is_err() { + // A failed settle leaves an exception pending on the VM; report + // it so it cannot ride the tick into unrelated JS (the error + // label erases Thrown, so probe the VM instead). + self.global_this + .report_active_exception_as_unhandled(bun_jsc::JsError::Thrown); + } } else { // we need to split - let _ = self.process_multi_part(part_size); // TODO: properly propagate exception upwards + if self.process_multi_part(part_size).is_err() { + // See the single-request arm above. + self.global_this + .report_active_exception_as_unhandled(bun_jsc::JsError::Thrown); + } } } diff --git a/test/js/bun/io/bun-write.test.js b/test/js/bun/io/bun-write.test.js index f02bdf4f8e17..4d0df4a7e500 100644 --- a/test/js/bun/io/bun-write.test.js +++ b/test/js/bun/io/bun-write.test.js @@ -943,4 +943,65 @@ int posix_fadvise(int fd, off_t offset, off_t len, int advice) { expect(f.name).toBe(filePath); }); + + // The native write completions (copy_file, write_file, and the locked-body + // wait task) settle their promises from the event loop and must never leave + // an exception riding the tick. Pin both paths with a hostile + // `Object.prototype.then` accessor installed, mirroring the + // prototype-pollution state fuzzer processes run in. + it("write completions settle under Object.prototype.then pollution", async () => { + using dir = tempDir("bun-write-pollution", { "src.txt": "copy me" }); + const fixture = ` + const fs = require("fs"); + const dir = ${JSON.stringify(String(dir))}; + const server = Bun.serve({ + port: 0, + async fetch() { + return new Response(new ReadableStream({ + async pull(c) { + c.enqueue(new TextEncoder().encode("hello ")); + await Bun.sleep(10); + c.enqueue(new TextEncoder().encode("world")); + c.close(); + }, + })); + }, + }); + const resp = await fetch("http://localhost:" + server.port + "/"); + + Object.defineProperty(Object.prototype, "then", { + configurable: true, + get() { + throw new Error("boom"); + }, + }); + // Body still streaming: Bun.write waits on the locked body, then + // resolves its promise with the inner write's promise. + const wroteStream = await Bun.write(dir + "/a.txt", resp); + // copy_file completion. The resolved count is backend-dependent on + // Windows (uv_fs_copyfile reports none, the fallback loop the real + // count), so pin only its type; the content check below is the proof. + const wroteCopy = await Bun.write(Bun.file(dir + "/dst.txt"), Bun.file(dir + "/src.txt")); + delete Object.prototype.then; + + const results = [ + wroteStream, + fs.readFileSync(dir + "/a.txt", "utf8"), + typeof wroteCopy, + fs.readFileSync(dir + "/dst.txt", "utf8"), + ]; + server.stop(true); + console.log(JSON.stringify(results)); + `; + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", fixture], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(JSON.parse(stdout)).toEqual([11, "hello world", "number", "copy me"]); + expect(exitCode).toBe(0); + }); }); diff --git a/test/js/bun/spawn/spawn.test.ts b/test/js/bun/spawn/spawn.test.ts index 5b6da5beda1c..400a65f81c79 100644 --- a/test/js/bun/spawn/spawn.test.ts +++ b/test/js/bun/spawn/spawn.test.ts @@ -1536,3 +1536,40 @@ describe("uid/gid", () => { expect(thrown?.code).toBe("EPERM"); }); }); + +// The native exit completion settles the cached `exited` promise and then +// immediately runs more JS (onExit callback, IPC teardown) in the same task, +// so a failed settle must never leave its exception pending on the VM. The +// settle values are primitives, so a hostile `Object.prototype.then` accessor +// (prototype pollution persists across scripts in fuzzer processes) must not +// be consulted and both the exit-code and signal paths must stay healthy. +it.if(isPosix)("exited settles under Object.prototype.then pollution", async () => { + await using proc = spawn({ + cmd: [ + bunExe(), + "-e", + ` + Object.defineProperty(Object.prototype, "then", { + configurable: true, + get() { + throw new Error("boom"); + }, + }); + const exited7 = Bun.spawn({ cmd: ["sh", "-c", "exit 7"] }).exited; + const killed = Bun.spawn({ cmd: ["sleep", "1000"] }); + const exitedKilled = killed.exited; + killed.kill("SIGKILL"); + const codes = [await exited7, await exitedKilled]; + delete Object.prototype.then; + console.log(JSON.stringify(codes)); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(JSON.parse(stdout)).toEqual([7, 137]); + expect(exitCode).toBe(0); +});