Fix segfault on cyclic or deeply-nested AST in compile()#7630
Conversation
|
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 |
|
awesome, thank you so much. |
Thanks for the review :) |
Closes #4862.
Symptom
compile()with a cyclic or pathologically deep AST object (built via theastmodule) overflows the native stack and crashes RustPython withSIGSEGV. CPython raisesRecursionError: maximum recursion depth exceeded while traversing <kind> node.Reproduction
SIGSEGVRecursionError: maximum recursion depth exceeded while traversing AST nodeRecursionError: maximum recursion depth exceeded while traversing 'expr' nodeRoot cause
The
ast_from_objectconversion from a Pythonastobject tree to the internalruff_python_asttypes incrates/vm/src/stdlib/_ast/node.rsrecursed unconditionally:Box<T>::ast_from_objectcalledT::ast_from_objectonce per descent (no guard)Vec<T>::ast_from_objectcalledNode::ast_from_objectper 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.
Fix
Wrap both recursive descents in
vm.with_recursion(...), which is the existing machinery RustPython uses elsewhere to translate native recursion into a Python-levelRecursionErrorat the configured limit (default 1000). No new infrastructure, no configuration changes.The
"while traversing AST node"label matches CPython'sRecursionErrormessage style for this failure mode.Why both
Box<T>andVec<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 thatList.elts = [self]andTuple.elts = [self]still crashed because they traverse viaVec<T>. Guarding both closes every cycle path I could construct.Verification
Direct-cycle cases (were segfaulting; now raise
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 = selfIndirect cycles
A.operand = B; B.operand = A— caught on the same counter.Deep non-cyclic input
RecursionError(safe; no crash)Regression
A new regression test in
extra_tests/snippets/stdlib_ast.pyexercises 6 representative cycle patterns across bothBoxandVecdescent paths; it passes on both RustPython (with this patch) and CPython 3.14.Scope
ast_from_objectpath, 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 byruff_python_parser, which does not emit cycles; no user-controlled path reaches it.validate_modruns afterast_from_object. Sinceast_from_objectnow caps depth via the VM's recursion counter, any AST reachingvalidate_modis already depth-bounded; local testing at depth 999 confirmsvalidate_modhandles it without crashing.Why this approach (vs. a dedicated counter in codegen)
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: theast_from_objecttraversal. Using the VM's existingwith_recursionkeeps the mechanism consistent with how RustPython already converts native overflows toRecursionErrorelsewhere (e.g. during__repr__recursion,__eq__recursion).Summary by CodeRabbit
Bug Fixes
Tests