Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/jsc/JSGlobalObject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1167,6 +1167,12 @@
ZigGlobalObject__makeNapiEnvForFFI(self)
}

/// The live `process.env` JS object (forces the lazy init). Mutations made
/// via `process.env.X = ...` / `delete process.env.X` are visible here.
pub fn process_env_object(&self) -> JSValue {
ZigGlobalObject__processEnvObject(self)
}

Check warning on line 1174 in src/jsc/JSGlobalObject.rs

View check run for this annotation

Claude / Claude Code Review

process_env_object() lacks exception scope for lazy-init path

`process_env_object()` can trigger the `processEnvObject` lazy initializer → `createEnvironmentVariablesMap`, which opens a `DECLARE_THROW_SCOPE`, but neither the new C++ shim nor this Rust wrapper opens a scope or returns `JsResult` — under `BUN_JSC_validateExceptionChecks=1`, if `Bun.which`/`Bun.spawn` is the very first `process.env` access, the next scope-opening call (`.get("PATH")` / `JSPropertyIterator::init`) asserts on the unchecked-exception bit. Other first-access callers wrap this (`c
Comment thread
claude[bot] marked this conversation as resolved.

#[inline]
pub fn assert_on_js_thread(&self) {
if cfg!(debug_assertions) {
Expand Down Expand Up @@ -1679,6 +1685,7 @@
-> JSValue;

safe fn ZigGlobalObject__makeNapiEnvForFFI(this: &JSGlobalObject) -> *mut c_void;
safe fn ZigGlobalObject__processEnvObject(this: &JSGlobalObject) -> JSValue;

safe fn JSC__JSGlobalObject__bunVM(this: &JSGlobalObject) -> *mut c_void;
safe fn JSC__JSGlobalObject__vm(this: &JSGlobalObject) -> *mut VM;
Expand Down
5 changes: 5 additions & 0 deletions src/jsc/bindings/ZigGlobalObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1882,6 +1882,11 @@ extern "C" napi_env ZigGlobalObject__makeNapiEnvForFFI(Zig::GlobalObject* global
return globalObject->makeNapiEnvForFFI();
}

extern "C" JSC::EncodedJSValue ZigGlobalObject__processEnvObject(Zig::GlobalObject* globalObject)
{
return JSValue::encode(globalObject->processEnvObject());
}

JSC_DEFINE_HOST_FUNCTION(jsFunctionPerformMicrotaskVariadic, (JSGlobalObject * globalObject, CallFrame* callframe))
{
auto& vm = JSC::getVM(globalObject);
Expand Down
7 changes: 4 additions & 3 deletions src/runtime/api/BunObject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -614,9 +614,10 @@ pub(crate) fn which(global_this: &JSGlobalObject, callframe: &CallFrame) -> JsRe
return Ok(JSValue::NULL);
}

// SAFETY: `transpiler.env` / `.fs` are process-lifetime singletons set during VM init.
let mut path_str =
ZigStringSlice::from_utf8_never_free(vm.env_loader().get(b"PATH").unwrap_or(b""));
let mut path_str = match global_this.process_env_object().get(global_this, "PATH")? {
Some(v) if !v.is_undefined_or_null() => v.to_slice(global_this)?,
_ => ZigStringSlice::from_utf8_never_free(b""),
};
let mut cwd_str = ZigStringSlice::from_utf8_never_free(vm.top_level_dir());

if let Some(arg) = arguments.next_eat() {
Expand Down
60 changes: 30 additions & 30 deletions src/runtime/api/bun/js_bun_spawn_bindings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,6 @@ pub(crate) fn spawn_maybe_sync<const IS_SYNC: bool>(
// drops after `spawn_process` returns, freeing all argv/env allocations.
let mut cstr_storage: Vec<ZBox> = Vec::new();

let mut override_env = false;
let mut env_array: Vec<CStrPtr> = Vec::new();
// SAFETY: `bun_vm()` returns the live VirtualMachine for this thread; it
// outlives this call frame.
Expand All @@ -383,9 +382,9 @@ pub(crate) fn spawn_maybe_sync<const IS_SYNC: bool>(
let mut lazy = false;
let mut on_exit_callback = JSValue::ZERO;
let mut on_disconnect_callback = JSValue::ZERO;
// `env_loader()` is the audited safe accessor for the per-VM DotEnv loader
// (process-lifetime; centralised non-null deref in `VirtualMachine`).
let mut path: &[u8] = jsc_vm.env_loader().get(b"PATH").unwrap_or(b"");
// Populated from the explicit `env:` option or, when absent, the live
// `process.env` object; both via `append_envp_from_js` below.
let mut path: &[u8] = b"";
let mut argv: Vec<CStrPtr> = Vec::new();
let cmd_value: JSValue;
let mut detached = false;
Expand Down Expand Up @@ -601,7 +600,6 @@ pub(crate) fn spawn_maybe_sync<const IS_SYNC: bool>(
);
};

override_env = true;
// If the env object does not include a $PATH, it must disable path lookup for argv[0]
let mut new_path: &[u8] = b"";
// `JSObject` is an `opaque_ffi!` ZST handle; `opaque_ref` is the
Expand All @@ -614,6 +612,20 @@ pub(crate) fn spawn_maybe_sync<const IS_SYNC: bool>(
&mut cstr_storage,
)?;
path = new_path;
} else {
// No explicit env: inherit the live `process.env` so runtime
// mutations (set/delete/PATH edits) reach the child.
let process_env = global_this.process_env_object();
process_env.ensure_still_alive();
if let Some(object) = process_env.get_object() {
append_envp_from_js(
global_this,
JSObject::opaque_ref(object),
&mut env_array,
&mut path,
&mut cstr_storage,
)?;
}
}

get_argv(
Expand Down Expand Up @@ -894,6 +906,19 @@ pub(crate) fn spawn_maybe_sync<const IS_SYNC: bool>(
}
}
} else {
// No options object means no explicit env: inherit the live
// `process.env` so runtime mutations reach the child.
let process_env = global_this.process_env_object();
process_env.ensure_still_alive();
if let Some(object) = process_env.get_object() {
append_envp_from_js(
global_this,
JSObject::opaque_ref(object),
&mut env_array,
&mut path,
&mut cstr_storage,
)?;
}
get_argv(
global_this,
cmd_value,
Expand All @@ -908,31 +933,6 @@ pub(crate) fn spawn_maybe_sync<const IS_SYNC: bool>(

bun_output::scoped_log!(Subprocess, "spawn maxBuffer: {:?}", max_buffer);

// Owns the `K=V\0` storage when inheriting the parent env; the struct
// lives until spawn returns.
let mut inherited_env_storage: Option<bun_dotenv::NullDelimitedEnvMap> = None;
if !override_env && env_array.is_empty() {
// `Transpiler::env_mut()` is the audited safe `&mut Loader` accessor
// (per-VM DotEnv loader, valid for VM lifetime; centralised
// single-unsafe deref). `.map` is its `&'a mut Map` slot.
let envmap = match jsc_vm
.transpiler
.env_mut()
.map
.create_null_delimited_env_map()
{
Ok(m) => m,
Err(_) => return Err(global_this.throw_out_of_memory()),
};
// Note: `as_slice()` *includes* the trailing null, so strip it; the
// common tail below re-appends one after the optional NODE_CHANNEL_*
// entries.
let entries = envmap.as_slice();
env_array.extend_from_slice(&entries[..entries.len().saturating_sub(1)]);
inherited_env_storage = Some(envmap);
}
let _ = &inherited_env_storage;

for fd_index in 0..stdio.len() {
if stdio[fd_index].can_use_memfd() {
if stdio[fd_index].use_memfd(fd_index as u32) {
Expand Down
71 changes: 69 additions & 2 deletions test/js/bun/spawn/spawn-env.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,73 @@
import { spawn } from "bun";
import { test } from "bun:test";
import { bunExe } from "harness";
import { describe, expect, test } from "bun:test";
import { bunEnv, bunExe, isWindows, tempDir } from "harness";
import { chmodSync } from "node:fs";
import { join } from "node:path";

// Bun.spawn / Bun.spawnSync with no `env:` option must inherit the *live*
// process.env, including runtime mutations, not the startup snapshot.
describe("default env inherits live process.env", () => {
const printEnv = isWindows
? `[process.execPath, "-e", "process.stdout.write((process.env.BUN_TEST_SPAWN_ENV_SET ?? '[unset]') + ',' + (process.env.BUN_TEST_SPAWN_ENV_DEL ?? '[unset]'))"]`
: `["/bin/sh", "-c", "printf '%s,%s' \\"\${BUN_TEST_SPAWN_ENV_SET-[unset]}\\" \\"\${BUN_TEST_SPAWN_ENV_DEL-[unset]}\\""]`;

for (const [label, call] of [
["Bun.spawnSync({cmd})", `Bun.spawnSync({ cmd: ${printEnv} }).stdout.toString()`],
["Bun.spawnSync([cmd])", `Bun.spawnSync(${printEnv}).stdout.toString()`],
["Bun.spawn({cmd})", `await (await Bun.spawn({ cmd: ${printEnv} }).stdout).text()`],
] as const) {
test.concurrent(label, async () => {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`process.env.BUN_TEST_SPAWN_ENV_SET = "runtime-value";
delete process.env.BUN_TEST_SPAWN_ENV_DEL;
process.stdout.write(${call});`,
],
env: { ...bunEnv, BUN_TEST_SPAWN_ENV_DEL: "startup-value" },
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([
proc.stdout.text(),
proc.stderr.text(),
proc.exited,
]);
expect(stderr).toBe("");

Check warning on line 36 in test/js/bun/spawn/spawn-env.test.ts

View check run for this annotation

Claude / Claude Code Review

Test asserts stderr is exactly empty (REVIEW.md subprocess-test rule)

REVIEW.md → "Subprocess tests" says to assert a combined `{ stdout, stderr, exitCode }` object rather than `expect(stderr).toBe("")` (same at line 66). Swapping to e.g. `expect({ stdout, stderr, exitCode }).toEqual({ stdout: "runtime-value,[unset]", stderr: "", exitCode: 0 })` shows all three values in one diff when the test fails, instead of a bare stderr mismatch with no stdout/exitCode context.
Comment thread
robobun marked this conversation as resolved.
Outdated
expect(stdout).toBe("runtime-value,[unset]");
expect(exitCode).toBe(0);
});
}

test.concurrent.skipIf(isWindows)("PATH mutation is used for argv0 lookup and Bun.which", async () => {
Comment thread
robobun marked this conversation as resolved.
using dir = tempDir("spawn-env-path", {
"bun_test_spawn_env_tool": "#!/bin/sh\nprintf found-the-tool\n",
});
const toolDir = String(dir);
chmodSync(join(toolDir, "bun_test_spawn_env_tool"), 0o755);

await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`process.env.PATH = ${JSON.stringify(toolDir)} + ":" + process.env.PATH;
console.log(Bun.which("bun_test_spawn_env_tool") !== null);
console.log(Bun.spawnSync({ cmd: ["bun_test_spawn_env_tool"] }).stdout.toString());
console.log(Bun.spawnSync(["bun_test_spawn_env_tool"]).stdout.toString());`,
],
env: bunEnv,
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([
proc.stdout.text(),
proc.stderr.text(),
proc.exited,
]);
expect(stderr).toBe("");
expect(stdout).toBe("true\nfound-the-tool\nfound-the-tool\n");
expect(exitCode).toBe(0);
});
});

test("spawn env", async () => {
const env = {};
Expand Down
Loading