Conversation
A .env value that nests one substitution inside another's :- default,
such as FOO="${FOO:-${BAR:-baz}}", crashed Bun on startup.
The value expander scanned backwards and spliced each expansion to the
front of a buffer, tracking the processed-tail boundary in `last`. It
did not account for a nested ${...} inside a default: the inner match
set `last` to its own position, then the outer match's forward `end`
scan overran that boundary, producing an inverted value[end..last]
range (a checked-slice panic in the Rust build, a segfault on release).
Rewrite the expander as a forward scan that expands nested defaults
recursively, with a depth cap so a pathologically nested value cannot
overflow the stack.
|
Updated 4:16 PM PT - Jun 16th, 2026
❌ @robobun, your commit 4a5d786 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 32413That installs a local version of the PR into your bun-32413 --bun |
WalkthroughThe dotenv variable expansion logic in ChangesDotenv nested default expansion fix
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The brace-balance scanner for a braced ${VAR:-default} broke on `\`
unconditionally, so a `\$` escape inside a nested default aborted the
scan with inner braces still open: ${FOO:-${BAR:-a\$b}} expanded to
a$b}} instead of a$b. Drop the backslash handling from the brace
scanner and let the recursive expansion resolve the escape, which also
removes the stray `}` the non-nested ${FOO:-a\$b} produced.
![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 `@src/dotenv/env_loader.rs`:
- Around line 1192-1268: In the expand_into method, the local variable declared
as `let mut depth = 0usize;` on line 1226 shadows the function parameter `depth:
u32` used for tracking recursion depth. Rename this local variable to a more
descriptive name like `brace_depth` or `nest_level` to eliminate the shadowing
and improve code clarity. Update all references to this local variable within
the brace-balancing while loop to use the new name consistently.
🪄 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: 8a24eb86-540f-4caa-8998-fcb54d95bb1c
📒 Files selected for processing (2)
src/dotenv/env_loader.rstest/cli/run/env.test.ts
The `\$` escape carried an `i + 1 != n - 1` guard so a `$` in the final
byte was left untouched. With recursive default expansion, `n` is the
length of the default sub-slice, so the guard misfired at the end of a
default: `${FOO:-a\$b}` yielded a$b but `${FOO:-a\$}` yielded a\$. Drop
the guard so `\$` always resolves to a literal `$` regardless of
position; a bare trailing `$` (no name) stays literal via the
substitution guard.
The brace-balance scanner counted every `{`, so a bare unbalanced `{`
in a `:-` default shifted which `}` closed the substitution:
`${FOO:-{}` expanded to `{}` instead of `{` (bash yields `{`). Count a
`{` only when it follows `$` (a nested `${`); a bare `{` stays literal.
Balanced literal braces (e.g. `${URL:-http://x/{id}}`) are unchanged.
|
CI status: the changed files (
None of these touch |
The brace-balance scanner detected a nested `${` by looking back one
byte for `$`, which could not tell an escaped `\$` apart from a real
substitution opener: `${FOO:-\${}` expanded to `${}` instead of `${`.
Scan forward instead, consuming a `\$` escape and a `${` opener as
units, so only an unescaped `${` opens a scope. Bare `{`, `\}`, and
balanced literal braces are unchanged.
There was a problem hiding this comment.
All four edge cases I flagged are now addressed with regression tests and the latest revision looks correct to me — deferring to a maintainer for final sign-off since this is a full rewrite of the .env value-expansion parser (runs on every bun run with a .env present) and intentionally changes a couple of escape-handling behaviors (e.g. top-level trailing \$ now resolves to $).
Extended reasoning...
Overview
This PR rewrites Parser::expand_value / adds expand_into in src/dotenv/env_loader.rs (~100 lines of new logic replacing ~60), switching from a backward-scan + splice-at-front expander to a forward-scan recursive one, and adds three regression tests in test/cli/run/env.test.ts. It fixes a real crash (#32411): nested ${FOO:-${BAR:-baz}} in a .env file panicked with an inverted slice (segfault in release).
Over the course of review I raised four inline edge cases (backslash breaking nested brace balance; trailing-\$-in-default inconsistency; bare { counted as a scope opener; escaped \${ counted as a scope opener). Each was fixed in a follow-up commit with a regression test, and the final brace-balance scanner (forward-consuming \$ and ${ as units) is cleaner than the look-back chain it replaced. The bug-hunting pass on 4a5d786 found nothing further.
Security risks
None identified. .env parsing reads local files into the process's own environment map; there's no injection surface, no privilege boundary crossed, and no network/auth path touched. The recursion is depth-capped at 64 and the input is the user's own .env file.
Level of scrutiny
Medium-high. The function itself is small and self-contained, but it runs on effectively every bun run/bun test invocation that has a .env file, so a regression here has wide reach. The four rounds of edge-case iteration during review show the escape/brace interaction is subtle. The PR also makes a couple of intentional behavior changes beyond the crash fix — most notably 5bbda60 drops the "trailing \$ stays literal" special case so a top-level x\$ now becomes x$ instead of x\$. That's arguably more correct/consistent, but it's a user-visible change a maintainer should be aware of and accept.
Other factors
- All review threads (mine and CodeRabbit's) are resolved.
- CI on the changed files is green; the red lanes in builds 62883/62896 are unrelated Docker-unavailable / RSS-threshold flakes per the author's CI summary.
- Existing
.env value expansionand.env escaped dollar signtests still pass alongside the new ones, and the new tests cover every edge case raised in review. - I'm not approving outright because this is a parser rewrite on a hot path with deliberate semantic tweaks, which is past my bar for "a human does not even need to look."
|
Superseded by #36199, which follows the depth-counting spec and applies cleanly on current main. |
1 participant
Fixes #32411
Repro
Expected
hello/ exit 0. A.envvalue that nests one substitution inside another's:-default crashes Bun before the script runs. On macOS the same logic error surfaces asSegmentation fault at address 0x240913FFFEA.Cause
Parser::expand_valueinsrc/dotenv/env_loader.rsscanned the value backwards, splicing each expansion to the front of a buffer and tracking the processed-tail boundary inlast. For${FOO:-${BAR:-baz}}the inner${BAR:-baz}is matched first and setslastto its own offset. The outer${FOO:-...}then scans its default forward, and because the default scan stopped at the first}(ignoring the nested braces) itsendoverranlast. The resultingvalue[end..last]splice hadstart > end, an inverted range: a checked-slice panic in the Rust build, UB/segfault in release.The backward-scan + splice-at-front design assumes left-to-right-disjoint matches, which nesting violates.
Fix
Rewrite the expander as a forward scan (
expand_into) that expands a nested${...}inside a:-default recursively, matching braces so the outer substitution's extent is computed correctly. Plain$VAR,${VAR},${VAR:-default}, and unknown-key behavior is preserved. A depth cap keeps a pathologically nested value from overflowing the stack, and a fast path skips values with no$.Behavior notes
The crash fix aside, escape/brace handling now follows bash consistently. One case is a user-visible change for input that previously did not crash:
\$whose$is the final byte of a value or default now resolves to a literal$, same as\$anywhere else. Before, such a trailing\$was left as\$(an artifact of the old scanner never visiting the last byte). Example:FOO=x\$now yieldsx$(previouslyx\$).The remaining refinements only affect input that previously crashed or was malformed:
\$inside a nested default resolves correctly, a bare{in a default stays literal (${FOO:-{}->{), and an escaped\${is not treated as a nested substitution (${FOO:-\${}->${). All match bash.Verification
Tests in
test/cli/run/env.test.ts:.env nested default substitution does not crash (#32411)runs the exact report and asserts exit 0..env nested default substitution resolves (#32411)asserts${NE_UA:-${NE_UB:-deep}}->deep, inner-resolves -> the set value, triple nesting ->z, and the bare/balanced brace cases above..env escaped $ inside a default resolves (#32411)covers\$mid-default, in a nested default, trailing, top-level, and the escaped\${case.The crashing cases fail on the current release binary (the panic above) and pass with this change. Existing
.env value expansionand special-character (escaped$) coverage still passes.