Fix out-of-bounds read in JS highlighter on trailing backslash in ${}#31435
Conversation
The quick-and-dirty JavaScript syntax highlighter scanned past the end of
its input when an unterminated `${ interpolation ended in a backslash
(e.g. `${\). The inner ${...} scan skipped the byte after a backslash
without checking it was in range, so a trailing backslash pushed the
cursor past text.len(), producing an out-of-bounds slice. In the REPL this
dumped adjacent memory and crashed.
Guard the backslash skip with i + 1 < text.len(), matching the sibling
string-scan loop a few lines below.
|
Updated 8:22 AM PT - May 26th, 2026
✅ @robobun, your commit a42896e8cac3e5d3d7d011d41ce90999063b9175 passed in 🧪 To try this PR locally: bunx bun-pr 31435That installs a local version of the PR into your bun-31435 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe PR fixes an out-of-bounds read bug in the JavaScript syntax highlighter's template-literal scanning logic. Both Rust ( ChangesHighlighter out-of-bounds read fix
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
LGTM — minimal bounds-check fix mirroring the existing guard a few lines below, with regression tests.
Extended reasoning...
Overview
This PR fixes an out-of-bounds read in QuickAndDirtyJavaScriptSyntaxHighlighter when a backslash is the final byte inside an unterminated ${...} template-literal interpolation. The change is a single added condition (i + 1 < text.len()) in the inner backslash-skip branch, applied identically to src/bun_core/fmt.rs and the src/bun_core/fmt.zig porting reference, plus four regression cases in test/js/bun/util/highlighter.test.ts.
Security risks
None introduced. The change strictly tightens a bounds check, eliminating an OOB read/panic. The highlighter is a display-only formatter (REPL line coloring); no auth, crypto, or permission logic is involved.
Level of scrutiny
Low. This is a one-line guard that exactly mirrors the pre-existing pattern in the sibling string-scan loop a few lines below (fmt.rs:1999, fmt.zig same). The fix is mechanical and the reasoning is straightforward: when \ is the last byte, the double-increment pushed i to len+1, breaking the subsequent slice. With the guard, the loop exits at i == len and the slice stays in range. Behavior for all other inputs (backslash followed by content, \\}, \\\\) is unchanged since the guard only suppresses the skip when there's nothing after the backslash.
Other factors
- Regression tests added that crash on the unpatched build and pass on the patched build (verified per PR description).
- No CODEOWNERS entries cover these files.
- No outstanding reviewer comments.
- Bug-hunting system found no issues.
escape-skip-without-bounds-recheck:
In a loop, `if (buf[i] == ESCAPE_CHAR) { i += 1; }` followed by
unconditional `i += 1` without `i + 1 < buf.len` bounds guard.
Catches oven-sh/bun#31435 (fmt.zig JS syntax highlighter OOB read).
recursive-parse-fn-without-stack-check:
Parser/visitor/scanner fn (name contains parse/skip/visit/scan)
calls itself recursively without is_safe_to_recurse() guard.
Catches oven-sh/bun#31361 / #31333 class (stack overflow on
deeply nested TypeScript types / statement blocks).
Both are pure Tier 1 token walks, zero new infrastructure.
…31700) ### Crash Sentry groups BUN-3C97 / BUN-3AA5 report a panic inside `QuickAndDirtyJavaScriptSyntaxHighlighter::fmt` (`src/bun_core/fmt.rs`) while printing an error message: ``` panic: range end index 5 out of range for slice of length 4 ``` Investigating that signature turned up **three** bugs in the highlighter's scan loop — one already fixed, two still live on main and fixed here: 1. **The Sentry signature itself** (`end 5 / len 4`) matches #31434 exactly: a trailing `\` inside an unterminated `` `${ `` (the 4 bytes `` `${\ ``) pushed the cursor to 5. That was fixed by #31435 (May 26); the reported events predate the fix reaching canary users. 2. **`${...}` ending the input** (still crashing on current canary): when a template interpolation ends *exactly* at the end of the input, the string-scan loop exits with `i == 0` and `text` fully consumed. The `redact_sensitive_information` branch then slices `text[1..i]` → `&[][1..0]`: ``` panic: range start index 1 out of range for slice of length 0 ``` 3. **Redacted keyword followed by only whitespace** (found in review): after a `RedactedKeyword` (`token`, `email`, `_auth`, …), the whitespace-skip loop can drain `text` to empty, and control falls through to `match text[0]`: ``` panic: index out of bounds: the len is 0 but the index is 0 ``` ### Repro (current canary, 1.4.0-canary.1) ```sh printf 'logLevel = 3 # `${}\n' > bunfig.toml echo 'console.log("hi")' > index.js FORCE_COLOR=1 bun run index.js # crashes, exit 132 ``` The bunfig type error is printed with `redact_sensitive_information: true` and the offending source line highlighted; the line ends with `` `${} ``, so the highlighter panics *while printing the error message* — the user sees a crash report instead of their config error. Same shape is reachable from `.npmrc` error printing (also redacting). Bug 3 reproduces with `highlightJavaScriptRedacted("token ")` via the testing binding. ### Fix - Guard the `i == 0` case before forming the `inner` slice in the redact branch; `i == 0` can only happen via the interpolation sub-path with an empty remainder, so there is no quoted content to inspect. - Hoist the emptiness check to right after the whitespace-skip loop; the existing check inside the `=`/`:` branch only fired when a separator was present. - Both mirrored into the `.zig` porting reference. Also exposes the redacting highlighter options through the `bun:internal-for-testing` fmt binding (`highlight-javascript-redacted`) so tests can drive the redact code path directly, matching how #31435 tests the non-redacting options. ### Verification - `USE_SYSTEM_BUN=1 bun test test/js/bun/util/highlighter.test.ts` → the end-to-end test crashes with `panic: range start index 1 out of range for slice of length 0` (exit 132), matching the live bug; the redacted-binding tests fail (binding doesn't exist yet). - `bun bd test test/js/bun/util/highlighter.test.ts` → 16 pass, 0 fail. - Fuzzed the patched highlighter through the real binary in both modes (plain + redacting): - exhaustive over all inputs of length ≤ 5 from a 24-char alphabet covering every token class the scanner dispatches on (backtick/quotes, `${`/`}`, `\`, digits, identifier chars, `/*`, `//`, newlines, `=`/`:`, and 2-, 3- and 4-byte UTF-8 chars) — ~8.3M inputs; - keyword-gated paths: every `RedactedKeyword` and the `prev_keyword`-setting syntax keywords as prefixes (optionally inside `` `${ ``), × exhaustive tails of length ≤ 3 (alphabet + `\t`) × keyword-not-at-start variants, plus ~1M keyword-prefixed random inputs up to 33 chars; - ~2.1M additional seeded-random inputs of length 5–30. No panics remain.
Fixes #31434
Repro
Launching the REPL and typing
`${\verbatim (backtick,$,{, trailing backslash) dumps garbage from adjacent memory and segfaults on 1.3.14, and panics on 1.4.0-canary:Node and Deno handle the same input without crashing.
Cause
The
QuickAndDirtyJavaScriptSyntaxHighlighter(src/bun_core/fmt.rs) — used by the REPL to highlight the current line — scans the contents of a${...}interpolation with this loop:When the backslash is the final byte, the skip runs the cursor past the end, and the subsequent slice
&text[curly_start + 2..i](fmt.rs:1970) is out of range. For`${\the bytes are['', '$', '{', '\'](len 4) andi` ends at 5.The old Zig build had no bounds check and read adjacent memory (the "garbage text" in the report) before segfaulting; the Rust port turns the over-read into a panic. Same root cause, both behaviors.
The sibling string-scan loop a few lines below already guards its backslash skip with
i + 1 < text.len()— the${...}loop was missing that guard.Fix
Add the same
i + 1 < text.len()guard to the${...}inner loop so the backslash skip can't push the cursor past the end. When the backslash is the last byte it's no longer skipped, the loop exits withi == text.len(), and the slice stays in range. All other inputs (backslash followed by more content,\},\\) are unchanged. Mirrored into the.zigporting reference.Verification
Regression cases added to
test/js/bun/util/highlighter.test.ts, exercising the highlighter viahighlightJavaScriptfrombun:internal-for-testing(same options the REPL uses).USE_SYSTEM_BUN=1 bun test test/js/bun/util/highlighter.test.ts→ crashes (exit 132,range end index 5 out of range for slice of length 4— matches the issue's crash report).bun bd test test/js/bun/util/highlighter.test.ts→ 5 pass, 0 fail.