fix(parser): stop the tokenizer hanging and overflowing on hostile input - #1270
Open
ferentinai wants to merge 1 commit into
Open
ferentinai wants to merge 1 commit into
ferentinai wants to merge 1 commit into
Conversation
Two ways a short command line could take down the process rather than return an error. An unterminated here tag whose text contains an unterminated expansion spun forever at end-of-input. `next_token_until` looked for a here-document *end* tag in any non-`None` here state, but in `NextLineIsHereDoc` the body never started, so there is no end tag to find; the lookup then succeeded spuriously against the empty current token, and that path only queues a token onto the pending here tag, so nothing changed and the loop went round again -- allocating each pass. `<<E$[` followed by two blanks (7 bytes) reached ~10 GB resident within seconds and never returned. Only attempt the lookup when we are actually inside a here-document body; the input now reports an unterminated here document, which is what bash does. `consume_nested_construct` and `next_token_until` are mutually recursive, with depth chosen entirely by the input, so deeply nested `$(...)` overflowed the stack and aborted -- around 7000 levels on an 8 MB main thread, but under 2000 on a 2 MB one, which is a command line of about 4 KB. Bound the recursion and report `ExpansionNestingTooDeep` instead. The limit is set well below what an unoptimized build can afford (roughly 8 KB of stack per level) and far above any plausible script. Both are reachable from a single untrusted string, which matters for any consumer that parses command lines it did not write.
Public API changes for crate: brush-parserRemoved itemsAdded itemsPerformance Benchmark Report
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is Test Summary: bash-completion test suite
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two ways a short, malformed command line could take the process down rather than
return an error. Both are in the tokenizer, both are reachable from a single
string, and neither depends on anything unusual about the environment.
I found these while evaluating
brush-parserfor a tool that parses commandlines it did not write, so untrusted input on a deadline is the case I care
about. They will matter less for interactive use, where you are already running
the shell, but they are cheap to fix either way.
1. Unterminated here tag with an unterminated expansion hangs and allocates forever
$ printf '<<E$[\t\t' | your-parser-harnessSeven bytes. Before this change that reaches roughly 10 GB resident within
seconds on my machine and never returns; I only ever saw it end by being killed.
next_token_untillooks for a here-document end tag whenever the here stateis anything other than
None. InNextLineIsHereDocthe body has not started,so there is no end tag to find, but
remove_here_end_tagstill succeeds againstthe empty current token. That path goes to the
delimit_current_tokenbranchthat only pushes onto
pending_tokens_afterand returnsOk(None), so no tokenis produced, no here tag is consumed, and the state is exactly what it was. The
loop goes round again and allocates again.
consume_nested_constructthen makes it unbounded rather than merely stuck,because it accumulates each of those here-document tokens into
pending_here_doc_tokens.The fix only attempts the end-tag lookup while we are actually inside a
here-document body. The input above now reports an unterminated here document.
For what it is worth,
bash -nalso rejects it:Variants that hit the same path:
$[,$(and$((as the unterminatedexpansion,
<<-as well as<<. Curiously it needs exactly two trailing blanks;one is fine and so are three, which is what made it look so arbitrary before the
cause was clear.
2. Nested expansions recurse without bound and abort on stack overflow
$ printf 'echo %s%s' "$(printf '$(%.0s' {1..8000})" "$(printf ')%.0s' {1..8000})" | your-parser-harnessconsume_nested_constructandnext_token_untilare mutually recursive, and theinput alone decides how deep that goes. On my machine it aborts somewhere between
6500 and 7000 levels on an 8 MB main thread, but under 2000 on a 2 MB one, which
is a command line of about 4 KB. Where the parse happens changes the threshold by
roughly 4x, which is worth knowing if a caller parses on a worker thread.
This adds a depth bound and a
TokenizerError::ExpansionNestingTooDeepratherthan letting it run into the guard page. I picked the limit by measurement rather
than taste: an unoptimized build uses roughly 8 KB of stack per level, so 64
levels stays around half a megabyte and is comfortable even on a small thread
stack. My first attempt at 256 still overflowed a 2 MB debug test thread, which
is how I arrived at the number.
If you would rather have this configurable through
TokenizerOptions, or set toa different value, say the word and I will rework it.
Not the same as #948
Worth stating explicitly since the symptom is identical. #948
(
nproc(){ nproc; }thenecho $(nproc)) is runtime recursion through aself-referencing function during expansion, and it still reproduces with this
change applied. It is a different code path and this PR does not claim to fix it.
Testing
cargo test -p brush-parser: 227 passed, 0 failed. That is the existing 224plus 3 added here.
cargo clippy -p brush-parser --all-targets: clean under the crate's existinglint configuration.
cargo test -p brush-shell --test brush-compat-tests: 1388 succeeded, 405failed, 366 known to fail, 43 skipped, which is byte for byte what a pristine
checkout produces on the same machine. The here-document change is the risky
one, so I ran that baseline specifically to show it moves nothing.
first of these bugs originally and reports no hangs or panics afterwards.
Added tests cover the hanging inputs, the depth bound firing, and nesting within
the limit still tokenizing normally.
Assisted-by: Claude Opus 5 (Claude Code)