Allow creating PyCapsule objects#7595
Conversation
📝 WalkthroughWalkthroughModified the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/builtins/capsule.rs`:
- Around line 28-35: The constructor Capsule::new currently accepts a raw ptr
and stores it without validation; update it to reject NULL pointers by
validating ptr before constructing—either change the signature to return
Result<Self, ErrorType> (e.g., Result<Capsule, InvalidPointerError>) and return
an Err when ptr.is_null(), or perform a defensive check and panic with a clear
message; ensure the check happens before setting self.ptr and mention
PyCapsule_New and PyCapsule_IsValid semantics in the error or panic so callers
know NULL is not allowed, and keep the destructor handling unchanged.
🪄 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: 89365b54-c937-4160-99bf-fe663f7677a7
📒 Files selected for processing (2)
crates/vm/src/builtins/capsule.rscrates/vm/src/vm/context.rs
| pub fn new( | ||
| ptr: *mut c_void, | ||
| destructor: Option<unsafe extern "C" fn(_: *mut PyObject)>, | ||
| ) -> Self { | ||
| Self { | ||
| ptr: ptr.into(), | ||
| destructor, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the capsule.rs file
find . -name "capsule.rs" -type fRepository: RustPython/RustPython
Length of output: 100
🏁 Script executed:
# Once found, read the file to see the constructor at lines 28-35
if [ -f "crates/vm/src/builtins/capsule.rs" ]; then
cat -n "crates/vm/src/builtins/capsule.rs" | head -50
fiRepository: RustPython/RustPython
Length of output: 1782
🏁 Script executed:
# Check the entire capsule.rs file to understand the implementation
if [ -f "crates/vm/src/builtins/capsule.rs" ]; then
wc -l "crates/vm/src/builtins/capsule.rs"
fiRepository: RustPython/RustPython
Length of output: 101
🏁 Script executed:
# Search for any null pointer validation in the capsule implementation
rg -i "null|is_null" crates/vm/src/builtins/capsule.rs -A 3 -B 3Repository: RustPython/RustPython
Length of output: 47
🌐 Web query:
Python C API PyCapsule_New null pointer requirement
💡 Result:
Yes, the Python C API function PyCapsule_New explicitly requires that the pointer argument must not be NULL. Passing a NULL pointer will raise a ValueError: "PyCapsule_New called with null pointer". From the official documentation: PyObject* PyCapsule_New(void *pointer, const char *name, PyCapsule_Destructor destructor) "Create a PyCapsule encapsulating the pointer. The pointer argument may not be NULL. On failure, set an exception and return NULL." This requirement is consistent across Python versions, including the latest (3.14.3) and earlier stable versions like 3.11. A valid capsule must have a non-NULL pointer, as stated in PyCapsule_IsValid: "A valid capsule is non-NULL, passes PyCapsule_CheckExact, has a non-NULL pointer stored in it, and its internal name matches the name parameter." Errors like "PyCapsule_New called with null pointer" occur when libraries (e.g., matplotlib on Windows) inadvertently pass NULL, which is a bug in the caller, not the API itself. Users should ensure the pointer is valid before calling PyCapsule_New and check the return value for NULL (due to memory allocation failure).
Citations:
Validate that the capsule pointer is non-NULL before constructing the object.
The Python C API explicitly forbids NULL pointers in PyCapsule_New() and raises a ValueError if one is passed. Additionally, PyCapsule_IsValid() requires a non-NULL pointer as part of its validity check. This constructor currently accepts null pointers without validation, allowing callers to construct capsules that violate CPython semantics. The non-fallible signature prevents proper error handling; consider returning Result<Self, ...> or validating the pointer before storing it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/builtins/capsule.rs` around lines 28 - 35, The constructor
Capsule::new currently accepts a raw ptr and stores it without validation;
update it to reject NULL pointers by validating ptr before constructing—either
change the signature to return Result<Self, ErrorType> (e.g., Result<Capsule,
InvalidPointerError>) and return an Err when ptr.is_null(), or perform a
defensive check and panic with a clear message; ensure the check happens before
setting self.ptr and mention PyCapsule_New and PyCapsule_IsValid semantics in
the error or panic so callers know NULL is not allowed, and keep the destructor
handling unchanged.
ShaharNaveh
left a comment
There was a problem hiding this comment.
welcome to the project:)
can you please share some information about this change? what is it trying to achieve?
| self.ptr.load(core::sync::atomic::Ordering::Relaxed) | ||
| } | ||
|
|
||
| fn destructor(&self) -> Option<unsafe extern "C" fn(_: *mut PyObject)> { |
There was a problem hiding this comment.
does
| fn destructor(&self) -> Option<unsafe extern "C" fn(_: *mut PyObject)> { | |
| const fn destructor(&self) -> Option<unsafe extern "C" fn(_: *mut PyObject)> { |
compile?
I needed this while experimenting with a capi at #7562. But this might be useful for other usecases as well. |
Allow creation of
PyCapsuleobjects. The destructor is aCfunction for easy interop with other languages.Summary by CodeRabbit
Release Notes