ctypes: fix Windows pip truststore - #8314
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds Changesctypes behavior and Windows validation
Native thread VM state preservation
Sequence Diagram(s)sequenceDiagram
participant NativeThread
participant PyEval_SaveThread
participant SavedThreadState
participant PyEval_RestoreThread
NativeThread->>PyEval_SaveThread: detach and save VM state
PyEval_SaveThread->>SavedThreadState: allocate saved state payload
PyEval_SaveThread-->>NativeThread: return thread-state pointer
NativeThread->>PyEval_RestoreThread: provide thread-state pointer
PyEval_RestoreThread->>SavedThreadState: reconstruct saved payload
PyEval_RestoreThread-->>NativeThread: restore and reattach VM state
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
youknowone
changed the title
9b3ae9d to
7eed565
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/capi/src/pystate.rs (1)
101-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a direct
PyEval_SaveThread/PyEval_RestoreThreadregression test.This test exercises
Python::attach/detachand stop-the-world transitions, but doesn't directly call the newly-implementedPyEval_SaveThread/PyEval_RestoreThreadpair, so the actual save/restore payload round-trip (and the gap noted inthread.rs) isn't covered.#[test] fn save_restore_thread() { Python::attach(|_py| { assert!(current_vm_is_set()); let saved = PyEval_SaveThread(); assert!(!current_vm_is_set()); unsafe { PyEval_RestoreThread(saved) }; assert!(current_vm_is_set()); }); }🤖 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/capi/src/pystate.rs` around lines 101 - 126, Add a direct regression test for the PyEval_SaveThread/PyEval_RestoreThread pair alongside the existing thread-state test. Within Python::attach, assert the VM is set, save the thread state, assert it is cleared, restore the saved payload, and assert the VM is set again; preserve the unsafe restore call as required by the API.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@crates/vm/src/vm/thread.rs`:
- Around line 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.
---
Nitpick comments:
In `@crates/capi/src/pystate.rs`:
- Around line 101-126: Add a direct regression test for the
PyEval_SaveThread/PyEval_RestoreThread pair alongside the existing thread-state
test. Within Python::attach, assert the VM is set, save the thread state, assert
it is cleared, restore the saved payload, and assert the VM is set again;
preserve the unsafe restore call as required by the API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e979c9cc-fe81-4dc5-a393-410cb3d16348
📒 Files selected for processing (2)
crates/capi/src/pystate.rscrates/vm/src/vm/thread.rs
| /// 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); | ||
| } |
There was a problem hiding this comment.
🩺 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_thread — VmRef::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.
| /// 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
|
@bschoenmaeckers Could you please review if C API changes are reasonable? |
| } | ||
|
|
||
| #[repr(C)] | ||
| struct SavedPyThreadState { |
There was a problem hiding this comment.
This can be merged into the PyThreadState struct. You may add private fields after the interp pub field.
c7d7ece to
c200885
Compare
2 participants
Summary
errcheckcallback returns the exactargstuplec_char_pandc_wchar_pinstances when initializing matching pointer arraysstdlib_ctypes.pyregressions and a Windows pip HTTPS smoke test using pip's vendored truststorePyEval_SaveThread/PyEval_RestoreThreadso blocking C API callers detach from stop-the-worldCloses #8281.
Root cause
RustPython unconditionally replaced a foreign function's result with the value returned by
errcheck. Truststore returns the originalargstuple to request normal result processing, so RustPython changed the Windows certificate-store handle into a tuple. Passing that tuple to the nextc_void_pargument raisedTypeError: wrong type.After preserving the handle, truststore reached its enhanced-key-usage setup and initialized a
c_char_parray from an existingc_char_pinstance. RustPython's specialized string-pointer array writer accepted bytes and integer addresses, but not a matching ctypes pointer instance.CI hang root cause
The Ubuntu C API job exposed an existing threading bug:
PyEval_SaveThreadandPyEval_RestoreThreadwere no-ops. A parent test thread could therefore remain attached while joining a child that requested stop-the-world, leaving each thread waiting for the other. The implementation now preserves the VM stack and GILState-owned VM across detach/restore, and the C API test forces a stop-the-world cycle to cover the deadlock deterministically.Validation
uvx prek run --all-filescargo clippy -p rustpython-vm -p rustpython-capi --all-targetsuvx zizmor --min-severity low .github/workflows/ci.yamlensurepipfollowed bypip download sixover HTTPS on WindowsSummary by CodeRabbit
_ctypeserrcheckhandling so the original result is only replaced when the checker returns a different value._ctypesarray element writing forc_char_p/c_wchar_p-style pointer types to write pointer values directly from compatible simple instances._ctypessnippet coverage for char pointers and Windowskernel32ctypes validation.