process.env: Node-semantics exotic object on POSIX; coerce/validate/setenv-sync, first-wins dup load, typed-cache invalidate - #35882
Conversation
The existing createEnvironmentVariablesMap builds a plain JSObject and installs a CustomGetterSetter per startup key with a null setter (CustomValue semantics since #20527), so the first write clobbers the accessor into a raw data property and jsSetterEnvironmentVariable is dead code. Brand-new keys were always plain data properties (the container has no OverridesPut), so Node's process.env contract (ToString coercion, Symbol key/value TypeError, '='/empty-key drop, NUL truncation, accessor/ partial defineProperty rejection, setenv write-through) was never enforced. Replace the POSIX process.env with a JSNonFinalObject subclass (JSProcessEnvMap) backed by the env_loader map: * put(): ToString-coerce, throw on Symbol key/value, silently drop '='/empty keys, NUL-truncate key and value, then update the env_loader map and (main thread only) setenv() + invalidate the typed env_var cache so os.homedir()/tmpdir() pick up the change. * deleteProperty(): remove from the map and (main thread only) unsetenv() + invalidate the typed cache. * defineOwnProperty(): accept only a fully-permissive data descriptor (Node's ERR_INVALID_OBJECT_DEFINE_PROPERTY otherwise) and route it through put(). * preventExtensions(): return false so Object.freeze/seal/ preventExtensions throw. * getOwnPropertySlot/getOwnPropertyNames: read the env_loader map. On Windows the existing Proxy stays; its set/defineProperty traps now apply the same '='/empty-key drop and defineProperty validation, and a preventExtensions trap is added so freeze/seal throw there too. load_process: duplicate environ keys now resolve to the FIRST occurrence (matching libc getenv and Node) and entries without '=' are dropped instead of fabricated as KEY="". env_var: add a per-var reset() and invalidate_for_setenv(key) so a process.env write to HOME/PATH/USER/TMPDIR/etc. unstales the typed accessor before the next os.* read. web_worker: set the worker transpiler's env behavior to LoadAllWithoutInlining (as run/test/repl already do). With main-thread setenv sync, a main-thread process.env write now reaches the worker's environ and was being inlined into worker source as a string literal. Tests: test/js/node/process/process-env-exotic.test.ts covers the 11-cell contract (coercion, Symbol validation, '='/empty/NUL handling, freeze/ defineProperty refusal, setenv/unsetenv via FFI getenv, os.homedir cache, first-wins dup, no-'=' drop). worker_threads/process tests updated for the defineProperty-accessor rejection and the previously dead 'TZ in process.env' branch.
|
Warning Review limit reached
Next review available in: 9 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 (13)
Comment |
|
Updated 9:14 AM PT - Jul 26th, 2026
❌ @robobun, your commit 6130d1f has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35882That installs a local version of the PR into your bun-35882 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…eanup, Symbol value in Windows defineProperty trap
…alue) musl setenv()/__putenv frees the previous setenv-allocated string via __env_rm_add; glibc leaks it. A &'static [u8] cached from getenv_z after a runtime setenv could dangle on musl after a second overwrite. Instead of reset()+re-read-getenv, invalidate_for_setenv now takes the new value and each var's set_owned() leaks a Box<[u8]> copy so the cache never holds a post-startup environ pointer. SAFETY comments in get_cached and getenv_z corrected. [skip size check]
|
Status at 5143c3e: 21/21 new test cells pass locally (3/21 on the released build, all of which exercise plain-object semantics that already worked; the file as a whole fails without the src/ changes). Review threads from the automated reviewers are all addressed (seqlock on the split-word CI builds 82232/82244 failed only on |
A second set_owned overwrites ptr_value, making the previous leaked Box unreachable to LSAN (which then SIGABRTs the test process on the asan lane). The leak is intentional: values are short paths and process.env.HOME/TMPDIR/... writes are rare. Add __lsan_ignore_object to bun_core::asan and call it on the leaked allocation. [skip size check]
…yncOSEnv doc Clearing the slot under the lock already held means a worker spawned after delete process.env.HTTP_PROXY no longer re-inserts the stale value via sync_into. syncOSEnv doc updated: on POSIX a worker write to a main-rooted tree only reaches the shared store (Bun__ProcessEnv__put still gates setenv on is_main_thread()); Windows is unchanged. [skip size check]
…et(); Windows deleteProperty Symbol handling string::Cache::deser_and_invalidate now takes &[u8] and leaks a Box<[u8]> copy before storing, so both the initial get_force_reload() path and the process.env write path cache Bun-owned bytes. An off-thread first read racing a main-thread setenv could previously cache the post-setenv musl-allocated string, which a later setenv frees. set_owned() is now a thin deser_and_invalidate wrapper. reset() (all four) removed as dead. Stale seqlock-struct and SAFETY comments updated. Windows Proxy deleteProperty: early-return true on a Symbol key instead of String(sym).toUpperCase() -> spurious SetEnvironmentVariableW + strict- mode TypeError; matches Node and the new POSIX deleteProperty. [skip size check]
…e-scan; document previous-Box leak getenv_z/getenv_z_any_case now leak a Box<[u8]> copy of the value while holding the read guard, so the returned &'static [u8] is valid regardless of a later setenv. Previously the guard dropped on return and the caller's Box::from memcpy ran unlocked; on musl a second main-thread setenv could free the source between the two. deser_and_invalidate reverts to storing the passed-in &'static directly; set_owned leaks before calling it and documents why the previous cached slice is never freed. web_worker.rs: set did_load_process=true on the cloned loader. The cloned map already has the parent's environ snapshot; re-walking __environ on the worker OS thread (via bun_sys::environ, which does not take ENVIRON_LOCK) raced a main-thread process.env write's setenv. [skip size check]
|
Status at b776d54: all automated-review memory-safety findings addressed. Summary of the hardening beyond the original design:
21/21 new test cells pass locally (3/21 on the released build; file as a whole fails without comment-cop continues to flag every multi-line doc/SAFETY comment; those threads are resolved without change since the comments document lock/seqlock/ownership invariants. |
…tform_get Closes the lost-update window where an off-thread first read completes getenv_z (with the pre-write value), main runs set_owned with the new value, then the off-thread deser_and_invalidate overwrites it with the stale copy. Correctness-only after b776d54 (both slices are Bun-owned). [skip size check]
2 participants
What
Replace the POSIX
process.envplain object with an exoticJSProcessEnvMapbacked by the env_loader map, matching Node'sRealEnvStorecontract. On Windows the existing Proxy gains the same validation.Why
createEnvironmentVariablesMapbuilds a plainobjectPrototypeobject; each startup key getsCustomGetterSetter::create(vm, jsGetterEnvironmentVariable, nullptr)underCustomValue, so the first write clobbers the accessor into a raw data property andjsSetterEnvironmentVariablehas zero live references (dead since #20527). Brand-new keys are plain data properties because the container has noOverridesPut, so Node's contract was never applied to any key.Node v26 contract (verified locally):
Duplicate
KEY=entries inenvironresolve to the first occurrence (matches libcgetenv()and Node); an entry without=is dropped (Node:'FOOBAR' in process.env === false).How
JSEnvironmentVariableMap.cpp: newJSProcessEnvMap : JSNonFinalObject(POSIX only) withOverridesPut | OverridesGetOwnPropertySlot | OverridesGetOwnPropertyNames | ProhibitsPropertyCaching:put: throw on Symbol key/value; NUL-truncate key and value; drop=/empty keys;applySharedEnvSideEffects(TZ/TLS/verbose/proxy);Bun__ProcessEnv__put.deleteProperty:Bun__ProcessEnv__delete.defineOwnProperty: only fully-permissive data descriptors (elseERR_INVALID_OBJECT_DEFINE_PROPERTY), routed throughput.preventExtensions: returnfalse→ freeze/seal/preventExtensions throw.getOwnPropertySlot/getOwnPropertyNames: read the env_loader map.The Windows
#elsekeeps the existing backing object +windowsEnvProxy; inner#if OS(WINDOWS)blocks inside that branch collapsed.BunObject.rs:Bun__ProcessEnv__put/Bun__ProcessEnv__delete: updateenv_mut().map, and on the main thread alsolibc::setenv/unsetenv+env_var::invalidate_for_setenv(key).env_var.rs: addreset()to each cache kind and expose it on the macro-generated module;invalidate_for_setenv(key)matchesHOME/PATH/USER/TMPDIR/TEMP/TMP/SHELL/XDG_*and resets. Without thisos.homedir()keeps returning the first-read value afterprocess.env.HOME = ....env_loader.rsload_process: first-wins for duplicate keys (while still letting pre-seeded entries likebun test'sNODE_ENVbe overwritten by environ); drop no-=entries.web_worker.rs: set the worker transpiler'senv.behavior = LoadAllWithoutInlining(asrun/test/replalready do). With main-threadsetenvsync a main-thread write now reaches the worker'senviron, which the worker's transpiler was then inlining into source as a string literal.ProcessObjectInternals.ts(Windows Proxy):setanddefinePropertythrow on Symbol, drop=/empty keys,definePropertyrejects accessor/partial descriptors and routes through the coercing setter;preventExtensions/isExtensibletraps added.Verification
test/js/node/process/process-env-exotic.test.ts(17 cells: coercion, Symbol key/value,=/empty/NUL, freeze/defineProperty refusal, delete, spawn-poison, FFIgetenvsync for set/delete,os.homedir()cache invalidation, execve'd duplicate-key / no-=launchers). 0/17 pass on the released build; 17/17 pass with this change (10/10 on Windows with 7 POSIX-only cells skipped).Existing
process.test.js,worker_threads.test.ts,env.test.ts,garbage-env.test.ts,spawn-env.test.tsremain green; two test expectations updated where the previously-dead'TZ' in process.envbranch is now reachable and where accessordefinePropertyis now rejected.Related
Subsumes #34727, #34728, #35264, #35877, #35879 and the
web_worker.rsinlining fix from #35270. Differs from #35270 in that reads go through the env_loader map rather than livegetenv().no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/process/process-env-exotic.test.ts test/js/node/process/process.test.js
Known exclusions
new Worker(url, {env: SHARE_ENV})), main'''sprocess.envis swapped toJSSharedEnvMap, which does not yet apply the Symbol-throw /=-empty-key drop / accessor-definePropertyreject /preventExtensionscontract (the OS side is safe:syncOSEnv→Bun__ProcessEnv__putfilters and truncates). Node keeps main onRealEnvStoreafter founding a tree.delete process.env.TZ(andNODE_TLS_REJECT_UNAUTHORIZED/BUN_CONFIG_VERBOSE_FETCH) does not yet clear the corresponding native side-effect cache;putapplies it viaapplySharedEnvSideEffectsbutdeletePropertyhas no delete-side variant. Not a regression (pre-PR delete had no side effect either).cron.rs:2239readsc_environ()withoutENVIRON_LOCKon the POSIX worker path; switching it tocreate_null_delimited_env_map()(matching the Windows arm andBun.spawn) is the follow-up.getenv_znow leaks a small LSAN-ignored Box per call; splitting into a private leaking variant for theenv_varcache plus a publicOption<Box<[u8]>>for the ~20 other callers would narrow the LSAN suppression scope.new Worker(url, {env: {...}})still gets a plainconstructEmptyObjectas itsprocess.env(set directly inZigGlobalObject.cppinitializeWorker, bypassingcreateEnvironmentVariablesMap), so the exotic contract does not apply there. Not a regression; needs a separate in-memory backing store to match Node'sMapKVStore.Fixes #34210
Fixes #29244