Add capsule support to c-api#7940
Conversation
📝 WalkthroughWalkthroughThe PR adds PyCapsule support to RustPython's C-ABI layer. The PyCapsule type is extended to store optional capsule names and context pointers. The VM's capsule constructor is updated to pass names through. CAPI utilities for C string handling are added, followed by a complete implementation of eight Python-compatible PyCapsule functions (New, GetPointer, GetName, GetContext, SetContext, SetPointer, IsValid, Import). ChangesPyCapsule C-ABI Implementation with Name and Context Support
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
crates/capi/src/util.rs (1)
79-85: 💤 Low valueConsider using
null()for const pointer.
core::ptr::null()is more idiomatic for*consttypes thannull_mut(), even though both work due to coercion.♻️ Suggested change
impl FfiResult for *const c_char { - const ERR_VALUE: *const c_char = core::ptr::null_mut(); + const ERR_VALUE: *const c_char = core::ptr::null(); fn into_output(self, _vm: &VirtualMachine) -> *const c_char { self } }🤖 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/util.rs` around lines 79 - 85, The impl of FfiResult for *const c_char uses core::ptr::null_mut() for ERR_VALUE; change it to core::ptr::null() to be idiomatic for const pointers (update the const ERR_VALUE in the impl and leave into_output(self, _vm: &VirtualMachine) -> *const c_char unchanged) so the null value reflects a *const type.crates/capi/src/pycapsule.rs (1)
134-149: ⚖️ Poor tradeoffTest uses pyo3's
PyCapsuleinstead of testing the C-API functions directly.The test validates capsule behavior through pyo3's wrapper API (using
PyCapsule::new_with_value(),is_valid_checked(),pointer_checked()), but does not exercise the C-API functions defined in this file (PyCapsule_New,PyCapsule_GetPointer,PyCapsule_GetName, etc.). Consider adding tests that call the C-API functions directly to verify the implementation.🤖 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/pycapsule.rs` around lines 134 - 149, The test currently exercises pyo3's PyCapsule wrapper but not the raw C-API implementations; add unit tests that directly call the C-API functions (e.g., PyCapsule_New, PyCapsule_GetPointer, PyCapsule_GetName, PyCapsule_IsValid) to validate your bindings: create a capsule via PyCapsule_New with a known pointer and name, assert PyCapsule_GetName returns the expected name, assert PyCapsule_GetPointer returns the original pointer (and that casting yields the original value), and test PyCapsule_IsValid for matching and non-matching names; place these tests alongside the existing ones and use Python::attach to obtain a PyO3 GIL for calling the unsafe C-API functions.
🤖 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/capi/src/pycapsule.rs`:
- Around line 41-49: The function PyCapsule_GetContext dereferences the raw
pointer `capsule` without checking for null; update it to first check if
`capsule` is null and return a null pointer on error, then proceed to
dereference and call downcast_ref_if_exact::<PyCapsule>(vm) as before (preserve
the existing with_vm usage and error path). Ensure the function returns a *mut
c_void null when the input is null or when downcast/with_vm fails, and keep
references to symbols PyCapsule_GetContext, with_vm, and
downcast_ref_if_exact::<PyCapsule> to locate the change.
- Around line 51-59: In PyCapsule_SetContext add a null-pointer check for the
incoming capsule before performing unsafe dereference: if capsule.is_null()
return an error (e.g., propagate a failure c_int) and set a Python error via the
VM (use vm.new_value_error or similar) rather than dereferencing; then proceed
to call downcast_ref_if_exact::<PyCapsule> and capsule.set_context(context) only
after the pointer is confirmed non-null.
- Around line 31-39: PyCapsule_GetName dereferences the raw pointer without a
null check which can UB if caller passes null; modify PyCapsule_GetName to
early-return a null C string pointer when capsule.is_null() (mirroring
PyCapsule_GetPointer), i.e., check the raw capsule pointer before the unsafe {
&*capsule } dereference and return std::ptr::null() (or equivalent) on null;
keep the rest of the with_vm flow and the downcast_ref_if_exact::<PyCapsule>(vm)
handling unchanged so valid capsules still resolve to
capsule.name().map(CStr::as_ptr).unwrap_or_default().
- Around line 26-29: The function PyCapsule_GetPointer dereferences capsule
without checking for null; add a null check at the start (if capsule.is_null())
and return std::ptr::null_mut() immediately to avoid undefined behavior, then
call with_vm and checked_capsule using unsafe { &*capsule } only after the null
check; mirror the null-handling pattern used in PyCapsule_IsValid to keep
behavior consistent.
- Around line 82-102: The function PyCapsule_Import must check for a null name
pointer before calling CStr::from_ptr to avoid UB; in PyCapsule_Import add an
initial null check (if name.is_null()) that sets an appropriate Python error via
the VM (e.g., vm.new_system_error or vm.new_import_error) and returns a null
*mut c_void, then proceed with the existing logic that calls CStr::from_ptr,
vm.import, attribute traversal, and checked_capsule; ensure the null-check is
performed before any unsafe dereference and that the function consistently
returns a null pointer when an error is set.
- Around line 61-69: The PyCapsule_SetPointer implementation must validate the
incoming raw pointers before dereferencing: check that the `capsule` (raw *mut
PyObject) is not null and return an error c_int if it is, then inside with_vm
still downcast the referenced object to `PyCapsule` as currently done;
additionally, enforce that the `pointer` argument is not null and return the
CPython-compatible error code when it is (matching other capsule APIs in this
crate). Update the function handling around `PyCapsule_SetPointer`, referencing
the `PyCapsule` type, the `with_vm` wrapper, and the `capsule`/`pointer`
parameters so null checks occur before any unsafe dereference and the function
returns the appropriate c_int error on null inputs.
In `@crates/vm/src/builtins/capsule.rs`:
- Line 17: The field and accessor for the capsule name currently use
Option<&'static CStr> which is too strict; change the capsule struct field
(currently "name: Option<&'static CStr>") to store a raw pointer (Option<*const
c_char>) and adjust the getter (the method returning the name at lines ~30–33
and similar at ~61–63) to return Option<&CStr> with a lifetime tied to &self by
checking for null and performing an unsafe dereference (e.g., convert the
pointer to &CStr inside the getter), ensuring you only assume the pointer is
valid for the capsule's lifetime rather than 'static; update all places
referencing the original &'static CStr type accordingly.
---
Nitpick comments:
In `@crates/capi/src/pycapsule.rs`:
- Around line 134-149: The test currently exercises pyo3's PyCapsule wrapper but
not the raw C-API implementations; add unit tests that directly call the C-API
functions (e.g., PyCapsule_New, PyCapsule_GetPointer, PyCapsule_GetName,
PyCapsule_IsValid) to validate your bindings: create a capsule via PyCapsule_New
with a known pointer and name, assert PyCapsule_GetName returns the expected
name, assert PyCapsule_GetPointer returns the original pointer (and that casting
yields the original value), and test PyCapsule_IsValid for matching and
non-matching names; place these tests alongside the existing ones and use
Python::attach to obtain a PyO3 GIL for calling the unsafe C-API functions.
In `@crates/capi/src/util.rs`:
- Around line 79-85: The impl of FfiResult for *const c_char uses
core::ptr::null_mut() for ERR_VALUE; change it to core::ptr::null() to be
idiomatic for const pointers (update the const ERR_VALUE in the impl and leave
into_output(self, _vm: &VirtualMachine) -> *const c_char unchanged) so the null
value reflects a *const type.
🪄 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: 73238666-03e7-42c7-8749-db5d7d9f8378
📒 Files selected for processing (5)
crates/capi/src/lib.rscrates/capi/src/pycapsule.rscrates/capi/src/util.rscrates/vm/src/builtins/capsule.rscrates/vm/src/vm/context.rs
| #[unsafe(no_mangle)] | ||
| pub extern "C" fn PyCapsule_GetPointer(capsule: *mut PyObject, name: *const c_char) -> *mut c_void { | ||
| with_vm(|vm| Ok(checked_capsule(vm, unsafe { &*capsule }, name)?.pointer())) | ||
| } |
There was a problem hiding this comment.
Missing null check before dereferencing capsule pointer.
Dereferencing capsule without a null check leads to undefined behavior if callers pass a null pointer. Other functions like PyCapsule_IsValid (line 74) correctly check for null before dereferencing.
🛡️ Proposed fix
#[unsafe(no_mangle)]
pub extern "C" fn PyCapsule_GetPointer(capsule: *mut PyObject, name: *const c_char) -> *mut c_void {
with_vm(|vm| {
+ if capsule.is_null() {
+ return Err(vm.new_value_error("PyCapsule_GetPointer called with null capsule"));
+ }
Ok(checked_capsule(vm, unsafe { &*capsule }, name)?.pointer())
})
}🤖 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/pycapsule.rs` around lines 26 - 29, The function
PyCapsule_GetPointer dereferences capsule without checking for null; add a null
check at the start (if capsule.is_null()) and return std::ptr::null_mut()
immediately to avoid undefined behavior, then call with_vm and checked_capsule
using unsafe { &*capsule } only after the null check; mirror the null-handling
pattern used in PyCapsule_IsValid to keep behavior consistent.
| #[unsafe(no_mangle)] | ||
| pub extern "C" fn PyCapsule_GetName(capsule: *mut PyObject) -> *const c_char { | ||
| with_vm(|vm| { | ||
| let capsule = unsafe { &*capsule } | ||
| .downcast_ref_if_exact::<PyCapsule>(vm) | ||
| .ok_or_else(|| vm.new_value_error("Invalid capsule"))?; | ||
| Ok(capsule.name().map(CStr::as_ptr).unwrap_or_default()) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Missing null check before dereferencing capsule pointer.
Same issue as PyCapsule_GetPointer — dereferencing without null check causes UB if caller passes null.
🛡️ Proposed fix
#[unsafe(no_mangle)]
pub extern "C" fn PyCapsule_GetName(capsule: *mut PyObject) -> *const c_char {
with_vm(|vm| {
+ if capsule.is_null() {
+ return Err(vm.new_value_error("PyCapsule_GetName called with null capsule"));
+ }
let capsule = unsafe { &*capsule }
.downcast_ref_if_exact::<PyCapsule>(vm)
.ok_or_else(|| vm.new_value_error("Invalid capsule"))?;
Ok(capsule.name().map(CStr::as_ptr).unwrap_or_default())
})
}🤖 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/pycapsule.rs` around lines 31 - 39, PyCapsule_GetName
dereferences the raw pointer without a null check which can UB if caller passes
null; modify PyCapsule_GetName to early-return a null C string pointer when
capsule.is_null() (mirroring PyCapsule_GetPointer), i.e., check the raw capsule
pointer before the unsafe { &*capsule } dereference and return std::ptr::null()
(or equivalent) on null; keep the rest of the with_vm flow and the
downcast_ref_if_exact::<PyCapsule>(vm) handling unchanged so valid capsules
still resolve to capsule.name().map(CStr::as_ptr).unwrap_or_default().
| #[unsafe(no_mangle)] | ||
| pub extern "C" fn PyCapsule_GetContext(capsule: *mut PyObject) -> *mut c_void { | ||
| with_vm(|vm| { | ||
| let capsule = unsafe { &*capsule } | ||
| .downcast_ref_if_exact::<PyCapsule>(vm) | ||
| .ok_or_else(|| vm.new_value_error("Invalid capsule"))?; | ||
| Ok(capsule.context()) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Missing null check before dereferencing capsule pointer.
Same pattern as above — add null check before dereferencing.
🤖 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/pycapsule.rs` around lines 41 - 49, The function
PyCapsule_GetContext dereferences the raw pointer `capsule` without checking for
null; update it to first check if `capsule` is null and return a null pointer on
error, then proceed to dereference and call
downcast_ref_if_exact::<PyCapsule>(vm) as before (preserve the existing with_vm
usage and error path). Ensure the function returns a *mut c_void null when the
input is null or when downcast/with_vm fails, and keep references to symbols
PyCapsule_GetContext, with_vm, and downcast_ref_if_exact::<PyCapsule> to locate
the change.
| #[unsafe(no_mangle)] | ||
| pub extern "C" fn PyCapsule_SetContext(capsule: *mut PyObject, context: *mut c_void) -> c_int { | ||
| with_vm(|vm| { | ||
| let capsule = unsafe { &*capsule } | ||
| .downcast_ref_if_exact::<PyCapsule>(vm) | ||
| .ok_or_else(|| vm.new_value_error("Invalid capsule"))?; | ||
| Ok(capsule.set_context(context)) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Missing null check before dereferencing capsule pointer.
Same pattern — add null check before dereferencing.
🤖 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/pycapsule.rs` around lines 51 - 59, In PyCapsule_SetContext
add a null-pointer check for the incoming capsule before performing unsafe
dereference: if capsule.is_null() return an error (e.g., propagate a failure
c_int) and set a Python error via the VM (use vm.new_value_error or similar)
rather than dereferencing; then proceed to call
downcast_ref_if_exact::<PyCapsule> and capsule.set_context(context) only after
the pointer is confirmed non-null.
| #[unsafe(no_mangle)] | ||
| pub extern "C" fn PyCapsule_SetPointer(capsule: *mut PyObject, pointer: *mut c_void) -> c_int { | ||
| with_vm(|vm| { | ||
| let capsule = unsafe { &*capsule } | ||
| .downcast_ref_if_exact::<PyCapsule>(vm) | ||
| .ok_or_else(|| vm.new_value_error("Invalid capsule"))?; | ||
| Ok(capsule.set_pointer(pointer)) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Missing null checks for both capsule and pointer.
- Same null capsule dereference issue as other functions.
- CPython's
PyCapsule_SetPointerreturns an error ifpointeris NULL — this should be validated for compatibility.
🛡️ Proposed fix
#[unsafe(no_mangle)]
pub extern "C" fn PyCapsule_SetPointer(capsule: *mut PyObject, pointer: *mut c_void) -> c_int {
with_vm(|vm| {
+ if capsule.is_null() {
+ return Err(vm.new_value_error("PyCapsule_SetPointer called with null capsule"));
+ }
+ if pointer.is_null() {
+ return Err(vm.new_value_error("PyCapsule_SetPointer called with null pointer"));
+ }
let capsule = unsafe { &*capsule }
.downcast_ref_if_exact::<PyCapsule>(vm)
.ok_or_else(|| vm.new_value_error("Invalid capsule"))?;
Ok(capsule.set_pointer(pointer))
})
}🤖 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/pycapsule.rs` around lines 61 - 69, The PyCapsule_SetPointer
implementation must validate the incoming raw pointers before dereferencing:
check that the `capsule` (raw *mut PyObject) is not null and return an error
c_int if it is, then inside with_vm still downcast the referenced object to
`PyCapsule` as currently done; additionally, enforce that the `pointer` argument
is not null and return the CPython-compatible error code when it is (matching
other capsule APIs in this crate). Update the function handling around
`PyCapsule_SetPointer`, referencing the `PyCapsule` type, the `with_vm` wrapper,
and the `capsule`/`pointer` parameters so null checks occur before any unsafe
dereference and the function returns the appropriate c_int error on null inputs.
| #[unsafe(no_mangle)] | ||
| pub extern "C" fn PyCapsule_Import(name: *const c_char, _no_block: c_int) -> *mut c_void { | ||
| with_vm(|vm| { | ||
| let capsule_name = unsafe { CStr::from_ptr(name) } | ||
| .to_str() | ||
| .map_err(|_| vm.new_system_error("capsule name is not valid UTF-8"))?; | ||
| let (module_name, attrs_path) = capsule_name.split_once('.').ok_or_else(|| { | ||
| vm.new_import_error( | ||
| "capsule name is missing attribute path", | ||
| vm.ctx.new_str(capsule_name), | ||
| ) | ||
| })?; | ||
| let mut obj: PyObjectRef = vm.import(module_name, 0)?.into(); | ||
|
|
||
| for attr in attrs_path.split('.') { | ||
| obj = obj.get_attr(attr, vm)?; | ||
| } | ||
|
|
||
| Ok(checked_capsule(vm, &obj, name)?.pointer()) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Missing null check for name parameter.
CStr::from_ptr(name) on line 85 is undefined behavior if name is null. CPython's PyCapsule_Import also requires a non-null name.
🛡️ Proposed fix
#[unsafe(no_mangle)]
pub extern "C" fn PyCapsule_Import(name: *const c_char, _no_block: c_int) -> *mut c_void {
with_vm(|vm| {
+ if name.is_null() {
+ return Err(vm.new_value_error("PyCapsule_Import called with null name"));
+ }
let capsule_name = unsafe { CStr::from_ptr(name) }
.to_str()
.map_err(|_| vm.new_system_error("capsule name is not valid UTF-8"))?;🤖 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/pycapsule.rs` around lines 82 - 102, The function
PyCapsule_Import must check for a null name pointer before calling
CStr::from_ptr to avoid UB; in PyCapsule_Import add an initial null check (if
name.is_null()) that sets an appropriate Python error via the VM (e.g.,
vm.new_system_error or vm.new_import_error) and returns a null *mut c_void, then
proceed with the existing logic that calls CStr::from_ptr, vm.import, attribute
traversal, and checked_capsule; ensure the null-check is performed before any
unsafe dereference and that the function consistently returns a null pointer
when an error is set.
| pub struct PyCapsule { | ||
| ptr: AtomicPtr<c_void>, | ||
| context: AtomicPtr<c_void>, | ||
| name: Option<&'static CStr>, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In CPython’s C-API docs for PyCapsule_New / PyCapsule_GetName, is the capsule name required to be process-static, or only valid for the capsule lifetime?
💡 Result:
In CPython’s C-API, the name provided to PyCapsule_New or PyCapsule_SetName is not required to be process-static (i.e., a literal string constant), but it must remain valid for at least the lifetime of the capsule [1][2]. According to the official documentation, if a non-NULL name is provided, "this string must outlive the capsule" [1][2]. The documentation explicitly notes that it is permitted to free the name string inside the capsule's destructor [1][2], which confirms that the string's lifetime only needs to be tied to the capsule's existence, rather than the lifetime of the entire process.
Citations:
Avoid requiring 'static for capsule names.
Lines 17 and 32 enforce &'static CStr, but CPython's C-API only requires the name to outlive the capsule itself, not the entire process. This stricter constraint forces unsafe lifetime widening in FFI code and violates Rust's memory safety best practices.
Use *const c_char and return Option<&CStr> from the getter by safely dereferencing the pointer, validating that the pointer remains valid for the capsule's lifetime.
Proposed direction
-use core::ffi::{CStr, c_void};
+use core::ffi::{c_char, CStr, c_void};
pub struct PyCapsule {
ptr: AtomicPtr<c_void>,
context: AtomicPtr<c_void>,
- name: Option<&'static CStr>,
+ name: *const c_char,
destructor: Option<unsafe extern "C" fn(_: *mut PyObject)>,
}
impl PyCapsule {
pub fn new(
ptr: *mut c_void,
- name: Option<&'static CStr>,
+ name: *const c_char,
destructor: Option<unsafe extern "C" fn(_: *mut PyObject)>,
) -> Self {
Self {
ptr: ptr.into(),
context: core::ptr::null_mut::<c_void>().into(),
name,
destructor,
}
}
- pub fn name(&self) -> Option<&'static CStr> {
- self.name
+ pub fn name(&self) -> Option<&CStr> {
+ let name = self.name;
+ if name.is_null() {
+ None
+ } else {
+ // SAFETY: caller guarantees capsule name pointer validity for capsule lifetime.
+ Some(unsafe { CStr::from_ptr(name) })
+ }
}
}Also applies to: lines 30–33, 61–63.
🤖 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/builtins/capsule.rs` at line 17, The field and accessor for the
capsule name currently use Option<&'static CStr> which is too strict; change the
capsule struct field (currently "name: Option<&'static CStr>") to store a raw
pointer (Option<*const c_char>) and adjust the getter (the method returning the
name at lines ~30–33 and similar at ~61–63) to return Option<&CStr> with a
lifetime tied to &self by checking for null and performing an unsafe dereference
(e.g., convert the pointer to &CStr inside the getter), ensuring you only assume
the pointer is valid for the capsule's lifetime rather than 'static; update all
places referencing the original &'static CStr type accordingly.
| pub mod import; | ||
| pub mod longobject; | ||
| pub mod object; | ||
| pub mod pycapsule; |
There was a problem hiding this comment.
This term should probably be added to the cpython-more.txt CSpell list.
Summary by CodeRabbit