diff --git a/src/install/bin.zig b/src/install/bin.zig index 3a072fc24c49..92e706e566f1 100644 --- a/src/install/bin.zig +++ b/src/install/bin.zig @@ -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 `\.bunx` past *two* `\` separators before + // appending the stored `bin_path`. That lands the anchor at the + // parent of ``. See the walk-back loop in + // `src/install/windows-shim/bun_shim_impl.zig`. + // + // We compute `rel_target` relative to `` (one dirname, + // always produces an absolute string) and then shape it into + // something that composes correctly when appended at the + // parent-of-`` anchor. Three possible shapes: + // + // - Starts with `..\\` — common case, target lives somewhere + // under ``'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 `` 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 + // `\\` 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: { @@ -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(); @@ -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| { diff --git a/src/install/windows-shim/BinLinkingShim.zig b/src/install/windows-shim/BinLinkingShim.zig index ab8dd2bfab1b..3f063f11c72c 100644 --- a/src/install/windows-shim/BinLinkingShim.zig +++ b/src/install/windows-shim/BinLinkingShim.zig @@ -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] @@ -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 @@ -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; } }; @@ -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| { diff --git a/src/install/windows-shim/bun_shim_impl.zig b/src/install/windows-shim/bun_shim_impl.zig index f4411b02841b..ce529a890e59 100644 --- a/src/install/windows-shim/bun_shim_impl.zig +++ b/src/install/windows-shim/bun_shim_impl.zig @@ -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| { @@ -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); + } + + 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 @@ -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 diff --git a/test/regression/issue/30129.test.ts b/test/regression/issue/30129.test.ts new file mode 100644 index 000000000000..cb2031902790 --- /dev/null +++ b/test/regression/issue/30129.test.ts @@ -0,0 +1,98 @@ +// https://github.com/oven-sh/bun/issues/30129 +// +// On Windows, `bun install -g ` 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 `/.bin/foo.js`. The relative + // path from `/.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:"); + + // 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 `\.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 { + const link = join(String(dir), "node_modules", ".bin", "weird"); + expect(readlinkSync(link)).toBe("foo.js"); + } + + expect(exitCode).toBe(0); +});