Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

crates/vm/src/frame.rs (2)

177-179: ⚠️ Potential issue | 🟡 Minor

Refresh datastack lifecycle comments to the current API.

These comments still instruct callers to use release_datastack(), but the migration path is now materialize_localsplus() before datastack_pop(). Please update both spots so the cleanup contract is accurate.
As per coding guidelines, "Do not delete or rewrite existing comments unless they are factually wrong or directly contradict the new code."

Also applies to: 470-472

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/frame.rs` around lines 177 - 179, Update the datastack
lifecycle comments in the docs for the frame constructor functions (e.g.,
new_on_datastack and the other commented spot near the Frame methods) to reflect
the current API: instruct callers to call materialize_localsplus() to
materialize localsplus and then call datastack_pop() (or the
DataStack::datastack_pop equivalent) instead of the outdated
release_datastack(); keep the rest of the comment intact but replace the
release_datastack() reference with the new sequence (materialize_localsplus()
before datastack_pop()) so the cleanup contract matches the migrated API.

---

290-351: ⚠️ Potential issue | 🔴 Critical

Harden LocalsPlus stack bounds checks for release builds.

stack_push, stack_pop, stack_index, and stack_index_mut rely on debug_assert! before unsafe/raw-pointer access. In release builds, invariant breaks can become out-of-bounds UB instead of controlled failure.

Suggested hardening

@@
     fn stack_push(&mut self, val: Option<PyStackRef>) {
         let idx = self.nlocalsplus as usize + self.stack_top as usize;
-        debug_assert!(
+        assert!(
             idx < self.capacity(),
             "stack overflow: stack_top={}, capacity={}",
             self.stack_top,
             self.stack_capacity()
         );
@@
     fn stack_pop(&mut self) -> Option<PyStackRef> {
-        debug_assert!(self.stack_top > 0, "stack underflow");
+        assert!(self.stack_top > 0, "stack underflow");
         self.stack_top -= 1;
@@
     fn stack_index(&self, idx: usize) -> &Option<PyStackRef> {
-        debug_assert!(idx < self.stack_top as usize);
+        assert!(idx < self.stack_top as usize, "stack index out of bounds");
         let data = self.data_as_slice();
         let raw_idx = self.nlocalsplus as usize + idx;
         unsafe { &*(data.as_ptr().add(raw_idx) as *const Option<PyStackRef>) }
     }
@@
     fn stack_index_mut(&mut self, idx: usize) -> &mut Option<PyStackRef> {
-        debug_assert!(idx < self.stack_top as usize);
+        assert!(idx < self.stack_top as usize, "stack index out of bounds");
         let raw_idx = self.nlocalsplus as usize + idx;
         let data = self.data_as_mut_slice();
         unsafe { &mut *(data.as_mut_ptr().add(raw_idx) as *mut Option<PyStackRef>) }
     }

As per coding guidelines, "Follow Rust best practices for error handling and memory management."

#!/bin/bash
set -euo pipefail

echo "=== stack helper implementations ==="
nl -ba crates/vm/src/frame.rs | sed -n '288,352p'

echo
echo "=== debug_assert usage in stack helpers ==="
rg -n 'fn stack_(push|pop|index|index_mut)\(|debug_assert!\(' crates/vm/src/frame.rs -n -C2

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/frame.rs` around lines 290 - 351, Replace reliance on
debug_assert! with explicit runtime checks that panic on violation so release
builds don't hit UB: in stack_push (function stack_push) check idx <
self.capacity() and panic! with the same "stack overflow..." message instead of
debug_assert; in stack_pop (function stack_pop) check self.stack_top > 0 and
panic! with "stack underflow"; in stack_index and stack_index_mut (functions
stack_index and stack_index_mut) check idx < self.stack_top as usize and panic!
with the same invariant message before doing the raw pointer casts; keep
stack_try_push as-is (it already returns Err) and preserve existing messages.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

crates/vm/src/frame.rs (1)

288-352: ⚠️ Potential issue | 🔴 Critical

Harden stack helper bounds checks for release builds.

stack_push, stack_pop, stack_index, and stack_index_mut gate unsafe pointer access with debug_assert!. In release, these checks disappear, so any invariant break can become UB instead of a controlled panic.

Proposed hardening

 fn stack_push(&mut self, val: Option<PyStackRef>) {
     let idx = self.nlocalsplus as usize + self.stack_top as usize;
-    debug_assert!(
+    assert!(
         idx < self.capacity(),
         "stack overflow: stack_top={}, capacity={}",
         self.stack_top,
         self.stack_capacity()
     );
@@
 fn stack_pop(&mut self) -> Option<PyStackRef> {
-    debug_assert!(self.stack_top > 0, "stack underflow");
+    assert!(self.stack_top > 0, "stack underflow");
     self.stack_top -= 1;
@@
 fn stack_index(&self, idx: usize) -> &Option<PyStackRef> {
-    debug_assert!(idx < self.stack_top as usize);
+    assert!(idx < self.stack_top as usize, "stack index out of bounds");
     let data = self.data_as_slice();
     let raw_idx = self.nlocalsplus as usize + idx;
     unsafe { &*(data.as_ptr().add(raw_idx) as *const Option<PyStackRef>) }
 }
@@
 fn stack_index_mut(&mut self, idx: usize) -> &mut Option<PyStackRef> {
-    debug_assert!(idx < self.stack_top as usize);
+    assert!(idx < self.stack_top as usize, "stack index out of bounds");
     let raw_idx = self.nlocalsplus as usize + idx;
     let data = self.data_as_mut_slice();
     unsafe { &mut *(data.as_mut_ptr().add(raw_idx) as *mut Option<PyStackRef>) }
 }

#!/bin/bash
set -euo pipefail

echo "=== Stack helpers using debug-only checks ==="
nl -ba crates/vm/src/frame.rs | sed -n '288,352p'

echo
echo "=== Nearby subtraction-based stack indexing callsites ==="
rg -n 'stack_len\s*-\s*|stack_index\(' crates/vm/src/frame.rs | sed -n '1,80p'

Expected result: Line 288-352 still shows debug_assert! in helper methods guarding unsafe accesses, confirming release-mode check elision risk.

As per coding guidelines, "Follow Rust best practices for error handling and memory management".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/frame.rs` around lines 288 - 352, The helper methods
(stack_push, stack_try_push already checks, stack_pop, stack_index,
stack_index_mut) use debug_assert! which is compiled out in release causing
potential UB; change those debug-only guards into release-time checks (use
assert! or explicit if .. { panic!(...) }) before performing unsafe pointer
accesses and ensure stack_push mirrors stack_try_push’s capacity check (panic on
overflow) so all unsafe transmute/slice/from_raw_parts calls are protected in
release builds.

crates/vm/src/frame.rs (1)

290-352: ⚠️ Potential issue | 🔴 Critical

Harden stack helper bounds checks in release builds.

LocalsPlus::stack_push, stack_pop, stack_index, and stack_index_mut still rely on debug_assert! before unsafe pointer-based access. In release builds, invariant violations can become UB instead of a controlled failure.

Suggested hardening

 fn stack_push(&mut self, val: Option<PyStackRef>) {
     let idx = self.nlocalsplus as usize + self.stack_top as usize;
-    debug_assert!(
+    assert!(
         idx < self.capacity(),
         "stack overflow: stack_top={}, capacity={}",
         self.stack_top,
         self.stack_capacity()
     );
@@
 fn stack_pop(&mut self) -> Option<PyStackRef> {
-    debug_assert!(self.stack_top > 0, "stack underflow");
+    assert!(self.stack_top > 0, "stack underflow");
@@
 fn stack_index(&self, idx: usize) -> &Option<PyStackRef> {
-    debug_assert!(idx < self.stack_top as usize);
+    assert!(idx < self.stack_top as usize, "stack index out of bounds");
@@
 fn stack_index_mut(&mut self, idx: usize) -> &mut Option<PyStackRef> {
-    debug_assert!(idx < self.stack_top as usize);
+    assert!(idx < self.stack_top as usize, "stack index out of bounds");

#!/bin/bash
set -euo pipefail

echo "Inspect stack helper implementations:"
nl -ba crates/vm/src/frame.rs | sed -n '288,352p'

echo
echo "Spot stack_len - N arithmetic call-sites that depend on these checks:"
rg -nP --type=rust 'stack_len\s*-\s*|stack_index\(' crates/vm/src/frame.rs | head -n 80

As per coding guidelines, "Follow Rust best practices for error handling and memory management."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/frame.rs` around lines 290 - 352, The stack helpers use
debug_assert! which is compiled out in release and can lead to UB; replace those
debug-only asserts with explicit runtime checks that panic with clear messages
before performing any unsafe pointer or transmute operations. Concretely: in
stack_push check idx < self.capacity() and panic on overflow (stack_push still
panics; stack_try_push remains as-is), in stack_pop check self.stack_top > 0 and
panic on underflow before decrementing/reading, and in stack_index and
stack_index_mut check idx < self.stack_top and that raw_idx < self.capacity()
(or otherwise ensure the computed pointer is valid) and panic with a descriptive
message before casting the slice pointer returned by
data_as_slice()/data_as_mut_slice(); perform these checks immediately prior to
any unsafe use so no unsafe transmute/pointer arithmetic runs when invariants
are violated.

crates/vm/src/gc_state.rs (1)

831-841: Consider adding a module-level documentation note about thread-local behavior in non-threading mode.

The thread-local GcState in non-threading mode is correctly implemented: static_cell! expands to thread_local! when the threading feature is disabled (confirmed at crates/common/src/static_cell.rs:113-125). This means each thread gets its own independent GcState, which is appropriate for satisfying !Sync constraints.

This behavior is already documented inline at line 139: "In non-threading mode, gc_state() is thread-local so Sync is not needed." However, adding a brief note to the module-level docs (lines 1-4) would help maintainers understand this design decision upfront. The reinit_after_fork() method is safely gated to threading-only mode, and callers do not make cross-thread assumptions about GC state visibility.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/gc_state.rs` around lines 831 - 841, Add a short module-level
documentation note at the top of the gc_state.rs module explaining that
gc_state() returns a true global When the `threading` feature is enabled but
expands to a thread-local `GcState` when `threading` is disabled (because
`rustpython_common::static_cell!` becomes `thread_local!`), and that this design
satisfies `!Sync` constraints; reference the `gc_state()` function, the
`GcState` type, the `static_cell!` macro, and mention that `reinit_after_fork()`
is gated to threading-only behavior so callers must not assume cross-thread
visibility of GC state.

crates/vm/src/frame.rs (1)

195-207: Mark materialization result as #[must_use] to prevent lifecycle mistakes.

Ignoring the returned base pointer makes it easy to skip datastack_pop, which can leak datastack chunks.

Suggested hardening

+#[must_use = "call VirtualMachine::datastack_pop() with the returned base pointer"]
 fn materialize_to_heap(&mut self) -> Option<*mut u8> {
@@
+#[must_use = "call VirtualMachine::datastack_pop() with the returned base pointer"]
 pub(crate) unsafe fn materialize_localsplus(&self) -> Option<*mut u8> {

Also applies to: 736-738

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/frame.rs` around lines 195 - 207, Mark the pointer-returning
materialization function with #[must_use] so callers cannot ignore the returned
base pointer; specifically add #[must_use] to the fn materialize_to_heap(&mut
self) -> Option<*mut u8> declaration and likewise annotate any other methods
that return a raw pointer from LocalsPlusData::DataStack (the same pattern
flagged around the other occurrence) to ensure callers handle
datastack_pop/lifecycle cleanup.

crates/vm/src/datastack.rs (1)

77-90: ⚠️ Potential issue | 🔴 Critical

Use checked alignment math and avoid pre-check .add() UB in the fast path.

Line 77 and Line 88 still use unchecked (size + ALIGN - 1), and Line 90 performs self.top.add(aligned_size) in the condition. A large size can wrap aligned_size, and .add() must only be used with a proven in-bounds offset.

Suggested fix

 impl DataStack {
+    #[inline(always)]
+    fn align_size(size: usize) -> usize {
+        size.checked_add(ALIGN - 1)
+            .map(|v| v & !(ALIGN - 1))
+            .expect("DataStack allocation size overflow")
+    }
+
     /// Check if the current chunk has at least `size` bytes available.
     #[inline(always)]
     pub fn has_space(&self, size: usize) -> bool {
-        let aligned_size = (size + ALIGN - 1) & !(ALIGN - 1);
+        let aligned_size = Self::align_size(size);
         (self.limit as usize).saturating_sub(self.top as usize) >= aligned_size
     }
@@
     #[inline(always)]
     pub fn push(&mut self, size: usize) -> *mut u8 {
-        let aligned_size = (size + ALIGN - 1) & !(ALIGN - 1);
+        let aligned_size = Self::align_size(size);
         unsafe {
-            if self.top.add(aligned_size) <= self.limit {
+            let available = (self.limit as usize).saturating_sub(self.top as usize);
+            if available >= aligned_size {
                 let ptr = self.top;
                 self.top = self.top.add(aligned_size);
                 ptr
             } else {
                 self.push_slow(aligned_size)
             }
         }
     }

As per coding guidelines: "Follow Rust best practices for error handling and memory management".

#!/bin/bash
set -euo pipefail

# Verify unchecked alignment arithmetic and pointer add in fast path.
rg -nP '\(size \+ ALIGN - 1\)|\.add\(aligned_size\)' crates/vm/src/datastack.rs

# Show the exact fast-path blocks for manual inspection.
sed -n '72,98p' crates/vm/src/datastack.rs

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/datastack.rs` around lines 77 - 90, The push method currently
uses unchecked alignment math `(size + ALIGN - 1)` and calls
`self.top.add(aligned_size)` in the conditional, which can overflow/wrap for
large `size` and invokes .add() without proven in-bounds offsets; fix push in
datastack.rs by (1) computing aligned_size with checked arithmetic (use
checked_add/checked_sub/checked_mul or a helper that returns None on overflow)
and return a null or handle allocation failure on overflow, and (2) replace the
conditional that uses `self.top.add(aligned_size) <= self.limit` with a
bounds-safe check (compute a checked pointer offset or compare sizes using
`(self.limit as usize).saturating_sub(self.top as usize) >= aligned_size` or use
`offset`/`wrapping_offset` only after verifying aligned_size fits) so .add() is
only called when the offset is proven in-bounds; update push to return early on
overflow and ensure pop still receives a valid pointer.