cli: refuse to start when the cwd is longer than PATH_MAX instead of using the executable's directory - #38363
cli: refuse to start when the cwd is longer than PATH_MAX instead of using the executable's directory#38363robobun wants to merge 1 commit into
Conversation
…ger than PATH_MAX getcwd_or_exe_dir fell back to the directory containing the bun binary on every getcwd failure. The fallback exists for a deleted cwd (ENOENT), where Node also boots and lets process.cwd() throw later, but it also fired when getcwd failed with ERANGE/ENAMETOOLONG because the cwd's path does not fit in PATH_MAX. That directory exists, so `bun x.cjs` ran <dir of bun>/x.cjs and relative requires, path.resolve() and Bun.main all pointed at the binary's directory while fs calls still used the real cwd, with exit code 0. The fallback now applies to ENOENT only; every other getcwd failure at startup is reported as "ERANGE: ...: Could not get the current working directory (getcwd)" with exit code 1, for the runtime and for the other commands that already refused with "An internal error occurred (ERANGE)". The [eval]/[stdin] callers in run_command.rs propagate the error instead of picking up the exe dir.
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Status: ready for review; build #95472 is marked failed only because its single macOS test job ( Reproduced on bun 1.4.0 and on a release build of main by hardlinking With this branch both print |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the CLI startup cwd-resolution path that every bun invocation goes through (narrowing the exe-dir fallback and adding a new hard-exit), a human look would still be worthwhile.
What was reviewed:
- All six
getcwd_or_exe_dircallers inrun_command.rsnow propagate with?;Arguments::parseruns first and callsexit_without_cwdon ERANGE/ENAMETOOLONG, so those?sites are gated in practice. - errno mapping in
getcwd_len(glibc ERANGE, musl ENAMETOOLONG, ENOENT →FileNotFound) matches thegetcwd_or_exe_dirfallback arm and the test regex. - ENOENT (deleted cwd) still falls through to
self_exe_dirfor run/auto/node commands only — pinned by the newbun -edeleted-cwd test. - Test mutates the runner's
process.cwd()but restores it infinallyaround synchronousspawnSync, so no leak into other tests.
Extended reasoning...
Overview
The PR narrows bun_core::getcwd_or_exe_dir so it only substitutes the executable's directory when getcwd fails with ENOENT (deleted cwd), and returns every other failure to the caller. getcwd_len now maps errno to specific CrateError variants instead of collapsing everything to Unexpected. The exe-dir fallback body is split into a new self_exe_dir helper. Arguments::parse mirrors this: on bun_sys::getcwd failure, ENOENT + a runtime command still uses the exe-dir fallback; anything else prints <ERRNO>: ...: Could not get the current working directory (getcwd) via a new exit_without_cwd helper and exits 1. Six run_command.rs call sites add ? to propagate the now-fallible result. Tests spawn bun from a cwd deeper than PATH_MAX (via relative-path chdir) across five command variants, plus one test pinning that a deleted cwd still boots.
Security risks
None introduced; this closes a security-adjacent bug where bun could silently run x.cjs from the directory containing the bun binary instead of the user's actual (over-long) cwd. The change is fail-closed: previously-silent wrong-directory execution now becomes a hard error.
Level of scrutiny
Medium-high. The diff is small (~40 native lines) and mechanically simple, but Arguments::parse and the run-command entry path execute on every bun invocation. A regression here would affect startup broadly. The change is strictly a narrowing (fewer cases reach the exe-dir fallback; more cases exit with an error), so the failure mode is refusing to start rather than running the wrong thing — but it still warrants a human sign-off on a startup-path behavior change.
Other factors
- The new tests use
process.chdir()on the test-runner process (a process-global mutation), restored infinallyaround synchronousspawnSync; this file has notest.concurrent, so it should be hermetic, but it's the kind of pattern reviewers sometimes flag. - The
?propagation inrun_command.rsreturns abun_core::CrateError(not the richbun_sys::Errorwith syscall tag); in practiceArguments::parsehas already exited by then, but if reached the message would be less specific thanexit_without_cwd's. - The PR description explicitly scopes out
bun install/bun pm/bunx(different code path), the near-PATH_MAX bunfig panic, and the deleted-cwd + relative-script case — all reasonable deferrals but worth a maintainer nod. - CI build #95472 was still running at review time.
|
Re the review notes: the CI: build #95472 has 175 of 179 jobs passed with none failed so far. The red |
Problem
bun x.cjsstarted from a working directory whose absolute path is longer than PATH_MAX (4096 bytes on Linux, 1024 on macOS) runs<directory containing the bun binary>/x.cjsand exits 0; thex.cjsin the real cwd is ignored. With-e,require("./mod.cjs"),path.resolve()andBun.mainall point at the binary's directory whilefs.readFileSync("mod.cjs")reads the real cwd. No warning is printed.bun_core::getcwd_or_exe_dir(src/bun_core/util.rs:4140) substituted the executable's directory on everygetcwdfailure, andArguments::parse(src/runtime/cli/Arguments.rs:867) seededtop_level_dirfor run/auto commands from it. The fallback exists for a cwd that was deleted (ENOENT), butgetcwdalso fails with ERANGE (glibc, macOS) or ENAMETOOLONG (musl) when the cwd's path does not fit in the 4096-byte buffer, and that directory still exists.ERANGE: process.cwd failed with error result too large, uv_cwd, exit 1). Bun's non-runtime commands already refused too, but witherror: An internal error occurred (ERANGE).Fix
getcwd_or_exe_dirfalls back to the executable's directory only for ENOENT and returns every other failure;getcwd_lennow maps errno instead of collapsing everything toUnexpected. The[eval]/[stdin]/cron/feedback callers inrun_command.rspropagate the error with?.Arguments::parsedoes the same for the startup cwd: ENOENT on a run/auto/node command keeps the exe-dir fallback (bun_core::self_exe_dir, the old fallback body split out); anything else, and a relative--cwdwithout a usable base, printsERANGE: Numerical result out of range: Could not get the current working directory (getcwd)and exits 1. This is the same error path--cwdalready uses for a failedchdir, sobun test/bun buildget the readable message as well.test/js/node/test/parallel/test-cwd-enoent*.jspin), while an over-long cwd does, so the only safe answer is to refuse.test/cli/run/run_command.test.ts: fivebun ...invocations (-e,x.cjs,run x.cjs,--cwd . x.cjs,test) from a cwd longer than PATH_MAX must print that error and exit 1; they fail on the current release build (-eexits 0 printing the exe dir,x.cjsandrun x.cjsprintModule not found "x.cjs",--cwd .andtestprint the internal-error line) and pass with this change. A sixth test pins thatbun -estill boots from a deleted cwd andprocess.cwd()throws ENOENT there; it passes before and after.test-cwd-enoent*.jsnode tests,test/cli/run/run-eval.test.tsandtest/cli/run/as-node.test.tsagainst the debug build; all pass. The original repro (bun hardlinked next to a plantedx.cjs, cwd of 4300 bytes) now prints the error instead of running the planted file, and a 4000-byte cwd still runs the real file.Background
chdirone component at a time works indefinitely, butgetcwdhas to return the whole path and fails once it does not fit. Bun'sPathBufferis exactly PATH_MAX bytes, so such a cwd cannot be represented anywhere in the resolver, and startup is the only place to reject it.top_level_diris the directory the runtime resolves the entry point, relativerequire/importandpath.resolve()against. It is captured once at startup;process.cwd()does a freshgetcwdon every call, which is why it reported ERANGE while everything else used the exe dir.Environment::GetCwd, which substitutes the executable's directory whenuv_cwdfails so thatnode -e,-rand the REPL still work after the shell's cwd is deleted; Node's own relative-script path fails regardless becausepath.resolvecallsprocess.cwd().Not changed here
bun install/bun pm/bunxdo not go through this block; they already refuse an over-long cwd viaFileSystem::init(None)with the genericAn internal error occurred (ERANGE)line, unchanged.cd dir; rmdir dir; bun x.cjs) still resolves against the executable's directory, as before: that is the ENOENT fallback working as designed. Node errors there instead; tightening that is a separate decision.process.chdir()into a directory whose path is exactly PATH_MAX-1 bytes panics inset_process_cwd; both are tracked separately.FileNotFoundmatch here need to agree.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/run/run_command.test.ts