Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
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 @@ impl JSGlobalObject {
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) -> JsResult<JSValue> {
crate::from_js_host_call(self, || ZigGlobalObject__processEnvObject(self))
}
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 @@ unsafe extern "C" {
-> 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
9 changes: 9 additions & 0 deletions src/jsc/bindings/ZigGlobalObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1882,6 +1882,15 @@ extern "C" napi_env ZigGlobalObject__makeNapiEnvForFFI(Zig::GlobalObject* global
return globalObject->makeNapiEnvForFFI();
}

extern "C" JSC::EncodedJSValue ZigGlobalObject__processEnvObject(Zig::GlobalObject* globalObject)
{
auto& vm = JSC::getVM(globalObject);
auto scope = DECLARE_THROW_SCOPE(vm);
JSC::JSObject* object = globalObject->processEnvObject();
RETURN_IF_EXCEPTION(scope, {});
return JSValue::encode(object);
}

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 @@
// 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 @@
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 @@
);
};

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 @@
&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,
)?;
}

Check warning on line 628 in src/runtime/api/bun/js_bun_spawn_bindings.rs

View check run for this annotation

Claude / Claude Code Review

Duplicated process.env-inheritance block in spawn_maybe_sync

The two new no-`env:` fallback blocks (here and at ~lines 909–921 in the no-options branch) are byte-identical: `process_env_object()?` → `ensure_still_alive()` → `if let Some(object) { append_envp_from_js(...) }`. REVIEW.md → "Code style & idioms" says: *"The second time a multi-line block appears in your diff, extract a named helper and use it at EVERY parallel site."* A local helper like `inherit_process_env(global_this, &mut env_array, &mut path, &mut cstr_storage)?` would keep the two branc
Comment thread
robobun marked this conversation as resolved.
Outdated
}

get_argv(
Expand Down Expand Up @@ -894,6 +906,19 @@
}
}
} 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 @@

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
67 changes: 65 additions & 2 deletions test/js/bun/spawn/spawn-env.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,69 @@
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({ stdout, stderr, exitCode }).toEqual({
stdout: "runtime-value,[unset]",
stderr: "",
exitCode: 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({ stdout, stderr, exitCode }).toEqual({
stdout: "true\nfound-the-tool\nfound-the-tool\n",
stderr: "",
exitCode: 0,
});
});
});

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