Skip to content
Merged
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
76 changes: 44 additions & 32 deletions src/install/bin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1519,71 +1519,78 @@ impl<'a> Linker<'a> {
resolve_path::join_abs_string_z::<PlatformAuto>(package_dir, &[target])
}

/// uses `self.abs_target_buf`
pub(crate) fn build_target_package_dir(&mut self) -> &[u8] {
/// Length of `<node_modules>/<package>/` in `abs_target_buf`, `None` if a NUL no longer fits.
pub(crate) fn build_target_package_dir(&mut self) -> Option<usize> {
// SAFETY: `target_node_modules_path` is set at construction to either
// a caller-owned `AbsPath` or the same buffer as `node_modules_path`;
// both outlive `self` and are not mutated for the duration of this
// read.
let dest_dir_without_trailing_slash =
strings::without_trailing_slash(unsafe { (*self.target_node_modules_path).slice() });
let package_name = self.target_package_name.slice();

let buf = &mut *self.abs_target_buf;
if dest_dir_without_trailing_slash.len() + 1 + package_name.len() + 1 >= buf.len() {
return None;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

// reshaped for borrowck — track offset instead of remain.ptr arithmetic
let mut off: usize = 0;
let buf = &mut *self.abs_target_buf;

buf[off..off + dest_dir_without_trailing_slash.len()]
.copy_from_slice(dest_dir_without_trailing_slash);
off += dest_dir_without_trailing_slash.len();
buf[off] = SEP;
off += 1;

let package_name = self.target_package_name.slice();
buf[off..off + package_name.len()].copy_from_slice(package_name);
off += package_name.len();
buf[off] = SEP;
off += 1;

&self.abs_target_buf[0..off]
Some(off)
}

/// Returns the offset into `self.abs_dest_buf` where the destination dir ends
/// (i.e. where the bin name should be written).
// Returning an offset (rather than a slice into abs_dest_buf) avoids
// overlapping &mut borrows of self.
pub(crate) fn build_destination_dir(&mut self, global: bool) -> usize {
let dest_dir_without_trailing_slash =
strings::without_trailing_slash(self.node_modules_path.slice());
/// Length of the `.bin/` (or global bin) dir in `abs_dest_buf`, `None` if a NUL no longer fits.
pub(crate) fn build_destination_dir(&mut self, global: bool) -> Option<usize> {
let dest_dir_without_trailing_slash = if global {
strings::without_trailing_slash(self.global_bin_path.as_bytes())
} else {
strings::without_trailing_slash(self.node_modules_path.slice())
};
let suffix_len = if global { b"/".len() } else { b"/.bin/".len() };

let buf = &mut *self.abs_dest_buf;
if dest_dir_without_trailing_slash.len() + suffix_len >= buf.len() {
return None;
}

let mut off: usize = 0;
if global {
let global_bin_path_without_trailing_slash =
strings::without_trailing_slash(self.global_bin_path.as_bytes());
buf[off..off + global_bin_path_without_trailing_slash.len()]
.copy_from_slice(global_bin_path_without_trailing_slash);
off += global_bin_path_without_trailing_slash.len();
buf[off..off + dest_dir_without_trailing_slash.len()]
.copy_from_slice(dest_dir_without_trailing_slash);
off += dest_dir_without_trailing_slash.len();
buf[off] = SEP;
off += 1;
if !global {
buf[off..off + b".bin".len()].copy_from_slice(b".bin");
off += b".bin".len();
buf[off] = SEP;
off += 1;
} else {
buf[off..off + dest_dir_without_trailing_slash.len()]
.copy_from_slice(dest_dir_without_trailing_slash);
off += dest_dir_without_trailing_slash.len();
// sep_str ++ ".bin" ++ sep_str
buf[off] = SEP;
buf[off + 1..off + 1 + b".bin".len()].copy_from_slice(b".bin");
buf[off + 1 + b".bin".len()] = SEP;
off += b"/.bin/".len();
}

off
Some(off)
}

// target: what the symlink points to
// destination: where the symlink exists on disk
pub fn link(&mut self, global: bool) {
let package_dir_len = self.build_target_package_dir().len();
let mut dest_off = self.build_destination_dir(global);
let (Some(package_dir_len), Some(mut dest_off)) = (
self.build_target_package_dir(),
self.build_destination_dir(global),
) else {
self.err = Some(crate::Error::Sys(bun_errno::SystemErrno::ENAMETOOLONG));
return;
};
let is_redirect = self.is_native_binlink_redirect();

debug_assert!(self.bin.tag != Tag::None);
Expand Down Expand Up @@ -1842,8 +1849,13 @@ impl<'a> Linker<'a> {
}

pub fn unlink(&mut self, global: bool) {
let package_dir_len = self.build_target_package_dir().len();
let mut dest_off = self.build_destination_dir(global);
let (Some(package_dir_len), Some(mut dest_off)) = (
self.build_target_package_dir(),
self.build_destination_dir(global),
) else {
self.err = Some(crate::Error::Sys(bun_errno::SystemErrno::ENAMETOOLONG));
return;
};

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

Expand Down
109 changes: 108 additions & 1 deletion test/cli/install/bun-install-registry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@ import { file, spawn, write } from "bun";
import { install_test_helpers, npm_manifest_test_helpers } from "bun:internal-for-testing";
import { afterAll, beforeEach, describe, expect, setDefaultTimeout, test } from "bun:test";
import { copyFileSync, mkdirSync } from "fs";
import { cp, exists, lstat, mkdir, readlink, rm, writeFile } from "fs/promises";
import { cp, exists, lstat, mkdir, readlink, rename, rm, writeFile } from "fs/promises";
import {
assertManifestsPopulated,
bunExe,
bunEnv as env,
isFlaky,
isLinux,
isMacOS,
isWindows,
mergeWindowEnvs,
Expand Down Expand Up @@ -3256,6 +3257,112 @@ describe("binaries", () => {
expect(await result.exited).toBe(0);
}

// Before it appends a bin name, the bin linker writes `<node_modules>/<package>/` and
// `<node_modules>/.bin/` into two path buffers of MAX_PATH_BYTES (4096 bytes on Linux, 1024 on
// macOS). A project deep enough for one of them not to fit has to fail like a bin name that does
// not fit. Windows is left out: its buffer holds more than any path the OS accepts.
describe.skipIf(isWindows)("directories longer than the path buffer", () => {
const maxPathBytes = isLinux ? 4096 : 1024;

// Writes `<dir>/<name>`, a package whose bin is named after it.
const writeBinPackage = (dir: string, name: string) =>
Promise.all([
write(
join(dir, name, "package.json"),
JSON.stringify({ name, version: "1.0.0", bin: { [name]: `${name}.js` } }),
),
write(join(dir, name, `${name}.js`), `#!/usr/bin/env node\nconsole.log("${name}")`),
]);

// Directory names of at most 200 bytes which bring `base` to exactly `length` bytes.
function componentsUpTo(base: string, length: number) {
const components: string[] = [];
let remaining = length - Buffer.byteLength(base);
while (remaining > 0) {
let len = Math.min(200, remaining - "/".length);
// Never leave exactly one byte: it would have to be a separator with no name after it.
if (remaining - "/".length - len === 1) len -= 1;
components.push(Buffer.alloc(len, "d").toString());
remaining -= "/".length + len;
}
return components;
}

// Creates, under `base`, a directory whose `<dir>/<subpath>` is exactly maxPathBytes long. The
// directory itself fits, so it can be created. Whatever bun creates below `<dir>/<subpath>` does
// not, so `shorten()` renames the first directory name to one byte before those paths are used.
function directoryFilledBy(base: string, subpath: string) {
const components = componentsUpTo(base, maxPathBytes - Buffer.byteLength("/" + subpath));
const dir = join(base, ...components);
expect(Buffer.byteLength(join(dir, subpath))).toBe(maxPathBytes);
mkdirSync(dir, { recursive: true });
const shorten = async () => {
await rename(join(base, components[0]), join(base, "x"));
return join(base, "x", ...components.slice(1));
};
return { dir, shorten };
}

async function run(args: string[], cwd: string, runEnv: NodeJS.Dict<string>) {
await using proc = spawn({
cmd: [bunExe(), ...args],
cwd,
stdout: "pipe",
stderr: "pipe",
env: runEnv,
});
const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
return { out, err, exitCode };
}

// With `has-bin`, `<project>/node_modules/has-bin/` is one byte too long for the buffer. With
// `a`, the package directory fits and `<project>/node_modules/.bin/` is one byte too long.
test.each([
["package directory", "has-bin", join("node_modules", "has-bin")],
[".bin directory", "a", join("node_modules", ".bin")],
])("install reports a %s that does not fit as ENAMETOOLONG", async (_, name, subpath) => {
await writeBinPackage(packageDir, name);
const { dir: project, shorten } = directoryFilledBy(packageDir, subpath);
await write(
join(project, "package.json"),
JSON.stringify({ name: "foo", dependencies: { [name]: `file:${join(packageDir, name)}` } }),
);

const { out, err, exitCode } = await run(["install"], project, env);
const shortened = await shorten();

expect(err).toContain(`error: Failed to link ${name}: ENAMETOOLONG`);
expect(out).not.toContain("installed");
// The package itself was installed. Only its bin is missing.
expect(await exists(join(shortened, "node_modules", name, "package.json"))).toBeTrue();
expect(await exists(join(shortened, "node_modules", ".bin", name))).toBeFalse();
expect(exitCode).toBe(1);
});

// `bun link` symlinks the package into the global directory's node_modules and links its bins
// from there, into the global bin directory.
test("bun link reports a global package directory that does not fit as ENAMETOOLONG", async () => {
await writeBinPackage(packageDir, "has-bin");
const { dir: globalDir, shorten } = directoryFilledBy(packageDir, join("node_modules", "has-bin"));
const globalBinDir = join(packageDir, "global-bin-dir");

const { out, err, exitCode } = await run(["link"], join(packageDir, "has-bin"), {
...env,
BUN_INSTALL: join(packageDir, "global-install-dir"),
BUN_INSTALL_GLOBAL_DIR: globalDir,
BUN_INSTALL_BIN: globalBinDir,
});
const shortened = await shorten();

expect(err).toContain("error: failed to link bin due to error ENAMETOOLONG");
expect(out).not.toContain("Success!");
// The package itself was registered. Only its bin is missing.
expect(await exists(join(shortened, "node_modules", "has-bin", "package.json"))).toBeTrue();
expect(await exists(join(globalBinDir, "has-bin"))).toBeFalse();
expect(exitCode).toBe(1);
});
});

test("it will skip (without errors) if a folder from `directories.bin` does not exist", async () => {
await Promise.all([
write(
Expand Down
Loading