GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Fix segfault on cyclic or deeply-nested AST in `compile()` by changjoon-park · Pull Request #7630 · RustPython/RustPython Conversation
|
| Before | After | |
|---|---|---|
| RustPython | SIGSEGV |
RecursionError: maximum recursion depth exceeded while traversing AST node |
| CPython 3.14 | RecursionError: maximum recursion depth exceeded while traversing 'expr' node |
(unchanged) |
The ast_from_object conversion from a Python ast object tree to the internal ruff_python_ast types in crates/vm/src/stdlib/_ast/node.rs recursed unconditionally:
Box<T>::ast_from_object called T::ast_from_object once per descent (no guard)Vec<T>::ast_from_object called Node::ast_from_object per element (no guard)A cycle in the Python-level AST turns either call into unbounded recursion, which overflows the Rust native stack. Rust cannot recover from a stack overflow — the process is killed.
Wrap both recursive descents in vm.with_recursion(...), which is the existing machinery RustPython uses elsewhere to translate native recursion into a Python-level RecursionError at the configured limit (default 1000). No new infrastructure, no configuration changes.
// Box<T>::ast_from_object — single recursive descent vm.with_recursion("while traversing AST node", || { T::ast_from_object(vm, source_file, object).map(Self::new) }) // Vec<T>::ast_from_object — per-element descent vm.extract_elements_with(&object, |obj| { vm.with_recursion("while traversing AST node", || { Node::ast_from_object(vm, source_file, obj) }) })
The "while traversing AST node" label matches CPython's RecursionError message style for this failure mode.
Box<T> and Vec<T> must be guardedAll recursive AST fields use one of these two containers:
An earlier iteration of this patch only guarded Box<T>. Testing showed that List.elts = [self] and Tuple.elts = [self] still crashed because they traverse via Vec<T>. Guarding both closes every cycle path I could construct.
RecursionError)UnaryOp.operand = selfBinOp.left = selfCall.func = selfAttribute.value = selfSubscript.value = selfIfExp.test = selfList.elts = [self]Tuple.elts = [self]Set.elts = [self]Dict.values = [self]ListComp.elt = selfA.operand = B; B.operand = A — caught on the same counter.| Depth | Behavior |
|---|---|
| 500 | compiles OK |
| 900 | compiles OK |
| 999 | compiles OK |
| 1500 / 3000 / 5000 | RecursionError (safe; no crash) |
$ ./target/release/rustpython -m test test_ast test_compile
All 2 tests OK.
Result: SUCCESS
A new regression test in extra_tests/snippets/stdlib_ast.py exercises 6 representative cycle patterns across both Box and Vec descent paths; it passes on both RustPython (with this patch) and CPython 3.14.
ast_from_object path, which is the only way a cyclic tree can enter RustPython's compilation pipeline (parsed source cannot be cyclic; ruff parser produces trees, not DAGs).ast_to_object (reverse direction) is only called on trees produced by ruff_python_parser, which does not emit cycles; no user-controlled path reaches it.validate_mod runs after ast_from_object. Since ast_from_object now caps depth via the VM's recursion counter, any AST reaching validate_mod is already depth-bounded; local testing at depth 999 confirms validate_mod handles it without crashing.The initial attempt placed a depth counter inside Compiler::compile_expression (crates/codegen/src/compile.rs). That turned out to be downstream of the crash site — the process is already dead before reaching codegen. The fix lives where the crash happens: the ast_from_object traversal. Using the VM's existing with_recursion keeps the mechanism consistent with how RustPython already converts native overflows to RecursionError elsewhere (e.g. during __repr__ recursion, __eq__ recursion).
Bug Fixes
Tests
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds VM recursion guards around AST node deserialization for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
awesome, thank you so much. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Thanks for the review :) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
Segfault when compiling a nested AST
There was an error while loading. Please reload this page.