-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix _at_fork_reinit to write INIT directly instead of calling unlock() #7312
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: cd /tmp && find . -name "Cargo.lock" -o -name "Cargo.toml" | head -5Repository: 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 -75Repository: RustPython/RustPython Length of output: 2898 🏁 Script executed: # Check lock_api dependency version
grep -A5 "lock_api" Cargo.lock | head -20Repository: 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")
EOFRepository: 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 rustRepository: 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 -100Repository: 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 -40Repository: 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 reprRepository: 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:
💡 Result:
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)
EOFRepository: 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)
EOFRepository: 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.rsRepository: 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)
EOFRepository: 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)
EOFRepository: 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)
EOFRepository: 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)
EOFRepository: 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.rsRepository: 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)
EOFRepository: 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)
EOFRepository: RustPython/RustPython Length of output: 2121 Address unsafe memory access that depends on undocumented Lines 52–60 cast A safer approach is already used elsewhere in the codebase ( The current implementation violates Rust best practices for memory safety and maintainability. 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| /// 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"))] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 255
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 1093
🏁 Script executed:
rg "type PyMutex|struct PyMutex|type RawMutex" crates/common/src/lock.rs -B 2 -A 3Repository: RustPython/RustPython
Length of output: 421
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 1443
🏁 Script executed:
rg "type RawMutex\|type RawCellMutex\|enum RawMutex" crates/common/src/lock.rs -B 2 -A 3Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 107
🏁 Script executed:
cat -n crates/common/src/lock.rs | head -60Repository: RustPython/RustPython
Length of output: 3139
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 5019
🏁 Script executed:
rg "impl.*Mutex|fn raw\(" crates/common/src/lock.rs -B 2 -A 3Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
web_search lock_api 0.4 Mutex raw() method API documentationRepository: RustPython/RustPython
Length of output: 250
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 121
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 162
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 91
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 94
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 91
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 91
🌐 Web query:
lock_api 0.4 Mutex raw() method API documentation💡 Result:
In
lock_api0.4.x,Mutexexposes this method:pub unsafe fn raw(&self) -> &RR) wrapped bylock_api::Mutex<R, T>. (docs.rs)unsafe: Because having access toRcan let you unlock the mutex while aMutexGuardstill exists, violatingMutex’s safety invariants. (docs.rs)use lock_api::RawMutex;in scope to call trait methods on the returned raw mutex. (docs.rs)Citations:
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 1770
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 2864
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 2400
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 80
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 488
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 265
Use
Mutex::raw()andRawMutex::INITinstead of layout-dependent byte writes when reinitializingPyMutexafter fork.The current implementation relies on two undocumented assumptions: (1)
lock_api::Mutexplaces the raw lock at offset 0, and (2) all-zero bytes equalRawMutex::INIT. This is fragile acrosslock_api/parking_lotversion changes.lock_api0.4.x providespub unsafe fn raw(&self) -> &Rto access the raw mutex and bothparking_lot::RawMutexandRawCellMutexdefineconst 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