Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
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
173 changes: 168 additions & 5 deletions src/runtime/webcore/Blob.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3033,18 +3033,23 @@ impl BlobExt for Blob {
if let Some(blob) = item.as_class_ref::<Blob>() {
could_have_non_ascii = could_have_non_ascii
|| blob.charset.get() != strings::AsciiStatus::AllAscii;
let view = blob.shared_view();
if view.is_empty() {
if blob.needs_to_read_file() || blob.is_s3() {
push_file_part(blob, &mut joiner, global)?;
Comment thread
robobun marked this conversation as resolved.
Resolved
}
continue;
Comment thread
robobun marked this conversation as resolved.
Resolved
}
// A later part may run user JS that drops the
// last ref to this Blob's Store before `done()`.
if parts_can_run_js {
joiner.push_cloned(blob.shared_view());
joiner.push_cloned(view);
} else {
// SAFETY: the prescan above proved no
// remaining part can run user JS, so this
// Blob (rooted via `_keep`/`arg`) keeps its
// Store alive until `joiner.done()` below.
joiner.push(unsafe {
bun_ptr::detach_lifetime(blob.shared_view())
});
joiner.push(unsafe { bun_ptr::detach_lifetime(view) });
}
continue;
} else {
Expand Down Expand Up @@ -3072,7 +3077,12 @@ impl BlobExt for Blob {
// This arm only handles entries deferred onto the walk
// stack; other pending entries may still run user JS and
// free this Blob's Store before `done()`, so always copy.
joiner.push_cloned(blob.shared_view());
let view = blob.shared_view();
if !view.is_empty() {
joiner.push_cloned(view);
} else if blob.needs_to_read_file() || blob.is_s3() {
push_file_part(blob, &mut joiner, global)?;
}
Comment on lines +3080 to +3085

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit (optional): maintainers get a second copy of the file-part dispatch in an arm no input can reach, so it is untested and will drift from the live one. Nothing ever pushes onto stack (its only writes are reserve at Blob.rs:2961 and pop at Blob.rs:3109), and a top-level Blob returns from the fast path at Blob.rs:2871-2906, so the Blob branch of this DOMWrapper arm never runs. Fix: do not extend dead code; either drop the Blob branch of this arm (keeping the to_utf8 fallback) or delete the unused walk stack, so the only push_file_part call site is the array loop at Blob.rs:3039.

Why this was flagged

The loop at Blob.rs:2948 only sees current as the original argument or a value from stack.pop() at Blob.rs:3109, and stack is never pushed anywhere in the file (grep shows only Vec::new, reserve, is_empty, pop). A Blob argument that is not inside an array returns earlier at Blob.rs:2871-2906 (dupe/move), and with REQUIRE_ARRAY the non-array case throws at Blob.rs:2933. So the new push_file_part call at Blob.rs:3084 and the view.is_empty() branch at Blob.rs:3081 cannot execute for any input; the base branch had the same unreachable push_cloned there. No user-visible failure; the cost is a duplicated, untestable copy of the file-part dispatch that REVIEW.md's 'every line you add must be demonstrably live' rule asks to avoid.

Verification: The Blob branch of the top-level DOMWrapper arm (src/runtime/webcore/Blob.rs:3074-3085), which this PR extends with a second push_file_part call, is unreachable, so the new dispatch there is dead and untested; the base already carried a dead joiner.push_cloned(blob.shared_view()) at the same spot, so the PR adds to pre-existing dead code rather than creating it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. This arm is dead on main too: nothing pushes onto stack, and a Blob at the top level returns before the loop. So my change to it cannot run, and no test can cover it.

The next push to this branch puts the line of main back here (joiner.push_cloned(blob.shared_view());). The array loop is then the only caller of push_file_part. I do not push for this alone: the finding is optional, and a push starts CI again on a head that the review calls not blocking. This thread stays open until that push.

} else {
let utf8 = current.to_utf8(global)?;
could_have_non_ascii = could_have_non_ascii || utf8.is_owned();
Expand Down Expand Up @@ -3266,6 +3276,159 @@ impl BlobExt for Blob {
}
}

/// The null device has no bytes, and a read of it cannot block.
fn is_null_device(stat: &bun_sys::Stat) -> bool {
if !bun_sys::S::ISCHR(stat.st_mode as _) {
return false;
}
// Linux fixes the null device at major 1, minor 3, so a sandbox with no /dev still finds it.
#[cfg(any(target_os = "linux", target_os = "android"))]
{
stat.st_rdev == libc::makedev(1, 3)
}
#[cfg(not(any(target_os = "linux", target_os = "android")))]
{
#[cfg(windows)]
let null_device = bun_core::zstr!("\\\\.\\NUL");
#[cfg(not(windows))]
let null_device = bun_core::zstr!("/dev/null");
bun_sys::stat(null_device).is_ok_and(|null| null.st_rdev == stat.st_rdev)
}
}

/// Reads a file-backed part of a multi-part `new Blob([...])`: `shared_view()` is empty for it.
#[cold]
#[inline(never)]
fn push_file_part(
blob: &Blob,
joiner: &mut bun_core::string_joiner::StringJoiner,
global: &JSGlobalObject,
) -> JsResult<()> {
let Some(store) = blob.store.get() else {
return Ok(());
};
let file = match &store.data {
store::Data::Bytes(_) => return Ok(()),
store::Data::S3(_) => {
return Err(global.throw_invalid_arguments(format_args!(
"Blob parts backed by S3 cannot be read synchronously; await .bytes() or .arrayBuffer() first"
)));
Comment thread
robobun marked this conversation as resolved.
Resolved
}
store::Data::File(file) => file,
};
let throw = |err: bun_sys::Error| {
let err = match &file.pathlike {
PathOrFileDescriptor::Path(path) => err.with_path(path.slice()),
PathOrFileDescriptor::Fd(fd) => err.with_fd(*fd),
};
global.throw_value(err.to_js(global))
};

let no_bytes_or_refused = |stat: &bun_sys::Stat| -> JsResult<()> {
if is_null_device(stat) {
return Ok(());
}
Err(global.throw_invalid_arguments(format_args!(
"Blob parts backed by a pipe, socket or device cannot be read synchronously; await .bytes() or .arrayBuffer() first"
)))
};
let is_file_or_dir = |mode| bun_sys::S::ISREG(mode) || bun_sys::S::ISDIR(mode);

let (fd, _close_on_drop) = match &file.pathlike {
PathOrFileDescriptor::Fd(fd) => (*fd, None),
PathOrFileDescriptor::Path(path) => {
let mut path_buf = bun_paths::path_buffer_pool::get();
let path = path.slice_z(&mut path_buf);
// open(2) of a FIFO wakes the writer that waits for a reader, so refuse it unopened.
match bun_sys::stat(path) {
Ok(stat) if !is_file_or_dir(stat.st_mode as _) => {
return no_bytes_or_refused(&stat);
}
_ => {}
}
// O_NONBLOCK: if the path is a FIFO by now, open(2) must not block the JS thread.
let flags = bun_sys::O::RDONLY
| bun_sys::O::NONBLOCK
| bun_sys::O::CLOEXEC
| bun_sys::O::NOCTTY;
match bun_sys::File::open(path, flags, 0) {
Ok(opened) => (opened.handle(), Some(opened)),
Err(err) => return Err(throw(err)),
}
}
};
let stat = match bun_sys::fstat(fd) {
Ok(stat) => stat,
Err(err) => return Err(throw(err)),
};
if bun_sys::S::ISDIR(stat.st_mode as _) {
return Err(throw(bun_sys::Error::from_code(
bun_sys::E::EISDIR,
bun_sys::Tag::read,
)));
}
if !bun_sys::S::ISREG(stat.st_mode as _) {
return no_bytes_or_refused(&stat);
}

let offset = blob.offset.get();
let size = blob.size.get();
let file_len = stat.st_size.max(0) as SizeType;
// st_size==0 may be a virtual file (procfs): grow-until-EOF instead of capping at 0.
let avail = if file_len > 0 {
file_len.saturating_sub(offset) as usize
} else {
usize::MAX
};
let cap = if size != MAX_SIZE {
(size as usize).min(avail)
} else {
avail
};
if cap == 0 {
return Ok(());
}
// The join holds a part twice (this buffer, then the joined one), so half the RAM is the most that fits.
if file_len > 0 && cap >= bun_core::get_total_memory_size() / 2 {
return Err(throw(bun_sys::Error::oom()));
}
Comment thread
robobun marked this conversation as resolved.
Resolved
let initial = if file_len > 0 { cap } else { 8192.min(cap) }.min(8 << 30);
let mut buf: Vec<u8> = Vec::new();
if buf.try_reserve_exact(initial).is_err() {
return Err(throw(bun_sys::Error::oom()));
}
buf.resize(initial, 0);
Comment thread
robobun marked this conversation as resolved.
Resolved
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
let mut total = 0usize;
loop {
if total == buf.len() {
if total == cap {
break;
}
let new_len = buf.len().saturating_mul(2).min(cap);
if buf.try_reserve(new_len - buf.len()).is_err() {
return Err(throw(bun_sys::Error::oom()));
}
buf.resize(new_len, 0);
}
// pread, not read: the cursor of a caller's fd must not decide what a repeated part reads.
let n = match bun_sys::pread(
fd,
&mut buf[total..],
(offset as i64).saturating_add(total as i64),
) {
Ok(n) => n,
Err(err) => return Err(throw(err)),
};
if n == 0 {
break;
}
total += n;
}
buf.truncate(total);
joiner.push_owned(buf.into_boxed_slice());
Ok(())
}

// ──────────────────────────────────────────────────────────────────────────
// Basic accessors
// ──────────────────────────────────────────────────────────────────────────
Expand Down
Loading
Loading