Visitar URL original
Use try_lock in py_os_after_fork_child by youknowone · Pull Request #7178 · RustPython/RustPython · GitHub
Skip to content

Use try_lock in py_os_after_fork_child #7178

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.

Sign up for GitHub

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

Merged
youknowone merged 1 commit into from
Feb 17, 2026
Merged

Use try_lock in py_os_after_fork_child #7178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 2 additions & 17 deletions crates/vm/src/stdlib/nt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pub use module::raw_set_handle_inheritable;
#[pymodule(name = "nt", with(super::os::_os))]
pub(crate) mod module {
use crate::{
Py, PyObjectRef, PyResult, TryFromObject, VirtualMachine,
Py, PyResult, TryFromObject, VirtualMachine,
builtins::{PyBaseExceptionRef, PyDictRef, PyListRef, PyStrRef, PyTupleRef},
common::{crt_fd, suppress_iph, windows::ToWideString},
convert::ToPyException,
Expand Down Expand Up @@ -1212,21 +1212,6 @@ pub(crate) mod module {
}
}

fn envobj_to_dict(env: ArgMapping, vm: &VirtualMachine) -> PyResult<PyDictRef> {
let obj = env.obj();
if let Some(dict) = obj.downcast_ref_if_exact::<crate::builtins::PyDict>(vm) {
return Ok(dict.to_owned());
}
let keys = vm.call_method(obj, "keys", ())?;
let dict = vm.ctx.new_dict();
for key in keys.get_iter(vm)?.into_iter::<PyObjectRef>(vm)? {
let key = key?;
let val = obj.get_item(&*key, vm)?;
dict.set_item(&*key, val, vm)?;
}
Ok(dict)
}

#[cfg(target_env = "msvc")]
#[pyfunction]
fn execve(
Expand Down Expand Up @@ -1261,7 +1246,7 @@ pub(crate) mod module {
.chain(once(core::ptr::null()))
.collect();

let env = envobj_to_dict(env, vm)?;
let env = crate::stdlib::os::envobj_to_dict(env, vm)?;
// Build environment strings as "KEY=VALUE\0" wide strings
let mut env_strings: Vec<widestring::WideCString> = Vec::new();
for (key, value) in env.into_iter() {
Expand Down
30 changes: 28 additions & 2 deletions crates/vm/src/stdlib/os.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@

use crate::{
AsObject, Py, PyObjectRef, PyPayload, PyResult, TryFromObject, VirtualMachine,
builtins::{PyModule, PySet},
builtins::{PyDictRef, PyModule, PySet},
common::crt_fd,
convert::{IntoPyException, ToPyException, ToPyObject},
function::{ArgumentError, FromArgs, FuncArgs},
function::{ArgMapping, ArgumentError, FromArgs, FuncArgs},
};
use std::{fs, io, path::Path};

Expand Down Expand Up @@ -2038,6 +2038,32 @@ pub fn module_exec(vm: &VirtualMachine, module: &Py<PyModule>) -> PyResult<()> {
Ok(())
}

/// Convert a mapping (e.g. os._Environ) to a plain dict for use by execve/posix_spawn.
///
/// For `os._Environ`, accesses the internal `_data` dict directly at the Rust level.
/// This avoids Python-level method calls that can deadlock after fork() when
/// parking_lot locks are held by threads that no longer exist.
pub(crate) fn envobj_to_dict(env: ArgMapping, vm: &VirtualMachine) -> PyResult<PyDictRef> {
let obj = env.obj();
if let Some(dict) = obj.downcast_ref_if_exact::<crate::builtins::PyDict>(vm) {
return Ok(dict.to_owned());
}
if let Some(inst_dict) = obj.dict()
&& let Ok(Some(data)) = inst_dict.get_item_opt("_data", vm)
&& let Some(dict) = data.downcast_ref_if_exact::<crate::builtins::PyDict>(vm)
{
return Ok(dict.to_owned());
}
let keys = vm.call_method(obj, "keys", ())?;
let dict = vm.ctx.new_dict();
for key in keys.get_iter(vm)?.into_iter::<PyObjectRef>(vm)? {
let key = key?;
let val = obj.get_item(&*key, vm)?;
dict.set_item(&*key, val, vm)?;
}
Ok(dict)
}

#[cfg(not(windows))]
use super::posix as platform;

Expand Down
27 changes: 10 additions & 17 deletions crates/vm/src/stdlib/posix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,7 +716,15 @@ pub mod module {
vm.signal_handlers
.get_or_init(crate::signal::new_signal_handlers);

let after_forkers_child: Vec<PyObjectRef> = vm.state.after_forkers_child.lock().clone();
let after_forkers_child = match vm.state.after_forkers_child.try_lock() {
Some(guard) => guard.clone(),
None => {
// SAFETY: After fork in child process, only the current thread
// exists. The lock holder no longer exists.
unsafe { vm.state.after_forkers_child.force_unlock() };
vm.state.after_forkers_child.lock().clone()
}
};
Comment on lines +719 to +727

@coderabbitai coderabbitai Bot Feb 17, 2026

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:

#!/bin/bash
# Find the definition of after_forkers_child and its type
rg -n "after_forkers_child" crates -g '*.rs' -B 2 -A 2 | head -100

Repository: RustPython/RustPython

Length of output: 2657

🏁 Script executed:

#!/bin/bash
# Check the vm.state struct definition to see the lock type
rg -n "struct.*State" crates/vm -g '*.rs' -A 30 | grep -A 30 "after_forkers"

Repository: RustPython/RustPython

Length of output: 1621

🏁 Script executed:

#!/bin/bash
# Look at the exact location in posix.rs to understand context
cat -n crates/vm/src/stdlib/posix.rs | sed -n '710,740p'

Repository: RustPython/RustPython

Length of output: 1693

🏁 Script executed:

#!/bin/bash
# Find PyMutex definition
rg -n "type.*PyMutex|struct.*PyMutex|PyMutex.*=" crates -g '*.rs' | head -20

Repository: RustPython/RustPython

Length of output: 406

🏁 Script executed:

#!/bin/bash
# Search for PyMutex definition more broadly
rg -n "PyMutex" crates/vm/src/vm/mod.rs -B 5 -A 5 | head -50

Repository: RustPython/RustPython

Length of output: 1833

🏁 Script executed:

#!/bin/bash
# Look for the imports and PyMutex definition
fd -e rs -x grep -l "PyMutex" {} \; | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && head -50 {}'

Repository: RustPython/RustPython

Length of output: 7889

🏁 Script executed:

#!/bin/bash
# Look at the lock.rs file to understand RawMutex and force_unlock
cat -n crates/common/src/lock.rs | head -80

Repository: RustPython/RustPython

Length of output: 3131

🏁 Script executed:

#!/bin/bash
# Check the parking_lot crate version and RawMutex type
rg -n "RawMutex|parking_lot" crates/common/src/lock.rs -A 2 -B 2

Repository: RustPython/RustPython

Length of output: 1102

🏁 Script executed:

#!/bin/bash
# Look at what run_at_forkers does when given empty vec
rg -n "fn run_at_forkers" crates/vm/src/stdlib/posix.rs -A 15

Repository: RustPython/RustPython

Length of output: 715

🏁 Script executed:

#!/bin/bash
# Search for force_unlock in parking_lot or lock_api documentation/usage
rg -n "force_unlock" crates -g '*.rs' -B 2 -A 2

Repository: RustPython/RustPython

Length of output: 926

🏁 Script executed:

#!/bin/bash
# Check if there are any tests or comments explaining force_unlock usage
rg -n "force_unlock|unsafe.*unlock" . -g '*.rs' --type-list | grep -i force

Repository: RustPython/RustPython

Length of output: 454

🏁 Script executed:

#!/bin/bash
# Look for any other similar fork handling code to see patterns
rg -n "after_fork|fork.*child|try_lock.*None" crates/vm/src/stdlib/posix.rs -B 3 -A 3

Repository: RustPython/RustPython

Length of output: 2568

Use safe callback skipping instead of unsafe force_unlock in the forked child.

The current code calls unsafe { force_unlock() } to recover from a failed try_lock() after fork, relying on the fact that the lock-holding thread no longer exists. While this approach aligns with POSIX fork semantics, it requires unsafe code that is harder to reason about and maintain.

A cleaner, safer alternative is to simply skip callbacks when the lock cannot be acquired, since callbacks are optional cleanup that already includes exception handling. Replace the unsafe block with:

-        let after_forkers_child = match vm.state.after_forkers_child.try_lock() {
-            Some(guard) => guard.clone(),
-            None => {
-                // SAFETY: After fork in child process, only the current thread
-                // exists. The lock holder no longer exists.
-                unsafe { vm.state.after_forkers_child.force_unlock() };
-                vm.state.after_forkers_child.lock().clone()
-            }
-        };
+        let after_forkers_child = vm
+            .state
+            .after_forkers_child
+            .try_lock()
+            .map(|guard| guard.clone())
+            .unwrap_or_default();
         run_at_forkers(after_forkers_child, false, vm);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let after_forkers_child = match vm.state.after_forkers_child.try_lock() {
Some(guard) => guard.clone(),
None => {
// SAFETY: After fork in child process, only the current thread
// exists. The lock holder no longer exists.
unsafe { vm.state.after_forkers_child.force_unlock() };
vm.state.after_forkers_child.lock().clone()
}
};
let after_forkers_child = vm
.state
.after_forkers_child
.try_lock()
.map(|guard| guard.clone())
.unwrap_or_default();
run_at_forkers(after_forkers_child, false, vm);
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/stdlib/posix.rs` around lines 719 - 727, The unsafe recovery
using vm.state.after_forkers_child.force_unlock() should be removed; instead,
when vm.state.after_forkers_child.try_lock() returns None in the forked child,
treat callbacks as optional and skip them (e.g., return an empty clone/Vec or
no-op) rather than forcing the mutex open. Update the match on
vm.state.after_forkers_child.try_lock() so the None branch does not call unsafe
force_unlock() but returns a safe empty value or sentinel so downstream code
that uses after_forkers_child simply sees "no callbacks" and proceeds safely.

run_at_forkers(after_forkers_child, false, vm);
}

Expand Down Expand Up @@ -1073,21 +1081,6 @@ pub mod module {
.map_err(|err| err.into_pyexception(vm))
}

fn envobj_to_dict(env: ArgMapping, vm: &VirtualMachine) -> PyResult<PyDictRef> {
let obj = env.obj();
if let Some(dict) = obj.downcast_ref_if_exact::<crate::builtins::PyDict>(vm) {
return Ok(dict.to_owned());
}
let keys = vm.call_method(obj, "keys", ())?;
let dict = vm.ctx.new_dict();
for key in keys.get_iter(vm)?.into_iter::<PyObjectRef>(vm)? {
let key = key?;
let val = obj.get_item(&*key, vm)?;
dict.set_item(&*key, val, vm)?;
}
Ok(dict)
}

#[pyfunction]
fn execve(
path: OsPath,
Expand All @@ -1110,7 +1103,7 @@ pub mod module {
return Err(vm.new_value_error("execve() arg 2 first element cannot be empty"));
}

let env = envobj_to_dict(env, vm)?;
let env = crate::stdlib::os::envobj_to_dict(env, vm)?;
let env = env
.into_iter()
.map(|(k, v)| -> PyResult<_> {
Expand Down
Loading