Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 1 addition & 1 deletion src/runtime/jsc_hooks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1331,7 +1331,7 @@ unsafe fn create_node_fs(vm: *mut VirtualMachine) -> *mut c_void {
None
};
bun_core::heap::into_raw(Box::new(NodeFS {
sync_error_buf: bun_paths::PathBuffer::uninit(),
sync_error_buf: bun_paths::path_buffer_pool::get(),
vm: vm_field,
}))
.cast::<c_void>()
Expand Down
23 changes: 6 additions & 17 deletions src/runtime/node/node_fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4406,27 +4406,16 @@ pub mod ret {
// https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/fs.d.ts
// ──────────────────────────────────────────────────────────────────────────

// `#[repr(C)]` pins `sync_error_buf` (a `[u8; N]`, nominal align = 1) at
// offset 0. The struct's overall alignment is ≥ `align_of::<*const ()>()`
// (from the `vm` field), so the buffer's address inherits that alignment.
// This is load-bearing on Windows where `sync_error_buf` is reinterpreted as
// `&mut [u16]` / `&mut WPathBuffer` (see `mkdir_recursive_os_path_impl` and
// the `os_path_kernel32` callers); a misaligned `&mut [u16]` is instant UB.
#[repr(C)]
pub struct NodeFS {
/// Buffer to store a temporary file path that might appear in a returned error message.
///
/// We want to avoid allocating a new path buffer for every error message so that jsc can clone + GC it.
/// That means a stack-allocated buffer won't suffice. Instead, we re-use
/// the heap allocated buffer on the NodeFS struct
pub(crate) sync_error_buf: PathBuffer, // must be align_of::<u16>()-aligned — enforced via #[repr(C)] + field order, see above
/// Scratch for a temporary file path that might appear in a returned error message.
pub(crate) sync_error_buf: paths::path_buffer_pool::Guard,
pub(crate) vm: Option<NonNull<VirtualMachine>>,
}

impl Default for NodeFS {
fn default() -> Self {
Self {
sync_error_buf: PathBuffer::uninit(),
sync_error_buf: paths::path_buffer_pool::get(),
vm: None,
}
}
Expand Down Expand Up @@ -5541,8 +5530,8 @@ impl NodeFS {
}
}

// SAFETY: `NodeFS` is `#[repr(C)]` with `sync_error_buf` at offset 0 and
// struct alignment ≥ pointer-align (from `vm`), so this address is
// SAFETY: `sync_error_buf` is a pooled heap allocation, and every
// supported allocator aligns it to at least 8 bytes, so this address is
// ≥ `align_of::<OSPathChar>()`-aligned. On Windows
// `OSPathBuffer = [u16; PATH_MAX_WIDE]` (65 534 B) which fits inside
// `PathBuffer` (`MAX_PATH_BYTES` = 98 302 B); on POSIX it is the same
Expand All @@ -5551,7 +5540,7 @@ impl NodeFS {
// `&mut PathBuffer` without reborrowing `&mut self` (which would alias
// `working_mem` under stacked borrows). On every such path `working_mem` is
// not used afterward, so the re-derive is sound.
let sync_error_buf_ptr: *mut PathBuffer = &raw mut self.sync_error_buf;
let sync_error_buf_ptr: *mut PathBuffer = &raw mut *self.sync_error_buf;
assert!(
sync_error_buf_ptr.cast::<OSPathChar>().is_aligned(),
"NodeFS.sync_error_buf misaligned for OSPathChar",
Expand Down
18 changes: 6 additions & 12 deletions src/runtime/webcore/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ use bun_http::{self as http, FetchRedirect, Headers, HeadersExt as _, MimeType};
use bun_http_jsc::method_jsc;
use bun_http_types::Method::Method;
use bun_jsc::{HTTPHeaderName, StringJsc as _, SysErrorJsc as _, URLJsc as _};
use bun_paths::{self, PathBuffer};
use bun_sys::FdExt as _;
// `FromJsEnum for FetchRedirect` lives in bun_http_jsc; importing the impl crate
// brings the trait impl into scope for `JSValue::get_optional_enum::<FetchRedirect>`.
Expand Down Expand Up @@ -1267,8 +1266,8 @@ fn fetch_impl<const ALLOW_GET_BODY: bool>(
// We don't pass along headers, we ignore method, we ignore status code...
// But it's better than status quo.
if url_type != URLType::Remote {
let mut path_buf = PathBuffer::uninit();
let mut path_buf2 = PathBuffer::uninit();
let mut path_buf = bun_paths::path_buffer_pool::get();
let mut path_buf2 = bun_paths::path_buffer_pool::get();
let decoded_len = match PercentEncoding::decode_into(
&mut path_buf2[..],
match url_type {
Expand Down Expand Up @@ -1347,11 +1346,11 @@ fn fetch_impl<const ALLOW_GET_BODY: bool>(
}

#[cfg(windows)]
let mut cwd_buf = PathBuffer::uninit();
let mut cwd_buf = bun_paths::path_buffer_pool::get();
#[cfg(windows)]
// `bun_sys::getcwd` returns the byte length written into
// `cwd_buf`; slice it here.
let cwd: &[u8] = match bun_sys::getcwd(&mut cwd_buf) {
let cwd: &[u8] = match bun_sys::getcwd(&mut cwd_buf[..]) {
Ok(len) => &cwd_buf[..len],
Err(err) => {
return Err(global_this.throw_error(err, "Failed to resolve file url"));
Expand Down Expand Up @@ -1517,10 +1516,7 @@ fn fetch_impl<const ALLOW_GET_BODY: bool>(
}
if body.needs_to_read_file() {
'prepare_body: {
// A local `PathBuffer` serves as NUL-termination scratch for
// `path.slice_z()` (the `vm.node_fs()` accessor is gated behind a
// jsc↔runtime cycle).
let mut open_path_buf = PathBuffer::uninit();
let mut open_path_buf = bun_paths::path_buffer_pool::get();
let opened_fd_res: bun_sys::Result<bun_sys::Fd> = {
let store = body.store().expect("needs_to_read_file implies store");
match &store.data.as_file().pathlike {
Expand Down Expand Up @@ -1617,9 +1613,7 @@ fn fetch_impl<const ALLOW_GET_BODY: bool>(
// TODO: make this async + lazy
let blob_offset = body.any_blob().blob().offset.get();
let blob_size = body.any_blob().blob().size.get();
// The `vm.node_fs()` accessor is a jsc↔runtime cycle. `read_file`
// with an `Fd` path only touches `self.sync_error_buf` for
// path-variant inputs, so a fresh `NodeFS` is sufficient here.
// A fresh `NodeFS` suffices: `read_file` on an `Fd` never touches `sync_error_buf`.
let mut node_fs = node::fs::NodeFS::default();
// `ReadFile` has `Drop`; can't use FRU `..Default::default()`.
let mut rf_args = node::fs::args::ReadFile::default();
Expand Down
35 changes: 35 additions & 0 deletions test/js/web/fetch/body-async-iterator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,41 @@ test("Response.bytes() with async iterable body does not crash with null deref",
expect(exitCode).toBe(0);
});

// Each level of this recursion holds one native fetch() frame on the stack.
// Windows used to reach 16 levels before the RangeError because that frame
// carried three 96 KB path buffers (Linux: 387 levels with the same script).
// A debug build of the fix reaches 96 on Windows; stock 1.4.1 reaches 16.
test("fetch() called from a body's Symbol.asyncIterator getter recurses deeply before the stack limit", async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
let depth = 0;
const signal = AbortSignal.abort();
const body = {
get [Symbol.asyncIterator]() {
depth++;
fetch("http://localhost/", { method: "POST", body, signal }).catch(() => {});
return undefined;
},
};
fetch("http://localhost/", { method: "POST", body, signal }).catch(() => {});
console.log(depth);
`,
],
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(Number(stdout.trim())).toBeGreaterThanOrEqual(32);
expect(exitCode).toBe(0);
});

test("Response.arrayBuffer() with async iterable body does not crash with null deref", async () => {
await using proc = Bun.spawn({
cmd: [
Expand Down
Loading