-
Notifications
You must be signed in to change notification settings - Fork 4.7k
cli: sync PWD env var with --cwd chdir #30458
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -813,6 +813,12 @@ | |
| ); | ||
| Global::exit(1); | ||
| } | ||
| // Keep `PWD` in sync with the new cwd so tools that read | ||
| // `process.env.PWD` (TypeScript / vue-tsc for module resolution, | ||
| // shell scripts, subprocess children, etc.) see the directory | ||
| // we just chdir'd into instead of the inherited parent. bash's | ||
| // builtin `cd` does the same — only on success. | ||
| set_pwd_env(&out_z); | ||
| Box::<[u8]>::from(out_z.as_bytes()) | ||
| } else { | ||
| let mut temp = PathBuffer::uninit(); | ||
|
|
@@ -1565,6 +1571,99 @@ | |
| Ok(opts) | ||
| } | ||
|
|
||
| /// Publish `cwd` as the `PWD` environment variable. Used after `--cwd` | ||
| /// successfully chdir's so that `process.env.PWD` and spawned children see | ||
| /// the new directory. bash's builtin `cd` does the same — only on success. | ||
| /// | ||
| /// Two writes are needed: | ||
| /// 1. OS env (`setenv` / `SetEnvironmentVariableW`) — so `Bun.spawn` children | ||
| /// inherit the new `PWD` via the OS-level env block. | ||
| /// 2. Frozen WTF-8 snapshot on Windows — `DotEnv::load_process` iterates | ||
| /// `bun_sys::environ()` which on Windows reads | ||
| /// `bun_core::os::environ()`, a snapshot taken once at startup by | ||
| /// `convert_env_to_wtf8` (`src/sys/windows/env.rs`); `SetEnvironmentVariableW` | ||
| /// doesn't update it, so we replace the `PWD=` slot in place or grow | ||
| /// the slice by one. POSIX is fine without this step — `bun_sys::environ` | ||
| /// reads libc's live `environ` pointer each call, which `setenv` updates. | ||
| fn set_pwd_env(cwd: &bun_core::ZBox) { | ||
| #[cfg(windows)] | ||
| { | ||
| patch_windows_environ_snapshot(cwd.as_bytes()); | ||
| let mut wbuf = bun_paths::WPathBuffer::uninit(); | ||
| let wcwd = bun_paths::strings::to_w_path(wbuf.as_mut_slice(), cwd.as_bytes()); | ||
| const PWD_W: [u16; 4] = [b'P' as u16, b'W' as u16, b'D' as u16, 0]; | ||
| // SAFETY: PWD_W is NUL-terminated; to_w_path writes a NUL-terminated WStr. | ||
| unsafe { | ||
| let _ = SetEnvironmentVariableW(PWD_W.as_ptr(), wcwd.as_ptr()); | ||
| } | ||
| } | ||
| #[cfg(not(windows))] | ||
| { | ||
| // SAFETY: literal is NUL-terminated; ZBox::as_ptr() points to NUL-terminated bytes. | ||
| unsafe { | ||
| let _ = setenv(c"PWD".as_ptr(), cwd.as_ptr(), 1); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(windows)] | ||
| fn patch_windows_environ_snapshot(cwd: &[u8]) { | ||
| // Mirror Zig's invariant in `convert_env_to_wtf8`: the allocation is | ||
| // leaked for the process lifetime, so no `Drop` worries here. Replace | ||
| // the existing `PWD=` entry in place when present, otherwise grow the | ||
| // slice by one — the `set_environ(ptr, len)` call publishes the new | ||
| // (ptr, len) pair atomically for subsequent readers. | ||
| let mut entry: Vec<u8> = Vec::with_capacity(4 + cwd.len() + 1); | ||
| entry.extend_from_slice(b"PWD="); | ||
| entry.extend_from_slice(cwd); | ||
| entry.push(0); | ||
| let entry_ptr: *mut core::ffi::c_char = Box::leak(entry.into_boxed_slice()).as_mut_ptr().cast(); | ||
|
|
||
| // SAFETY: single-threaded argv parse; no other thread reads `environ` yet. | ||
| let env = unsafe { bun_core::os::environ() }; | ||
| for (i, slot) in env.iter().enumerate() { | ||
| // SAFETY: entries are NUL-terminated by construction in convert_env_to_wtf8. | ||
| let bytes = unsafe { bun_core::ffi::cstr_bytes(*slot as *const _) }; | ||
| if bytes.starts_with(b"PWD=") { | ||
| // SAFETY: `env.as_ptr()` is a `*const *mut c_char` into the Box::leak'd | ||
| // slice from convert_env_to_wtf8; we have exclusive access at argv-parse | ||
| // time (before any other thread exists). | ||
| unsafe { | ||
| let mut_base = env.as_ptr() as *mut *mut core::ffi::c_char; | ||
| *mut_base.add(i) = entry_ptr; | ||
| } | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // No existing PWD entry — grow the envp slice by one. We rebuild the | ||
| // full NUL-terminated array (Box::leak'd) and swap it in via | ||
| // `set_environ`. | ||
| let mut new: Vec<*mut core::ffi::c_char> = Vec::with_capacity(env.len() + 2); | ||
| new.extend_from_slice(env); | ||
| new.push(entry_ptr); | ||
| new.push(core::ptr::null_mut()); | ||
| let new = Box::leak(new.into_boxed_slice()); | ||
| // SAFETY: single-threaded startup; `new` is live for the process lifetime. | ||
| unsafe { | ||
| bun_core::os::set_environ(new.as_mut_ptr(), new.len() - 1); | ||
| } | ||
| } | ||
|
|
||
| #[cfg(not(windows))] | ||
| unsafe extern "C" { | ||
| fn setenv( | ||
| name: *const core::ffi::c_char, | ||
| value: *const core::ffi::c_char, | ||
| overwrite: core::ffi::c_int, | ||
| ) -> core::ffi::c_int; | ||
| } | ||
|
|
||
| #[cfg(windows)] | ||
| unsafe extern "C" { | ||
| fn SetEnvironmentVariableW(name: *const u16, value: *const u16) -> i32; | ||
| } | ||
|
Check warning on line 1665 in src/runtime/cli/Arguments.rs
|
||
|
Comment on lines
+1662
to
+1665
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 nit: Extended reasoning...WhatThe new declaration at #[cfg(windows)]
unsafe extern "C" {
fn SetEnvironmentVariableW(name: *const u16, value: *const u16) -> i32;
}
Why this is only a nitOn the Windows targets Bun actually ships ( The only target where the distinction matters is 32-bit x86 Windows, where Step-by-step
ImpactNone on shipped binaries. This is purely a documented-convention violation in new code, with a one-word fix. Suggested fix#[cfg(windows)]
unsafe extern "system" {
fn SetEnvironmentVariableW(name: *const u16, value: *const u16) -> i32;
}Optionally also fix the pre-existing copy at |
||
|
|
||
| /// Cold path: `bun test` option-group parsing — timeout / coverage / reporter / | ||
| /// shard / parallel / seed / etc. Split out of [`parse`] so the `bun run <script>` | ||
| /// and bare-`bun <file>` hot path (`USES_GLOBAL_OPTIONS` ⇒ `parse` runs on every | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -329,6 +329,112 @@ describe.concurrent("bun run", () => { | |
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| // https://github.com/oven-sh/bun/issues/30456 | ||
| // | ||
| // The reported repro is `bun --cwd=frontend run lint`, so cover both the | ||
| // bare-entrypoint form (`bun --cwd=subdir test.js`) and the `run` | ||
| // subcommand form (`bun --cwd=subdir run test.js`) — each hits a different | ||
| // Arguments.parse branch. | ||
| // | ||
| // `PWD` is a POSIX shell convention — these tests exist for the tools that | ||
| // read it (vue-tsc via TypeScript's module resolution, shell scripts, | ||
| // subprocess children). On Windows the path / env conventions differ enough | ||
| // (case-insensitive keys, backslash vs forward-slash in `process.cwd()` vs | ||
| // the `.loose`-normalised path we store, `%CD%` as the native equivalent) | ||
| // that a strict equality test would produce false failures; skip there. | ||
| for (const withRun of [false, true]) { | ||
| const label = withRun ? "bun --cwd run" : "bun --cwd"; | ||
| const buildCmd = (...extra: string[]) => [bunExe(), "--cwd=subdir", ...(withRun ? ["run"] : []), ...extra]; | ||
|
Comment on lines
+345
to
+347
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
rg -n 'for \(const withRun of \[false, true\]\)' test/cli/install/bun-run.test.ts
rg -n 'describe\.each\(' test/cli/install/bun-run.test.tsRepository: oven-sh/bun Length of output: 178 🏁 Script executed: sed -n '340,370p' test/cli/install/bun-run.test.tsRepository: oven-sh/bun Length of output: 1504 🏁 Script executed: sed -n '340,437p' test/cli/install/bun-run.test.tsRepository: oven-sh/bun Length of output: 3794 🏁 Script executed: sed -n '140,160p' test/cli/install/bun-run.test.tsRepository: oven-sh/bun Length of output: 923 Use The test block at line 345 uses 🤖 Prompt for AI Agents |
||
|
|
||
| it.skipIf(isWindows)(`${label} updates process.env.PWD to match the new cwd`, async () => { | ||
| using dir = tempDir(`bun-run-cwd-pwd-${withRun ? "run" : "bare"}`, { | ||
| "subdir/test.js": `console.log(JSON.stringify({ pwd: process.env.PWD, cwd: process.cwd() }));`, | ||
| }); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: buildCmd("test.js"), | ||
| cwd: String(dir), | ||
| stdin: "ignore", | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| env: { | ||
| ...bunEnv, | ||
| // Set a known PWD so we detect when it's not overwritten — don't | ||
| // rely on the inherited parent PWD (Bun's test harness can leave | ||
| // it pointing at an unexpected directory on some platforms). | ||
| PWD: String(dir), | ||
| }, | ||
| }); | ||
|
|
||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | ||
|
|
||
| const { pwd, cwd } = JSON.parse(stdout); | ||
| // realpath-resolved subdir under the tempdir, since macOS tempdirs | ||
| // symlink through /private. | ||
| expect(cwd).toMatch(/subdir$/); | ||
| // PWD must match cwd — tools like vue-tsc / TypeScript's module | ||
| // resolution read PWD and use it as the resolution root. | ||
| expect(pwd).toBe(cwd); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| it.skipIf(isWindows)(`${label} PWD is inherited by spawned child processes`, async () => { | ||
| using dir = tempDir(`bun-run-cwd-pwd-child-${withRun ? "run" : "bare"}`, { | ||
| "subdir/test.js": ` | ||
| import { spawnSync } from "child_process"; | ||
| const out = spawnSync(process.execPath, ["-e", "console.log(process.env.PWD)"], { | ||
| env: process.env, | ||
| encoding: "utf8", | ||
| }); | ||
| process.stdout.write(out.stdout); | ||
| `, | ||
| }); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: buildCmd("test.js"), | ||
| cwd: String(dir), | ||
| stdin: "ignore", | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| env: { | ||
| ...bunEnv, | ||
| PWD: String(dir), | ||
| }, | ||
| }); | ||
|
|
||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | ||
|
|
||
| expect(stdout.trim()).toMatch(/subdir$/); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| // When PWD is absent from the inherited env (env -u PWD, cron, systemd, | ||
| // minimal Docker), libc's setenv reallocates the environ array and the | ||
| // naive `std.os.environ` snapshot misses the addition. Make sure --cwd | ||
| // still publishes PWD through to `process.env` in that case. | ||
| it.skipIf(isWindows)(`${label} adds PWD when parent had none`, async () => { | ||
| using dir = tempDir(`bun-run-cwd-pwd-unset-${withRun ? "run" : "bare"}`, { | ||
| "subdir/test.js": `console.log(process.env.PWD ?? "<unset>");`, | ||
| }); | ||
|
|
||
| const { PWD, ...envNoPwd } = bunEnv; | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: buildCmd("test.js"), | ||
| cwd: String(dir), | ||
| stdin: "ignore", | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| env: envNoPwd, | ||
| }); | ||
|
|
||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | ||
|
|
||
| expect(stdout.trim()).toMatch(/subdir$/); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
| } | ||
|
|
||
| it("DCE annotations are respected", async () => { | ||
| using dir = tempDir("test", { | ||
| "index.ts": ` | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Writing through
env.as_ptr() as *mut *mut c_charis UB under Stacked Borrows / Tree Borrows:envis a&'static [*mut c_char]built viaslice::from_raw_parts(src/bun_core/lib.rs:245-253), so.as_ptr()carriesSharedReadOnlyprovenance and the cast doesn't change that — Miri will reject the write at line 1634. Thebun_core::osmodule already stores the raw*mut *mut c_charinENVIRONprecisely to avoid this round-trip; either expose anenviron_raw()/environ_mut()accessor and write through that, or just rebuild +set_environ()here the same way the no-PWDbranch already does.Extended reasoning...
What the bug is
patch_windows_environ_snapshotobtains the envp slice viabun_core::os::environ(), which returns a shared reference:The PR then writes through a pointer derived from that shared reference:
<&[T]>::as_ptr()yields a*const Twhose provenance is inherited from the&[T]borrow. Under Stacked Borrows that tag isSharedReadOnly; under Tree Borrows the shared reborrow creates aFrozennode. Anas-cast to*mutdoes not grant write permission — it preserves the same tag — so the store at*mut_base.add(i)writes through a read-only tag, which is undefined behavior. Miri will flag this as "attempting a write access using … but that tag only grants SharedReadOnly".Why existing code doesn't prevent it
The
SAFETY:comment on the block argues exclusive access ("single-threaded argv parse … no other thread readsenvironyet"). That justifies the absence of data races, but provenance is a per-pointer property independent of threading: even with truly exclusive access, a pointer derived from&T(noUnsafeCell) may not be used to writeT. The underlying allocation is in fact writable (it came fromBox::leakinconvert_env_to_wtf8), but that mutable provenance was discarded the moment the access went throughslice::from_raw_parts::<*mut c_char>→&[*mut c_char].The codebase documents exactly this hazard at
src/CLAUDE.md:272-276("Pointer provenance at FFI boundaries": "a&self-derived raw pointer carriesSharedReadOnlyprovenance"), andbun_core::oswas deliberately designed around it — the comment at lib.rs:221-225 explains thatENVIRONis stored as a raw(*mut *mut c_char, usize)pair specifically so writers don't have to alias a live&mutor round-trip through a shared borrow.Step-by-step proof
convert_env_to_wtf8builds aBox<[*mut c_char]>, leaks it, and stores(ptr, len)inENVIRONviaset_environ. The leaked allocation has a Unique root tag with full read/write permission.bun --cwd=subdirruns withPWDinherited (the common case).Arguments::parsecallsset_pwd_env→patch_windows_environ_snapshot.environ()reads(p, n)fromENVIRONand callscore::slice::from_raw_parts(p, n). This creates a newSharedReadOnlyborrow (Stacked Borrows) /Frozenchild (Tree Borrows) of the allocation, covering elements[0, n), and returnsenv: &'static [*mut c_char]carrying that tag.env[i]starting withb"PWD=".env.as_ptr()returns*const *mut c_charwith the sameSharedReadOnlytag asenv. Casting itas *mut *mut c_charchanges only the static type; the tag is unchanged.*mut_base.add(i) = entry_ptrperforms a write at offsetiusing a tag that grants only read permission → UB. Under Miri this aborts; under rustc the optimizer is free to assumeenv[i]is unchanged for the lifetime ofenv(it's still live through thereturn) and could e.g. reorder or eliminate the store.Impact
This is textbook aliasing-model UB in new
unsafecode introduced by this PR, on the path taken by every Windows--cwdinvocation wherePWDis inherited. Practical miscompilation risk with current rustc/LLVM is low (the&[T]-derivednoaliasis read-only and there's no competing read after the write here), but the project's own conventions explicitly forbid this pattern, the surrounding module was designed to make the correct approach easy, and the new testsskipIf(isWindows)so this path has zero CI coverage.Suggested fix
Don't round-trip through the
&[..]view for the write. Two easy options:PWDbranch in this same function already does it correctly — build a freshBox::leak'd array and publish it viabun_core::os::set_environ(ptr, len). The in-place branch can do the same (copyenv, replace sloti, push the trailing null,set_environ). This costs one small allocation on a one-time startup path.pub unsafe fn environ_raw() -> (*mut *mut c_char, usize)inbun_core::os(it's justcore::ptr::read(&raw const ENVIRON)) and write through that pointer, which retains the originalBox::leakmutable provenance: