fix(bin): run interpreter on a large-stack thread to avoid deep-nesting overflow 🌀#176
fix(bin): run interpreter on a large-stack thread to avoid deep-nesting overflow 🌀#176timfennis wants to merge 1 commit into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61e97df1bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Every re-entry into the precedence chain — grouped parentheses, list | ||
| // and tuple elements, call arguments, block statements — flows through | ||
| // here, so guarding this one method bounds nesting depth for all of them. | ||
| self.recurse(Self::logic_or) |
There was a problem hiding this comment.
Postfix calls with no arguments bypass this depth counter: operand parses f()()()... in a loop, and each empty () never calls single_expression, so the parser accepts an arbitrarily deep Call { function: Box::new(previous), ... } tree while depth stays at the outer expression. That tree is still walked recursively later (for example ndc_analyser/src/analyser.rs:490 analyses a non-identifier callee recursively and ndc_vm/src/compiler.rs:423 compiles the call's function recursively), so this crafted input can still reach the stack-overflow abort this change is meant to prevent instead of returning the new parse error.
Useful? React with 👍 / 👎.
…ng overflow 🌀
Deeply nested input (((…))) / [[[…]]] / {{{…}}}, long ---… / not … /
2^2^… chains, and nested calls walk the AST recursively in the parser,
analyser, compiler, and VM, one native frame per level. On the default
main-thread stack this overflowed and aborted the process (SIGABRT, not a
catchable panic) at a few hundred levels.
Run the interpreter on a worker thread with a large explicit stack. The
interpreter uses Rc and isn't Send, but the closure creates, uses, and
drops it on the worker thread, so nothing crosses the boundary. Input far
deeper than any realistic source now evaluates instead of crashing.
Closes #175
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
timfennis
force-pushed
the
bugfix/parser-recursion-depth-limit
branch
from
61e97df to
fb4872d
Compare
timfennis
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb4872d7da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // dropped entirely inside this closure, so nothing crosses the boundary. | ||
| let worker = std::thread::Builder::new() | ||
| .name("ndc-interpreter".into()) | ||
| .stack_size(INTERPRETER_STACK_SIZE) |
There was a problem hiding this comment.
For source nested beyond what this fixed 512 MiB stack can hold, this still reaches the same recursive parser/analyser/compiler walks and aborts the process; in the inspected diff ndc_parser is unchanged, so this line only raises the crash threshold rather than turning crafted deep input into a normal error. A file like print( + many more nested parentheses than the worker stack can handle + 1 will still terminate with a stack overflow, so the CLI remains vulnerable unless the parser enforces a depth limit before later phases walk the AST.
Useful? React with 👍 / 👎.
1 participant
Context
While hunting for crashes in the pipeline, I found that deeply nested input aborts the whole process with
fatal runtime error: stack overflow(SIGABRT; a Rust panic handler can't catch it). It reproduces from short scripts: nested(((…))),[[[…]]],{{{…}}}, nested calls and tuples, and long---…,not not …,2^2^…chains. Each nesting level costs one native stack frame in the parser and in every later phase that walks the AST. Tracked in #175.Approach
The parser, analyser, compiler, and VM all recurse over the AST, so the overflow isn't confined to one crate. Rather than thread a depth cap through the recursive descent (which clutters the parser),
ndcnow runs the interpreter on a worker thread with a large explicit stack. Input far deeper than any realistic source evaluates instead of crashing.Changes
ndc_bin: spawn a worker thread with a 512 MiB stack and run the whole interpreter on it. The interpreter usesRcand isn'tSend, but the closure creates, uses, and drops it on the worker thread, so nothing crosses the boundary. The reservation is virtual address space; only touched pages cost memory.ndc_bin/tests/deep_nesting.rs: drive the built binary on 2000-deep nesting and assert it evaluates rather than aborting. That depth overflows the default main-thread stack but fits the worker stack with room to spare.Notes for reviewers
cargo testfunctional harness also walk the AST off the main thread on default-sized stacks, so they remain susceptible to very deep input. They can get the same large-stack treatment if it ever matters.🤖