Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 19 additions & 18 deletions src/runtime/api/bun/subprocess.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
13 changes: 9 additions & 4 deletions src/runtime/valkey_jsc/js_valkey.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
}
}
}
}
Expand Down
16 changes: 11 additions & 5 deletions src/runtime/webcore/ByteStream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down
15 changes: 13 additions & 2 deletions src/runtime/webcore/blob/copy_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1642,7 +1642,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) {
Expand Down Expand Up @@ -1729,7 +1734,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]
Expand Down
41 changes: 32 additions & 9 deletions src/runtime/webcore/blob/write_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -927,7 +927,8 @@ mod windows_impl {
)
} {
WriteFileWindowsError::WriteFileWindowsDeinitialized => {}
WriteFileWindowsError::JSTerminated => {} // TODO: properly propagate exception upwards
// `run_from_js_thread` already reported the failed settle.
WriteFileWindowsError::JSTerminated => {}
}
return;
}
Expand All @@ -940,7 +941,8 @@ 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
// `run_from_js_thread` already reported the failed settle.
WriteFileWindowsError::JSTerminated => {}
}
}
}
Expand Down Expand Up @@ -989,7 +991,8 @@ 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
// `run_from_js_thread` already reported the failed settle.
WriteFileWindowsError::JSTerminated => {}
}
return;
}
Expand All @@ -998,14 +1001,15 @@ mod windows_impl {
if let Err(e) = unsafe { Self::open(this) } {
match e {
WriteFileWindowsError::WriteFileWindowsDeinitialized => {}
WriteFileWindowsError::JSTerminated => {} // TODO: properly propagate exception upwards
// `run_from_js_thread` already reported the failed settle.
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 `run_from_js_thread`).
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;
Expand Down Expand Up @@ -1059,7 +1063,8 @@ mod windows_impl {
)
} {
WriteFileWindowsError::WriteFileWindowsDeinitialized => {}
WriteFileWindowsError::JSTerminated => {} // TODO: properly propagate exception upwards
// `run_from_js_thread` already reported the failed settle.
WriteFileWindowsError::JSTerminated => {}
}
return;
}
Expand All @@ -1070,7 +1075,8 @@ 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
// `run_from_js_thread` already reported the failed settle.
WriteFileWindowsError::JSTerminated => {}
}
}
}
Expand All @@ -1096,12 +1102,19 @@ mod windows_impl {
// SAFETY: caller contract — `this` is live; copy out everything we
// need before `deinit` frees the allocation.
let (cb, cb_ctx) = unsafe { ((*this).on_complete_callback, (*this).on_complete_ctx) };
// SAFETY: caller contract — `this` is live; the VM-owned event
// loop (and its global) outlives the request.
let global = unsafe { (*(*this).event_loop).global_ref() };

// SAFETY: caller contract — `this` is live.
if let Some(err) = unsafe { (*this).to_system_error() } {
// SAFETY: caller contract — `this` is live; consumed here.
unsafe { Self::deinit(this) };
if let Err(e) = cb(cb_ctx, WriteFileResultType::Err(Box::new(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.report_active_exception_as_unhandled(jsc::JsError::Thrown);
return e.into();
}
} else {
Expand All @@ -1110,6 +1123,8 @@ mod windows_impl {
// SAFETY: caller contract — `this` is live; consumed here.
unsafe { Self::deinit(this) };
if let Err(e) = cb(cb_ctx, WriteFileResultType::Result(wrote as SizeType)) {
// See the error arm above.
global.report_active_exception_as_unhandled(jsc::JsError::Thrown);
return e.into();
}
}
Expand Down Expand Up @@ -1343,8 +1358,16 @@ impl WriteFileWaitFromLockedValueTask {
let this = unsafe {
bun_core::heap::take(this.cast::<WriteFileWaitFromLockedValueTask>().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(
Expand Down
17 changes: 14 additions & 3 deletions src/runtime/webcore/s3/multipart.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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);
}
}
}

Expand Down
59 changes: 59 additions & 0 deletions test/js/bun/io/bun-write.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -943,4 +943,63 @@

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 () => {
const fixture = `
const fs = require("fs");
const dir = fs.mkdtempSync(require("os").tmpdir() + "/bun-write-pollution-");

Check warning on line 955 in test/js/bun/io/bun-write.test.js

View check run for this annotation

Claude / Claude Code Review

Test fixture leaks a temp directory on every run

The child fixture creates a temp directory via `fs.mkdtempSync(require("os").tmpdir() + "/bun-write-pollution-")` and writes three files into it, but never removes it — and the parent test has no handle to the path so it cannot clean up either. Every neighboring subprocess-fixture test in this file creates the directory in the parent via `using dir = tempDir(...)` and interpolates `String(dir)` into the fixture; this test should do the same (or add `fs.rmSync(dir, { recursive: true, force: true
Comment thread
robobun marked this conversation as resolved.
Outdated
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 + "/");
fs.writeFileSync(dir + "/src.txt", "copy me");

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 resolves with the byte count.
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"),
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", 7, "copy me"]);
expect(exitCode).toBe(0);
});
});
37 changes: 37 additions & 0 deletions test/js/bun/spawn/spawn.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});