Skip to content

shell: only treat digits as redirect fd numbers at word boundaries - #33997

Closed
robobun wants to merge 6 commits into
mainfrom
claude/farm/58e239bc/shell-redirect-fd-word-boundary
Closed

robobun wants to merge 6 commits into
mainfrom
claude/farm/58e239bc/shell-redirect-fd-word-boundary

Conversation

@robobun

@robobun robobun commented Jul 12, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes two related bugs in the Bun Shell lexer's handling of digit-prefixed redirects.

Reproduction

import { $ } from "bun";
await $`echo z1>f`;        // writes "z"   (bash writes "z1")
await $`echo z2>f`;        // writes ""    (bash writes "z2"; stdout went to terminal)
await $`echo z 3> f`;      // writes "z 3" (bash writes nothing; 3> is an fd redirect)
await $`echo z 10> f`;     // ENOENT       (bash: fd 10 redirect)

All four cases are silent: exit code 0 with plausible-looking output.

Cause

In src/shell_parser/parse.rs, the lexer's digit case called eat_redirect on any unquoted digit with no word-boundary check. For echo z1>f, the z was already buffered when 1 arrived; eat_redirect saw 1> and emitted a stdout redirect, so break_word flushed only z as the argument and the trailing 1 was lost. eat_redirect also only mapped 0, 1, and 2 to fds; any other digit returned None, so a standalone 3> backtracked and the 3 became an ordinary argument, and 10> split into argument 1 plus a 0> redirect.

POSIX 2.7 says the fd number applies only when the digit string is its own word immediately preceding the redirect operator.

Fix

  • Gate the fd parse on the digit starting a fresh word: word_start == j and the previous token is not word-continuing (Var, quoted text, CmdSubstEnd, etc.). Mid-word digits fall through as ordinary text, so z1>f now lexes as z1, >, f.
  • In eat_redirect, consume the full digit run so multi-digit fds are recognized as redirects, and return a tristate (Redirect / NotRedirect / UnsupportedFd). fd numbers other than 0, 1, or 2 now surface a parse error "Redirecting to file descriptors other than 0, 1, and 2 is not supported yet." instead of silently producing wrong output. (RedirectFlags only represents stdin/stdout/stderr today; wiring up arbitrary fds through the interpreter is a separate feature.)

How did you verify your code works?

New tests in test/js/bun/shell/bunshell.test.ts (16 runtime cases) and test/js/bun/shell/lex.test.ts (token-level cases). They fail on the released binary and pass on this branch. The existing shell redirect tests, lex.test.ts, parse.test.ts, and the full bunshell.test.ts suite continue to pass.


no test proof · iteration 1 · 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

Fixes #12602

The shell lexer's digit case called eat_redirect on any unquoted digit
with no word-boundary check, so `echo z1>f` stole the trailing `1` as
the fd and wrote "z" instead of "z1" to the file. eat_redirect also
mapped only 0/1/2 to fds, so standalone `3>` backtracked into an
argument and `10>` split into arg "1" plus a `0>` redirect.

Gate the fd parse on the digit sequence being a complete unquoted word
immediately before the operator (POSIX 2.7), consume the full digit
run, and reject fd numbers other than 0/1/2 with an explicit error
instead of silently producing wrong output.
@coderabbitai

coderabbitai Bot commented Jul 12, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

The shell lexer now parses file-descriptor redirects using explicit result categories, enforces standalone fd-word boundaries, handles unsupported descriptors with targeted errors, and updates delimiter behavior for **. Lexer and shell tests cover these cases.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: restricting redirect fd parsing to word boundaries.
Description check ✅ Passed The description includes the required purpose and verification sections and provides concrete repros and fix details.
Linked Issues check ✅ Passed The change addresses #12602 by fixing digit-prefixed redirect parsing, including stdin redirects like 1< and 2<.
Out of Scope Changes check ✅ Passed The additional lexer adjustments still relate to shell redirect tokenization and appear within the PR's stated scope.

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Jul 12, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 12:50 AM PT - Jul 12th, 2026

❌ @robobun, your commit 262db8c has 3 failures in Build #72074 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 33997

That installs a local version of the PR into your bun-33997 executable, so you can run:

bun-33997 --bun

@github-actions

Copy link
Copy Markdown
Contributor

Found 1 issue this PR may fix:

  1. Bun shell parses some redirections incorrectly #12602 - Directly reports eat_redirect misparsing digit-prefixed redirects (e.g. 1<file interpreted as 1>>file); this PR's word-boundary gate and corrected fd/direction logic in eat_redirect fixes the same root cause.

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #12602

🤖 Generated with Claude Code

Comment thread src/shell_parser/parse.rs
Comment thread src/shell_parser/parse.rs Outdated
robobun and others added 3 commits July 12, 2026 04:31
…-zero fds; Windows-safe #12602 test

- Add DoubleAsterisk to the word-continuing token set so `echo **3>f`
  keeps the digit in the glob instead of hitting UnsupportedFd.
- Accumulate the fd digit run as a decimal value so `01>` / `02>`
  resolve to fd 1 / fd 2 instead of being rejected.
- Rewrite the #12602 regression test to assert on stdout and file
  contents instead of cat's error message, which includes the absolute
  path on Windows.
Comment thread src/shell_parser/parse.rs
Comment thread src/shell_parser/parse.rs
- Move DoubleAsterisk to the word-continuing arm of break_word_impl so
  whitespace after `**` emits a Delimit, letting the fd word-boundary
  check distinguish `** 2>f` (stderr redirect) from `**2>f` (glob).
- Consume the first `<` before eat_simple_redirect_operator so `0<f`
  no longer carries a spurious APPEND flag and `0<<f` lexes as one
  token. `1<` and `2<` are rejected as unsupported instead of
  silently opening the file for writing.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/shell_parser/parse.rs (1)

3385-3425: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

0>/0>> take the input branch in shell execution. RedirectFlags::STDIN is being used as the fd selector, so src/runtime/shell/Builtin.rs opens these redirects with O_RDONLY; fd-0 output redirects end up with the wrong mode. Split target-fd from open direction, or special-case fd 0 in the >/>> path, and add a runtime test for 0>/0>>.

🤖 Prompt for 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.

In `@src/shell_parser/parse.rs` around lines 3385 - 3425, Update the fd-0 handling
across the redirect parsing/execution flow anchored by the
`FdRedirect::Redirect(flags)` branch so `0>` and `0>>` are treated as output
redirects, not input redirects: separate the target file descriptor from the
open direction or special-case fd 0 in the `>`/`>>` path, ensuring append mode
remains correct for `0>>`. Add runtime coverage for both forms.
🤖 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.

Outside diff comments:
In `@src/shell_parser/parse.rs`:
- Around line 3385-3425: Update the fd-0 handling across the redirect
parsing/execution flow anchored by the `FdRedirect::Redirect(flags)` branch so
`0>` and `0>>` are treated as output redirects, not input redirects: separate
the target file descriptor from the open direction or special-case fd 0 in the
`>`/`>>` path, ensuring append mode remains correct for `0>>`. Add runtime
coverage for both forms.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c5321130-f1df-4689-b693-7b7cd82d2666

📥 Commits

Reviewing files that changed from the base of the PR and between 8624c2b and 262db8c.

📒 Files selected for processing (3)
  • src/shell_parser/parse.rs
  • test/js/bun/shell/bunshell.test.ts
  • test/js/bun/shell/lex.test.ts

@robobun

robobun commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator Author

Re: the coderabbit note on 0>/0>> opening with O_RDONLY: that's pre-existing behaviour this PR doesn't touch. On the released build echo hi 0>newfile already fails with ENOENT and echo hi 0>existing leaves the file unchanged, because RedirectFlags conflates the target fd with the open direction (STDIN implies O_RDONLY in to_flags()).

A proper fix means extending RedirectFlags to track fd and direction independently and wiring that through the interpreter, which is the same work as supporting arbitrary fd numbers and is out of scope here. The 1</2< case was handled in this PR only because correcting the missing eat() in the < arm would have changed it from silent-append to silent-truncate; 0> has no such forcing function from this diff, so I've left it as-is rather than widen the change set further.

@robobun

robobun commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator Author

Shell tests (bunshell.test.ts, lex.test.ts, parse.test.ts) pass on all lanes in build 72074. The remaining red is unrelated to this diff:

  • test-worker-message-port-transfer-terminate.js SIGABRT on debian x64-asan (JSC getOwnPropertyDescriptor assertion, also present on build 72025; being tracked separately)
  • test-fs-promises-file-handle-readFile.js, zlib/leak.test.ts, napi.test.ts flaked on single retries

Ready for review.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All four earlier findings are addressed in 262db8c and the bug hunter found nothing new, but this is a subtle lexer state-machine change (word-boundary detection, break_word_impl delimiter categorization, new hard-error paths) that took three rounds to converge — worth a human look before merge.

What was reviewed:

  • Word-boundary guard: verified word_start/last_tok_tag list matches break_word_impl's true arm exactly, so no token can be word-continuing in one place and not the other.
  • break_word_impl moving DoubleAsterisk to the delimit-emitting arm: traced the parser's compound-atom handling — it already treats DoubleAsterisk like Asterisk, so the extra Delimit after ** is consumed identically.
  • eat_redirect tristate: checked backtrack still fires on NotRedirect (escaped >, digit-only word, N>&3), and that UnsupportedFd returns before any irreversible eat() past the operator.
  • < arm: confirmed the added self.eat() mirrors the > arm and 0<<f now consumes both <; 1</2< reject before eating so no cursor drift.
Extended reasoning...

Overview

This PR modifies the Bun Shell lexer (src/shell_parser/parse.rs) to only treat digit sequences as fd numbers for redirects when they form a complete word, per POSIX 2.7. It touches three areas: (1) a new word-boundary guard in the digit case of the main lex loop, (2) a rewrite of eat_redirect to consume full digit runs and return a four-state FdRedirect enum instead of Option<RedirectFlags>, and (3) moving TokenTag::DoubleAsterisk between arms of break_word_impl's delimiter-emission match. ~70 lines of Rust logic plus ~220 lines of new lex/runtime tests.

Security risks

None. The shell lexer parses trusted template-literal input from the user's own code; this change tightens tokenization and adds explicit errors where the old code silently mis-parsed. No new allocation, no untrusted-length arithmetic (the digit accumulator uses saturating_mul/saturating_add).

Level of scrutiny

Medium-high. The shell lexer runs on every $ invocation and its state machine is dense — word_start, j, the strpool, and the token stream interact in ways that are easy to get subtly wrong, as the three prior review rounds on this PR demonstrate (each found a real regression: **3>f, then ** 2>f, plus the pre-existing 0< APPEND bug). The break_word_impl change in particular affects delimiter emission for ** in all contexts, not just before digits.

Other factors

  • All four of my earlier findings are resolved and covered by new tests (**1>f/**3>f/** 2>f/* 2>f, 01>/02>, cat 0<f, 1</2< error cases).
  • Test coverage is thorough at both the token level (lex.test.ts) and runtime level (bunshell.test.ts), including the #12602 repro, quoted-text/cmd-subst/glob prefixes, standalone vs mid-word digits, and the new error messages.
  • Behavior change: 3>/10>/1</2< now hard-error where they previously produced silent wrong output. This is the right call (loud > silently-wrong) but is a user-visible change a maintainer should sign off on.
  • The DoubleAsterisk recategorization in break_word_impl is correct for the redirect case and consistent with how the parser consumes it, but it changes delimiter emission for ** globally — I traced the parser's atom handling and it looks fine, but a maintainer familiar with the shell parser should confirm no other consumer depended on the old no-delimit behavior.

Given the interaction complexity and that this is production lexer code that needed multiple iterations, I'm deferring rather than approving.

@robobun

robobun commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-12, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. The linked issue (#12602) stays open. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bun shell parses some redirections incorrectly

1 participant