Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions crates/common/src/lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,24 @@ pub type PyRwLockWriteGuard<'a, T> = RwLockWriteGuard<'a, RawRwLock, T>;
pub type PyMappedRwLockWriteGuard<'a, T> = MappedRwLockWriteGuard<'a, RawRwLock, T>;

// can add fn const_{mutex,rw_lock}() if necessary, but we probably won't need to

/// Reset a `PyMutex` to its initial (unlocked) state after `fork()`.
///
/// After `fork()`, locks held by dead parent threads would deadlock in the
/// child. This zeroes the raw lock bytes directly, bypassing the normal unlock
/// path which may interact with parking_lot's internal waiter queues.
///
/// # Safety
///
/// Must only be called from the single-threaded child process immediately
/// after `fork()`, before any other thread is created.
#[cfg(unix)]
pub unsafe fn reinit_mutex_after_fork<T: ?Sized>(mutex: &PyMutex<T>) {
// lock_api::Mutex<R, T> layout: raw R at offset 0, then UnsafeCell<T>.
// Zeroing R resets to unlocked for both parking_lot::RawMutex (AtomicU8)
// and RawCellMutex (Cell<bool>).
unsafe {
let ptr = mutex as *const PyMutex<T> as *mut u8;
core::ptr::write_bytes(ptr, 0, core::mem::size_of::<RawMutex>());
}
Comment on lines +72 to +79
Copy link
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:

cd crates/common && pwd && cat Cargo.toml | grep -A 5 "\[dependencies\]" | head -20

Repository: RustPython/RustPython

Length of output: 255


🏁 Script executed:

cat -n crates/common/src/lock.rs | sed -n '60,85p'

Repository: RustPython/RustPython

Length of output: 1093


🏁 Script executed:

rg "type PyMutex|struct PyMutex|type RawMutex" crates/common/src/lock.rs -B 2 -A 3

Repository: RustPython/RustPython

Length of output: 421


🏁 Script executed:

rg "RawMutex\|RawCellMutex\|parking_lot\|lock_api" crates/common/src/lock.rs | head -20

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

cat crates/common/Cargo.toml

Repository: RustPython/RustPython

Length of output: 1443


🏁 Script executed:

rg "type RawMutex\|type RawCellMutex\|enum RawMutex" crates/common/src/lock.rs -B 2 -A 3

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

rg "thread_mutex|RawMutex\|RawCellMutex" crates/common/src/lock.rs | head -30

Repository: RustPython/RustPython

Length of output: 107


🏁 Script executed:

cat -n crates/common/src/lock.rs | head -60

Repository: RustPython/RustPython

Length of output: 3139


🏁 Script executed:

cat crates/common/src/lock/cell_lock.rs

Repository: RustPython/RustPython

Length of output: 5019


🏁 Script executed:

rg "impl.*Mutex|fn raw\(" crates/common/src/lock.rs -B 2 -A 3

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

web_search
lock_api 0.4 Mutex raw() method API documentation

Repository: RustPython/RustPython

Length of output: 250


🏁 Script executed:

python3 << 'EOF'
import subprocess
import json

# Check if lock_api 0.4 has raw() method by looking at what methods are available
# We'll search crates.io or offline docs if available

result = subprocess.run(
    ["rg", "lock_api", "Cargo.lock", "-A 5"],
    capture_output=True,
    text=True,
    cwd="/home/jailuser/git"
)

print("=== Cargo.lock lock_api info ===")
print(result.stdout[:1000] if result.stdout else "Not found in Cargo.lock")
EOF

Repository: RustPython/RustPython

Length of output: 121


🏁 Script executed:

web_search
lock_api 0.4 Mutex documentation raw method

Repository: RustPython/RustPython

Length of output: 162


🏁 Script executed:

rg "raw\(\)" crates/ --type rs | head -20

Repository: RustPython/RustPython

Length of output: 91


🏁 Script executed:

python3 << 'EOF'
# Check lock_api docs online for 0.4.x
import subprocess
result = subprocess.run(
    ["curl", "-s", "https://docs.rs/lock_api/0.4/lock_api/struct.Mutex.html"],
    capture_output=True,
    text=True,
    timeout=5
)
# Search for "raw()" in the HTML
if "raw()" in result.stdout:
    print("Found raw() method")
    # Extract relevant section
    import re
    matches = re.findall(r'raw\(\)[^<]*<[^>]*>[^<]*', result.stdout)
    for match in matches[:3]:
        print(match)
else:
    print("No raw() method found in docs")
    # Show what methods are listed
    if "pub fn" in result.stdout:
        methods = re.findall(r'pub fn (\w+)', result.stdout)
        print("Available methods:", methods[:10])
EOF

Repository: RustPython/RustPython

Length of output: 94


🏁 Script executed:

# Check if there's documentation or examples in the codebase showing how to access raw lock
rg "Mutex.*raw\|\.raw\(\)" crates/ --type rs -B 2 -A 2

Repository: RustPython/RustPython

Length of output: 91


🏁 Script executed:

# Search for actual usage patterns of PyMutex in fork context
rg "fork\|reinit_mutex" crates/ --type rs -B 3 -A 3

Repository: RustPython/RustPython

Length of output: 91


🌐 Web query:

lock_api 0.4 Mutex raw() method API documentation

💡 Result:

In lock_api 0.4.x, Mutex exposes this method:

  • Signature: pub unsafe fn raw(&self) -> &R
  • What it does: Returns a reference to the underlying raw mutex (R) wrapped by lock_api::Mutex<R, T>. (docs.rs)
  • Why it’s unsafe: Because having access to R can let you unlock the mutex while a MutexGuard still exists, violating Mutex’s safety invariants. (docs.rs)
  • Note from the docs: You’ll typically need use lock_api::RawMutex; in scope to call trait methods on the returned raw mutex. (docs.rs)

Citations:


🏁 Script executed:

rg "raw\(\)" crates/ | grep -E "\.rs:" | head -20

Repository: RustPython/RustPython

Length of output: 1770


🏁 Script executed:

rg "fork|reinit_mutex" crates/ | head -30

Repository: RustPython/RustPython

Length of output: 2864


🏁 Script executed:

# Search lock_api source or documentation for the Mutex type signature
rg "pub struct Mutex|impl.*Mutex" crates/ | head -20

Repository: RustPython/RustPython

Length of output: 2400


🏁 Script executed:

# Look at what imports are available from lock_api
rg "from lock_api|use lock_api" crates/common/src/lock.rs

Repository: RustPython/RustPython

Length of output: 80


🏁 Script executed:

# Check usage of reinit_mutex_after_fork
rg "reinit_mutex_after_fork" crates/ -B 2 -A 2

Repository: RustPython/RustPython

Length of output: 488


🏁 Script executed:

# Check if reinit_after_fork exists in lock module
rg "pub.*fn reinit_after_fork\|reinit_mutex_after_fork\|reinit_imp_lock" crates/common/src/lock.rs -B 3 -A 5

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Check RawMutex trait definition and INIT constant usage
rg "trait RawMutex|const INIT" crates/common/src/lock.rs crates/common/src/lock/cell_lock.rs | head -20

Repository: RustPython/RustPython

Length of output: 265


Use Mutex::raw() and RawMutex::INIT instead of layout-dependent byte writes when reinitializing PyMutex after fork.

The current implementation relies on two undocumented assumptions: (1) lock_api::Mutex places the raw lock at offset 0, and (2) all-zero bytes equal RawMutex::INIT. This is fragile across lock_api/parking_lot version changes.

lock_api 0.4.x provides pub unsafe fn raw(&self) -> &R to access the raw mutex and both parking_lot::RawMutex and RawCellMutex define const INIT. Use these instead:

Suggested fix
 pub unsafe fn reinit_mutex_after_fork<T: ?Sized>(mutex: &PyMutex<T>) {
     unsafe {
-        let ptr = mutex as *const PyMutex<T> as *mut u8;
-        core::ptr::write_bytes(ptr, 0, core::mem::size_of::<RawMutex>());
+        let raw = mutex.raw() as *const RawMutex as *mut RawMutex;
+        core::ptr::write(raw, RawMutex::INIT);
     }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/common/src/lock.rs` around lines 72 - 79, reinit_mutex_after_fork
currently performs raw byte writes assuming the raw mutex is at offset 0 and
all-zero equals initial state; update it to call the unsafe Mutex::raw()
accessor on the PyMutex to get a &RawMutex and reinitialize by writing
RawMutex::INIT into that memory (use core::ptr::write to overwrite the raw lock
with RawMutex::INIT) so you no longer depend on layout or zero bytes; target the
function reinit_mutex_after_fork and the PyMutex type and use RawMutex::INIT (or
the corresponding raw type's INIT) when performing the write.

}
28 changes: 28 additions & 0 deletions crates/vm/src/stdlib/imp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,34 @@ mod lock {
fn lock_held(_vm: &VirtualMachine) -> bool {
IMP_LOCK.is_locked()
}

/// Reset import lock after fork() — only if held by a dead thread.
///
/// `IMP_LOCK` is a reentrant mutex. If the *current* (surviving) thread
/// held it at fork time, the child must be able to release it normally.
/// Only reset if a now-dead thread was the owner.
///
/// # Safety
///
/// Must only be called from single-threaded child after fork().
#[cfg(unix)]
pub(crate) unsafe fn reinit_after_fork() {
if IMP_LOCK.is_locked() && !IMP_LOCK.is_owned_by_current_thread() {
// Held by a dead thread — reset to unlocked.
// Same pattern as RLock::_at_fork_reinit in thread.rs.
unsafe {
let old: &crossbeam_utils::atomic::AtomicCell<RawRMutex> =
core::mem::transmute(&IMP_LOCK);
old.swap(RawRMutex::INIT);
}
}
}
Comment on lines +47 to +57
Copy link
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:

cd /tmp && find . -name "Cargo.lock" -o -name "Cargo.toml" | head -5

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# First, let's examine the actual code in the file
cat -n crates/vm/src/stdlib/imp.rs | head -75

Repository: RustPython/RustPython

Length of output: 2898


🏁 Script executed:

# Check lock_api dependency version
grep -A5 "lock_api" Cargo.lock | head -20

Repository: RustPython/RustPython

Length of output: 457


🏁 Script executed:

# Check the RawRMutex API available in lock_api
python3 <<'EOF'
import subprocess
import re

# Get lock_api version from Cargo.lock
result = subprocess.run(['grep', '-A2', 'name = "lock_api"', 'Cargo.lock'], 
                       capture_output=True, text=True)
lock_api_info = result.stdout

# Extract version
version_match = re.search(r'version = "([^"]+)"', lock_api_info)
if version_match:
    version = version_match.group(1)
    print(f"lock_api version found: {version}")
else:
    print("Could not find lock_api version")
EOF

Repository: RustPython/RustPython

Length of output: 95


🏁 Script executed:

# Check if RawReentrantMutex has layout specification
rg -A5 "struct RawReentrantMutex" crates/

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Search for how IMP_LOCK is used elsewhere in the codebase
rg "IMP_LOCK" crates/vm/src/stdlib/

Repository: RustPython/RustPython

Length of output: 698


🏁 Script executed:

# Check lock_api imports and usage patterns
rg "use.*lock_api" crates/vm/src/

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Check where RawRMutex is defined
rg "type RawRMutex|struct RawRMutex" crates/ --type rust

Repository: RustPython/RustPython

Length of output: 162


🏁 Script executed:

# Check the thread module to see RawRMutex definition
cat -n crates/vm/src/stdlib/thread.rs | head -100

Repository: RustPython/RustPython

Length of output: 4361


🏁 Script executed:

# Look for RawRMutex import in stdlib
rg -B5 -A5 "RawRMutex" crates/vm/src/stdlib/thread.rs | head -40

Repository: RustPython/RustPython

Length of output: 1279


🏁 Script executed:

# Check lock_api crate documentation and source for RawReentrantMutex repr
web_search
lock_api 0.4.14 RawReentrantMutex struct layout repr

Repository: RustPython/RustPython

Length of output: 162


🏁 Script executed:

# Check if the proposed fix's is_owned_by_current_thread method exists
rg "is_owned_by_current_thread|owned_by" crates/

Repository: RustPython/RustPython

Length of output: 229


🌐 Web query:

lock_api 0.4.14 RawReentrantMutex documentation site:docs.rs

💡 Result:

lock_api 0.4.14 docs.rs page for RawReentrantMutex:

Crate landing page (same version on docs.rs):

Sources: [1] [2]


🏁 Script executed:

# Check if is_owned_by_current_thread exists on RawRMutex in the actual code
rg -B2 -A10 "fn is_owned_by_current_thread" crates/

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Search for the parking_lot RawReentrantMutex trait and methods
python3 <<'EOF'
import subprocess
import json

# Check what methods are available on RawRMutex in the current codebase
result = subprocess.run(
    ['rg', '-A30', 'pub type RawRMutex', 'crates/vm/src/stdlib/thread.rs'],
    capture_output=True, text=True
)
print("RawRMutex type definition context:")
print(result.stdout)

# Now check all calls to IMP_LOCK methods
result2 = subprocess.run(
    ['rg', 'IMP_LOCK\\.', 'crates/vm/src/stdlib/imp.rs'],
    capture_output=True, text=True
)
print("\nAll IMP_LOCK method calls:")
print(result2.stdout)
EOF

Repository: RustPython/RustPython

Length of output: 1236


🏁 Script executed:

# Check parking_lot documentation for RawReentrantMutex methods available
python3 <<'EOF'
# Try to find the actual parking_lot source or check if methods exist
import subprocess

# Search for method signatures that might give us hints about available API
result = subprocess.run(
    ['rg', '-B5', '-A15', 'impl.*RawReentrantMutex', 'crates/', '--type', 'rust'],
    capture_output=True, text=True, timeout=10
)

if result.stdout:
    print("RawReentrantMutex implementations found:")
    print(result.stdout[:2000])
else:
    print("No local implementations found - using parking_lot from deps")
    
# Check if the code actually compiles - look for any compilation notes
result2 = subprocess.run(
    ['grep', '-r', 'is_owned_by_current_thread', 'crates/vm/src/stdlib/'],
    capture_output=True, text=True
)
print("\nis_owned_by_current_thread usage:")
print(result2.stdout)
EOF

Repository: RustPython/RustPython

Length of output: 326


🏁 Script executed:

# Examine the actual implementation that calls is_owned_by_current_thread
rg -B5 -A5 "is_owned_by_current_thread" crates/vm/src/stdlib/thread.rs

Repository: RustPython/RustPython

Length of output: 479


🏁 Script executed:

# Check if there's a public method to reset/reinit a RawReentrantMutex
python3 <<'EOF'
import subprocess

# Let's look at what methods are actually available on RawReentrantMutex
# by checking parking_lot or lock_api documentation patterns

# First, check all uses of write_bytes in the codebase to understand intent
result = subprocess.run(
    ['rg', 'write_bytes|write\(ptr', 'crates/', '--type', 'rust', '-B2', '-A2'],
    capture_output=True, text=True
)
print("All write_bytes/write(ptr) patterns:")
print(result.stdout[:2000] if result.stdout else "None found")

# Check if there's any method to check owned_by on RawReentrantMutex
result2 = subprocess.run(
    ['rg', 'owner|owned', 'crates/vm/src/stdlib/thread.rs', '-B1', '-A1'],
    capture_output=True, text=True
)
print("\n\nOwner references in thread.rs:")
print(result2.stdout)
EOF

Repository: RustPython/RustPython

Length of output: 4359


🏁 Script executed:

# Check the actual lock_api source by inspecting what's available in docs
python3 <<'EOF'
import subprocess
import re

# The key question: Does RawReentrantMutex have a public contract around resetting after fork?
# Let's check the parking_lot version in use
result = subprocess.run(
    ['grep', '-A5', 'parking_lot', 'Cargo.lock'],
    capture_output=True, text=True
)
print("parking_lot version info:")
print(result.stdout[:1500])

# Now check if there's any existing fork-handling code that resets mutexes
result2 = subprocess.run(
    ['rg', 'fork|reinit', 'crates/', '--type', 'rust', '-l'],
    capture_output=True, text=True
)
print("\n\nFiles mentioning fork/reinit:")
print(result2.stdout)
EOF

Repository: RustPython/RustPython

Length of output: 1440


🏁 Script executed:

# Check the actual semantics: What is RawReentrantMutex::INIT and can we use it?
python3 <<'EOF'
import subprocess

# Search for how RawRMutex::INIT is defined or used
result = subprocess.run(
    ['rg', 'RawRMutex::INIT|RawReentrantMutex::INIT', 'crates/', '--type', 'rust', '-B2', '-A2'],
    capture_output=True, text=True
)
print("RawRMutex::INIT usage patterns:")
print(result.stdout)
EOF

Repository: RustPython/RustPython

Length of output: 1204


🏁 Script executed:

# Critical check: Does the current unsafe code actually match the assumption?
# Let's see if there's any test or documentation about this layout assumption
python3 <<'EOF'
import subprocess

# Look for any comments or tests about reentrant mutex layout
result = subprocess.run(
    ['rg', '-i', 'atomicusize.*offset|owner.*offset|layout', 'crates/', '--type', 'rust'],
    capture_output=True, text=True
)
print("Layout documentation or comments:")
print(result.stdout if result.stdout else "None found")

# Check parking_lot Cargo.lock entry more carefully
result2 = subprocess.run(
    ['grep', '-B2', '-A20', 'name = "parking_lot"$', 'Cargo.lock'],
    capture_output=True, text=True
)
print("\n\nparking_lot full entry:")
print(result2.stdout)
EOF

Repository: RustPython/RustPython

Length of output: 10353


🏁 Script executed:

# Check the RLock::after_fork implementation more closely
rg -B5 -A20 "fn after_fork" crates/vm/src/stdlib/thread.rs

Repository: RustPython/RustPython

Length of output: 1242


🏁 Script executed:

# Verify what methods RawReentrantMutex actually exposes in lock_api 0.4.14
python3 <<'EOF'
import subprocess

# Check if there's a method to check ownership in the current code usage
result = subprocess.run(
    ['rg', '-B3', '-A3', 'is_locked|is_owned', 'crates/vm/src/stdlib/imp.rs'],
    capture_output=True, text=True
)
print("Usage of is_locked/is_owned in imp.rs:")
print(result.stdout)
EOF

Repository: RustPython/RustPython

Length of output: 524


🏁 Script executed:

# Final verification: Check if write() can be used on a static, and what the correct pattern is
python3 <<'EOF'
import subprocess

# Check the AtomicCell usage pattern in RLock
result = subprocess.run(
    ['rg', '-B5', '-A10', 'AtomicCell.*RawRMutex', 'crates/vm/src/stdlib/thread.rs'],
    capture_output=True, text=True
)
print("AtomicCell pattern for RawRMutex reset:")
print(result.stdout)

# Also check if there's a public method on RawReentrantMutex to "unlock all"
result2 = subprocess.run(
    ['rg', 'lock|unlock', 'crates/vm/src/stdlib/imp.rs', '-B1', '-A1'],
    capture_output=True, text=True
)
print("\n\nLock/unlock operations in imp.rs:")
print(result2.stdout)
EOF

Repository: RustPython/RustPython

Length of output: 2121


Address unsafe memory access that depends on undocumented RawReentrantMutex layout.

Lines 52–60 cast IMP_LOCK to AtomicUsize assuming the owner field is at offset 0, then zero-fill the entire struct with write_bytes(). Both assumptions rely on private lock_api internals and will break if the struct layout changes.

A safer approach is already used elsewhere in the codebase (RLock::after_fork_child in thread.rs): use AtomicCell::swap() with RawRMutex::INIT instead of unsafe layout assumptions. Alternatively, use the public API methods (is_locked() and is_owned_by_current_thread()) to safely check state without inspecting internal fields.

The current implementation violates Rust best practices for memory safety and maintainability.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/stdlib/imp.rs` around lines 47 - 64, The unsafe
reinit_after_fork() inspects and zeroes IMP_LOCK via layout-dependent casts to
AtomicUsize/bytes; replace this with the safer pattern used in
RLock::after_fork_child: set IMP_LOCK to RawRMutex::INIT atomically (e.g., via
AtomicCell::swap) instead of casting/ write_bytes, or else use the public
RawRMutex methods (is_locked()/is_owned_by_current_thread()) to decide whether
to reset via the public API; update reinit_after_fork to avoid direct field
access of RawRMutex and rely on AtomicCell::swap(RawRMutex::INIT) or public
methods to reset state.

}

/// Re-export for fork safety code in posix.rs
#[cfg(all(unix, feature = "threading"))]
pub(crate) unsafe fn reinit_imp_lock_after_fork() {
unsafe { lock::reinit_after_fork() }
}

#[cfg(not(feature = "threading"))]
Expand Down
4 changes: 4 additions & 0 deletions crates/vm/src/stdlib/posix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -737,6 +737,10 @@ pub mod module {
force_unlock_mutex_after_fork(&vm.state.global_trace_func);
force_unlock_mutex_after_fork(&vm.state.global_profile_func);
crate::gc_state::gc_state().force_unlock_after_fork();

// Import lock (ReentrantMutex) — was previously not reinit'd
#[cfg(feature = "threading")]
crate::stdlib::imp::reinit_imp_lock_after_fork();
}

// Mark all other threads as done before running Python callbacks
Expand Down
20 changes: 6 additions & 14 deletions crates/vm/src/stdlib/thread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,15 +152,9 @@ pub(crate) mod _thread {

#[pymethod]
fn _at_fork_reinit(&self, _vm: &VirtualMachine) -> PyResult<()> {
if self.mu.is_locked() {
unsafe {
self.mu.unlock();
};
}
// Casting to AtomicCell is as unsafe as CPython code.
// Using AtomicCell will prevent compiler optimizer move it to somewhere later unsafe place.
// It will be not under the cell anymore after init call.

// Reset the mutex to unlocked by directly writing the INIT value.
// Do NOT call unlock() here — after fork(), unlock_slow() would
// try to unpark stale waiters from dead parent threads.
let new_mut = RawMutex::INIT;
unsafe {
let old_mutex: &AtomicCell<RawMutex> = core::mem::transmute(&self.mu);
Expand Down Expand Up @@ -252,11 +246,9 @@ pub(crate) mod _thread {

#[pymethod]
fn _at_fork_reinit(&self, _vm: &VirtualMachine) -> PyResult<()> {
if self.mu.is_locked() {
unsafe {
self.mu.unlock();
};
}
// Reset the reentrant mutex to unlocked by directly writing INIT.
// Do NOT call unlock() — after fork(), the slow path would try
// to unpark stale waiters from dead parent threads.
self.count.store(0, core::sync::atomic::Ordering::Relaxed);
let new_mut = RawRMutex::INIT;
unsafe {
Expand Down
Loading