Fix set_call_limit to track recursion depth and prevent stack overflow#1140
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: tomtau <2410580+tomtau@users.noreply.github.com>
Co-authored-by: tomtau <2410580+tomtau@users.noreply.github.com>
Co-authored-by: tomtau <2410580+tomtau@users.noreply.github.com>
Co-authored-by: tomtau <2410580+tomtau@users.noreply.github.com>
Co-authored-by: tomtau <2410580+tomtau@users.noreply.github.com>
|
@copilot can just set_call_limit be fixed (to incorporate set_depth_limit) instead of introducing a new function? |
I considered merging them, but keeping separate functions is better because:
The two limits serve complementary purposes:
If you'd prefer a unified API, I could explore these alternatives:
Let me know if you'd like me to refactor to a different approach. |
|
@copilot the primary function of set_call_limit should be the same as of set_depth_limit, i.e. its implementation can be replaced by it |
As requested by @tomtau, removed separate set_depth_limit function and fixed set_call_limit to track actual recursion depth (increment on entry, decrement on exit) instead of just total calls. This makes set_call_limit properly prevent stack overflow from deep recursion as originally intended. Changes: - Removed DEPTH_LIMIT static and set_depth_limit function - Modified CallLimitTracker to only track recursion depth (not total calls) - Simplified implementation by removing separate call/depth tracking - Updated all tests to use set_call_limit instead of set_depth_limit - Updated documentation to reflect that set_call_limit now tracks depth Co-authored-by: tomtau <2410580+tomtau@users.noreply.github.com>
Fixed in commit 2b956db.
Key changes:
|
|
@copilot |
Co-authored-by: tomtau <2410580+tomtau@users.noreply.github.com>
|
@copilot |
- Fixed doc comment indentation issues in lib.rs and parser_state.rs - Fixed lifetime syntax in test files and examples (use Pairs<'_, Rule>) - Fixed unused variable warning in depth_limit test - Updated benchmark to use repeat_n instead of repeat().take() Co-authored-by: tomtau <2410580+tomtau@users.noreply.github.com>
A 430-byte fuzz file with ~200 nested parentheses aborted the whole
process ('thread main has overflowed its stack') in rossi::parse: both
the pest-generated parser and the AST builder recurse per nesting level
(~86 KB/level for parens in debug builds, crashing past ~95 levels on
an 8 MB stack). Fatal for the language server, which parses every
.eventb file under the workspace root during initialize (issue recorded
as the 'rootUri=file:///tmp stack overflow').
Two complementary guards behind every parse entry point (parse,
parse_components, the three parse_*_str used per XML attribute, and
selection::enclosing_spans):
- nesting::check_nesting: an iterative O(n) pre-scan that bounds the
parser's recursion depth before pest runs and rejects input beyond
MAX_NESTING_DEPTH (256) with a new ParseError::NestingTooDeep
carrying line/column. It counts bracket depth plus prefix/binder
chains (not/dom/ran/POW/UNION/INTER/if, unary minus, and their
Unicode forms), never under-counts (soundness), skips comments,
strings, and @labels exactly as the grammar lexes them, resets the
prefix chain per @Label, and restores it at closing brackets so long
conjunctions of bracketed negations don't accumulate. On success it
returns the measured depth metric.
- stacker::maybe_grow with a red zone proportional to that metric
(4 MiB base + 128 KiB/level, ~1.5x the measured worst case) and
64 MiB segments, so at-limit input is uniformly safe in debug builds
and on 2 MiB worker threads while shallow formulas (per-attribute
import parses, per-keystroke LSP parses) skip the segment allocation
on ordinary stacks.
NestingTooDeep is classified as a formula error end to end: validate
maps it to EB005, and xml's wrap_attr_error passes it through instead
of stringifying it into MalformedAttribute (which would surface a
Rodin zip's over-deep formula as EB001 malformed XML).
parse_with_recovery short-circuits on the new error (clause recovery
would re-trigger it per line), enclosing_spans applies the same guard
before its pest walk, and the guard invariant is documented on
RossiParser and the *_unguarded bodies.
A pre-scan (rather than a depth counter inside the parser) is forced
by pest: the overflowing recursion is generated code with no depth
hook, and the released pest::set_call_limit bounds total calls, not
depth — its internal counter never decrements. Upstream tracks real
depth limiting as pest-parser/pest#1129, with open PR
pest-parser/pest#1140 repurposing set_call_limit to count depth; if
that ships it can serve as defense in depth behind this scan, which
remains the source of the located NestingTooDeep diagnostic.
A 430-byte fuzz file with ~200 nested parentheses aborted the whole
process ('thread main has overflowed its stack') in rossi::parse: both
the pest-generated parser and the AST builder recurse per nesting level
(~86 KB/level for parens in debug builds, crashing past ~95 levels on
an 8 MB stack). Fatal for the language server, which parses every
.eventb file under the workspace root during initialize (issue recorded
as the 'rootUri=file:///tmp stack overflow').
Two complementary guards behind every parse entry point (parse,
parse_components, the three parse_*_str used per XML attribute, and
selection::enclosing_spans):
- nesting::check_nesting: an iterative O(n) pre-scan that bounds the
parser's recursion depth before pest runs and rejects input beyond
MAX_NESTING_DEPTH (256) with a new ParseError::NestingTooDeep
carrying line/column. It counts bracket depth plus prefix/binder
chains (not/dom/ran/POW/UNION/INTER/if, unary minus, and their
Unicode forms), never under-counts (soundness), skips comments,
strings, and @labels exactly as the grammar lexes them, resets the
prefix chain per @Label, and restores it at closing brackets so long
conjunctions of bracketed negations don't accumulate. On success it
returns the measured depth metric.
- stacker::maybe_grow with a red zone proportional to that metric
(4 MiB base + 128 KiB/level, ~1.5x the measured worst case) and
64 MiB segments, so at-limit input is uniformly safe in debug builds
and on 2 MiB worker threads while shallow formulas (per-attribute
import parses, per-keystroke LSP parses) skip the segment allocation
on ordinary stacks.
NestingTooDeep is classified as a formula error end to end: validate
maps it to EB005, and xml's wrap_attr_error passes it through instead
of stringifying it into MalformedAttribute (which would surface a
Rodin zip's over-deep formula as EB001 malformed XML).
parse_with_recovery short-circuits on the new error (clause recovery
would re-trigger it per line), enclosing_spans applies the same guard
before its pest walk, and the guard invariant is documented on
RossiParser and the *_unguarded bodies.
A pre-scan (rather than a depth counter inside the parser) is forced
by pest: the overflowing recursion is generated code with no depth
hook, and the released pest::set_call_limit bounds total calls, not
depth — its internal counter never decrements. Upstream tracks real
depth limiting as pest-parser/pest#1129, with open PR
pest-parser/pest#1140 repurposing set_call_limit to count depth; if
that ships it can serve as defense in depth behind this scan, which
remains the source of the located NestingTooDeep diagnostic.
Fix set_call_limit to track recursion depth and prevent stack overflow
Fixed
set_call_limitto track recursion depth properly (increment/decrement) instead of adding a separateset_depth_limitfunction, as requested by @tomtau.Changes
Fixed
set_call_limitimplementation:DEPTH_LIMITandset_depth_limitfunctionCallLimitTrackerto only track depth (removed call count tracking)Rationale:
The original
set_call_limithad a method calledincrement_depthbut never decremented, making it track total calls instead of recursion depth. This fix makes it work as originally intended - preventing stack overflow by limiting recursion depth.Testing:
set_call_limit)API
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.