From 58ab0ebc1955e9d058cb42fc08b7b72513c77f2e Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Tue, 28 Jun 2022 18:03:36 +0100 Subject: [PATCH] types: rework PyCapsule for soundness --- CHANGELOG.md | 6 ++ src/types/capsule.rs | 176 +++++++++++++++++++++++++++++-------------- 2 files changed, 124 insertions(+), 58 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fc0a29a05aa..b20a6e048af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Prevent multiple `#[pymethods]` with the same name for a single `#[pyclass]`. [#2399](https://github.com/PyO3/pyo3/pull/2399) - Fixup `lib_name` when using `PYO3_CONFIG_FILE`. [#2404](https://github.com/PyO3/pyo3/pull/2404) - Iterators over `PySet` and `PyDict` will now panic if the underlying collection is mutated during the iteration. [#2380](https://github.com/PyO3/pyo3/pull/2380) +- Rework `PyCapsule` type to resolve multiple soundness issues: [#2485](https://github.com/PyO3/pyo3/pull/2485) + - `PyCapsule::new` and `PyCapsule::new_with_destructor` now take `name: Option` instead of `&CStr`. + - The destructor `F` in `PyCapsule::new_with_destructor` must now be `Send`. + - `PyCapsule::get_context` deprecated in favour of `PyCapsule::context` which doesn't take a `py: Python<'_>` argument. + - `PyCapsule::set_context` no longer takes a `py: Python<'_>` argument. + - `PyCapsule::name` now returns `PyResult>` instead of `&CStr`. - `FromPyObject::extract` now raises an error if source type is `PyString` and target type is `Vec`. [#2500](https://github.com/PyO3/pyo3/pull/2500) ### Fixed diff --git a/src/types/capsule.rs b/src/types/capsule.rs index 525b68ac2fe..d7b5f0d46f0 100644 --- a/src/types/capsule.rs +++ b/src/types/capsule.rs @@ -2,8 +2,8 @@ use crate::Python; use crate::{ffi, AsPyPointer, PyAny}; use crate::{pyobject_native_type_core, PyErr, PyResult}; -use std::ffi::{c_void, CStr, CString}; -use std::os::raw::c_int; +use std::ffi::{CStr, CString}; +use std::os::raw::{c_char, c_int, c_void}; /// Represents a Python Capsule /// as described in [Capsules](https://docs.python.org/3/c-api/capsule.html#capsules): @@ -28,7 +28,7 @@ use std::os::raw::c_int; /// let foo = Foo { val: 123 }; /// let name = CString::new("builtins.capsule").unwrap(); /// -/// let capsule = PyCapsule::new(py, foo, name.as_ref())?; +/// let capsule = PyCapsule::new(py, foo, Some(name.clone()))?; /// /// let module = PyModule::import(py, "builtins")?; /// module.add("capsule", capsule)?; @@ -60,7 +60,7 @@ impl PyCapsule { /// /// Python::with_gil(|py| { /// let name = CString::new("foo").unwrap(); - /// let capsule = PyCapsule::new(py, 123_u32, &name).unwrap(); + /// let capsule = PyCapsule::new(py, 123_u32, Some(name)).unwrap(); /// let val = unsafe { capsule.reference::() }; /// assert_eq!(*val, 123); /// }); @@ -73,14 +73,13 @@ impl PyCapsule { /// use std::ffi::CString; /// /// Python::with_gil(|py| { - /// let name = CString::new("foo").unwrap(); - /// let capsule = PyCapsule::new(py, (), &name).unwrap(); // Oops! `()` is zero sized! + /// let capsule = PyCapsule::new(py, (), None).unwrap(); // Oops! `()` is zero sized! /// }); /// ``` pub fn new<'py, T: 'static + Send + AssertNotZeroSized>( py: Python<'py>, value: T, - name: &CStr, + name: Option, ) -> PyResult<&'py Self> { Self::new_with_destructor(py, value, name, |_, _| {}) } @@ -89,36 +88,39 @@ impl PyCapsule { /// /// Also provides a destructor: when the `PyCapsule` is destroyed, it will be passed the original object, /// as well as a `*mut c_void` which will point to the capsule's context, if any. + /// + /// The `destructor` must be `Send`, because there is no guarantee which thread it will eventually + /// be called from. pub fn new_with_destructor< 'py, T: 'static + Send + AssertNotZeroSized, - F: FnOnce(T, *mut c_void), + F: FnOnce(T, *mut c_void) + Send, >( py: Python<'py>, value: T, - name: &CStr, + name: Option, destructor: F, ) -> PyResult<&'py Self> { AssertNotZeroSized::assert_not_zero_sized(&value); - // Take ownership of a copy of `name` so that the string is guaranteed to live as long - // as the capsule. PyCapsule_new purely saves the pointer in the capsule so doesn't - // guarantee ownership itself. - let name = name.to_owned(); - let name_ptr = name.as_ptr(); + + // Sanity check for capsule layout + debug_assert_eq!(memoffset::offset_of!(CapsuleContents::, value), 0); + + let name_ptr = name.as_ref().map_or(std::ptr::null(), |name| name.as_ptr()); let val = Box::new(CapsuleContents { value, destructor, name, }); - let cap_ptr = unsafe { - ffi::PyCapsule_New( + unsafe { + let cap_ptr = ffi::PyCapsule_New( Box::into_raw(val) as *mut c_void, name_ptr, Some(capsule_destructor::), - ) - }; - unsafe { py.from_owned_ptr_or_err(cap_ptr) } + ); + py.from_owned_ptr_or_err(cap_ptr) + } } /// Imports an existing capsule. @@ -128,7 +130,7 @@ impl PyCapsule { /// /// # Safety /// - /// It must be known that this capsule's value pointer is to an item of type `T`. + /// It must be known that the capsule imported by `name` contains an item of type `T`. pub unsafe fn import<'py, T>(py: Python<'py>, name: &CStr) -> PyResult<&'py T> { let ptr = ffi::PyCapsule_Import(name.as_ptr(), false as c_int); if ptr.is_null() { @@ -140,6 +142,8 @@ impl PyCapsule { /// Sets the context pointer in the capsule. /// + /// Returns an error if this capsule is not valid. + /// /// # Notes /// /// The context is treated much like the value of the capsule, but should likely act as @@ -151,7 +155,6 @@ impl PyCapsule { /// # Example /// /// ``` - /// use std::ffi::CString; /// use std::sync::mpsc::{channel, Sender}; /// use libc::c_void; /// use pyo3::{prelude::*, types::PyCapsule}; @@ -164,22 +167,21 @@ impl PyCapsule { /// } /// /// Python::with_gil(|py| { - /// let name = CString::new("foo").unwrap(); /// let capsule = - /// PyCapsule::new_with_destructor(py, 123, &name, destructor as fn(u32, *mut c_void)) + /// PyCapsule::new_with_destructor(py, 123, None, destructor as fn(u32, *mut c_void)) /// .unwrap(); /// let context = Box::new(tx); // `Sender` is our context, box it up and ship it! - /// capsule.set_context(py, Box::into_raw(context) as *mut c_void).unwrap(); + /// capsule.set_context(Box::into_raw(context) as *mut c_void).unwrap(); /// // This scope will end, causing our destructor to be called... /// }); /// /// assert_eq!(rx.recv(), Ok("Destructor called!".to_string())); /// ``` #[allow(clippy::not_unsafe_ptr_arg_deref)] - pub fn set_context(&self, py: Python<'_>, context: *mut c_void) -> PyResult<()> { - let result = unsafe { ffi::PyCapsule_SetContext(self.as_ptr(), context) as u8 }; + pub fn set_context(&self, context: *mut c_void) -> PyResult<()> { + let result = unsafe { ffi::PyCapsule_SetContext(self.as_ptr(), context) }; if result != 0 { - Err(PyErr::fetch(py)) + Err(PyErr::fetch(self.py())) } else { Ok(()) } @@ -187,54 +189,96 @@ impl PyCapsule { /// Gets the current context stored in the capsule. If there is no context, the pointer /// will be null. - pub fn get_context(&self, py: Python<'_>) -> PyResult<*mut c_void> { + /// + /// Returns an error if this capsule is not valid. + pub fn context(&self) -> PyResult<*mut c_void> { let ctx = unsafe { ffi::PyCapsule_GetContext(self.as_ptr()) }; - if ctx.is_null() && self.is_valid() && PyErr::occurred(py) { - Err(PyErr::fetch(py)) - } else { - Ok(ctx) + if ctx.is_null() { + ensure_no_error(self.py())? } + Ok(ctx) + } + + /// Deprecated form of `.context()`. + #[deprecated(since = "0.17.0", note = "replaced with .context()")] + pub fn get_context(&self, _: Python<'_>) -> PyResult<*mut c_void> { + self.context() } /// Obtains a reference to the value of this capsule. /// /// # Safety /// - /// It must be known that this capsule's pointer is to an item of type `T`. + /// It must be known that this capsule is valid and its pointer is to an item of type `T`. pub unsafe fn reference(&self) -> &T { &*(self.pointer() as *const T) } /// Gets the raw `c_void` pointer to the value in this capsule. + /// + /// Returns null if this capsule is not valid. pub fn pointer(&self) -> *mut c_void { - unsafe { ffi::PyCapsule_GetPointer(self.0.as_ptr(), self.name().as_ptr()) } + unsafe { + let ptr = ffi::PyCapsule_GetPointer(self.0.as_ptr(), self.name_ptr_ignore_error()); + if ptr.is_null() { + ffi::PyErr_Clear(); + } + ptr + } } /// Checks if this is a valid capsule. + /// + /// Returns true if the stored `pointer()` is non-null. pub fn is_valid(&self) -> bool { - let r = unsafe { ffi::PyCapsule_IsValid(self.as_ptr(), self.name().as_ptr()) } as u8; + // As well as if the stored pointer is null, PyCapsule_IsValid also returns false if + // self.as_ptr() is null or not a ptr to a PyCapsule object. Both of these are guaranteed + // to not be the case thanks to invariants of this PyCapsule struct. + let r = unsafe { ffi::PyCapsule_IsValid(self.as_ptr(), self.name_ptr_ignore_error()) }; r != 0 } - /// Retrieves the name of this capsule. - pub fn name(&self) -> &CStr { + /// Retrieves the name of this capsule, if set. + /// + /// Returns an error if this capsule is not valid. + pub fn name(&self) -> PyResult> { unsafe { let ptr = ffi::PyCapsule_GetName(self.as_ptr()); - CStr::from_ptr(ptr) + if ptr.is_null() { + ensure_no_error(self.py())?; + Ok(None) + } else { + Ok(Some(CStr::from_ptr(ptr))) + } } } + + /// Attempts to retrieve the raw name pointer of this capsule. + /// + /// On error, clears the error indicator and returns NULL. This is a private function and next + /// use of this capsule will error anyway. + fn name_ptr_ignore_error(&self) -> *const c_char { + let ptr = unsafe { ffi::PyCapsule_GetName(self.as_ptr()) }; + if ptr.is_null() { + unsafe { ffi::PyErr_Clear() }; + } + ptr + } } // C layout, as PyCapsule::get_reference depends on `T` being first. #[repr(C)] -struct CapsuleContents { +struct CapsuleContents { + /// Value of the capsule value: T, + /// Destructor to be used by the capsule destructor: D, - name: CString, + /// Name used when creating the capsule + name: Option, } // Wrapping ffi::PyCapsule_Destructor for a user supplied FnOnce(T) for capsule destructor -unsafe extern "C" fn capsule_destructor( +unsafe extern "C" fn capsule_destructor( capsule: *mut ffi::PyObject, ) { let ptr = ffi::PyCapsule_GetPointer(capsule, ffi::PyCapsule_GetName(capsule)); @@ -260,6 +304,14 @@ pub trait AssertNotZeroSized: Sized { impl AssertNotZeroSized for T {} +fn ensure_no_error(py: Python<'_>) -> PyResult<()> { + if let Some(err) = PyErr::take(py) { + Err(err) + } else { + Ok(()) + } +} + #[cfg(test)] mod tests { use libc::c_void; @@ -286,13 +338,13 @@ mod tests { let foo = Foo { val: 123 }; let name = CString::new("foo").unwrap(); - let cap = PyCapsule::new(py, foo, &name)?; + let cap = PyCapsule::new(py, foo, Some(name.clone()))?; assert!(cap.is_valid()); let foo_capi = unsafe { cap.reference::() }; assert_eq!(foo_capi.val, 123); assert_eq!(foo_capi.get_val(), 123); - assert_eq!(cap.name(), name.as_ref()); + assert_eq!(cap.name().unwrap(), Some(name.as_ref())); Ok(()) }) } @@ -305,7 +357,7 @@ mod tests { let cap: Py = Python::with_gil(|py| { let name = CString::new("foo").unwrap(); - let cap = PyCapsule::new(py, foo as fn(u32) -> u32, &name).unwrap(); + let cap = PyCapsule::new(py, foo as fn(u32) -> u32, Some(name)).unwrap(); cap.into() }); @@ -319,15 +371,15 @@ mod tests { fn test_pycapsule_context() -> PyResult<()> { Python::with_gil(|py| { let name = CString::new("foo").unwrap(); - let cap = PyCapsule::new(py, 0, &name)?; + let cap = PyCapsule::new(py, 0, Some(name))?; - let c = cap.get_context(py)?; + let c = cap.get_context()?; assert!(c.is_null()); let ctx = Box::new(123_u32); - cap.set_context(py, Box::into_raw(ctx) as _)?; + cap.set_context(Box::into_raw(ctx) as _)?; - let ctx_ptr: *mut c_void = cap.get_context(py)?; + let ctx_ptr: *mut c_void = cap.get_context()?; let ctx = unsafe { *Box::from_raw(ctx_ptr as *mut u32) }; assert_eq!(ctx, 123); Ok(()) @@ -345,7 +397,7 @@ mod tests { let foo = Foo { val: 123 }; let name = CString::new("builtins.capsule").unwrap(); - let capsule = PyCapsule::new(py, foo, &name)?; + let capsule = PyCapsule::new(py, foo, Some(name.clone()))?; let module = PyModule::import(py, "builtins")?; module.add("capsule", capsule)?; @@ -368,7 +420,7 @@ mod tests { let name = CString::new("foo").unwrap(); let stuff: Vec = vec![1, 2, 3, 4]; - let cap = PyCapsule::new(py, stuff, &name).unwrap(); + let cap = PyCapsule::new(py, stuff, Some(name)).unwrap(); cap.into() }); @@ -385,15 +437,15 @@ mod tests { let cap: Py = Python::with_gil(|py| { let name = CString::new("foo").unwrap(); - let cap = PyCapsule::new(py, 0, &name).unwrap(); - cap.set_context(py, Box::into_raw(Box::new(&context)) as _) + let cap = PyCapsule::new(py, 0, Some(name)).unwrap(); + cap.set_context(Box::into_raw(Box::new(&context)) as _) .unwrap(); cap.into() }); Python::with_gil(|py| { - let ctx_ptr: *mut c_void = cap.as_ref(py).get_context(py).unwrap(); + let ctx_ptr: *mut c_void = cap.as_ref(py).get_context().unwrap(); let ctx = unsafe { *Box::from_raw(ctx_ptr as *mut &Vec) }; assert_eq!(ctx, &vec![1_u8, 2, 3, 4]); }) @@ -411,14 +463,22 @@ mod tests { Python::with_gil(|py| { let name = CString::new("foo").unwrap(); - let cap = - PyCapsule::new_with_destructor(py, 0, &name, destructor as fn(u32, *mut c_void)) - .unwrap(); - cap.set_context(py, Box::into_raw(Box::new(tx)) as _) - .unwrap(); + let cap = PyCapsule::new_with_destructor(py, 0, Some(name), destructor).unwrap(); + cap.set_context(Box::into_raw(Box::new(tx)) as _).unwrap(); }); // the destructor was called. assert_eq!(rx.recv(), Ok(true)); } + + #[test] + fn test_pycapsule_no_name() { + Python::with_gil(|py| { + let cap = PyCapsule::new(py, 0usize, None).unwrap(); + + assert_eq!(unsafe { cap.reference::() }, &0usize); + assert_eq!(cap.name().unwrap(), None); + assert_eq!(cap.get_context().unwrap(), std::ptr::null_mut()); + }); + } }