shell: expand $RANDOM, $UID, $PPID, $SECONDS, $IFS, $OSTYPE and friends - #34905
shell: expand $RANDOM, $UID, $PPID, $SECONDS, $IFS, $OSTYPE and friends#34905robobun wants to merge 7 commits into
Conversation
Bash's dynamic / auto-initialized shell variables ($RANDOM, $UID, $EUID, $GROUPS, $PPID, $SECONDS, $LINENO, $IFS, $SHLVL, $HOSTNAME, $OSTYPE) were being looked up as ordinary unset environment variables and silently expanding to the empty string. The ubiquitous temp-name idiom 'n=tmp.$RANDOM.$RANDOM' therefore yielded the constant 'tmp..' on every run, and '[ "$UID" = 0 ]' root checks were always false. Add a fallback in variable expansion: when a $NAME misses both shell_env and export_env, ShellExecEnv::append_special_var computes the value for the known special names. A user assignment (which lands in one of the env maps) still shadows the fallback. $RANDOM draws a fresh 15-bit value from bun_core::fast_random on every reference; $SECONDS reports whole seconds since the interpreter was constructed (start_instant is carried into subshells so a subshell sees the parent's elapsed time, matching bash); $UID/$EUID/$GROUPS use getuid/geteuid/getgid on POSIX; $PPID uses getppid/uv_os_getppid; $HOSTNAME uses gethostname/uv_os_gethostname; $OSTYPE is the target-triple-derived string bash uses; $IFS is the POSIX default ' \t\n'; $SHLVL defaults to 1; $LINENO is 0 (source positions are not tracked through expansion).
WalkthroughChangesShell environments preserve interpreter startup time and expand Bash-like dynamic, static, and platform-specific variables when environment lookups do not provide values. Windows hostname retrieval is centralized, and shell tests cover defaults, overrides, and platform-specific variables. Special shell variables
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:45 AM PT - Jul 21st, 2026
❌ @robobun, your commit 2991dce has 2 failures in
Add 🧪 To try this PR locally: bunx bun-pr 34905That installs a local version of the PR into your bun-34905 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this adds user-facing shell semantics (11 new special variables with bash-compat value choices like $OSTYPE→msys on Windows, $GROUPS→getgid(), $LINENO→0), so it warrants a maintainer sign-off on the API surface.
What was reviewed
- Fallback ordering: user assignments in
shell_env/export_envare consulted first, soRANDOM=xstill shadows the dynamic value; unknown vars still expand to"". start_instantis set at bothShellExecEnvconstruction sites and copied (not reset) indupe_for_subshell.- Helper existence:
bun_core::fast_random,bun_core::slice_to_nul,bun_sys::c::getuid/geteuid/getgid,bun_sys::posix::gethostname, andbun_libuv_sys::uv_os_getppid/gethostnameall resolve;bun_libuv_sysis already a dep of theruntimecrate. - Test flakiness: the
$RANDOMuniqueness assertions have collision probability ≈2⁻³⁰.
Extended reasoning...
Overview
Adds a fallback in Expansion::expand_simple_no_io for SimpleAtom::Var: after shell_env and export_env both miss, ShellExecEnv::append_special_var computes bash's dynamic/auto-initialized variables ($RANDOM, $SECONDS, $IFS, $SHLVL, $LINENO, $OSTYPE, $HOSTNAME, $PPID, and on POSIX $UID/$EUID/$GROUPS). A new start_instant: Instant field on ShellExecEnv backs $SECONDS and is copied into subshells. 12 new tests in bunshell.test.ts strip the names from the inherited env and assert the fallback values.
Security risks
None identified. The values come from process-local syscalls (getuid, getppid, gethostname) and a non-crypto PRNG that bash's $RANDOM also uses. No untrusted input flows into buffer sizing; the hostname buffers are fixed-size stack arrays with the OS writing at most len bytes.
Level of scrutiny
Medium-high. The implementation itself is straightforward and localized (a single match arm plus one new method), but it is new user-facing API surface for Bun Shell with several bash-compat judgment calls: $OSTYPE values (darwin without a version suffix, msys on Windows), $GROUPS returning getgid() rather than the getgroups() array, $LINENO stubbed to 0, and $IFS being reported as the POSIX default even though Bun Shell's field-splitting is hardcoded. These are all defensible defaults, but per the repo's API-design guidance a maintainer should confirm them before they become observable behavior scripts depend on.
Other factors
- Only two
ShellExecEnvstruct-literal sites exist (Interpreter::newanddupe_for_subshell); both receivedstart_instant, so there's no missing initializer. - Only one
SimpleAtom::Varexpansion site exists in the shell states, so there's no sibling path that misses the fallback. - Tests look non-flaky (the
$RANDOMdistinct-draws assertion has ~2⁻³⁰ collision probability) and correctly strip inherited env entries so the fallback path is what's exercised. The$UID/$EUID/$GROUPStest is POSIX-gated to match the#[cfg(unix)]arms. - No prior human review on the timeline; CI is still building.
|
Rationale for the bash-compat choices flagged above, for whoever signs off:
All of these only apply when the name is absent from both |
uv_os_gethostname was returning an error on the Windows CI lanes, leaving $HOSTNAME empty. Use the same GetHostNameW-then-WSAStartup retry path that node_os::hostname uses, which is what os.hostname() (and therefore the test's expected value) runs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/shell/bunshell.test.ts`:
- Around line 1392-1394: Strengthen the assertion in the "$LINENO expands to a
number" test to require the documented exact output "0" instead of matching any
digit string. Keep the existing command execution and environment setup
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bbd88378-786a-4fcc-820a-2cd183c19f86
📒 Files selected for processing (3)
src/runtime/shell/interpreter.rssrc/runtime/shell/states/Expansion.rstest/js/bun/shell/bunshell.test.ts
…into bun_sys convert_utf16_to_utf8_append requires the caller to pre-reserve the simdutf output length; it was being handed Expansion::current_out, which for a bare $HOSTNAME reference is a zero-capacity Vec, so simdutf wrote the hostname to the 0x1 dangling spare pointer and segfaulted on all three Windows CI lanes. Move the GetHostNameW/WSAStartup retry into bun_sys::windows::gethostname_w and call it from both the shell fallback and node_os::hostname so the winsock dance has one implementation, and transcode with the reserving to_utf8_append_to_list instead of the raw append helper.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/shell/interpreter.rs`:
- Around line 2256-2257: Update the hostname expansion logic around
bun_sys::windows::gethostname_w so that a None result appends the same "unknown"
fallback used by node_os.rs. Preserve the existing UTF-8 hostname handling when
retrieval succeeds, ensuring $HOSTNAME always produces a value.
In `@src/sys/windows/mod.rs`:
- Around line 3443-3448: The GetHostNameW retry in the surrounding hostname
lookup must only run when WSAGetLastError() reports WSANOTINITIALISED. Track
whether WSAStartup succeeded, perform the retry only in that case, and call
WSACleanup after a successful startup regardless of the retry result; preserve
the existing behavior for other GetHostNameW failures.
- Around line 3439-3440: Update the GetHostNameW buffer-capacity handling around
the existing debug_assert: return None when buf.len() is less than 2, then
convert the full buf.len() to c_int with try_from and propagate conversion
failure via ?. Remove the -1 subtraction while preserving the existing hostname
retrieval flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac181139-3dc4-49ea-b59c-5eb9ae2c1906
📒 Files selected for processing (3)
src/runtime/node/node_os.rssrc/runtime/shell/interpreter.rssrc/sys/windows/mod.rs
namelen counts WCHARs including the NUL, and MSDN bounds the result at 256. The -1 reserved slot was carried over from the pre-extraction node_os.rs code and made a 256-element buffer report WSAEFAULT on a 256-character hostname.
The -> bool on append_special_var was never read by the sole caller;
drop it rather than documenting a contract nothing observes.
The $SECONDS test asserted .toBe("0"), i.e. that <1s of wall clock
elapses between Instant::now() and expansion. Nothing structurally
guarantees that on a loaded debug+ASAN runner, so assert the durable
contract (non-negative integer, bounded small) instead.
|
CI status as of 2991dce (build 76811, finished):
Ready for review. |
There was a problem hiding this comment.
No issues found — the earlier nits (dead -> bool return, $SECONDS timing assertion, gethostname_w doc/namelen) are all addressed. Deferring to a human for sign-off since this adds user-facing shell semantics (11 new special variables with bash-compat value choices like $OSTYPE="msys" and the $LINENO=0 placeholder) and refactors the shipping Windows os.hostname() path.
What was reviewed:
- Env-map lookup order in
expand_simple_no_io— user assignments still shadow the fallback; unknown names still expand to"". start_instantis threaded through both theInterpreter::initconstructor anddupe_for_subshell, so$SECONDSin a subshell sees the parent's clock.- The
node_os::hostnameWindows refactor is behavior-preserving modulo the 130→256 buffer bump (which is the MSDN-documented size);slice_to_nul_u16is correctly deleted with its last caller. - Test env strips every name under test from
bunEnvso the fallback path is what's exercised; the$RANDOMcollision assertions have ~2⁻³⁰ false-positive probability.
Extended reasoning...
Overview
Adds a fallback in Bun Shell's variable expansion for bash's dynamic/auto-initialized variables ($RANDOM, $SECONDS, $IFS, $SHLVL, $LINENO, $OSTYPE, $HOSTNAME, $PPID, and on POSIX $UID/$EUID/$GROUPS). The fallback runs only after both shell_env and export_env miss, so user assignments continue to shadow it. Also extracts the Windows GetHostNameW + WSAStartup-retry sequence from node_os::hostname into a shared bun_sys::windows::gethostname_w helper (now used by both os.hostname() and $HOSTNAME), and adds a start_instant: Instant field to ShellExecEnv for $SECONDS. ~130 net lines of Rust across four files plus a 12-test describe block.
Security risks
None identified. The new code only reads process/host metadata via existing bun_sys::c/bun_sys::posix/libuv wrappers (getuid, geteuid, getgid, getppid, gethostname, uv_os_getppid) and bun_core::fast_random. No user-controlled input reaches a syscall, allocation size, or path; the match on label is a fixed byte-string dispatch.
Level of scrutiny
Medium-high. This is a real feature touching the shell interpreter's core variable-expansion path and changing user-visible semantics for 11 variable names, plus a refactor of shipping Windows node:os code. It's not a mechanical change — there are bash-compat design choices a maintainer should ratify (e.g. $OSTYPE="msys" on Windows vs. cygwin/win32; $GROUPS = getgid() only rather than the full supplementary-group array; $LINENO=0 as a placeholder). The implementation itself is straightforward and well-tested, but per the approval bar this is beyond "simple, mechanical, or obvious".
Other factors
All prior review threads are resolved: CodeRabbit's namelen off-by-one was fixed in a8e3a28; its WSACleanup and "unknown"-fallback suggestions were withdrawn as intentional behavior-preservation; my own three nits (doc-comment wording, dead return type, $SECONDS flake bound) were applied in 2d04246 and 2991dce. The bug-hunting pass on the current head found nothing. robobun reports the full bunshell.test.ts suite green on all lanes including Windows. Test coverage is thorough — every new variable is asserted, the shadowing contract is tested, and the tmp.$RANDOM.$RANDOM motivating idiom is covered directly.
1 participant
Problem
Bash's dynamic and auto-initialized shell variables are looked up as ordinary unset environment variables in Bun Shell and silently expand to the empty string:
So the
tmp.$RANDOM.$RANDOMuniqueness idiom collides on every run,[ "$UID" = 0 ]root checks are always false,$SECONDStiming is empty, and so on. (This is the named-variable half of the special-parameter gap;$?/$$/$#are being handled separately in #33274.)Cause
Expansion::expand_simple_no_ioresolvesSimpleAtom::Varasshell_env.get(label) → export_env.get(label) → ""with no special-name fallback, so any name not present in the environment becomes the empty string.Fix
After both env-map lookups miss, fall through to a new
ShellExecEnv::append_special_varthat computes the value for the known special names. A user assignment still shadows the fallback because it lands inshell_env/export_env, which are consulted first.$RANDOMbun_core::fast_randomon every reference$SECONDSstart_instantis carried into subshells so$(... $SECONDS ...)sees the parent's elapsed time, matching bash$IFS" \t\n"$SHLVL1when not inherited from the environment$LINENO0(source positions are not tracked through expansion)$OSTYPElinux-gnu,linux-musl,darwin,msys, ...)$HOSTNAMEgethostname(2)/uv_os_gethostname$PPIDgetppid(2)/uv_os_getppid$UID/$EUID/$GROUPSgetuid/geteuid/getgid(POSIX only)Verification
New
special shell variablesdescribe block intest/js/bun/shell/bunshell.test.tsstrips every name under test from the inherited environment and asserts the fallback values, including that$RANDOMdiffers across references, that thetmp.$RANDOM.$RANDOMidiom yields distinct names across runs, that$HOSTNAMEmatchesos.hostname()and$PPIDmatchesprocess.ppid, and that a user assignment shadows the fallback. 10 of the 12 new tests fail on the released binary and all pass on this change; the fullbunshell.test.tssuite stays green.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/bun/shell/bunshell.test.ts