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
12 changes: 11 additions & 1 deletion .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,17 @@ jobs:
target/release/rustpython -m ensurepip
target/release/rustpython -c "import pip"

- if: runner.os == 'Windows'
name: Check pip HTTPS with the Windows trust store
run: >-
target/release/rustpython -m pip download
--disable-pip-version-check
--no-cache-dir
--no-deps
--only-binary=:all:
--dest "$env:RUNNER_TEMP\rustpython-pip-smoke"
six

- if: runner.os != 'Windows'
name: Check if pip inside venv is functional
run: |
Expand Down Expand Up @@ -829,4 +840,3 @@ jobs:

- name: cargo doc
run: cargo doc --locked

40 changes: 29 additions & 11 deletions crates/capi/src/pystate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ use crate::get_main_interpreter;
use crate::pylifecycle::request_vm_from_interpreter;
use crate::util::FfiResult;
use core::ffi::c_int;
use core::ptr;
use rustpython_vm::vm::thread::{
CurrentVmAttachState, attach_current_thread, release_current_thread, with_current_vm,
CurrentVmAttachState, SavedThreadState, attach_current_thread, release_current_thread,
restore_current_thread, save_current_thread, with_current_vm,
};
use rustpython_vm::{Interpreter, VirtualMachine};

Expand All @@ -22,6 +22,7 @@ pub type PyInterpreterState = Interpreter;
#[repr(C)]
pub struct PyThreadState {
pub interp: *mut PyInterpreterState,
vm: SavedThreadState,
}

/// Make sure this thread has a running vm attached. This only creates a new vm if we don't already
Expand All @@ -47,11 +48,22 @@ pub extern "C" fn PyGILState_Release(state: PyGILState_STATE) {

#[unsafe(no_mangle)]
pub extern "C" fn PyEval_SaveThread() -> *mut PyThreadState {
ptr::null_mut()
let interp = PyInterpreterState_Get();
let state = Box::new(PyThreadState {
interp,
vm: save_current_thread(),
});
Box::into_raw(state)
}

#[unsafe(no_mangle)]
pub extern "C" fn PyEval_RestoreThread(_state: *mut PyThreadState) {}
pub unsafe extern "C" fn PyEval_RestoreThread(state: *mut PyThreadState) {
assert!(!state.is_null(), "PyEval_RestoreThread called with null");
// SAFETY: PyEval_SaveThread returns this allocation and CPython's API
// requires callers to restore exactly that thread state once.
let state = unsafe { Box::from_raw(state) };
restore_current_thread(state.vm);
}

#[unsafe(no_mangle)]
pub extern "C" fn PyInterpreterState_Get() -> *mut PyInterpreterState {
Expand Down Expand Up @@ -81,26 +93,32 @@ mod tests {

#[test]
fn new_thread() {
Python::attach(|_py| {
Python::attach(|py| {
with_current_vm(|_vm| {
assert!(
current_vm_is_set(),
"This thread did not have a vm attached"
)
});

std::thread::spawn(move || {
let handle = std::thread::spawn(move || {
Python::attach(|_py| {
with_current_vm(|_vm| {
with_current_vm(|vm| {
assert!(
current_vm_is_set(),
"This thread did not have a vm attached"
)
);
vm.state.stop_the_world.stop_the_world(vm);
vm.state.stop_the_world.start_the_world(vm);
});
});
})
.join()
.unwrap();
});

py.detach(|| {
assert!(!current_vm_is_set());
handle.join().unwrap();
});
assert!(current_vm_is_set());
})
}

Expand Down
3 changes: 2 additions & 1 deletion crates/stdlib/src/overlapped.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1163,7 +1163,8 @@ mod _overlapped {

#[pyfunction]
fn GetQueuedCompletionStatus(port: isize, msecs: u32, vm: &VirtualMachine) -> PyResult {
match host_overlapped::get_queued_completion_status(port, msecs)
match vm
.allow_threads(|| host_overlapped::get_queued_completion_status(port, msecs))
.map_err(|err| set_from_windows_err(err.raw_os_error().unwrap_or(0) as u32, vm))?
{
host_overlapped::WaitResult::Timeout => Ok(vm.ctx.none()),
Expand Down
16 changes: 16 additions & 0 deletions crates/vm/src/stdlib/_ctypes/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,14 @@ impl PyCArray {
let (kept_alive, ptr) = super::base::ensure_z_null_terminated(bytes, vm);
zelf.0.keep_alive(index, kept_alive);
(ptr, Some(value.to_owned()))
} else if let Some(simple) = value.downcast_ref::<super::PyCSimple>()
&& value.class().type_code(vm).as_deref() == Some("z")
{
let buffer = simple.0.buffer.read();
(
rustpython_host_env::ctypes::read_pointer_from_buffer(&buffer),
None,
)
} else if let Ok(int_val) = value.try_index(vm) {
(int_val.as_bigint().to_usize().unwrap_or(0), None)
} else {
Expand All @@ -667,6 +675,14 @@ impl PyCArray {
} else if let Some(s) = value.downcast_ref::<PyStr>() {
let (holder, ptr) = super::base::str_to_wchar_bytes(s.as_wtf8(), vm);
(ptr, Some(holder))
} else if let Some(simple) = value.downcast_ref::<super::PyCSimple>()
&& value.class().type_code(vm).as_deref() == Some("Z")
{
let buffer = simple.0.buffer.read();
(
rustpython_host_env::ctypes::read_pointer_from_buffer(&buffer),
None,
)
} else if let Ok(int_val) = value.try_index(vm) {
(int_val.as_bigint().to_usize().unwrap_or(0), None)
} else {
Expand Down
6 changes: 5 additions & 1 deletion crates/vm/src/stdlib/_ctypes/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1554,7 +1554,11 @@ fn build_result(
let args_tuple = PyTuple::new_ref(args.args.clone(), &vm.ctx);
let func_obj = zelf.as_object().to_owned();
let result_obj = result.clone().unwrap_or_else(|| vm.ctx.none());
result = Some(errcheck.call((result_obj, func_obj, args_tuple), vm)?);
let checked = errcheck.call((result_obj, func_obj, args_tuple.clone()), vm)?;
// Returning the original args tuple requests normal result processing.
if !checked.is(&args_tuple) {
result = Some(checked);
}
}

// Handle OUT parameter return values
Expand Down
55 changes: 55 additions & 0 deletions crates/vm/src/vm/thread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,61 @@ pub enum CurrentVmAttachState {
Attached,
}

/// State preserved while the current native thread is detached from its VM.
#[cfg(feature = "threading")]
pub struct SavedThreadState {
vm_stack: Vec<NonNull<VirtualMachine>>,
gilstate_vm: Option<Box<ThreadedVirtualMachine>>,
}

/// Detach the current native thread and preserve its VM context for restoration.
#[cfg(feature = "threading")]
#[must_use = "the saved thread state must be restored"]
pub fn save_current_thread() -> SavedThreadState {
let vm_stack = VM_STACK.with(|vms| core::mem::take(&mut *vms.borrow_mut()));
assert!(
!vm_stack.is_empty(),
"save_current_thread() called without an attached VM"
);
let gilstate_vm = GILSTATE_VM.with(|gilstate_vm| gilstate_vm.borrow_mut().take());
detach_thread();
SavedThreadState {
vm_stack,
gilstate_vm,
}
}

/// Restore a VM context previously returned by [`save_current_thread`].
#[cfg(feature = "threading")]
pub fn restore_current_thread(state: SavedThreadState) {
assert!(
!current_vm_is_set(),
"restore_current_thread() called with an attached VM"
);
let SavedThreadState {
vm_stack,
gilstate_vm,
} = state;
let vm = vm_stack
.last()
.copied()
.expect("saved thread state has no VM");

GILSTATE_VM.with(|current| {
let mut current = current.borrow_mut();
assert!(
current.is_none(),
"restore_current_thread() called with a GILState VM"
);
*current = gilstate_vm;
});

// SAFETY: borrowed VMs remain alive for the dynamic save/restore scope,
// while an owned GILState VM was restored above before this dereference.
attach_thread(unsafe { vm.as_ref() });
VM_STACK.with(|vms| *vms.borrow_mut() = vm_stack);
}
Comment on lines +251 to +280

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.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

restore_current_thread skips thread-slot initialization before attach_thread.

attach_thread (Line 380-410) only performs the ATTACHED/DETACHED CAS and QSBR.online when CURRENT_THREAD_SLOT is already Some; if the slot is None it silently no-ops. Every other path that calls attach_threadVmRef::new (Line 183-203) and attach_current_thread (context snippet) — first calls init_thread_slot_if_needed(vm). restore_current_thread calls attach_thread directly without that init step.

This is safe only when restore always runs on the exact same native thread that previously called save_current_thread (whose slot was already initialized before the save). But CPython's own PyEval_RestoreThread contract explicitly allows attaching "whichever thread calls it" — i.e. handing the saved state to a different native thread is a legitimate, documented use case. On such a thread, this gap leaves the thread un-registered for stop-the-world/QSBR tracking while VM_STACK/current_vm_is_set() reports it as attached — a silent, hard-to-diagnose state divergence rather than a clear panic.

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" — extracting a shared attach_vm_to_current_thread helper (used by VmRef::new, attach_current_thread, and restore_current_thread) would both fix this gap and remove the duplication.

🔧 Proposed fix
     // SAFETY: borrowed VMs remain alive for the dynamic save/restore scope,
     // while an owned GILState VM was restored above before this dereference.
-    attach_thread(unsafe { vm.as_ref() });
+    let vm_ref = unsafe { vm.as_ref() };
+    init_thread_slot_if_needed(vm_ref);
+    attach_thread(vm_ref);
     VM_STACK.with(|vms| *vms.borrow_mut() = vm_stack);
📝 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
/// Restore a VM context previously returned by [`save_current_thread`].
#[cfg(feature = "threading")]
pub fn restore_current_thread(state: SavedThreadState) {
assert!(
!current_vm_is_set(),
"restore_current_thread() called with an attached VM"
);
let SavedThreadState {
vm_stack,
gilstate_vm,
} = state;
let vm = vm_stack
.last()
.copied()
.expect("saved thread state has no VM");
GILSTATE_VM.with(|current| {
let mut current = current.borrow_mut();
assert!(
current.is_none(),
"restore_current_thread() called with a GILState VM"
);
*current = gilstate_vm;
});
// SAFETY: borrowed VMs remain alive for the dynamic save/restore scope,
// while an owned GILState VM was restored above before this dereference.
attach_thread(unsafe { vm.as_ref() });
VM_STACK.with(|vms| *vms.borrow_mut() = vm_stack);
}
/// Restore a VM context previously returned by [`save_current_thread`].
#[cfg(feature = "threading")]
pub fn restore_current_thread(state: SavedThreadState) {
assert!(
!current_vm_is_set(),
"restore_current_thread() called with an attached VM"
);
let SavedThreadState {
vm_stack,
gilstate_vm,
} = state;
let vm = vm_stack
.last()
.copied()
.expect("saved thread state has no VM");
GILSTATE_VM.with(|current| {
let mut current = current.borrow_mut();
assert!(
current.is_none(),
"restore_current_thread() called with a GILState VM"
);
*current = gilstate_vm;
});
// SAFETY: borrowed VMs remain alive for the dynamic save/restore scope,
// while an owned GILState VM was restored above before this dereference.
let vm_ref = unsafe { vm.as_ref() };
init_thread_slot_if_needed(vm_ref);
attach_thread(vm_ref);
VM_STACK.with(|vms| *vms.borrow_mut() = vm_stack);
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/vm/src/vm/thread.rs` around lines 251 - 280, Update
restore_current_thread and the existing VmRef::new and attach_current_thread
paths to share an attach_vm_to_current_thread helper that initializes the thread
slot with init_thread_slot_if_needed before calling attach_thread. Ensure the
helper accepts the VM to attach and preserves each caller’s existing VM source,
so restoring state on a different native thread registers it for ATTACHED/QSBR
tracking before updating the VM stack.

Source: Coding guidelines


/// Attach the current native thread to a RustPython VM until
/// `release_current_thread()` is called.
#[cfg(feature = "threading")]
Expand Down
31 changes: 29 additions & 2 deletions extra_tests/snippets/stdlib_ctypes.py
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,10 @@ def __repr__(self):

_check_size(c_char_p, "P")

char_pointer = c_char_p(b"1.3.6.1.5.5.7.3.1")
char_pointer_array = (c_char_p * 1)(char_pointer)
assert char_pointer_array[0] == b"1.3.6.1.5.5.7.3.1"


class c_void_p(_SimpleCData):
_type_ = "P"
Expand Down Expand Up @@ -344,7 +348,9 @@ def LoadLibrary(self, name):
# print(libc.srand(i))
# print(test_byte_array)
else:
import ctypes
import os
from ctypes import wintypes

libc = cdll.msvcrt
libc.rand()
Expand All @@ -356,6 +362,29 @@ def LoadLibrary(self, name):
# print("start printf")
# libc.printf(test_byte_array)

kernel32 = ctypes.WinDLL("kernel32", use_last_error=True)
get_current_process = kernel32.GetCurrentProcess
get_current_process.argtypes = ()
get_current_process.restype = ctypes.c_void_p

def preserve_result(_result, _func, args):
return args

get_current_process.errcheck = preserve_result
process_handle = get_current_process()
assert isinstance(process_handle, int)

get_process_id = kernel32.GetProcessId
get_process_id.argtypes = (ctypes.c_void_p,)
get_process_id.restype = wintypes.DWORD
assert get_process_id(process_handle) == os.getpid()

def replace_result(_result, _func, _args):
return "replacement"

get_current_process.errcheck = replace_result
assert get_current_process() == "replacement"

# windows pip support

def get_win_folder_via_ctypes(csidl_name: str) -> str:
Expand All @@ -364,8 +393,6 @@ def get_win_folder_via_ctypes(csidl_name: str) -> str:
# Use 'CSIDL_PROFILE' (40) and append the default folder 'Downloads' instead.
# https://learn.microsoft.com/en-us/windows/win32/shell/knownfolderid

import ctypes # noqa: PLC0415

csidl_const = {
"CSIDL_APPDATA": 26,
"CSIDL_COMMON_APPDATA": 35,
Expand Down
Loading