install: only replace or remove a package's own bins in the global bin dir

Closed
RO
1mo ago
+448-38

Problem

  • The bin linker trusts the names in a package's bin map. On EEXIST, create_symlink (src/install/bin.rs:1318 on main) ran delete_tree_absolute on the destination. unlink_bin_or_shim (bin.rs:856) ran a bare unlink. The Windows shim writer truncated <name>.exe.
  • The global bin dir (~/.bun/bin) also holds bun itself and the user's files. bun add -g and bun link replaced any of them, directories included. bun unlink deleted them by name. All three reported success.

Fix

  • Linker::existing_bin_belongs_to_package reads the path stored in the entry (the symlink target, or the .bunx sidecar on Windows). The entry is ours when it points into the package being linked.
  • In the global bin dir an entry is replaced only when it is ours. Otherwise the link fails with EEXIST (Failed to link what-bin: EEXIST, exit 1) and the entry stays, as in npm. bun unlink and the cleanup after a failed link use the same check. A failed destination also leaves the seen set, so the native binlink retry reports the error instead of skipping it.
  • node_modules/.bin belongs to the installer. A stale entry there is still replaced, but with unlink, never recursively.
  • Verified: 5 new tests in test/cli/install/bun-link.test.ts and 3 in test/cli/install/bun-install-registry.test.ts, on Linux and Windows. 5 of the 8 fail on current bun. More suites in the notes.

Background

  • The linker (src/install/bin.rs) writes one entry per bin key, into node_modules/.bin for a project and into $BUN_INSTALL/bin for bun add -g and bun link.
  • On POSIX an entry is a relative symlink, foo -> ../install/global/node_modules/foo/cli.js. On Windows it is foo.exe, a copy of the shim, plus foo.bunx, which starts with the script path.
  • The native binlink redirect points a bin at a platform package (esbuild at @esbuild/linux-x64). PackageInstaller links with the redirect first and retries without it on an error. A link into either directory is ours.
  • bun remove -g was already safe. It removes dangling symlinks and does not read the bin map.
Notes

Repro on 1.4.0-canary:

export BUN_INSTALL=$(mktemp -d); mkdir -p $BUN_INSTALL/bin/adir lib && cd lib
echo real > $BUN_INSTALL/bin/bun; ln -s /usr/bin/true $BUN_INSTALL/bin/foreign; touch $BUN_INSTALL/bin/adir/inner
echo '{"name":"lib6","version":"1.0.0","bin":{"lib6cli":"cli.js","bun":"cli.js","foreign":"cli.js","adir":"cli.js"}}' > package.json
echo '#!/usr/bin/env node' > cli.js
bun link      # before: bun, foreign and adir are symlinks into lib6 now, adir/inner is gone, exit 0
              # after: "error: failed to link bin due to error EEXIST", exit 1, the three entries are untouched, lib6cli is linked
bun unlink    # before: removes every key that exists, including a regular file named bun
              # after: removes lib6cli only

On Windows the unfixed build overwrites bin\bun.exe with the shim executable, and bun unlink deletes it. The new tests fail that way on the current build there and pass with this change.

Why the cleanup needs the check too: link_bin_or_create_shim unlinks the destination after any error. The refusal is such an error, so without the check the cleanup would delete the foreign entry the refusal just protected.

Windows details: the collision check runs before the shim files are opened, because they are opened with truncation. A .exe with no .bunx next to it counts as a real program. When writing the pair fails after the sidecar was truncated, create_windows_shim removes the pair itself (test "removes a shim it could not finish writing"); the ownership check could not attribute an empty sidecar, and the leftover would block the name with EEXIST. The suffix helpers report a name that does not fit the path buffer as foreign (or as a conflict) instead of indexing past the buffer. The prefix comparison ignores ASCII case on Windows, since the stored path and the one computed now can come from an fd path and from an environment variable.

What the ownership check accepts: the relative form bun writes (../install/global/node_modules/<pkg>/... in a symlink, install\global\node_modules\<pkg>\... in a .bunx, which is relative to the parent of the bin dir) and an absolute path into the package dir. Under the native binlink redirect it checks the root package dir and the platform package dir. When the redirect is off (the feature flag, or a package dropped from the list in a later release), a link that an earlier redirected install pointed into the platform package is foreign to it and the install fails with EEXIST. That is the same rule npm applies, it fails loudly, and bun remove -g followed by bun add -g clears it, so it is left as is.

The seen fix: link_bin_or_create_shim records the destination before it links. Without removing it on failure, the retry without the redirect found it recorded, returned without an error, and bun add -g of a native binlink package exited 0 with nothing linked (test "a foreign entry is reported for a package with a native binlink as well", which fails that way without the fix).

Behavior changes beyond the repro:

  • A package that claims a bin name held by a different global package is refused as well (test "does not take a bin name over from another linked package"). Before, the last link won silently. npm refuses too.
  • The refused path is in the debug log (BUN_DEBUG_BinLinker=1). The callers' messages name the package, as for other link errors.
  • When one bin of a package is refused, the bins after it in the map are linked and then removed again by the pre-existing cleanup, which does not tell this bin's error from an earlier one. #38972 fixes that on its own. The new tests put the free name first.
  • existing non-symlink in bun-install-registry.test.ts (from #11886) pins the node_modules/.bin behavior and still passes.

Related open PRs: #36701 and #38972 change neighboring lines in create_symlink and link_bin_or_create_shim (#38972 also removes the destination from seen on an error) but keep the delete_tree_absolute and the bare unlink, as does the install fold #39403. If #36200 lands (shim metadata in an NTFS stream instead of .bunx), the Windows read in existing_bin_belongs_to_package has to read the stream instead.

Suites run with the debug build on Linux, the bin linking ones also on Windows: bun-link, bun-install-registry -t binaries, isolated-install, isolated-relink, bunx, bun-prune, bun-remove, bun-install-native-binlink, shebang-normalize, bun-add-catalog -t "with --global", bun-update -t --global. Two failures in local debug builds are unrelated to this change: bun-link "should link dependency without crashing" (debug stack trace in stdout, Linux only) and bunx "npm_config_user_agent" (debug version string).


no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install-registry.test.ts test/cli/install/bun-link.test.ts