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
73 changes: 66 additions & 7 deletions src/install/bin.zig
Original file line number Diff line number Diff line change
Expand Up @@ -734,10 +734,62 @@ pub const Bin = extern struct {
};
defer bunx_file.close();

const rel_target = path.relativeBufZ(this.rel_buf, path.dirname(abs_dest, .auto), abs_target);
bun.assertWithLocation(strings.hasPrefixComptime(rel_target, "..\\"), @src());
// At runtime `bun_shim_impl.exe` walks back from its own image
// path `<bin_dir>\<name>.bunx` past *two* `\` separators before
// appending the stored `bin_path`. That lands the anchor at the
// parent of `<bin_dir>`. See the walk-back loop in
// `src/install/windows-shim/bun_shim_impl.zig`.
//
// We compute `rel_target` relative to `<bin_dir>` (one dirname,
// always produces an absolute string) and then shape it into
// something that composes correctly when appended at the
// parent-of-`<bin_dir>` anchor. Three possible shapes:
//
// - Starts with `..\\` — common case, target lives somewhere
// under `<bin_dir>`'s parent (e.g. `..\\some-pkg\\cli.js`).
// Strip the single `..\\`: the launcher's walk-back already
// took that step.
// - Absolute — cross-volume (e.g. `BUN_INSTALL_BIN` on `E:` +
// global store on `C:`). There's no `..`-relative form
// between volumes, so we flip to absolute-target mode and
// set `is_absolute_target`. The launcher skips the walk-back
// reconstruction and uses the stored path verbatim.
// - Anything else — `.bin`-local shape (e.g. a package whose
// `bin` field resolves to `../.bin/foo.js`). Prepend the
// basename of `<bin_dir>` so the composed runtime path
// reaches the target inside the bin directory.
//
// We deliberately do *not* anchor via `dirname(dirname(abs_dest))`:
// for a one-level `BUN_INSTALL_BIN` like `E:\bin`, the outer
// `dirname` returns `"E:"` without the trailing `\`, which
// `path.relative` mishandles as a drive-relative anchor and
// silently resolves against the current working directory.
const abs_dest_dir = path.dirname(abs_dest, .auto);
const rel_target_from_bin = path.relativeBufZ(this.rel_buf, abs_dest_dir, abs_target);

const rel_target_w = strings.toWPathNormalized(&target_buf, rel_target["..\\".len..]);
var bin_path_utf8_buf: bun.PathBuffer = undefined;
const bin_path_utf8: []const u8, const is_absolute_target: bool = b: {
if (strings.hasPrefixComptime(rel_target_from_bin, "..\\")) {
break :b .{ rel_target_from_bin["..\\".len..], false };
}
if (std.fs.path.isAbsoluteWindows(rel_target_from_bin)) {
break :b .{ rel_target_from_bin, true };
}
// Zero-`..` shape: target lives inside `abs_dest_dir`. Build
// `<basename>\\<rel>` so the launcher's post-walk-back
// concatenation lands back inside the bin directory.
const bin_dir_name = path.basename(abs_dest_dir);
const composed_len = bin_dir_name.len + 1 + rel_target_from_bin.len;
if (composed_len >= bin_path_utf8_buf.len) {
this.err = error.InvalidBinContent;
return;
}
@memcpy(bin_path_utf8_buf[0..bin_dir_name.len], bin_dir_name);
bin_path_utf8_buf[bin_dir_name.len] = '\\';
@memcpy(bin_path_utf8_buf[bin_dir_name.len + 1 ..][0..rel_target_from_bin.len], rel_target_from_bin);
break :b .{ bin_path_utf8_buf[0..composed_len], false };
};
const bin_path_w = strings.toWPathNormalized(&target_buf, bin_path_utf8);

const shebang = shebang: {
const first_content_chunk = contents: {
Expand All @@ -749,18 +801,19 @@ pub const Bin = extern struct {
};

if (first_content_chunk) |chunk| {
break :shebang WinBinLinkingShim.Shebang.parse(chunk, rel_target_w) catch {
break :shebang WinBinLinkingShim.Shebang.parse(chunk, bin_path_w) catch {
this.err = error.InvalidBinCount;
return;
};
} else {
break :shebang WinBinLinkingShim.Shebang.parseFromBinPath(rel_target_w);
break :shebang WinBinLinkingShim.Shebang.parseFromBinPath(bin_path_w);
}
};

const shim = WinBinLinkingShim{
.bin_path = rel_target_w,
.bin_path = bin_path_w,
.shebang = shebang,
.is_absolute_target = is_absolute_target,
};

const len = shim.encodedLength();
Expand Down Expand Up @@ -804,7 +857,13 @@ pub const Bin = extern struct {
const abs_dest_dir = path.dirname(abs_dest, .auto);
const rel_target = path.relativeBufZ(this.rel_buf, abs_dest_dir, abs_target);

bun.assertWithLocation(strings.hasPrefixComptime(rel_target, ".."), @src());
// `rel_target` is almost always `../package/bin.js`-shaped on
// POSIX (this function is POSIX-only — the Windows path goes
// through `createWindowsShim`). It can also be a bare filename
// when the target lives inside `abs_dest_dir` itself, e.g. a
// package whose `bin` field resolves to `../.bin/foo.js`.
// `symlink(2)` accepts any relative string, so don't assert the
// leading `..`.

switch (bun.sys.symlinkRunningExecutable(rel_target, abs_dest)) {
.err => |err| {
Expand Down
38 changes: 29 additions & 9 deletions src/install/windows-shim/BinLinkingShim.zig
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
//!
//! The format is as follows:
//!
//! [WSTR:bin_path][u16'"'][u16:0](shebang?)[flags:u16]
//! [WSTR:bin_path][u16'"'][u16:0](shebang?)[flags:u32]
//!
//! if shebang:
//! [WSTR:program][u16:0][WSTR:args][u32:bin_path_byte_len][u32:arg_byte_len]
Expand All @@ -15,16 +15,23 @@ fn eqlComptime(a: []const u8, comptime b: []const u8) bool {
return std.mem.eql(u8, a, b);
}

/// Relative to node_modules. Do not include slash
/// Relative to node_modules. Do not include slash.
/// When `is_absolute_target` is true, this is instead a full absolute
/// Windows path (drive-letter or UNC) to the target executable — used for
/// cross-volume installs where a `..`-relative path between the shim's
/// directory and the target is impossible.
bin_path: []const u16,
/// Information found within the target file's shebang
shebang: ?Shebang,
/// When true, `bin_path` is an absolute path. The launcher will skip the
/// `..`-relative reconstruction trick and use `bin_path` as-is.
is_absolute_target: bool = false,

/// Random numbers are chosen for validation purposes
/// These arbitrary numbers will probably not show up in the other fields.
/// This will reveal off-by-one mistakes.
pub const VersionFlag = enum(u13) {
pub const current = .v5;
pub const VersionFlag = enum(u28) {
pub const current = .v6;

v1 = 5474,
/// Fix bug where paths were not joined correctly
Expand All @@ -37,34 +44,46 @@ pub const VersionFlag = enum(u13) {
v4 = 5477,
/// Fixed bugs where passing arguments did not always work.
v5 = 5478,
/// Added `is_absolute_target` flag. When set, `bin_path` is stored as an
/// absolute Windows path (with drive letter or UNC prefix) instead of the
/// usual `..`-stripped relative path. Needed when `BUN_INSTALL_BIN` and
/// the package store live on different drives, where a relative target is
/// impossible.
v6 = 5479,
_,
};

pub const Flags = packed struct(u16) {
pub const Flags = packed struct(u32) {
// this is set if the shebang content is "node" or "bun"
is_node_or_bun: bool,
// this is for validation that the shim is not corrupt and to detect offset memory reads
is_node: bool,
// indicates if a shebang is present
has_shebang: bool,
// indicates `bin_path` is an absolute Windows path. When set, the launcher
// must not use the `..`-relative reconstruction trick and instead treat the
// stored `bin_path` as the full absolute target path.
is_absolute_target: bool,

version_tag: VersionFlag = VersionFlag.current,

pub fn isValid(flags: Flags) bool {
const mask: u16 = @bitCast(Flags{
const mask: u32 = @bitCast(Flags{
.is_node_or_bun = false,
.is_node = false,
.has_shebang = false,
.version_tag = @enumFromInt(std.math.maxInt(u13)),
.is_absolute_target = false,
.version_tag = @enumFromInt(std.math.maxInt(u28)),
});

const compare_to: u16 = @bitCast(Flags{
const compare_to: u32 = @bitCast(Flags{
.is_node_or_bun = false,
.is_node = false,
.has_shebang = false,
.is_absolute_target = false,
});

return (@as(u16, @bitCast(flags)) & comptime mask) == comptime compare_to;
return (@as(u32, @bitCast(flags)) & comptime mask) == comptime compare_to;
}
};

Expand Down Expand Up @@ -237,6 +256,7 @@ pub fn encodeInto(options: @This(), buf: []u8) !void {
.has_shebang = options.shebang != null,
.is_node_or_bun = is_node_or_bun,
.is_node = false,
.is_absolute_target = options.is_absolute_target,
};

if (options.shebang) |s| {
Expand Down
73 changes: 68 additions & 5 deletions src/install/windows-shim/bun_shim_impl.zig
Original file line number Diff line number Diff line change
Expand Up @@ -545,12 +545,32 @@ fn launcher(comptime mode: LauncherMode, bun_ctx: anytype) mode.RetType() {

if (dbg) debug("BufferAfterRead: '{f}'", .{fmt16(buf1_u16[0 .. ((@intFromPtr(read_ptr) - @intFromPtr(buf1_u8)) + read_len) / 2])});

// Sanity check: the read must include at least the Flags struct at the end.
// Otherwise the pointer arithmetic below would underflow.
if (read_len < @sizeOf(Flags)) {
return mode.fail(.InvalidShimDataSize);
}

// `metadata_start_ptr` points at the first byte NtReadFile wrote into buf1.
// For the relative-target case (the default) the encoded bin_path at offset
// 0 of the metadata file naturally completes the absolute target path
// because `read_ptr` sits one byte past the `\` that precedes the basename
// in the shim's own image path.
//
// For the cross-volume (absolute-target) case, that trick is impossible —
// the target lives on a different drive. So the encoder stores the
// complete absolute path as `bin_path`, and below we relocate the metadata
// so it starts at `buf1_u8[2 * nt_object_prefix.len]` (i.e. right after the
// `\??\` prefix). That makes the rest of the decode logic see an absolute
// path beginning at the same offset the relative case would have produced.
const metadata_start_ptr: [*]u8 = @ptrCast(read_ptr);

read_ptr = @ptrFromInt(@intFromPtr(read_ptr) + read_len - @sizeOf(Flags));
const flags: Flags = @as(*align(1) Flags, @ptrCast(read_ptr)).*;

if (dbg) {
const flags_u16: u16 = @as(*align(1) u16, @ptrCast(read_ptr)).*;
debug("FlagsInt: {d}", .{flags_u16});
const flags_u32: u32 = @as(*align(1) u32, @ptrCast(read_ptr)).*;
debug("FlagsInt: {d}", .{flags_u32});

debug("Flags:", .{});
inline for (comptime std.meta.fieldNames(Flags)) |name| {
Expand All @@ -567,6 +587,43 @@ fn launcher(comptime mode: LauncherMode, bun_ctx: anytype) mode.RetType() {
return mode.fail(.InvalidShimValidation);
}

if (flags.is_absolute_target) {
// Relocate the read metadata from its current position to
// `buf1_u8[2 * nt_object_prefix.len]`. The new start offset is
// strictly smaller than the original (the walked-back `\` always sits
// past the NT prefix), so a forward-direction copy is safe even when
// the source and destination ranges overlap.
const new_offset = 2 * nt_object_prefix.len;
const current_offset = @intFromPtr(metadata_start_ptr) - @intFromPtr(buf1_u8);
assert(current_offset >= new_offset);

// Destination must fit inside the valid byte range reachable from
// `buf1_u8[0]`. The `[*]u8` is anchored at the physical midpoint of
// `buf1`, so the forward-reachable byte length equals `buf1.len`
// (each `u16` slot in the back half of `buf1` is exactly one byte
// of the front of `buf1_u8`). Metadata files are small in practice,
// but defensively fail the shim rather than corrupt adjacent memory.
const buf1_u8_len: usize = buf1.len; // bytes, see buf1_u8 definition
if (read_len > buf1_u8_len - new_offset) {
return mode.fail(.InvalidShimDataSize);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

if (current_offset != new_offset) {
std.mem.copyForwards(
u8,
buf1_u8[new_offset..][0..read_len],
buf1_u8[current_offset..][0..read_len],
);
}
// Re-anchor `read_ptr` to the new Flags location.
read_ptr = @ptrCast(@alignCast(&buf1_u8[new_offset + read_len - @sizeOf(Flags)]));
// Sanity check that Flags still decodes identically after the move.
if (dbg) {
const moved_flags: Flags = @as(*align(1) Flags, @ptrCast(read_ptr)).*;
assert(@as(u32, @bitCast(moved_flags)) == @as(u32, @bitCast(flags)));
}
}

var spawn_command_line: [*:0]u16 = switch (flags.has_shebang) {
false => spawn_command_line: {
// no shebang, which means the command line is simply going to be the joined file exe
Expand Down Expand Up @@ -626,9 +683,15 @@ fn launcher(comptime mode: LauncherMode, bun_ctx: anytype) mode.RetType() {
debug("args_len_bytes: {}", .{shebang_metadata.args_len_bytes});
}

// magic number related to how BinLinkingShim.zig writes the metadata
// i'm sorry, i don't have a good explanation for why this number is this number. it just is.
const validation_length_offset = 14;
// magic number related to how BinLinkingShim.zig writes the metadata.
// bytes that appear in the metadata file AROUND the bin_path and
// shebang arg content:
// `"` + `\0` after bin_path = 4 bytes
// ` ` (space) after shebang launcher = 2 bytes (counted in shebang_arg_len_u8)
// u32 bin_path_len_bytes + u32 args_len_bytes = 8 bytes
// @sizeOf(Flags) = 4 bytes (was 2 before v6)
// total extra = 4 + 8 + @sizeOf(Flags) = 16 bytes
const validation_length_offset = 4 + 8 + @sizeOf(Flags);

// very careful here to not overflow u32, so that we properly error if you hijack the file
if (shebang_arg_len_u8 == 0 or
Expand Down
98 changes: 98 additions & 0 deletions test/regression/issue/30129.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
// https://github.com/oven-sh/bun/issues/30129
//
// On Windows, `bun install -g <pkg>` panicked at `install/bin.zig:738` with
// "Internal assertion failure" when `BUN_INSTALL_BIN` lived on a different
// physical drive from the global package store. `createWindowsShim` assumed
// the stored `bin_path` could always be expressed as a `..\`-relative walk
// from the shim's `.bin` directory. Windows cannot produce such a walk
// between two volumes, so `path.relative` returned the absolute target
// instead and the assertion fired — after the `.bunx` file had already been
// opened with O_TRUNC, leaving an empty 0-byte `.bunx` behind.
//
// The same assertion family also fires on POSIX (`createSymlink`) and on
// Windows (`createWindowsShim`) for the *zero-`..`* shape, where a package's
// `bin` field resolves to a file *inside* the sibling `.bin` directory. That
// is the shape this test exercises on both Linux and Windows CI. Without the
// fix, `bun install` panics during bin linking and exits non-zero before any
// node_modules artefacts land on disk.
import { expect, test } from "bun:test";
import { bunEnv, bunExe, isWindows, tempDir } from "harness";
import { existsSync, readlinkSync, statSync } from "node:fs";
import { join } from "node:path";

test("bun install does not panic when bin target's relative path has no `..` prefix", { timeout: 60_000 }, async () => {
using dir = tempDir("issue-30129", {
"package.json": JSON.stringify({
name: "root",
version: "1.0.0",
dependencies: { weird: "file:./pkg" },
}),
"pkg/package.json": JSON.stringify({
name: "weird",
version: "1.0.0",
bin: "../.bin/foo.js",
}),
"pkg/index.js": "module.exports = 1;\n",
// Pre-create the .bin directory with the file `bin` points at so that
// `abs_target` resolves to `<node_modules>/.bin/foo.js`. The relative
// path from `<node_modules>/.bin` to that target is just `foo.js`,
// which is the shape that trips the overly strict assertion on both
// POSIX (`createSymlink`) and Windows (`createWindowsShim`).
"node_modules/.bin/foo.js": "#!/usr/bin/env node\nprocess.exit(0);\n",
});

await using proc = Bun.spawn({
cmd: [bunExe(), "install"],
cwd: String(dir),
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

const [, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

// Bun prints "error:" for any user-facing install failure. The fix must
// let the install complete cleanly — no error markers in stderr.
expect(stderr).not.toContain("error:");

Comment thread
coderabbitai[bot] marked this conversation as resolved.
// The package itself must be extracted.
const pkgJson = join(String(dir), "node_modules", "weird", "package.json");
expect(statSync(pkgJson).isFile()).toBe(true);

// The lockfile must have been written to disk. Without the fix the install
// panics before saving the lockfile, so the file simply doesn't exist.
const lockfile = existsSync(join(String(dir), "bun.lock"))
? join(String(dir), "bun.lock")
: join(String(dir), "bun.lockb");
expect(existsSync(lockfile)).toBe(true);

// Bin-link artefacts differ by platform:
// - POSIX: a symlink at `.bin/weird` pointing at the target relative to
// `.bin`. For our fixture that path is just `foo.js`.
// - Windows: a `.exe` + `.bunx` shim pair. The `.bunx` file encodes the
// bin path as a UTF-16LE prefix terminated by a `"\0` sequence; for
// this fixture it must resolve to `<node_modules>\.bin\foo.js` at
// runtime, which in the parent-of-`.bin`-anchored form is
// `.bin\foo.js`.
if (isWindows) {
const exe = join(String(dir), "node_modules", ".bin", "weird.exe");
const bunx = join(String(dir), "node_modules", ".bin", "weird.bunx");
expect(statSync(exe).isFile()).toBe(true);
expect(statSync(bunx).isFile()).toBe(true);

// `.bunx` must not be a truncated zero-byte file left behind by a
// panic between `O_TRUNC` and the metadata write.
expect(statSync(bunx).size).toBeGreaterThan(0);

const bunxBytes = await Bun.file(bunx).bytes();
const decoded = new TextDecoder("utf-16le").decode(bunxBytes);
const terminator = decoded.indexOf('"\0');
expect(terminator).toBeGreaterThan(0);
expect(decoded.slice(0, terminator)).toBe(".bin\\foo.js");
} else {
Comment thread
robobun marked this conversation as resolved.
const link = join(String(dir), "node_modules", ".bin", "weird");
expect(readlinkSync(link)).toBe("foo.js");
}

expect(exitCode).toBe(0);
});
Loading