From d031d00d9a89a9d30fe3d0301355b05f54d6f3f6 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:32:53 +0000 Subject: [PATCH 1/4] pack: fill every placeholder in multi-argument error messages Three pack errors passed a single format_args! to a template with two or more placeholders. Output::err and err_generic treat a fmt::Arguments as one positional, so every value landed in the first placeholder and the rest rendered empty: filename "a.tgz out" and destination "" archive destination name too long: ".../pkg-1.0.0.tgz/" EISDIR: failed to read .npmignore /pkg/sub/.npmignore at: "" Pass tuples instead, like the other multi-argument messages in this file. The bundled dependency loop's stat error printed the file descriptor number instead of the path; print the path like the loop above it. substitute_template now fails a debug assertion when a template has more placeholders than the arguments it was given, so a call site like these crashes in debug builds instead of printing a plausible-looking message. These three were the only such sites in src/. --- src/bun_core/output.rs | 16 ++++++++-- src/runtime/cli/pack_command.rs | 13 +++----- test/cli/install/bun-pack.test.ts | 51 +++++++++++++++++++++++++++++-- 3 files changed, 67 insertions(+), 13 deletions(-) diff --git a/src/bun_core/output.rs b/src/bun_core/output.rs index 3274e5c074cb..ee4f6d00f681 100644 --- a/src/bun_core/output.rs +++ b/src/bun_core/output.rs @@ -1797,7 +1797,7 @@ impl fmt::Display for PrettyBuf { /// Positional-argument bundle for runtime template substitution. pub trait FmtTuple { /// Write the `idx`-th positional into `f`. Returns `false` if `idx` is out - /// of range (caller emits the literal `{}` then). + /// of range. fn write_nth(&self, idx: usize, f: &mut dyn fmt::Write) -> Result; fn len(&self) -> usize; } @@ -1877,7 +1877,10 @@ impl_fmt_tuple!(0 A, 1 B, 2 C, 3 D, 4 E, 5 F, 6 G, 7 H); /// Substitute `{}` / `{s}` / `{d}` / `{any}` / `{f}` placeholders in `template` /// with successive entries from `args`. `{{` / `}}` are emitted as literal -/// braces. Unrecognised specs are passed through verbatim. +/// braces. Every other `{...}` consumes an entry, whatever the spec says; a +/// placeholder with no entry left renders as nothing (and fails a debug +/// assertion, since it means the call site packed several values into one +/// `format_args!` instead of passing a tuple). fn substitute_template( template: &[u8], args: &impl FmtTuple, @@ -1901,7 +1904,14 @@ fn substitute_template( } if j < t.len() { // consume placeholder - if args.write_nth(argi, f)? { + let filled = args.write_nth(argi, f)?; + debug_assert!( + filled, + "template has more placeholders than the {} arg(s) passed for it: {:?}", + args.len(), + bstr::BStr::new(t), + ); + if filled { argi += 1; } i = j + 1; diff --git a/src/runtime/cli/pack_command.rs b/src/runtime/cli/pack_command.rs index a1ce7dd2c0e1..4b767be83280 100644 --- a/src/runtime/cli/pack_command.rs +++ b/src/runtime/cli/pack_command.rs @@ -2713,7 +2713,7 @@ pub(crate) fn pack( Output::err( err, "failed to stat file: \"{}\"", - format_args!("{}", file.handle), + format_args!("{}", bstr::BStr::new(item.path.as_bytes())), ); Global::crash(); } @@ -3000,8 +3000,7 @@ fn tarball_destination<'a>( if !pack_filename.is_empty() && !pack_destination.is_empty() { Output::err_generic( "cannot use both filename and destination at the same time with tarball: filename \"{}\" and destination \"{}\"", - format_args!( - "{} {}", + ( bstr::BStr::new(strings::without_trailing_slash(pack_filename)), bstr::BStr::new(strings::without_trailing_slash(pack_destination)), ), @@ -3045,13 +3044,12 @@ fn tarball_destination<'a>( if res.is_err() { Output::err_generic( "archive destination name too long: \"{}/{}\"", - format_args!( - "{}/{}", + ( bstr::BStr::new(strings::without_trailing_slash(&dest_buf[..dir_len_full])), fmt_tarball_filename( package_name, package_version, - TarballNameStyle::Normalize + TarballNameStyle::Normalize, ), ), ); @@ -3654,8 +3652,7 @@ impl IgnorePatterns { Output::err( err, "failed to {} {} at: \"{}{}{}\"", - format_args!( - "{} {} {}{}{}", + ( <&str>::from(reason), <&str>::from(ignore_kind), bstr::BStr::new(strings::without_trailing_slash(dir_path)), diff --git a/test/cli/install/bun-pack.test.ts b/test/cli/install/bun-pack.test.ts index 77fe7a55685a..4bfc3cc0b449 100644 --- a/test/cli/install/bun-pack.test.ts +++ b/test/cli/install/bun-pack.test.ts @@ -2,7 +2,7 @@ import { file, spawn, write } from "bun"; import { readTarball } from "bun:internal-for-testing"; import { beforeEach, describe, expect, test } from "bun:test"; import { exists, mkdir, rm } from "fs/promises"; -import { bunEnv, bunExe, pack, runBunInstall, tempDir, tmpdirSync } from "harness"; +import { bunEnv, bunExe, isLinux, isWindows, pack, runBunInstall, tempDir, tmpdirSync } from "harness"; import fs from "node:fs/promises"; import { join } from "path"; @@ -331,6 +331,28 @@ describe("flags", () => { }); } + // The destination directory itself still fits in the path buffer, but appending + // "/-.tgz" to it does not. Windows command lines cannot carry an + // argument anywhere near its path buffer size. + test.skipIf(isWindows)("--destination with no room left for the tarball name", async () => { + const name = `pack-dest-too-long-${Buffer.alloc(180, "n").toString()}`; + await Promise.all([ + write( + join(packageDir, "package.json"), + JSON.stringify({ + name, + version: "1.0.0", + }), + ), + write(join(packageDir, "index.js"), "console.log('hello ./index.js')"), + ]); + + const pathMax = isLinux ? 4096 : 1024; + const dest = join(packageDir, Buffer.alloc(pathMax - 100 - packageDir.length - 1, "d").toString()); + const { err } = await packExpectError(packageDir, bunEnv, `--destination=${dest}`); + expect(err).toContain(`error: archive destination name too long: "${dest}/${name}-1.0.0.tgz"\n`); + }); + const filenameTests = [ { filename: "test.tgz", @@ -394,7 +416,12 @@ describe("flags", () => { write(join(packageDir, "index.js"), "console.log('hello ./index.js')"), ]); - expect(async () => await pack(packageDir, bunEnv, "--filename=test.tgz", "--destination=packed")).toThrowError(); + const { err } = await packExpectError(packageDir, bunEnv, "--filename=test.tgz", "--destination=packed"); + expect(err).toContain( + 'error: cannot use both filename and destination at the same time with tarball: filename "test.tgz" and destination "packed"', + ); + expect(await exists(join(packageDir, "test.tgz"))).toBeFalse(); + expect(await exists(join(packageDir, "packed"))).toBeFalse(); }); test("--ignore-scripts", async () => { @@ -1405,6 +1432,26 @@ describe(".gitignore/.npmignore", () => { }); } + for (const ignoreFile of [".gitignore", ".npmignore"]) { + test(`reports which ${ignoreFile} could not be read`, async () => { + await Promise.all([ + write( + join(packageDir, "package.json"), + JSON.stringify({ + name: "pack-ignore-unreadable", + version: "1.0.0", + }), + ), + write(join(packageDir, "subdir", "index.js"), "console.log('hello ./subdir/index.js')"), + // a directory where the ignore file is expected: opening it succeeds, reading it fails + mkdir(join(packageDir, "subdir", ignoreFile), { recursive: true }), + ]); + + const { err } = await packExpectError(packageDir, bunEnv); + expect(err).toContain(`EISDIR: failed to read ${ignoreFile} at: "${join(packageDir, "subdir", ignoreFile)}"\n`); + }); + } + test("excludes files recursively", async () => { await Promise.all([ write( From 3a74a5b5bc35a9bfa43adf3f3751571658aad55b Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:46:26 +0000 Subject: [PATCH 2/4] test: size the over-long destination in bytes --- test/cli/install/bun-pack.test.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/test/cli/install/bun-pack.test.ts b/test/cli/install/bun-pack.test.ts index 4bfc3cc0b449..b89765712514 100644 --- a/test/cli/install/bun-pack.test.ts +++ b/test/cli/install/bun-pack.test.ts @@ -331,11 +331,13 @@ describe("flags", () => { }); } - // The destination directory itself still fits in the path buffer, but appending - // "/-.tgz" to it does not. Windows command lines cannot carry an - // argument anywhere near its path buffer size. + // The destination directory ends `headroom` bytes short of the path buffer, so it still fits, + // but the longer "/-.tgz" appended to it does not. Windows command lines cannot + // carry an argument anywhere near its path buffer size. test.skipIf(isWindows)("--destination with no room left for the tarball name", async () => { - const name = `pack-dest-too-long-${Buffer.alloc(180, "n").toString()}`; + const pathMax = isLinux ? 4096 : 1024; + const headroom = 100; + const name = `pack-dest-too-long-${Buffer.alloc(2 * headroom, "n").toString()}`; await Promise.all([ write( join(packageDir, "package.json"), @@ -347,8 +349,9 @@ describe("flags", () => { write(join(packageDir, "index.js"), "console.log('hello ./index.js')"), ]); - const pathMax = isLinux ? 4096 : 1024; - const dest = join(packageDir, Buffer.alloc(pathMax - 100 - packageDir.length - 1, "d").toString()); + // packageDir + "/" + filler is `headroom` bytes short of the buffer + const filler = Buffer.alloc(pathMax - headroom - Buffer.byteLength(packageDir) - 1, "d").toString(); + const dest = join(packageDir, filler); const { err } = await packExpectError(packageDir, bunEnv, `--destination=${dest}`); expect(err).toContain(`error: archive destination name too long: "${dest}/${name}-1.0.0.tgz"\n`); }); From 2a0fe51835eb9fde00dc75264cab34e4aee20d91 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:49:23 +0000 Subject: [PATCH 3/4] output: shorten the substitute_template doc, point the assertion at the fix --- src/bun_core/output.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/bun_core/output.rs b/src/bun_core/output.rs index ee4f6d00f681..343a3ba6975d 100644 --- a/src/bun_core/output.rs +++ b/src/bun_core/output.rs @@ -1876,11 +1876,8 @@ impl_fmt_tuple!(0 A, 1 B, 2 C, 3 D, 4 E, 5 F, 6 G); impl_fmt_tuple!(0 A, 1 B, 2 C, 3 D, 4 E, 5 F, 6 G, 7 H); /// Substitute `{}` / `{s}` / `{d}` / `{any}` / `{f}` placeholders in `template` -/// with successive entries from `args`. `{{` / `}}` are emitted as literal -/// braces. Every other `{...}` consumes an entry, whatever the spec says; a -/// placeholder with no entry left renders as nothing (and fails a debug -/// assertion, since it means the call site packed several values into one -/// `format_args!` instead of passing a tuple). +/// with successive entries from `args`; the spec inside the braces is ignored. +/// `{{` / `}}` are emitted as literal braces. fn substitute_template( template: &[u8], args: &impl FmtTuple, @@ -1907,7 +1904,7 @@ fn substitute_template( let filled = args.write_nth(argi, f)?; debug_assert!( filled, - "template has more placeholders than the {} arg(s) passed for it: {:?}", + "template has more placeholders than the {} arg(s) passed with it (a format_args! counts as one; pass a tuple): {:?}", args.len(), bstr::BStr::new(t), ); From 9948148ec2611f1f2e831fb50df5f70abd24eb80 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:51:53 +0000 Subject: [PATCH 4/4] output: keep the substitute_template doc to a one-line correction --- src/bun_core/output.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bun_core/output.rs b/src/bun_core/output.rs index 343a3ba6975d..e0f1aa363edc 100644 --- a/src/bun_core/output.rs +++ b/src/bun_core/output.rs @@ -1876,8 +1876,8 @@ impl_fmt_tuple!(0 A, 1 B, 2 C, 3 D, 4 E, 5 F, 6 G); impl_fmt_tuple!(0 A, 1 B, 2 C, 3 D, 4 E, 5 F, 6 G, 7 H); /// Substitute `{}` / `{s}` / `{d}` / `{any}` / `{f}` placeholders in `template` -/// with successive entries from `args`; the spec inside the braces is ignored. -/// `{{` / `}}` are emitted as literal braces. +/// with successive entries from `args`. `{{` / `}}` are emitted as literal +/// braces. The spec inside any other `{...}` is ignored. fn substitute_template( template: &[u8], args: &impl FmtTuple,