Skip to content
Prev Previous commit
Next Next commit
Guard FastLocals access in locals() with try_lock on state mutex
Address CodeRabbit review: f_locals() could access fastlocals without
synchronization when called from another thread. Use try_lock on the
state mutex so concurrent access is properly serialized.
  • Loading branch information
youknowone committed Mar 1, 2026
commit 67ed603466a4ec7f3ce3dcc20143bc7a3b5269ce
17 changes: 11 additions & 6 deletions crates/vm/src/frame.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,11 @@ impl FrameOwner {
/// Lock-free storage for local variables (localsplus).
///
/// # Safety
/// Access is serialized by the frame's state mutex in `with_exec()`, which
/// prevents concurrent frame execution. Trace callbacks that access `f_locals`
/// run sequentially on the same thread as instruction execution.
/// Mutable access is serialized by the frame's state mutex in `with_exec()`.
/// External readers (e.g. `f_locals`) must use `try_lock` on the state mutex:
/// if acquired, the frame is not executing and access is exclusive; if not,
/// the caller is on the same thread as `with_exec()` (trace callback) and
/// access is safe because frame execution is single-threaded.
pub struct FastLocals {
inner: UnsafeCell<Box<[Option<PyObjectRef>]>>,
}
Expand Down Expand Up @@ -387,12 +389,17 @@ impl Frame {
}

pub fn locals(&self, vm: &VirtualMachine) -> PyResult<ArgMapping> {
// Acquire the state mutex to synchronize with frame execution.
// If try_lock fails, the frame is executing on this thread (e.g.
// trace callback accessing f_locals), so fastlocals access is safe.
let _guard = self.state.try_lock();
let locals = &self.locals;
let code = &**self.code;
let map = &code.varnames;
let j = core::cmp::min(map.len(), code.varnames.len());
if !code.varnames.is_empty() {
// SAFETY: Trace callbacks run sequentially on the same thread.
// SAFETY: Either _guard holds the state mutex (frame not executing),
// or we're in a trace callback on the same thread that holds it.
let fastlocals = unsafe { self.fastlocals.borrow() };
Comment on lines 395 to 407
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's understand the broader context of the locals() method
fd frame.rs | head -5

Repository: RustPython/RustPython

Length of output: 119


🏁 Script executed:

# Read the frame.rs file to understand the full locals() implementation and FastLocals
wc -l crates/vm/src/frame.rs

Repository: RustPython/RustPython

Length of output: 92


🏁 Script executed:

# Get the full locals() method implementation with surrounding context
sed -n '380,425p' crates/vm/src/frame.rs

Repository: RustPython/RustPython

Length of output: 2254


🏁 Script executed:

# Search for FastLocals definition and its safety contract
rg -A 10 "struct FastLocals|type FastLocals" crates/vm/src/frame.rs

Repository: RustPython/RustPython

Length of output: 352


🏁 Script executed:

# Look for thread-safety patterns and comments about try_lock semantics
rg -B 5 -A 5 "try_lock" crates/vm/src/frame.rs

Repository: RustPython/RustPython

Length of output: 1711


🏁 Script executed:

# Check state field type and mutex implementation
rg -B 3 -A 3 "state.*:" crates/vm/src/frame.rs | head -40

Repository: RustPython/RustPython

Length of output: 1316


🏁 Script executed:

# Let's examine the with_exec() method to understand the frame execution model
sed -n '1450,1550p' crates/vm/src/frame.rs

Repository: RustPython/RustPython

Length of output: 4681


🏁 Script executed:

# Check how trace callbacks are called during execution
rg -B 5 -A 10 "call_trace_function|trace_function" crates/vm/src/frame.rs | head -80

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Look for any documentation or architecture comments about the thread model
rg -B 3 -A 8 "trace callback|single-threaded|same thread" crates/vm/src/frame.rs

Repository: RustPython/RustPython

Length of output: 2144


🏁 Script executed:

# Check if there are other methods that use the same try_lock pattern
rg -B 3 -A 8 "try_lock\(\)" crates/vm/src/frame.rs

Repository: RustPython/RustPython

Length of output: 1293


🏁 Script executed:

# Understand the PyMutex type to verify it's a real mutex
rg "type PyMutex|pub.*PyMutex" crates/vm/src/ -A 2 | head -30

Repository: RustPython/RustPython

Length of output: 1869


🏁 Script executed:

# Let's understand the architectural execution model - examine with_exec()
sed -n '1680,1750p' crates/vm/src/frame.rs

Repository: RustPython/RustPython

Length of output: 3429


🏁 Script executed:

# Check if frame execution is indeed serialized and single-threaded
rg -B 5 -A 15 "pub fn with_exec" crates/vm/src/frame.rs | head -80

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Verify the execution is always from a single thread by checking call sites
rg "\.with_exec\(" crates/vm/src/ | head -20

Repository: RustPython/RustPython

Length of output: 310


🏁 Script executed:

# Get the with_exec method definition
rg -B 3 -A 25 "fn with_exec" crates/vm/src/frame.rs

Repository: RustPython/RustPython

Length of output: 1117


🏁 Script executed:

# Check FrameState structure to understand what state is being protected
rg -B 2 -A 15 "struct FrameState|enum FrameState" crates/vm/src/frame.rs

Repository: RustPython/RustPython

Length of output: 624


🏁 Script executed:

# Search for how trace callbacks are invoked during execution
rg -B 5 -A 15 "call_trace" crates/vm/src/frame.rs | head -100

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Look at the call_function implementation to see if trace callbacks are invoked during execution
sed -n '2400,2500p' crates/vm/src/frame.rs | head -60

Repository: RustPython/RustPython

Length of output: 2883


🏁 Script executed:

# Let's verify if there's a GIL (Global Interpreter Lock) or similar mechanism
rg -i "gil|global.*lock|interpreter.*lock" crates/vm/src/vm/mod.rs | head -20

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Check how threads interact with frame execution
rg "thread::|thread_id|current_thread" crates/vm/src/frame.rs | head -20

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Look for any threading safety documentation in the repo
rg -i "threading.*model|concurrent.*frame|same.thread" crates/vm/src/ | head -30

Repository: RustPython/RustPython

Length of output: 466


🏁 Script executed:

# Let's check if locals() is meant to be called only from specific contexts
rg -B 10 "def f_locals\|def locals\|pub fn locals" crates/vm/src/builtins/frame.rs

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Check if trace callbacks are invoked from within with_exec or from outside
rg -B 10 -A 20 "call_trace\|invoke_trace" crates/vm/src/frame.rs | head -150

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Look at where f_locals is accessed from to understand the context
rg "f_locals" crates/vm/src/builtins/frame.rs -B 5 -A 10

Repository: RustPython/RustPython

Length of output: 476


🏁 Script executed:

# Check if there's any runtime assertion or check that ensures same-thread access
rg "thread_id\|thread.*assert\|same.*thread.*check" crates/vm/src/

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Look at the context around where locals() is called
rg "\.locals\(" crates/vm/src/ -B 3 -A 3 | head -50

Repository: RustPython/RustPython

Length of output: 936


🏁 Script executed:

# Let's check if Python code can call f_locals from outside the execution context
# Search for where PyMutex is defined to understand its behavior
rg -B 5 -A 15 "pub struct PyMutex|impl.*PyMutex" crates/vm/src/builtins/mutexobject.rs | head -50

Repository: RustPython/RustPython

Length of output: 142


🏁 Script executed:

# Look for where trace functions are called
rg -B 5 -A 20 "line_event\|call_event\|return_event" crates/vm/src/frame.rs | head -100

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Check the actual behavior of PyMutex try_lock - does it return None if current thread holds it?
rg "try_lock\|lock\(\)" crates/vm/src/ -B 2 -A 5 | grep -A 10 "fn try_lock\|fn lock" | head -40

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Check if PyMutex is parking_lot or std Mutex
fd "Mutex|mutex" crates/vm/src/ -t f | head -10

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Search for PyMutex definition in the codebase
rg "type PyMutex\|pub.*PyMutex" crates/vm/src/ --type rust | head -10

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Search for PyMutex definition more broadly
rg "PyMutex\s*=" crates/vm/src/ --type rust | head -5

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Look in the common module
fd mod.rs crates/vm/src/ | xargs grep -l "PyMutex"

Repository: RustPython/RustPython

Length of output: 88


🏁 Script executed:

# Check pyobject or object files
rg "type PyMutex" crates/vm/src/ | head -10

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Let's look at what module exports PyMutex
rg "pub.*PyMutex|^use.*PyMutex" crates/vm/src/lib.rs

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Search across the entire crates directory
rg "type PyMutex|pub.*type.*Mutex" crates/ | head -20

Repository: RustPython/RustPython

Length of output: 857


🏁 Script executed:

# Let's check the parking_lot Mutex behavior by looking at the lock.rs file
cat crates/common/src/lock.rs | head -80

Repository: RustPython/RustPython

Length of output: 2718


🏁 Script executed:

# Check parking_lot version and understand try_lock semantics
rg "parking_lot" crates/common/Cargo.toml

Repository: RustPython/RustPython

Length of output: 144


🏁 Script executed:

# Look at how try_lock is used in the codebase to understand its semantics
rg "try_lock\(" crates/vm/src/frame.rs -B 2 -A 5

Repository: RustPython/RustPython

Length of output: 831


🏁 Script executed:

# Check if there's documentation on try_lock behavior when current thread already holds the lock
rg -A 30 "pub fn try_lock" crates/common/src/lock.rs

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Check if trace callbacks can be called from outside the with_exec lock
rg -B 20 "call_trace\|trace_function\|line_event\|call_event" crates/vm/src/frame.rs | head -150

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Look for where trace is invoked during frame execution
sed -n '2800,3000p' crates/vm/src/frame.rs | head -120

Repository: RustPython/RustPython

Length of output: 6123


🏁 Script executed:

# Check if Python code can directly call frame.f_locals while frame is executing on another thread
rg "f_locals" crates/vm/src/ -B 5 -A 5 | head -80

Repository: RustPython/RustPython

Length of output: 3906


🏁 Script executed:

# Understand the threading model - can frames be executed on different threads?
rg "spawn\|thread::new\|Thread::new" crates/vm/src/ | head -20

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Check if with_exec holds the lock for the entire duration of execution
sed -n '1700,1750p' crates/vm/src/frame.rs

Repository: RustPython/RustPython

Length of output: 2455


🏁 Script executed:

# Check if there's any protection against calling f_locals from a different thread while frame is executing
# Look at how frame objects are shared/passed
rg "send_frame\|frame.*thread\|cross.thread" crates/vm/src/ | head -20

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Verify that parking_lot mutex's try_lock doesn't distinguish between current thread and other threads
web_search parking_lot mutex try_lock same thread behavior

Repository: RustPython/RustPython

Length of output: 115


🏁 Script executed:

# Check if Python frames are meant to be thread-local
rg "thread_local\|current_frame\|frame.*thread" crates/vm/src/vm/mod.rs | head -30

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Let's verify if try_lock can fail from a different thread holding the lock
python3 << 'EOF'
# Verify parking_lot behavior documentation
# parking_lot Mutex is non-reentrant: if ANY thread holds it, try_lock returns None
# It doesn't return None only for the current thread - it returns None for ANY thread
print("parking_lot::Mutex behavior:")
print("- try_lock() returns Some if NO thread holds the lock")
print("- try_lock() returns None if ANY thread holds the lock")
print("- Does NOT distinguish between 'current thread holds it' vs 'other thread holds it'")
print("\nTherefore:")
print("- If try_lock fails, it could be:")
print("  1. Current thread holds the lock (safe to access)")
print("  2. A different thread holds the lock (UNSAFE to access)")
EOF

Repository: RustPython/RustPython

Length of output: 442


🏁 Script executed:

# Check if there's any ThreadId or thread-awareness in the FastLocals or Frame implementation
rg "thread_id\|current_thread\|ThreadId" crates/vm/src/frame.rs | head -20

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Check if there's a thread-local stack to ensure frames are only on one thread
rg "thread_local\|ThreadLocal" crates/vm/src/vm/mod.rs -A 5 | head -50

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Verify the architectural model - are frames always executed on the thread they were created on?
sed -n '1700,1800p' crates/vm/src/frame.rs

Repository: RustPython/RustPython

Length of output: 4828


🏁 Script executed:

# Check the call stack model in the VM - how frames relate to threads
rg "frame_stack\|frames\|call_stack" crates/vm/src/vm/mod.rs -B 2 -A 5 | head -100

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Look for documentation about frame threading model
rg -B 5 -A 10 "frame.*thread\|thread.*frame" crates/vm/src/frame.rs | head -60

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Check if there's any documentation about thread safety in the threading module
rg -B 5 -A 20 "pub fn exec\|execute.*vm\|VirtualMachine.*thread" crates/vm/src/vm/thread.rs | head -100

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Look for any evidence of frame being tied to a specific thread
rg "owner.*thread\|thread_id\|current_thread" crates/vm/src/frame.rs -B 3 -A 3

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Check how frames are created and passed
rg "Frame::new\|create_frame" crates/vm/src/frame.rs | head -20

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Verify that with_exec holds the lock for the entire execution
sed -n '1707,1730p' crates/vm/src/frame.rs

Repository: RustPython/RustPython

Length of output: 1129


🏁 Script executed:

# Find the complete with_exec implementation
rg -A 40 "fn with_exec.*ExecutingFrame" crates/vm/src/frame.rs

Repository: RustPython/RustPython

Length of output: 1455


🏁 Script executed:

# Check if run() calls in with_exec complete or if execution can be suspended
rg -B 5 -A 30 "pub fn run.*ExecutionResult" crates/vm/src/frame.rs | head -80

Repository: RustPython/RustPython

Length of output: 1147


🏁 Script executed:

# Look for generator/coroutine suspension points
rg "suspend\|yield\|await" crates/vm/src/frame.rs | head -30

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Check what ExecutionResult contains
rg "enum ExecutionResult\|struct ExecutionResult" crates/vm/src/

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Let me verify if there's any architectural guarantee that frames are single-threaded
# Check if VirtualMachine is thread-local
rg "thread_local!\|ThreadLocal" crates/vm/src/ | head -20

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Check the GIL or equivalent - is there a global lock that serializes frame execution?
rg "GIL\|global.*lock\|interpreter.*lock" crates/vm/src/vm/mod.rs -i | head -20

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Verify if Python code can actually create a scenario where f_locals is accessed from a different thread
# Look for examples in tests
find . -name "*.rs" -path "*/tests/*" -exec grep -l "f_locals\|frame.*thread" {} \; 2>/dev/null | head -5

Repository: RustPython/RustPython

Length of output: 47


Guard unsafe fastlocals reads when try_lock fails.

On line 399, try_lock() may fail when any thread holds the lock, not just when the current thread holds it. If try_lock fails, accessing fastlocals via unsafe { self.fastlocals.borrow() } on line 407 violates safety: a different thread executing the frame could be modifying fastlocals concurrently, causing a data race. The assumption that "try_lock failure means same-thread access" lacks runtime verification and contradicts parking_lot::Mutex semantics.

Check _guard before unsafe access and return early if lock acquisition failed.

🤖 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 395 - 407, In locals(), don't assume
try_lock() succeeded: check the result of self.state.try_lock() and only perform
unsafe { self.fastlocals.borrow() } when the guard is Some; if try_lock()
returns None return early with a PyResult error (e.g. a runtime error via the
VM) or an appropriate fallback instead of unsafely reading fastlocals while
another thread may be executing the frame; update the locals() function to
inspect the try_lock outcome before the unsafe borrow to prevent data races.

for (&k, v) in zip(&map[..j], fastlocals) {
match locals.mapping().ass_subscript(k, v.clone(), vm) {
Expand All @@ -403,8 +410,6 @@ impl Frame {
}
}
if !code.cellvars.is_empty() || !code.freevars.is_empty() {
// Access cells through fastlocals to avoid locking state
// (state may be held by with_exec during frame execution).
for (i, &k) in code.cellvars.iter().enumerate() {
let cell_value = self.get_cell_contents(i);
match locals.mapping().ass_subscript(k, cell_value, vm) {
Expand Down