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
46 changes: 39 additions & 7 deletions crates/vm/src/builtins/capsule.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
use super::PyType;
use crate::{Context, Py, PyPayload, PyResult, class::PyClassImpl, types::Representable};
use crate::{
AsObject, Context, Py, PyObject, PyPayload, PyResult, VirtualMachine,
class::PyClassImpl,
types::{Destructor, Representable},
};
use core::ffi::c_void;
use core::sync::atomic::AtomicPtr;

/// PyCapsule - a container for C pointers.
/// In RustPython, this is a minimal implementation for compatibility.
#[pyclass(module = false, name = "PyCapsule")]
#[derive(Debug, Clone, Copy)]
#[derive(Debug)]
pub struct PyCapsule {
// Capsules store opaque pointers; we don't expose the actual pointer functionality
// since RustPython doesn't have the same C extension model as CPython.
_private: (),
ptr: AtomicPtr<c_void>,
destructor: Option<unsafe extern "C" fn(_: *mut PyObject)>,
}

impl PyPayload for PyCapsule {
Expand All @@ -18,8 +23,26 @@ impl PyPayload for PyCapsule {
}
}

#[pyclass(with(Representable), flags(DISALLOW_INSTANTIATION))]
impl PyCapsule {}
#[pyclass(with(Representable, Destructor), flags(DISALLOW_INSTANTIATION))]
impl PyCapsule {
pub fn new(
ptr: *mut c_void,
destructor: Option<unsafe extern "C" fn(_: *mut PyObject)>,
) -> Self {
Self {
ptr: ptr.into(),
destructor,
}
Comment on lines +28 to +35
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:

# First, let's find and examine the capsule.rs file
find . -name "capsule.rs" -type f

Repository: 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
fi

Repository: 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"
fi

Repository: 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 3

Repository: 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.

}

pub fn pointer(&self) -> *mut c_void {
self.ptr.load(core::sync::atomic::Ordering::Relaxed)
}

fn destructor(&self) -> Option<unsafe extern "C" fn(_: *mut PyObject)> {
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.

does

Suggested change
fn destructor(&self) -> Option<unsafe extern "C" fn(_: *mut PyObject)> {
const fn destructor(&self) -> Option<unsafe extern "C" fn(_: *mut PyObject)> {

compile?

self.destructor
}
}

impl Representable for PyCapsule {
#[inline]
Expand All @@ -28,6 +51,15 @@ impl Representable for PyCapsule {
}
}

impl Destructor for PyCapsule {
fn del(zelf: &Py<Self>, _vm: &VirtualMachine) -> PyResult<()> {
if let Some(destructor) = zelf.destructor() {
unsafe { destructor(zelf.as_object().as_raw().cast_mut()) };
}
Ok(())
}
}

pub fn init(context: &'static Context) {
PyCapsule::extend_class(context, context.types.capsule_type);
}
16 changes: 12 additions & 4 deletions crates/vm/src/vm/context.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
use crate::{
PyResult, VirtualMachine,
PyObject, PyResult, VirtualMachine,
builtins::{
PyByteArray, PyBytes, PyComplex, PyDict, PyDictRef, PyEllipsis, PyFloat, PyFrozenSet,
PyInt, PyIntRef, PyList, PyListRef, PyNone, PyNotImplemented, PyStr, PyStrInterned,
PyTuple, PyTupleRef, PyType, PyTypeRef, PyUtf8Str,
PyByteArray, PyBytes, PyCapsule, PyComplex, PyDict, PyDictRef, PyEllipsis, PyFloat,
PyFrozenSet, PyInt, PyIntRef, PyList, PyListRef, PyNone, PyNotImplemented, PyStr,
PyStrInterned, PyTuple, PyTupleRef, PyType, PyTypeRef, PyUtf8Str,
bool_::PyBool,
code::{self, PyCode},
descriptor::{
Expand Down Expand Up @@ -752,6 +752,14 @@ impl Context {
let code = code.into_code_object(self);
PyRef::new_ref(PyCode::new(code), self.types.code_type.to_owned(), None)
}

pub fn new_capsule(
&self,
ptr: *mut core::ffi::c_void,
destructor: Option<unsafe extern "C" fn(_: *mut PyObject)>,
) -> PyRef<PyCapsule> {
PyCapsule::new(ptr, destructor).into_ref(self)
}
}

impl AsRef<Self> for Context {
Expand Down
Loading