Harden arith parse recursion - #122
Merged
Merged
Conversation
A list whose dot is the last token before EOF (`(1 .`, `(.`) panicked: `parse_list` called `.unwrap()` on `parse_value()`, which returns `None` at end of input. Return a `ParsingError` instead, like the quote / backquote / comma arms already do.
Emacs has `%` as the truncated integer remainder (sign of the dividend), distinct from the floored `mod`. It was missing. Integer-only operands (enforced by the `i64` args, matching Emacs), errors on a zero divisor, and uses `wrapping_rem` so `i64::MIN % -1` yields 0 rather than overflow-panicking.
Emacs returns 0, 1, and 0 for `(+)`, `(*)`, and `(-)`; tulisp required at least one argument. Both eval paths needed fixing — the VM compiles these as special forms (`compile_fn_*`) while the tree-walker dispatches the typed builtin — so each now yields its identity on empty input. `/` still requires an argument, as in Emacs. The funcall too-few-args test moves to `1+`, since `+` no longer errors on zero args.
Integer `(/ X Y)` and `(mod X Y)` panicked the host on `i64::MIN / -1` (Rust traps division overflow in debug *and* release, so `condition-case` could not catch it). Add `Number::checked_div` / `checked_mod` and route the `/` and `mod` builtins and the VM `BinaryOp::Div` through them, surfacing overflow as a catchable `OutOfRange`. `mod` also used Rust truncated remainder, so it took the dividend's sign — `(mod -7 3)` gave -1 where Emacs gives 2. `checked_mod` is floored (sign of the divisor) for both integer and float operands. Tests for `/` and `mod` move to colocated unit tests.
Deep non-tail recursion grew the native stack until the process aborted with a stack overflow — uncatchable by `condition-case`, fatal to the embedding host. Track Lisp-call nesting on the context and raise a catchable `LispError` once it exceeds `max_eval_depth`. The counter is incremented at the two points that actually push native frames — the VM `run_impl` and the tree-walker `eval_lambda` — so tail calls, which trampoline through their own loops, do not count (a 30000-deep tail recursion still completes). The default is sized to the stack each build needs. Under `cfg(test)` it is 16, since the harness runs on ~2 MiB threads; otherwise it targets the 8 MiB main thread of `cargo run` and typical embeddings — 64 in debug, where each frame is an order of magnitude larger, and 1000 in release. A test recurses past the default on an 8 MiB thread, on both eval paths, and asserts a catchable error rather than an overflow — so a cap set too high for that stack aborts the test instead of shipping. `set_max_eval_depth` tunes it for other stacks.
Deeply nested input (e.g. tens of thousands of nested parens, or a runtime-built nested form passed to `macroexpand`) recursed unguarded through `parse_value` / `parse_list` and `macroexpand`, overflowing the native stack and aborting the host — uncatchable, like the eval-recursion case. Add a `MAX_NESTING_DEPTH` cap (256 debug / 4000 release, sized for the 8 MiB main thread): the parser tracks nesting on a `Parser` field, `macroexpand` on a recursion-depth argument, and both raise a catchable error past it. Guarding the parser is the keystone — every nested list/quote re-enters `parse_value`, so no parsed AST exceeds the cap, which transitively bounds the downstream compile-time walks (`recursive_update_ctxobj`, `free_vars`, backquote, substitute, …) for parsed input. `macroexpand` is additionally guarded because it is directly callable on runtime-built structures. Addresses the parser/macroexpand part of todo b4. The guards cost a couple of integer ops per node; benchmarks (parse_long_list, parse_many_forms, macroexpand_long) are unchanged within run-to-run noise.
A compiled `defun` that recurses through `(eval …)` bounces VM -> tree-walker -> VM each level. Because the depth counter is incremented at every `run_impl` / `eval_lambda` entry — one per interleaved level — the cap bounds this with a catchable error instead of a stack overflow. Lock that, since a regression would only surface as an uncatchable host abort.
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
1 participant
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.
No description provided.