GeezerVison

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

1322-1329: Consolidate duplicated dict-key snapshot branches.

Both branches do the same map/collect after selecting key source; extracting only the differing source keeps this tighter and easier to maintain.

Refactor sketch

-        } else if cls.is(self.ctx.types.dict_type) {
-            // Atomic snapshot of dict keys for thread-safe iteration.
-            let keys = value.downcast_ref::<PyDict>().unwrap().keys_vec();
-            return keys.into_iter().map(func).collect();
-        } else if cls.is(self.ctx.types.dict_keys_type) {
-            // Atomic snapshot of dict keys - prevents race condition during iteration
-            let keys = value.downcast_ref::<PyDictKeys>().unwrap().dict.keys_vec();
+        } else if cls.is(self.ctx.types.dict_type) || cls.is(self.ctx.types.dict_keys_type) {
+            let keys = if cls.is(self.ctx.types.dict_type) {
+                value.downcast_ref::<PyDict>().unwrap().keys_vec()
+            } else {
+                value.downcast_ref::<PyDictKeys>().unwrap().dict.keys_vec()
+            };
             return keys.into_iter().map(func).collect();

As per coding guidelines, "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code".

🤖 Prompt for AI Agents

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

In `@crates/vm/src/vm/mod.rs` around lines 1322 - 1329, The two branches that
handle dict and dict_keys are duplicated: both compute an atomic snapshot via
keys_vec() and then map/collect with func; refactor by first selecting the key
source into a single variable (e.g., if cls.is(self.ctx.types.dict_type) use
value.downcast_ref::<PyDict>().unwrap().keys_vec(), else if
cls.is(self.ctx.types.dict_keys_type) use
value.downcast_ref::<PyDictKeys>().unwrap().dict.keys_vec()), assign that Vec to
a local variable (e.g., keys), and then perform
keys.into_iter().map(func).collect() once; update the code around the cls checks
and preserve the unwrap() calls and func usage so behavior is unchanged.

crates/compiler-core/src/bytecode.rs (1)

630-635: Document read_cache_ptr panic behavior for API parity.

This method indexes directly and can panic on out-of-bounds; adding # Panics would align it with read_cache_u16.

Suggested doc update

     /// Load a pointer-sized value atomically from the pointer cache at `index`.
     ///
     /// Uses a single `AtomicUsize` load to prevent torn reads.
+    ///
+    /// # Panics
+    /// Panics if `index` is out of bounds.
     pub fn read_cache_ptr(&self, index: usize) -> usize {
         self.pointer_cache[index].load(Ordering::Relaxed)
     }

🤖 Prompt for AI Agents

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

In `@crates/compiler-core/src/bytecode.rs` around lines 630 - 635, The doc comment
for read_cache_ptr is missing a Panics section describing that direct indexing
into pointer_cache can panic on out-of-bounds; update the documentation for pub
fn read_cache_ptr(&self, index: usize) -> usize to include a "# Panics"
paragraph mirroring read_cache_u16, stating that it will panic if index is out
of bounds of self.pointer_cache (because it uses direct indexing), so callers
know the behavior and API parity is maintained.

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

3702-3704: ⚠️ Potential issue | 🔴 Critical

Avoid popping owner before confirming property getter exists.

Line 3702 pops from the stack before prop.get_fget() is validated at Line 3703. If fget is missing, fallback to load_attr_slow() runs with a mutated stack and can double-pop.

Proposed fix

-                            let owner = self.pop_value();
-                            if let Some(getter) = prop.get_fget() {
+                            if let Some(getter) = prop.get_fget() {
+                                let owner = self.pop_value();
                                 let result = getter.call((owner,), vm)?;
                                 self.push_value(result);
                                 return Ok(None);
                             }

🤖 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 3702 - 3704, The code pops the owner
unconditionally before checking prop.get_fget(), which corrupts the stack if
fget is None and load_attr_slow() runs; change the flow so you first check
prop.get_fget() and only call self.pop_value() when the getter exists (i.e.,
move the let owner = self.pop_value() inside the if let Some(getter) branch),
and ensure the else path calls load_attr_slow() without having mutated the stack
(use existing stack state/peek rather than pop). Update uses of
getter.call((owner,), vm) accordingly so owner is only created when needed.

crates/vm/src/builtins/type.rs (1)

786-788: ⚠️ Potential issue | 🔴 Critical

Do not form &*ptr before ownership upgrade; this is still a UAF/UB window.

At Line 786, creating &PyObject from raw ptr happens before successful try_to_owned(). If another thread drops the cached strong ref first, that dereference is invalid.

Suggested direction

- let obj: &PyObject = unsafe { &*ptr };
- if let Some(cloned) = obj.try_to_owned() {
+ // Upgrade from raw pointer atomically first; only then materialize references.
+ if let Some(cloned) = unsafe { PyObject::try_to_owned_raw(ptr) } {
     let same_ptr = core::ptr::eq(entry.value.load(Ordering::Relaxed), ptr);
     if same_ptr && entry.end_read(seq1) {
         return Some(cloned);
     }
     drop(cloned);
     continue;
 }

(If try_to_owned_raw does not exist yet, add it in crates/vm/src/object/core.rs and keep all pre-upgrade handling pointer-only.)

#!/bin/bash
set -euo pipefail

echo "== cache read path around raw deref =="
sed -n '770,800p' crates/vm/src/builtins/type.rs

echo
echo "== all raw-pointer -> reference conversions in type cache path =="
rg -n -C3 'unsafe \{ &\*ptr \}|try_to_owned\(' crates/vm/src/builtins/type.rs

🤖 Prompt for AI Agents

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

In `@crates/vm/src/builtins/type.rs` around lines 786 - 788, The code currently
forms a reference with `let obj: &PyObject = unsafe { &*ptr };` before calling
`obj.try_to_owned()`, which can cause a UAF/UB if another thread drops the
cached strong ref; change the upgrade path to operate on the raw pointer only
(e.g., add and use a pointer-based upgrade like `try_to_owned_raw` in
crates/vm/src/object/core.rs) so you never create `&PyObject` before the
ownership is confirmed; update the call site in the type cache (the block using
`ptr`, `entry.value.load(Ordering::Relaxed)`, and `try_to_owned`) to call the
new raw-pointer upgrade routine and only create a safe reference after the
ownership upgrade succeeds and the pointer is still the same.

crates/compiler-core/src/bytecode.rs (1)

619-638: Add focused tests for the new pointer-cache API.

Please add direct coverage for round-trip read/write and out-of-bounds panic behavior to prevent regressions in this concurrency-critical path.

✅ Suggested tests

 #[cfg(test)]
 mod tests {
     use super::*;
     use alloc::{vec, vec::Vec};

+    #[test]
+    fn test_pointer_cache_roundtrip_and_clone() {
+        let units = CodeUnits::from(vec![CodeUnit::new(Instruction::Cache, 0u8.into())]);
+        unsafe {
+            units.write_cache_ptr(0, 0x1234usize);
+        }
+        assert_eq!(units.read_cache_ptr(0), 0x1234usize);
+
+        let cloned = units.clone();
+        assert_eq!(cloned.read_cache_ptr(0), 0x1234usize);
+    }
+
+    #[test]
+    #[should_panic]
+    fn test_pointer_cache_oob_panics() {
+        let units = CodeUnits::from(vec![CodeUnit::new(Instruction::Cache, 0u8.into())]);
+        let _ = units.read_cache_ptr(1);
+    }
+
     #[test]
     fn test_exception_table_encode_decode() {

🤖 Prompt for AI Agents

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

In `@crates/compiler-core/src/bytecode.rs` around lines 619 - 638, Add focused
unit tests exercising the pointer-cache API: create tests that (1) perform a
round-trip using write_cache_ptr and read_cache_ptr to store and retrieve a
usize value (covering typical and edge values) and assert equality, and (2)
assert that read_cache_ptr and unsafe write_cache_ptr panic or trigger UB when
given an out-of-bounds index (use std::panic::catch_unwind or #[should_panic] as
appropriate). Locate usages of pointer_cache, read_cache_ptr, and
write_cache_ptr to implement these tests, and include concurrent access variants
if possible to exercise atomic behavior.

crates/vm/src/builtins/type.rs (1)

807-809: ⚠️ Potential issue | 🔴 Critical

Unsafe &*ptr is created before ownership upgrade.

At Line 807, the code builds &PyObject from a raw cache pointer before try_to_owned(). Under concurrent writer replacement/drop, this can create a reference to freed memory.

Suggested direction

- let obj: &PyObject = unsafe { &*ptr };
- if let Some(cloned) = obj.try_to_owned() {
+ // Avoid creating `&PyObject` before ownership is safely upgraded.
+ // Use a raw-pointer upgrade helper (e.g. `PyObject::try_to_owned_from_ptr(ptr)`)
+ // that performs safe refcount CAS first, then materializes owned ref.
+ if let Some(cloned) = unsafe { PyObject::try_to_owned_from_ptr(ptr) } {

#!/bin/bash
set -euo pipefail

echo "=== Cache-hit dereference site ==="
rg -n -C4 'let obj: &PyObject = unsafe \{ &\*ptr \};|try_to_owned\(' crates/vm/src/builtins/type.rs

echo
echo "=== Ownership upgrade APIs currently available ==="
rg -n -C4 'fn try_to_owned|safe_inc' crates/vm/src/object/core.rs crates/common/src/refcount.rs

🤖 Prompt for AI Agents

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

In `@crates/vm/src/builtins/type.rs` around lines 807 - 809, The code currently
forms a borrowed reference let obj: &PyObject = unsafe { &*ptr } before
attempting an ownership upgrade, which can produce a dangling reference under
concurrent replacement; instead, perform the ownership upgrade first from the
raw pointer (use the available try_to_owned API on the raw pointer/handle
returned by entry.value.load(Ordering::Relaxed)) and only if try_to_owned
succeeds obtain a safe reference from the owned handle (e.g.,
owned.as_ref()/deref) to use for validation; also compute same_ptr against the
original raw pointer value from entry.value.load so comparisons remain correct
(avoid creating any &PyObject from ptr until after successful try_to_owned).

crates/vm/src/stdlib/thread.rs (1)

295-303: ⚠️ Potential issue | 🟠 Major

_acquire_restore currently ignores saved owner and can accept cross-thread state.

At Line 302, owner is parsed but discarded. That lets a different thread call _acquire_restore with foreign saved state and proceed, which diverges from the expected restore contract.

Suggested fix

 fn _acquire_restore(&self, state: PyTupleRef, vm: &VirtualMachine) -> PyResult<()> {
     let [count_obj, owner_obj] = state.as_slice() else {
         return Err(
             vm.new_type_error("_acquire_restore() argument 1 must be a 2-item tuple")
         );
     };
     let count: usize = count_obj.clone().try_into_value(vm)?;
-    let _owner: u64 = owner_obj.clone().try_into_value(vm)?;
+    let owner: u64 = owner_obj.clone().try_into_value(vm)?;
+    if owner != current_thread_id() {
+        return Err(vm.new_runtime_error(
+            "cannot restore lock acquired by another thread".to_owned(),
+        ));
+    }
     if count == 0 {
         return Ok(());
     }

#!/bin/bash
set -euo pipefail

echo "=== Rust _thread RLock restore implementation ==="
sed -n '281,312p' crates/vm/src/stdlib/thread.rs

echo
echo "=== Python threading protocol reference ==="
sed -n '251,263p' Lib/threading.py

🤖 Prompt for AI Agents

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

In `@crates/vm/src/stdlib/thread.rs` around lines 295 - 303, The _acquire_restore
implementation parses but ignores the saved owner; change it to validate that
the restored owner matches the current thread before accepting the state. In
function _acquire_restore, keep the parsed owner (currently assigned to _owner),
fetch the current thread's native id (or equivalent runtime thread id used
elsewhere in this module), and if owner != current_thread_id return an
Err(vm.new_runtime_error(...) or appropriate PyError) so cross-thread restore is
rejected; only when the owner matches and count is valid should you proceed with
the acquire restore logic. Ensure you reference and use the symbols
_acquire_restore, owner (parsed from state), and whatever helper/field provides
the current thread id in this module.