Skip to content
Open
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
163 changes: 154 additions & 9 deletions src/install/PackageInstaller.rs
Original file line number Diff line number Diff line change
Expand Up @@ -731,6 +731,9 @@
let pkg_names = pkgs.items_name();

let completed_trees = &self.completed_trees;
let copied_tree = self
.copy_trees
.is_set_allow_out_of_bound(tree_id as usize, false);
let tree = &mut self.trees[tree_id as usize];
let mut deferred: Vec<DependencyID> = Vec::new();

Expand Down Expand Up @@ -769,6 +772,7 @@
.slice(string_buf);
let package_name_ = strings::StringOrTinyString::init(alias);
let mut target_package_name = package_name_;
let mut target_package_id = package_id;
let mut can_retry_without_native_binlink_optimization = false;
let mut target_node_modules_path_opt: Option<AbsPath> = None;
let mut defer_this_bin = false;
Expand Down Expand Up @@ -836,6 +840,7 @@
pkg_names[replacement_pkg_id as usize].slice(string_buf);
target_package_name =
strings::StringOrTinyString::init(replacement_name);
target_package_id = replacement_pkg_id;
can_retry_without_native_binlink_optimization = true;
}
}
Comment thread
robobun marked this conversation as resolved.
Resolved
Expand Down Expand Up @@ -865,6 +870,29 @@
}

loop {
// `link_rel_buf` is free until the linker runs.
if let Some(source) = Self::symlink_install_source(
manager,
lockfile,
string_buf,
pkg_metas,
pkg_names,
self.resolutions.slice(),
tree_id,
dep_id,
target_package_id,
copied_tree,
link_rel_buf,
) {
bin::make_bins_executable(
bin,
source,
(target_package_id != package_id).then_some(alias),
string_buf,
lockfile.buffers.extern_strings.as_slice(),
);
}

// `node_modules_path` (mut) and `target_node_modules_path`
// (read-only) refer to the same buffer when no replacement is
// set. Derive both from a single `*mut` so the read pointer
Expand Down Expand Up @@ -896,6 +924,7 @@
is_behind_link,
err: None,
skipped_due_to_missing_bin: false,
chmod_only: false,
};

bin_linker.link(global);
Expand All @@ -913,6 +942,7 @@
);
}
target_package_name = package_name_;
target_package_id = package_id;
target_node_modules_path_opt = None;
continue;
}
Expand Down Expand Up @@ -1274,38 +1304,146 @@

/// The entry of the cache that a package which `can_enqueue_install_task` is linked from.
fn cache_entry_name(
&self,
manager: &PackageManager,
string_buf: &[u8],
metas: &[Package::Meta],
package_id: PackageID,
pkg_name: String,
resolution: &Resolution,
patch_contents_hash: Option<u64>,
) -> &'static ZStr {
match resolution.tag {
resolution::Tag::Npm => package_manager::cached_npm_package_folder_name(
self.manager(),
pkg_name.slice(self.lockfile().buffers.string_bytes.as_slice()),
manager,
pkg_name.slice(string_buf),
resolution.npm().version,
patch_contents_hash,
),
resolution::Tag::Git => package_manager::cached_git_folder_name(
self.manager(),
manager,
resolution.git(),
patch_contents_hash,
),
resolution::Tag::Github => package_manager::cached_github_folder_name(
self.manager(),
manager,
resolution.github(),
patch_contents_hash,
),
_ => package_manager::cached_tarball_folder_name(
self.manager(),
&self.metas[package_id as usize].integrity,
manager,
&metas[package_id as usize].integrity,
resolution,
patch_contents_hash,
),
}
}

/// The hash of the patch file applied to a package, `None` if it is not patched.
fn patch_contents_hash(
lockfile: &Lockfile,
string_buf: &[u8],
pkg_name: String,
resolution: &Resolution,
) -> Option<u64> {
if lockfile.patched_dependencies.count() == 0 {
return None;
}
let mut name_and_version: Vec<u8> = Vec::new();
use std::io::Write;
write!(
&mut name_and_version,
"{}@{}",
bstr::BStr::new(pkg_name.slice(string_buf)),
resolution.fmt(string_buf, PathSep::Posix),
)
.expect("unreachable");
lockfile
.patched_dependencies
.get(&bun_semver::string::Builder::string_hash(&name_and_version))?
.patchfile_hash()
}

/// The directory that the files of `package_id` in `node_modules` are symlinks into: its cache
/// entry, or the folder of a `file:` dependency. `None` when they are the real files. The
/// cases follow `install_package`.
Comment thread
robobun marked this conversation as resolved.
Resolved
fn symlink_install_source<'b>(
manager: &mut PackageManager,
lockfile: &Lockfile,
string_buf: &[u8],
metas: &[Package::Meta],
pkg_names: &[String],
resolutions: &[Resolution],
tree_id: lockfile::tree::Id,
dependency_id: DependencyID,
package_id: PackageID,
copied_tree: bool,
buf: &'b mut [u8],
) -> Option<&'b [u8]> {
let resolution = &resolutions[package_id as usize];
let symlink_backend =
PackageInstall::supported_method() == package_install::Method::Symlink;
match resolution.tag {
resolution::Tag::Npm
| resolution::Tag::Git
| resolution::Tag::Github
| resolution::Tag::LocalTarball
| resolution::Tag::RemoteTarball
if symlink_backend && !copied_tree =>
{
let pkg_name = pkg_names[package_id as usize];
// Opens the cache directory, which sets its path.
let _ = package_manager::get_cache_directory(manager);
let cache_path = manager.cache_directory_path.as_bytes();
let entry = Self::cache_entry_name(
manager,
string_buf,
metas,
package_id,
pkg_name,
resolution,
Self::patch_contents_hash(lockfile, string_buf, pkg_name, resolution),
);
// A tarball without an integrity has no entry yet.
if entry.is_empty() {
return None;
}
join_abs_string_buf_z_checked::<platform::Auto>(
cache_path,
buf,
Comment thread
robobun marked this conversation as resolved.
Resolved
&[entry.as_bytes()],
)
.map(ZStr::as_bytes)
}
resolution::Tag::Folder => {
let folder = resolution.folder().slice(string_buf);
let is_transitive = !lockfile.is_workspace_tree_id(tree_id);
// A folder that an npm package declares is a link to the directory.
if is_transitive
&& !lockfile.is_folder_tree_id(tree_id)
&& !lockfile.is_overridden_dependency(dependency_id)
{
return None;
}

Check notice on line 1426 in src/install/PackageInstaller.rs

View check run for this annotation

Claude / Claude Code Review

Pre-existing: users on `--backend symlink` whose npm dependency declares a `file:` dependency with a `bin` still get `Permission denied` for that bin after this merges, the same symptom #46777 reports. The arm at PackageInstaller.rs:1421-1426 returns `Non

Pre-existing: users on `--backend symlink` whose npm dependency declares a `file:` dependency with a `bin` still get `Permission denied` for that bin after this merges, the same symptom #46777 reports. The arm at PackageInstaller.rs:1421-1426 returns `None` for an npm-declared folder, yet its files under `node_modules/<npm pkg>/<folder>` are per-file symlinks into the cache, so the normal linker's `lchmod` at bin.rs:1052 never reaches the real file. Fix: run the chmod-only pass on the cache entr
// `get_install_method` symlinks a package from its own folder.
let own_folder = if is_transitive {
folder == b"."
} else {
matches!(folder, b"" | b".")
};
if !symlink_backend && !own_folder {
return None;
}
join_abs_string_buf_z_checked::<platform::Auto>(
FileSystem::instance().top_level_dir(),
buf,
&[folder],
)
Comment thread
robobun marked this conversation as resolved.
Resolved
.map(ZStr::as_bytes)
}
_ => None,
}
}

/// Returns true if the thread pool can link this package's files. The package is in the current
/// tree.
fn can_defer_link(&self, alias: &[u8], pkg_name: &[u8], package_id: PackageID) -> bool {
Expand Down Expand Up @@ -1887,8 +2025,15 @@
| resolution::Tag::Github
| resolution::Tag::LocalTarball
| resolution::Tag::RemoteTarball => {
installer.cache_dir_subpath =
self.cache_entry_name(package_id, pkg_name, resolution, patch_contents_hash);
installer.cache_dir_subpath = Self::cache_entry_name(
self.manager(),
string_buf!(),
self.metas.slice(),
package_id,
pkg_name,
resolution,
patch_contents_hash,
);
installer.cache_dir = package_manager::get_cache_directory(self.manager_mut());
}
resolution::Tag::Folder | resolution::Tag::Workspace => {
Expand Down
59 changes: 16 additions & 43 deletions src/install/PackageManager/patchPackage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use bun_core::{ZBox, ZStr, handle_oom, strings};
use bun_install_types::NodeLinker::NodeLinker;
use bun_paths::platform;
use bun_paths::resolve_path;
use bun_paths::{AbsPath, Platform, SEP};
use bun_paths::{Platform, SEP};
use bun_sys::{self as sys, Dir, Fd, FdDirExt as _, FdExt as _};

use crate::bin;
Expand Down Expand Up @@ -524,13 +524,7 @@ pub fn do_patch_commit(
let old_folder =
resolve_path::join::<platform::Posix>(&[&cache_path, &cache_dir_subpath]).to_vec();
let new_folder = real_folder(&manager.lockfile, &cache_path, &target.folder, false);
make_bins_executable(
&manager.lockfile,
&pkg,
cache_dir,
&cache_dir_subpath,
&new_folder,
);
make_bins_executable(&manager.lockfile, &pkg, &cache_path, &cache_dir_subpath);

let Some(patchfile_contents) = diff_against_cache(manager, &old_folder, &new_folder) else {
Global::crash();
Expand Down Expand Up @@ -1021,9 +1015,8 @@ pub fn prepare_patch(manager: &mut PackageManager) -> Result<(), crate::Error> {
make_bins_executable(
&manager.lockfile,
&pkg,
cache_result.cache_dir,
&cache_path,
cache_result.cache_dir_subpath.as_bytes(),
&folder,
);
Comment on lines 1015 to 1020

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.

🔴 Users running bun patch <pkg> on a package whose bin starts with a CRLF shebang get a broken node_modules/.bin/<name> ("bad interpreter") for the whole patch session. The chmod-only pass at src/install/PackageManager/patchPackage.rs:1015-1020 no longer normalizes the cache entry's shebang, and replace_with_cache_copy then copies the #!/bin/sh\r file into node_modules/<pkg>. Fix: keep shebang normalization for cache entries in both patch callers (prepare and commit), e.g. a normalize flag on bin::make_bins_executable that the install pass leaves off for file: folders, or normalize the copied bin after replace_with_cache_copy. The base ran the full linker here, so the cache file was rewritten to LF before the copy.

Why this was flagged

A registry package ships bin/cli.js whose first line ends in \r\n. bun install normalizes only the copy in node_modules (src/install/bin.rs:1045), so the cache entry keeps the CRLF. The user runs bun patch <pkg>: prepare_patch calls make_bins_executable at src/install/PackageManager/patchPackage.rs:1015-1020, which now reaches bin::make_bins_executable with chmod_only: true (src/install/bin.rs:2046); the early return at src/install/bin.rs:981-985 calls only set_exec_mode, never try_normalize_shebang. replace_with_cache_copy (patchPackage.rs:1026) then replaces node_modules/<pkg> with the CRLF copy, while node_modules/.bin/<name> still points at node_modules/<pkg>/bin/cli.js. Running that bin fails with the kernel's "bad interpreter: No such file or directory". On the base branch the same call (base patchPackage.rs:1145-1186) ran the full Linker::link into a temp .bin, whose make_executable normalized the cache file first, so the copy was LF and the bin ran. The test at test/regression/issue/46777.test.ts:120 only covers the install path, not bun patch.

Verification: Base patchPackage.rs:1145-1185 ran a full Linker::link, reaching try_normalize_shebang (src/install/bin.rs:1045), which rewrote the cache entry's bin/cli.js to LF before the copy. New patchPackage.rs:1015-1020 uses chmod_only: true (bin.rs:2046); bin.rs:981-985 returns after set_exec_mode, so try_normalize_shebang never runs on the cache file, and replace_with_cache_copy (patchPackage.rs:1026-1031) byte-copies #!/bin/sh\r into node_modules/<pkg>.


// The package may be installed using the hard link method,
Expand Down Expand Up @@ -1145,44 +1138,24 @@ fn download_if_not_cached(manager: &mut PackageManager, pkg: &Package) {
fn make_bins_executable(
lockfile: &Lockfile,
pkg: &Package,
cache_dir: Fd,
cache_path: &[u8],
cache_dir_subpath: &[u8],
folder: &[u8],
) {
if cfg!(windows) || pkg.bin.tag == bin::Tag::None {
return;
}
// `bin::Linker` sets the mode of the files it links to. The links themselves are not needed, so
// they go in a temporary folder.
let links = TempTree(temp_sibling(folder));
let (Ok(cache_path), Ok(mut links_path)) = (
<AbsPath>::init_fd_path(cache_dir),
<AbsPath>::from(links.0.as_slice()),
let mut buf = bun_paths::path_buffer_pool::get();
let Some(cache_entry) = resolve_path::join_abs_string_buf_z_checked::<platform::Auto>(
cache_path,
&mut buf[..],
&[cache_dir_subpath],
) else {
return;
};
let cache_entry = strings::StringOrTinyString::init(cache_dir_subpath);
let mut abs_target_buf = bun_paths::path_buffer_pool::get();
let mut abs_dest_buf = bun_paths::path_buffer_pool::get();
let mut rel_buf = bun_paths::path_buffer_pool::get();
bin::Linker {
bin: pkg.bin,
target_node_modules_path: &raw const cache_path,
target_package_name: cache_entry,
seen: None,
node_modules_path: &mut links_path,
package_name: cache_entry,
global_bin_path: ZStr::EMPTY,
string_buf: lockfile.buffers.string_bytes.as_slice(),
extern_string_buf: lockfile.buffers.extern_strings.as_slice(),
abs_target_buf: abs_target_buf.as_mut_slice(),
abs_dest_buf: abs_dest_buf.as_mut_slice(),
rel_buf: rel_buf.as_mut_slice(),
is_behind_link: Some(false),
err: None,
skipped_due_to_missing_bin: false,
}
.link(false);
bin::make_bins_executable(
pkg.bin,
cache_entry.as_bytes(),
None,
lockfile.buffers.string_bytes.as_slice(),
lockfile.buffers.extern_strings.as_slice(),
);
}

/// The patched package's cache entry can be missing, because an install that finds the patched
Expand Down
Loading
Loading