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
36 changes: 19 additions & 17 deletions src/install/PackageManager/PackageManagerEnqueue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1903,18 +1903,25 @@ fn enqueue_local_tarball(
// other dependencies (e.g. `appendPackage` / `StringBuilder.allocate`
// in `Package.fromNPM`).
let mut abs_buf = PathBuffer::uninit();
let (tarball_path, normalize): (&[u8], bool) =
match local_tarball_base_dir(&this.lockfile, dependency_id, path) {
None => (path, true),
Some(base_dir) => (
Path::resolve_path::join_abs_string_buf::<Path::platform::Auto>(
FileSystem::instance().top_level_dir(),
&mut abs_buf,
&[base_dir, path],
let tarball_path = match local_tarball_base_dir(&this.lockfile, dependency_id, path) {
None => Task::TarballPath::Url,
Some(base_dir) => {
match Path::resolve_path::join_abs_string_buf_checked::<Path::platform::Auto>(
FileSystem::instance().top_level_dir(),
&mut abs_buf,
&[base_dir, path],
) {
None => Task::TarballPath::TooLong,
Some(joined) => Task::TarballPath::Absolute(
StringOrTinyString::init_append_if_needed(
joined,
&mut crate::network_task::filename_store_appender(),
)
.expect("unreachable"),
),
false,
),
};
}
}
};

// Build the `Task` value *before* claiming a hive slot — the `.expect()`s
// below can unwind, and `Task` carries drop glue. See `enqueue_git_clone`.
Expand Down Expand Up @@ -1949,12 +1956,7 @@ fn enqueue_local_tarball(
skip_verify: false,
in_trusted_dependencies: false,
},
tarball_path: StringOrTinyString::init_append_if_needed(
tarball_path,
&mut crate::network_task::filename_store_appender(),
)
.expect("unreachable"),
normalize,
tarball_path,
}),
},
id: task_id,
Expand Down
59 changes: 26 additions & 33 deletions src/install/PackageManagerTask.rs
Original file line number Diff line number Diff line change
Expand Up @@ -535,23 +535,12 @@ impl<'a> Task<'a> {
this.status = Status::Success;
}
Tag::LocalTarball => {
// `tarball_path` and `normalize` are computed on the main thread when the
// task is enqueued. This callback runs on a ThreadPool worker and must not
// read `manager.lockfile.packages` / `manager.lockfile.buffers.string_bytes`:
// the main thread may reallocate those buffers concurrently while processing
// other dependencies.
// Worker thread: the request holds everything needed from the lockfile.

// SAFETY: tag == LocalTarball discriminates the union
let req = unsafe { &mut *this.request.local_tarball };
let tarball_path = req.tarball_path.slice();
let normalize = req.normalize;

let result = match read_and_extract(
&req.tarball,
tarball_path,
normalize,
&mut this.log,
) {
let req = unsafe { &*this.request.local_tarball };

let result = match read_and_extract(req, &mut this.log) {
Ok(v) => v,
Err(err) => {
this.err = Some(err);
Expand Down Expand Up @@ -602,29 +591,26 @@ impl<'a> Task<'a> {
}
}

fn read_and_extract(
tarball: &ExtractTarball,
tarball_path: &[u8],
normalize: bool,
log: &mut Log,
) -> crate::Result<ExtractData> {
let bytes = if normalize {
fn read_and_extract(req: &LocalTarballRequest, log: &mut Log) -> crate::Result<ExtractData> {
let bytes = match &req.tarball_path {
// Resolves
// a user-provided relative path against `bun.fs.FileSystem.instance.top_level_dir`
// (the absolute project root cached at startup — NOT the live process cwd).
// `bun_sys::File::read_from_user_input` takes that base
// explicitly (T1 `bun_sys` cannot depend on T5 `bun_resolver::fs`), so thread it
// through here from the install crate's `FileSystem` shim.
File::read_from_user_input(
TarballPath::Url => File::read_from_user_input(
Fd::cwd(),
crate::bun_fs::FileSystem::instance().top_level_dir(),
tarball_path,
)?
} else {
File::read_from(Fd::cwd(), tarball_path)?
req.tarball.url.slice(),
)?,
TarballPath::Absolute(path) => File::read_from(Fd::cwd(), path.slice())?,
TarballPath::TooLong => {
return Err(crate::Error::Sys(bun_errno::SystemErrno::ENAMETOOLONG));
}
};
// `defer allocator.free(bytes)` → Vec<u8> drops at scope exit
tarball.run(log, &bytes)
req.tarball.run(log, &bytes)
}

#[repr(u8)]
Expand Down Expand Up @@ -699,9 +685,16 @@ pub struct GitCheckoutRequest {

pub struct LocalTarballRequest {
pub(crate) tarball: ExtractTarball,
/// Resolved by `enqueue_local_tarball` on the main thread; the worker must not read the lockfile.
pub(crate) tarball_path: StringOrTinyString,
/// When true, `tarball_path` is a user-provided path resolved relative to
/// cwd. When false, it is already an absolute path.
pub(crate) normalize: bool,
/// Built on the main thread by `enqueue_local_tarball`; the worker must not read the lockfile.
pub(crate) tarball_path: TarballPath,
}

/// Where a `LocalTarballRequest` reads its tarball from.
pub(crate) enum TarballPath {
/// `tarball.url` as written in the dependency, resolved against the project root.
Url,
/// A tarball declared by a workspace or `file:` folder package, already joined with that package's directory.
Absolute(StringOrTinyString),
/// The joined path does not fit a `PathBuffer`; the task fails with `ENAMETOOLONG`.
TooLong,
}
23 changes: 18 additions & 5 deletions src/install/lockfile/Package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1968,14 +1968,27 @@ impl Package<u64> {
} else {
'brk: {
let mut buf2 = PathBuffer::uninit();
let Some(joined) = resolve_path::join_abs_string_buf_checked::<
path::platform::Auto,
>(
FileSystem::instance().top_level_dir(),
&mut buf2.0,
&[source.path.name().dir, workspace],
) else {
log.add_error_fmt(
source,
value_loc_of(source, key_loc),
format_args!(
"Dependency \"{}\" has an unsafe workspace path",
bstr::BStr::new(external_alias.slice(buf)),
),
);
return Err(crate::Error::InstallFailed);
};
let rel =
resolve_path::relative_platform::<path::platform::Auto, false>(
FileSystem::instance().top_level_dir(),
resolve_path::join_abs_string_buf::<path::platform::Auto>(
FileSystem::instance().top_level_dir(),
&mut buf2.0,
&[source.path.name().dir, workspace],
),
joined,
);
#[cfg(windows)]
{
Expand Down
13 changes: 7 additions & 6 deletions src/sys/file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,7 @@ impl File {
/// Normalize a
/// user-provided relative path against the resolver's cached
/// `top_level_dir` (NOT a fresh `getcwd()`), then `readFrom`.
/// Fails with `ENAMETOOLONG` when the normalized path does not fit a `PathBuffer`.
///
/// The cached `top_level_dir` lives in `bun_resolver::fs` (T5), which
/// `bun_sys` (T1) must not depend on, so callers pass it explicitly.
Expand All @@ -371,12 +372,12 @@ impl File {
) -> Maybe<Vec<u8>> {
let dir = dir.as_fd();
let mut buf = bun_paths::PathBuffer::default();
let normalized = bun_paths::resolve_path::join_abs_string_buf_z::<bun_paths::platform::Loose>(
top_level_dir,
&mut buf.0,
&[input_path],
);
Self::read_from(dir, normalized.as_bytes())
let Some(normalized) = bun_paths::resolve_path::join_abs_string_buf_checked::<
bun_paths::platform::Loose,
>(top_level_dir, &mut buf.0, &[input_path]) else {
return Err(Error::from_code(E::ENAMETOOLONG, Tag::open).with_path(input_path));
};
Self::read_from(dir, normalized)
}
/// `bun.sys.File.writeFile` — open + write + close.
pub fn write_file(dir: impl AsFd, path: &ZStr, data: &[u8]) -> Maybe<()> {
Expand Down
4 changes: 2 additions & 2 deletions src/sys/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6149,7 +6149,7 @@ pub fn openat_a(dir: impl AsFd, path: &[u8], flags: i32, perm: Mode) -> Maybe<Fd
let dir = dir.as_fd();
let mut buf = bun_paths::PathBuffer::default();
if path.len() >= buf.0.len() {
return Err(Error::from_code_int(libc::ENAMETOOLONG, Tag::open).with_path(path));
return Err(Error::from_code(E::ENAMETOOLONG, Tag::open).with_path(path));
}
buf.0[..path.len()].copy_from_slice(path);
buf.0[path.len()] = 0;
Expand Down Expand Up @@ -9209,7 +9209,7 @@ pub fn write_file_with_path_buffer(
PathOrFileDescriptor::Fd(fd) => fd,
PathOrFileDescriptor::Path(bytes) => {
if bytes.len() >= path_buf.0.len() {
return Err(Error::from_code_int(libc::ENAMETOOLONG, Tag::open).with_path(bytes));
return Err(Error::from_code(E::ENAMETOOLONG, Tag::open).with_path(bytes));
}
path_buf.0[..bytes.len()].copy_from_slice(bytes);
path_buf.0[bytes.len()] = 0;
Expand Down
138 changes: 138 additions & 0 deletions test/cli/install/bun-workspaces.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import {
assertManifestsPopulated,
bunEnv as baseEnv,
bunExe,
isLinux,
isWindows,
readdirSorted,
runBunInstall,
toMatchNodeModulesAt,
Expand Down Expand Up @@ -653,6 +655,39 @@ test.concurrent("cwd in workspace script is not the symlink path on windows", as
expect(await file(join(packageDir, "node_modules", "pkg1", "cwd")).text()).toBe(join(packageDir, "pkg1"));
});

// Local dependency paths are joined into a PathBuffer of MAX_PATH_BYTES: 4096
// bytes on Linux, 1024 on the other POSIX platforms, 32767 * 3 + 1 on Windows.
const PATH_BUFFER_BYTES = isWindows ? 32767 * 3 + 1 : isLinux ? 4096 : 1024;
// Longer than the buffer on every platform.
const LONG_SPEC_BYTES = 100_000;
const WORKSPACE_DIR = "pkgs/pkg1";

// Writes a root package with one workspace at WORKSPACE_DIR, declaring `spec`
// as a dependency of either the root package or the workspace package.
async function writeDependency(packageDir: string, owner: "root" | "workspace", name: string, spec: string) {
await Promise.all([
write(
join(packageDir, "package.json"),
JSON.stringify({
name: "foo",
workspaces: ["pkgs/*"],
dependencies: owner === "root" ? { [name]: spec } : {},
}),
),
write(
join(packageDir, WORKSPACE_DIR, "package.json"),
JSON.stringify({
name: "pkg1",
dependencies: owner === "workspace" ? { [name]: spec } : {},
}),
),
]);
}

function runFailingInstall(env: Record<string, string>, packageDir: string) {
return runBunInstall(env, packageDir, { allowErrors: true, expectedExitCode: 1, savesLockfile: false });
}

describe("relative tarballs", async () => {
test.concurrent("from package.json", async () => {
using ctx = await setupTest();
Expand Down Expand Up @@ -856,8 +891,111 @@ describe("relative tarballs", async () => {
}
}
});

test.concurrent.each(["root", "workspace"] as const)(
"%s dependency on a tarball path longer than the path buffer fails with ENAMETOOLONG",
async owner => {
using ctx = await setupTest();
const { packageDir, env } = ctx;
const spec = "file:./" + Buffer.alloc(LONG_SPEC_BYTES, "a").toString() + ".tgz";
await writeDependency(packageDir, owner, "too-long", spec);

const { err } = await runFailingInstall(env, packageDir);

expect(err).toContain("error: ENAMETOOLONG extracting tarball from too-long");
expect(err).toContain("failed to resolve");
},
);

// A relative tarball path of exactly `bytes` bytes made of one letter
// directories, so that a path which fits the buffer is looked up by the OS
// (ENOENT) instead of tripping its own limit on the length of a single name.
function tarballPathOfLength(bytes: number) {
const tail = bytes % 2 === 0 ? "dd.tgz" : "d.tgz";
return Buffer.alloc(bytes - tail.length, "d/").toString() + tail;
}

// packageDir is a real path and the cwd of the install, so the joined path
// is `${packageDir}/${rel}` for the root package and
// `${packageDir}/${WORKSPACE_DIR}/${rel}` for the workspace. One byte below
// the buffer size the path still reaches the OS; at the buffer size there is
// no room for the NUL terminator; above it the join itself overflows. The
// workspace rows also tell the workspace join apart from resolving the same
// path against the root, which fits the buffer and reports ENOENT instead.
test.concurrent.each([
["root", "one byte below", -1],
["root", "exactly", 0],
["root", "one byte above", 1],
["workspace", "one byte below", -1],
["workspace", "exactly", 0],
["workspace", "one byte above", 1],
] as const)("%s dependency whose joined tarball path is %s the path buffer size", async (owner, _, offset) => {
using ctx = await setupTest();
const { packageDir, env } = ctx;
const joinedPrefixBytes =
Buffer.byteLength(packageDir) + 1 + (owner === "workspace" ? Buffer.byteLength(WORKSPACE_DIR) + 1 : 0);
const spec = "file:" + tarballPathOfLength(PATH_BUFFER_BYTES + offset - joinedPrefixBytes);
await writeDependency(packageDir, owner, "edge", spec);

const { err } = await runFailingInstall(env, packageDir);

// Windows never looks the fitting path up: it is longer than the 32767
// UTF-16 units a Windows path may have.
const expected = offset < 0 && !isWindows ? "ENOENT" : "ENAMETOOLONG";
expect(err).toContain(`error: ${expected} extracting tarball from edge`);
expect(err).toContain("failed to resolve");
});

test.concurrent.each(["root", "workspace"] as const)(
"%s dependency on a tarball path that only fits the path buffer once normalized resolves",
async owner => {
using ctx = await setupTest();
const { packageDir, env } = ctx;
// "x/../" repeated: longer than the path buffer as written, but it
// normalizes to the tarball next to the root package.json.
const detour = Buffer.alloc(LONG_SPEC_BYTES, "x/../").toString();
const spec = "./" + detour + (owner === "root" ? "" : "../../") + "qux-0.0.2.tgz";
await Promise.all([
writeDependency(packageDir, owner, "qux", spec),
cp(join(import.meta.dir, "qux-0.0.2.tgz"), join(packageDir, "qux-0.0.2.tgz")),
]);

// Reading the tarball happens while resolving. Linking it into
// node_modules is skipped: the linker formats the resolution into a
// 512 byte buffer and cannot install a spec this long yet.
await using proc = spawn({
cmd: [bunExe(), "install", "--lockfile-only"],
cwd: packageDir,
stdout: "pipe",
stderr: "pipe",
env,
});
const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(err).not.toContain("error:");
expect(err).toContain("Saved lockfile");
expect(out).toContain("Saved bun.lock (3 packages)");
expect(exitCode).toBe(0);
const qux = parseLockfile(packageDir).packages.find((pkg: { name: string }) => pkg.name === "qux");
expect(qux.resolution).toMatchObject({ tag: "local_tarball", value: spec });
},
);
});

test.concurrent.each(["root", "workspace"] as const)(
"%s dependency on a workspace: path longer than the path buffer fails to install",
async owner => {
using ctx = await setupTest();
const { packageDir, env } = ctx;
const spec = "workspace:./" + Buffer.alloc(LONG_SPEC_BYTES, "a").toString();
await writeDependency(packageDir, owner, "too-long", spec);

const { err } = await runFailingInstall(env, packageDir);

expect(err).toContain('Dependency "too-long" has an unsafe workspace path');
},
);

test.concurrent("$npm_package_config_ works in root", async () => {
using ctx = await setupTest();
const { packageDir, env } = ctx;
Expand Down
Loading