Skip to content

install: stop panicking on bin values longer than the path buffer #38954

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 7 commits into
base: main
Choose a base branch
from
Open

install: stop panicking on bin values longer than the path buffer #38954

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c1c936a
install: stop panicking on bin values longer than the path buffer
robobun Aug 15, 2026
e766739
test: keep the deep bin directory's link targets under 1 KiB
robobun Aug 15, 2026
ea67906
install: trim the bin linker comments
robobun Aug 15, 2026
e70b86b
install: shorten two bin linker comments
robobun Aug 15, 2026
e077ea6
install: size the relative link target before computing it
robobun Aug 15, 2026
6aecc30
test: cover a native binlink redirect whose bin value does not fit th…
robobun Aug 15, 2026
eecb876
test: keep the created link target in the bun link test under 1 KiB
robobun Aug 15, 2026
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
219 changes: 124 additions & 95 deletions src/install/bin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1042,11 +1042,10 @@ impl<'a> Linker<'a> {

// Create temporary file path
let mut tmppath_buf = [0u8; MAX_PATH_BYTES];
let tmppath = resolve_path::join_abs_string_buf_z::<PlatformAuto>(
dir_path,
&mut tmppath_buf,
&[tmpname.as_bytes()],
);
let Some(tmppath) = Self::join_z_checked(dir_path, tmpname.as_bytes(), &mut tmppath_buf)
else {
return;
};
let mut needs_unlink = true;
let unlink_guard = scopeguard::guard(&mut needs_unlink, |needs_unlink| {
if *needs_unlink {
Expand Down Expand Up @@ -1464,59 +1463,85 @@ impl<'a> Linker<'a> {
///
/// Falls through to (1) when nothing exists so the existing
/// `skipped_due_to_missing_bin` retry-without-redirect path still fires.
///
/// Returns `None` when the path to link does not fit `buf`; `sys::exists`
/// reports such a path missing, so callers record a missing bin.
Comment thread
robobun marked this conversation as resolved.
Outdated
// `is_native_binlink_redirect()` is hoisted to a parameter so the
// caller can drop its `&self` borrow before mutably calling
// `link_bin_or_create_shim`. Result borrows the threadlocal join buffer
// (lifetime tied to `package_dir` per `join_abs_string_z`'s signature).
// `link_bin_or_create_shim`. The result borrows only `buf`, never
// `package_dir` (which lives in `self.abs_target_buf`).
Comment thread
robobun marked this conversation as resolved.
Outdated
fn resolve_bin_target<'b>(
is_native_binlink_redirect: bool,
package_dir: &'b [u8],
package_dir: &[u8],
target: &[u8],
bin_name: &[u8],
) -> &'b ZStr {
buf: &'b mut [u8],
) -> Option<&'b ZStr> {
// A trailing separator would make `lchmod` follow a symlinked target; npm drops it too.
let target = strings::without_trailing_slash(target);
let primary = resolve_path::join_abs_string_z::<PlatformAuto>(package_dir, &[target]);

if !is_native_binlink_redirect {
return primary;
}

if sys::exists(primary.as_bytes()) {
return primary;
}

if !bin_name.is_empty() {
let at_root = resolve_path::join_abs_string_z::<PlatformAuto>(package_dir, &[bin_name]);
if sys::exists(at_root.as_bytes()) {
return at_root;
}
return Self::join_z_checked(package_dir, target, buf);
}

let target_basename = path::basename(target);
if !target_basename.is_empty() && target_basename.len() != target.len() {
let at_root =
resolve_path::join_abs_string_z::<PlatformAuto>(package_dir, &[target_basename]);
if sys::exists(at_root.as_bytes()) {
return at_root;
}
}
let exe_name: Vec<u8> =
if !bin_name.is_empty() && !strings::has_suffix_comptime(bin_name, b".exe") {
[bin_name, b".exe"].concat()
} else {
Vec::new()
};
let candidates: [&[u8]; 4] = [
// (1)
target,
// (2)
bin_name,
// (3), only when `target` has a directory component
if target_basename.len() != target.len() {
target_basename
} else {
b""
},
// (4)
&exe_name,
];

if !bin_name.is_empty() && !strings::has_suffix_comptime(bin_name, b".exe") {
let mut exe_name = Vec::with_capacity(bin_name.len() + b".exe".len());
exe_name.extend_from_slice(bin_name);
exe_name.extend_from_slice(b".exe");
let at_root =
resolve_path::join_abs_string_z::<PlatformAuto>(package_dir, &[&exe_name]);
if sys::exists(at_root.as_bytes()) {
return at_root;
for candidate in candidates {
if candidate.is_empty() {
continue;
}
let Some(abs_candidate) = Self::join_z_checked(package_dir, candidate, buf) else {
continue;
};
if sys::exists_z(abs_candidate) {
let len = abs_candidate.len();
return Some(ZStr::from_buf(buf, len));
}
}

// Nothing found; return the primary so `linkBinOrCreateShim` sets
// Nothing found; return the primary so `link_bin_or_create_shim` sets
// `skipped_due_to_missing_bin` and the caller retries without the
// redirect.
resolve_path::join_abs_string_z::<PlatformAuto>(package_dir, &[target])
Self::join_z_checked(package_dir, target, buf)
}

/// `<dir>/<relative>`, normalized and NUL-terminated in `buf`, or `None`
/// when the normalized path does not fit. `relative` can be any length: bin
/// values are stored as written in package.json, and `dir` itself may be
/// close to the limit. Every `buf` passed here is `PathBuffer` sized, so a
/// path that does not fit is one `sys::exists` reports missing and
/// `sys::open_dir_absolute` rejects with `ENAMETOOLONG`; callers take the
/// branch they take for that outcome.
Comment thread
robobun marked this conversation as resolved.
Outdated
fn join_z_checked<'b>(dir: &[u8], relative: &[u8], buf: &'b mut [u8]) -> Option<&'b ZStr> {
let without_nul = buf.len() - 1;
let len = resolve_path::join_abs_string_buf_checked::<PlatformAuto>(
dir,
&mut buf[..without_nul],
&[relative],
)?
.len();
buf[len] = 0;
Some(ZStr::from_buf(buf, len))
}

/// uses `self.abs_target_buf`
Expand Down Expand Up @@ -1589,8 +1614,9 @@ impl<'a> Linker<'a> {
debug_assert!(self.bin.tag != Tag::None);

// `link_bin_or_create_shim(&mut self, ..)`
// is called while `abs_target` / `abs_dest` borrow `self.abs_target_buf`
// / `self.abs_dest_buf`. `link_bin_or_create_shim` never reads or writes
// is called while `abs_dest` borrows `self.abs_dest_buf` (and, in the
// `Dir` arm, `abs_target` borrows `self.abs_target_buf`).
// `link_bin_or_create_shim` never reads or writes
Comment thread
robobun marked this conversation as resolved.
Outdated
// those two buffers (it only touches `rel_buf`, `node_modules_path`, `seen`, `err`,
// `skipped_due_to_missing_bin`). Detach the `abs_dest` borrow via a raw
// pointer so borrowck allows the disjoint access; the SAFETY invariant
Expand All @@ -1599,6 +1625,11 @@ impl<'a> Linker<'a> {
// is re-derived inside each arm so no detached borrow is needed for it.
let abs_dest_buf_ptr: *mut u8 = self.abs_dest_buf.as_mut_ptr();

// The resolved target cannot go into `self.abs_target_buf`: that holds
// `package_dir`, which the target is joined onto.
Comment thread
robobun marked this conversation as resolved.
Outdated
let mut resolved_target_pool_buf = path::path_buffer_pool::get();
let resolved_target_buf = resolved_target_pool_buf.as_mut_slice();

// SAFETY: tag determines the active union field
unsafe {
match self.bin.tag {
Expand All @@ -1616,20 +1647,15 @@ impl<'a> Linker<'a> {
Dependency::unscoped_package_name(self.package_name.slice());

// for normalizing `target`
let abs_target: &ZStr = {
let package_dir = &self.abs_target_buf[0..package_dir_len];
let r = Self::resolve_bin_target(
is_redirect,
package_dir,
target,
unscoped_package_name,
);
// SAFETY: `resolve_bin_target` writes into the thread-local
// `PARSER_JOIN_INPUT_BUFFER` (via `join_abs_string_z`); the
// returned slice does not actually borrow `self` or
// `package_dir`. Detach the lifetime so `self` can be
// re-borrowed mutably below.
ZStr::from_raw(r.as_bytes().as_ptr(), r.len())
let Some(abs_target) = Self::resolve_bin_target(
is_redirect,
&self.abs_target_buf[0..package_dir_len],
target,
unscoped_package_name,
resolved_target_buf,
) else {
self.skipped_due_to_missing_bin = true;
return;
};

if unscoped_package_name.len()
Expand Down Expand Up @@ -1672,16 +1698,15 @@ impl<'a> Linker<'a> {
}

// for normalizing `target`
let abs_target: &ZStr = {
let package_dir = &self.abs_target_buf[0..package_dir_len];
let r = Self::resolve_bin_target(
is_redirect,
package_dir,
target,
normalized_name,
);
// SAFETY: thread-local buffer; see Tag::File above.
ZStr::from_raw(r.as_bytes().as_ptr(), r.len())
let Some(abs_target) = Self::resolve_bin_target(
is_redirect,
&self.abs_target_buf[0..package_dir_len],
target,
normalized_name,
resolved_target_buf,
) else {
self.skipped_due_to_missing_bin = true;
return;
};

self.abs_dest_buf[dest_off..dest_off + normalized_name.len()]
Expand Down Expand Up @@ -1728,16 +1753,16 @@ impl<'a> Linker<'a> {
return;
}

let abs_target: &ZStr = {
let package_dir = &self.abs_target_buf[0..package_dir_len];
let r = Self::resolve_bin_target(
is_redirect,
package_dir,
bin_target,
normalized_bin_dest,
);
// SAFETY: thread-local buffer; see Tag::File above.
ZStr::from_raw(r.as_bytes().as_ptr(), r.len())
let Some(abs_target) = Self::resolve_bin_target(
is_redirect,
&self.abs_target_buf[0..package_dir_len],
bin_target,
normalized_bin_dest,
resolved_target_buf,
) else {
self.skipped_due_to_missing_bin = true;
i += 2;
continue;
};

dest_off = abs_dest_dir_end;
Expand Down Expand Up @@ -1766,15 +1791,13 @@ impl<'a> Linker<'a> {
return;
}
// for normalizing `target`
let abs_target_dir: &ZStr = {
let package_dir = &self.abs_target_buf[0..package_dir_len];
let r =
resolve_path::join_abs_string_z::<PlatformAuto>(package_dir, &[target]);
// SAFETY: `join_abs_string_z` writes into the thread-local
// `PARSER_JOIN_INPUT_BUFFER`; result does not borrow
// `package_dir`. Detached so `abs_target_buf` can be
// reused inside the loop body (see the SAFETY note below).
ZStr::from_raw(r.as_bytes().as_ptr(), r.len())
let Some(abs_target_dir) = Self::join_z_checked(
&self.abs_target_buf[0..package_dir_len],
target,
resolved_target_buf,
) else {
self.err = Some(crate::Error::Sys(bun_errno::SystemErrno::ENAMETOOLONG));
return;
};

let target_dir = match sys::open_dir_absolute(abs_target_dir.as_bytes()) {
Expand All @@ -1800,13 +1823,17 @@ impl<'a> Linker<'a> {
match entry.kind {
sys::EntryKind::SymLink | sys::EntryKind::File => {
let entry_name = entry.name.slice_u8();
// `self.abs_target_buf` is available now because `path::join_abs_string_z` copied everything into `parse_join_input_buffer`
// `self.abs_target_buf` is free now: `package_dir` has been
// folded into `abs_target_dir`.
Comment thread
robobun marked this conversation as resolved.
Outdated
let abs_target: &ZStr = {
let r = resolve_path::join_abs_string_buf_z::<PlatformAuto>(
let Some(r) = Self::join_z_checked(
abs_target_dir.as_bytes(),
entry_name,
self.abs_target_buf,
&[entry_name],
);
) else {
self.skipped_due_to_missing_bin = true;
continue;
};
// SAFETY: result lives in `self.abs_target_buf`, which
// `link_bin_or_create_shim` does not write to (only
// `rel_buf`/`node_modules_path`/`seen`/`err`/
Expand Down Expand Up @@ -1847,11 +1874,6 @@ impl<'a> Linker<'a> {

debug_assert!(self.bin.tag != Tag::None);

// see `link()` — detach abs_target_buf borrow via raw ptr.
let abs_target_buf_ptr: *const u8 = self.abs_target_buf.as_ptr();
// SAFETY: abs_target_buf is not written between here and use.
let package_dir = unsafe { bun_core::ffi::slice(abs_target_buf_ptr, package_dir_len) };

// SAFETY: tag determines the active union field
unsafe {
match self.bin.tag {
Expand Down Expand Up @@ -1936,8 +1958,15 @@ impl<'a> Linker<'a> {
return;
}

let abs_target_dir =
resolve_path::join_abs_string_z::<PlatformAuto>(package_dir, &[target]);
let mut abs_target_dir_buf = path::path_buffer_pool::get();
let Some(abs_target_dir) = Self::join_z_checked(
&self.abs_target_buf[0..package_dir_len],
target,
abs_target_dir_buf.as_mut_slice(),
) else {
self.err = Some(crate::Error::Sys(bun_errno::SystemErrno::ENAMETOOLONG));
return;
};

let target_dir = match sys::open_dir_absolute(abs_target_dir.as_bytes()) {
Ok(d) => d,
Expand Down
Loading
Loading