process: back main-thread process.env by live libc environ on POSIX - #35270
process: back main-thread process.env by live libc environ on POSIX#35270robobun wants to merge 9 commits into
Code review found 2 important issues
Found 5 candidates, confirmed 4. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 2 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/runtime/api/BunObject.rs:2389-2416 |
Data race: setOSEnv/unsetOSEnv mutate env_map without proxy_env_storage lock |
| 🔴 Important | src/jsc/bindings/JSEnvironmentVariableMap.cpp:762-771 |
BunString ref leak on every process.env read (POSIX main thread) |
| 🟡 Nit | test/js/node/process/process.test.js:254-259 |
TZ assertion now live and fails on non-UTC dev machines |
| 🟡 Nit | src/runtime/api/BunObject.rs:2385-2394 |
setenv() failure ignored: env_map and environ diverge on '=' in key / NUL / ENOMEM |
Annotations
Check failure on line 2416 in src/runtime/api/BunObject.rs
claude / Claude Code Review
Data race: setOSEnv/unsetOSEnv mutate env_map without proxy_env_storage lock
`Bun__Process__setOSEnv` / `Bun__Process__unsetOSEnv` mutate `vm.transpiler.env_mut().map` without holding `vm.proxy_env_storage.lock()`, which `rare_data.rs:389-395` documents as the env-map serialisation point (and which the sibling `Bun__setEnvValue` at line 2262 takes for exactly this reason). A main-thread `process.env.X = ...` now races `WebWorker::start_vm()`'s `clone_with_allocator()` iteration of the same map on the worker thread — a `put()` rehash mid-iteration is a UAF. Wrap the `env_
Check failure on line 771 in src/jsc/bindings/JSEnvironmentVariableMap.cpp
claude / Claude Code Review
BunString ref leak on every process.env read (POSIX main thread)
`Bun__Process__getOSEnv` writes an owned `BunString` (`clone_utf8` → WTFStringImpl at refcount 1), but `value.toWTFString(BunString::ZeroCopy)` **refs** the impl rather than adopting it, and the stack `BunString` POD has no destructor — so every `process.env.X` read on the POSIX main thread leaks one `StringImpl`. The same pattern occurs at `existing.toWTFString(BunString::ZeroCopy)` in `defineOwnProperty`. Use `value.transferToWTFString()` (or an explicit `value.deref()` after building the JSSt
Check warning on line 259 in test/js/node/process/process.test.js
claude / Claude Code Review
TZ assertion now live and fails on non-UTC dev machines
This branch was previously dead code (`'TZ' in process.env` was always `true` on POSIX because the snapshot object unconditionally installed a TZ CustomAccessor); it's now live and will spuriously fail on any local dev machine whose system timezone isn't UTC. `test/preload.ts` explicitly skips `TZ` when copying `bunEnv` into `process.env`, so a macOS/Linux laptop with no `TZ` in the shell and `/etc/localtime → America/Los_Angeles` hits `expect(['Etc/UTC','UTC']).toContain('America/Los_Angeles')`
Check warning on line 2394 in src/runtime/api/BunObject.rs
claude / Claude Code Review
setenv() failure ignored: env_map and environ diverge on '=' in key / NUL / ENOMEM
The return value of `libc::setenv()` is discarded and `env_map.put()` runs unconditionally afterward, so when `setenv` fails (POSIX returns `EINVAL` for a name containing `=`, or `ENOMEM`) `environ` is unchanged but Bun's DotEnv map IS updated — the exact JS↔native split-brain this PR exists to eliminate. Checking the `setenv` return and skipping `env_map.put` on failure (and doing the same in `Bun__Process__seedOSEnvFromMap`) closes the gap; the embedded-NUL early return similarly reports succe