Merge pull request #2522 from davidhewitt/capsule-soundness-backport

capsule: soundness backports

.github/workflows/ci.yml

@@ -177,11 +177,14 @@ jobs:
         run: |
           set -x
           cargo update -p indexmap --precise 1.6.2
-          cargo update -p hashbrown:0.12.1 --precise 0.9.1
+          cargo update -p hashbrown:0.12.3 --precise 0.9.1
           PROJECTS=("." "examples/decorator" "examples/maturin-starter" "examples/setuptools-rust-starter" "examples/word-count")
           for PROJ in ${PROJECTS[@]}; do
             cargo update --manifest-path "$PROJ/Cargo.toml" -p parking_lot --precise 0.11.0
           done
+          cargo update -p plotters --precise 0.3.1
+          cargo update -p plotters-svg --precise 0.3.1
+          cargo update -p plotters-backend --precise 0.3.2
 
       - name: Build docs
         run: cargo doc --no-deps --no-default-features --features "full ${{ matrix.extra_features }}"

CHANGELOG.md

@@ -6,6 +6,14 @@ PyO3 versions, please see the [migration guide](https://pyo3.rs/latest/migration
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [Unreleased]
+
+### Changed
+- Fix soundness issues with `PyCapsule` type with select workarounds. Users are encourage to upgrade to PyO3 0.17 at their earliest convenience which contains API breakages which fix the issues in a long-term fashion. [#2522](https://github.com/PyO3/pyo3/pull/2522)
+  - `PyCapsule::new` and `PyCapsule::new_with_destructor` now take ownership of a copy of the `name` to resolve a possible use-after-free.
+  - `PyCapsule::name` now returns an empty `CStr` instead of dereferencing a null pointer if the capsule has no name.
+  - The destructor `F` in `PyCapsule::new_with_destructor` will never be called if the capsule is deleted from a thread other than the one which the capsule was created in (a warning will be emitted).
+
 ## [0.16.5] - 2022-05-15
 
 ### Added

pyo3-build-config/src/impl_.rs

@@ -1076,7 +1076,8 @@ impl BuildFlags {
         script.push_str("config = sysconfig.get_config_vars()\n");
 
         for k in &BuildFlags::ALL {
-            script.push_str(&format!("print(config.get('{}', '0'))\n", k));
+            use std::fmt::Write;
+            writeln!(&mut script, "print(config.get('{}', '0'))", k).unwrap();
         }
 
         let stdout = run_python_script(interpreter.as_ref(), &script)?;
@@ -1225,7 +1226,8 @@ fn find_sysconfigdata(cross: &CrossCompileConfig) -> Result<Option<PathBuf>> {
             sysconfigdata files found:",
         );
         for path in sysconfig_paths {
-            error_msg += &format!("\n\t{}", path.display());
+            use std::fmt::Write;
+            write!(&mut error_msg, "\n\t{}", path.display()).unwrap();
         }
         bail!("{}\n", error_msg);
     }

pytests/src/misc.rs

@@ -1,4 +1,9 @@
-use pyo3::prelude::*;
+use std::{
+    ffi::CString,
+    sync::atomic::{AtomicBool, Ordering},
+};
+
+use pyo3::{prelude::*, types::PyCapsule};
 
 #[pyfunction]
 fn issue_219() {
@@ -7,8 +12,35 @@ fn issue_219() {
     let _py = gil.python();
 }
 
+#[pyfunction]
+fn capsule_send_destructor(py: Python<'_>) {
+    // safety defence - due to lack of send bound in PyO3 0.16, the PyCapsule type
+    // must not execute destructors in different thread
+    // (and will emit a Python warning)
+    let destructor_called = AtomicBool::new(false);
+
+    let cap: PyObject = {
+        // so that intermediate capsule references are cleared, use a pool
+        let _pool = unsafe { pyo3::GILPool::new() };
+        PyCapsule::new_with_destructor(py, 0i32, &CString::new("some name").unwrap(), |_, _| {
+            destructor_called.store(true, Ordering::SeqCst)
+        })
+        .unwrap()
+        .into()
+    };
+
+    py.allow_threads(|| {
+        std::thread::spawn(move || Python::with_gil(|_| drop(cap)))
+            .join()
+            .unwrap();
+    });
+
+    assert!(!destructor_called.load(Ordering::SeqCst));
+}
+
 #[pymodule]
 pub fn misc(_py: Python<'_>, m: &PyModule) -> PyResult<()> {
     m.add_function(wrap_pyfunction!(issue_219, m)?)?;
+    m.add_function(wrap_pyfunction!(capsule_send_destructor, m)?)?;
     Ok(())
 }

pytests/tests/test_misc.py

@@ -1,6 +1,15 @@
 import pyo3_pytests.misc
+import pytest
 
 
 def test_issue_219():
     # Should not deadlock
     pyo3_pytests.misc.issue_219()
+
+
+def test_capsule_send_destructor():
+    with pytest.warns(
+        RuntimeWarning,
+        match="capsule destructor called in thread other than the one the capsule was created in",
+    ):
+        pyo3_pytests.misc.capsule_send_destructor()

src/conversion.rs

@@ -521,6 +521,23 @@ where
     }
 }
 
+/// ```rust,compile_fail
+/// use pyo3::prelude::*;
+///
+/// #[pyclass]
+/// struct TestClass {
+///     num: u32,
+/// }
+///
+/// let t = TestClass { num: 10 };
+///
+/// Python::with_gil(|py| {
+///     let pyvalue = Py::new(py, t).unwrap().to_object(py);
+///     let t: TestClass = pyvalue.extract(py).unwrap();
+/// })
+/// ```
+mod test_no_clone {}
+
 #[cfg(test)]
 mod tests {
     use crate::types::{IntoPyDict, PyAny, PyDict, PyList};

src/exceptions.rs

@@ -5,8 +5,8 @@
 //! The structs in this module represent Python's built-in exceptions, while the modules comprise
 //! structs representing errors defined in Python code.
 //!
-//! The latter are created with the [`import_exception`] macro, which you can use yourself
-//! to import Python exceptions.
+//! The latter are created with the [`import_exception`](crate::import_exception) macro, which you
+//! can use yourself to import Python exceptions.
 
 use crate::{ffi, PyResult, Python};
 use std::ffi::CStr;

src/impl_/frompyobject.rs

@@ -14,13 +14,15 @@ pub fn failed_to_extract_enum(
         error_names.join(" | ")
     );
     for ((variant_name, error_name), error) in variant_names.iter().zip(error_names).zip(errors) {
-        err_msg.push('\n');
-        err_msg.push_str(&format!(
-            "- variant {variant_name} ({error_name}): {error_msg}",
+        use std::fmt::Write;
+        write!(
+            &mut err_msg,
+            "\n- variant {variant_name} ({error_name}): {error_msg}",
             variant_name = variant_name,
             error_name = error_name,
             error_msg = error.value(py).str().unwrap().to_str().unwrap(),
-        ));
+        )
+        .unwrap();
     }
     PyTypeError::new_err(err_msg)
 }

src/instance.rs

@@ -535,8 +535,9 @@ impl<T> Py<T> {
     ///
     /// This is equivalent to the Python expression `self.attr_name`.
     ///
-    /// If calling this method becomes performance-critical, the [`intern!`] macro can be used
-    /// to intern `attr_name`, thereby avoiding repeated temporary allocations of Python strings.
+    /// If calling this method becomes performance-critical, the [`intern!`](crate::intern) macro
+    /// can be used to intern `attr_name`, thereby avoiding repeated temporary allocations of
+    /// Python strings.
     ///
     /// # Example: `intern!`ing the attribute name
     ///
@@ -566,8 +567,8 @@ impl<T> Py<T> {
     ///
     /// This is equivalent to the Python expression `self.attr_name = value`.
     ///
-    /// If calling this method becomes performance-critical, the [`intern!`] macro can be used
-    /// to intern `attr_name`, thereby avoiding repeated temporary allocations of Python strings.
+    /// To avoid repeated temporary allocations of Python strings, the [`intern!`](crate::intern)
+    /// macro can be used to intern `attr_name`.
     ///
     /// # Example: `intern!`ing the attribute name
     ///

src/types/capsule.rs

@@ -2,8 +2,9 @@
 use crate::Python;
 use crate::{ffi, AsPyPointer, PyAny};
 use crate::{pyobject_native_type_core, PyErr, PyResult};
-use std::ffi::{c_void, CStr};
+use std::ffi::{c_void, CStr, CString};
 use std::os::raw::c_int;
+use std::thread::{self, ThreadId};
 
 /// Represents a Python Capsule
 /// as described in [Capsules](https://docs.python.org/3/c-api/capsule.html#capsules):
@@ -100,12 +101,23 @@ impl PyCapsule {
         destructor: F,
     ) -> PyResult<&'py Self> {
         AssertNotZeroSized::assert_not_zero_sized(&value);
-        let val = Box::new(CapsuleContents { value, destructor });
+        // 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();
+        let thread_id = thread::current().id();
+        let val = Box::new(CapsuleContents {
+            value,
+            destructor,
+            thread_id,
+            name,
+        });
 
         let cap_ptr = unsafe {
             ffi::PyCapsule_New(
                 Box::into_raw(val) as *mut c_void,
-                name.as_ptr(),
+                name_ptr,
                 Some(capsule_destructor::<T, F>),
             )
         };
@@ -211,7 +223,12 @@ impl PyCapsule {
     pub fn name(&self) -> &CStr {
         unsafe {
             let ptr = ffi::PyCapsule_GetName(self.as_ptr());
-            CStr::from_ptr(ptr)
+            if ptr.is_null() {
+                ffi::PyErr_Clear();
+                CStr::from_bytes_with_nul_unchecked(b"\0")
+            } else {
+                CStr::from_ptr(ptr)
+            }
         }
     }
 }
@@ -221,6 +238,9 @@ impl PyCapsule {
 struct CapsuleContents<T: 'static + Send, D: FnOnce(T, *mut c_void)> {
     value: T,
     destructor: D,
+    // Thread id is stored as a safety fix for lack of D: Send bound on the destructor
+    thread_id: ThreadId,
+    name: CString,
 }
 
 // Wrapping ffi::PyCapsule_Destructor for a user supplied FnOnce(T) for capsule destructor
@@ -229,7 +249,23 @@ unsafe extern "C" fn capsule_destructor<T: 'static + Send, F: FnOnce(T, *mut c_v
 ) {
     let ptr = ffi::PyCapsule_GetPointer(capsule, ffi::PyCapsule_GetName(capsule));
     let ctx = ffi::PyCapsule_GetContext(capsule);
-    let CapsuleContents { value, destructor } = *Box::from_raw(ptr as *mut CapsuleContents<T, F>);
+    let CapsuleContents {
+        value,
+        destructor,
+        thread_id,
+        ..
+    } = *Box::from_raw(ptr as *mut CapsuleContents<T, F>);
+    if thread_id != thread::current().id() {
+        ffi::PyErr_WarnEx(
+            ffi::PyExc_RuntimeWarning,
+            b"capsule destructor called in thread other than the one the capsule was created in, skipping the destructor\0".as_ptr().cast(),
+            1,
+        );
+        if !ffi::PyErr_Occurred().is_null() {
+            ffi::PyErr_WriteUnraisable(ffi::_Py_NewRef(ffi::Py_None()));
+        }
+        return;
+    }
     destructor(value, ctx)
 }
 

tests/test_compile_error.rs

@@ -47,7 +47,7 @@ fn _test_compile_errors() {
     tests_rust_1_56(&t);
     tests_rust_1_57(&t);
     tests_rust_1_58(&t);
-    tests_rust_1_60(&t);
+    tests_rust_1_62(&t);
 
     #[rustversion::since(1.49)]
     fn tests_rust_1_49(t: &trybuild::TestCases) {
@@ -59,7 +59,7 @@ fn _test_compile_errors() {
     #[rustversion::since(1.56)]
     fn tests_rust_1_56(t: &trybuild::TestCases) {
         t.compile_fail("tests/ui/invalid_closure.rs");
-        t.compile_fail("tests/ui/invalid_result_conversion.rs");
+
         t.compile_fail("tests/ui/pyclass_send.rs");
     }
 
@@ -81,7 +81,6 @@ fn _test_compile_errors() {
     fn tests_rust_1_58(t: &trybuild::TestCases) {
         t.compile_fail("tests/ui/invalid_pyfunctions.rs");
         t.compile_fail("tests/ui/invalid_pymethods.rs");
-        t.compile_fail("tests/ui/missing_clone.rs");
         t.compile_fail("tests/ui/not_send.rs");
         t.compile_fail("tests/ui/not_send2.rs");
         t.compile_fail("tests/ui/not_send3.rs");
@@ -92,13 +91,14 @@ fn _test_compile_errors() {
     #[rustversion::before(1.58)]
     fn tests_rust_1_58(_t: &trybuild::TestCases) {}
 
-    #[rustversion::since(1.60)]
-    fn tests_rust_1_60(t: &trybuild::TestCases) {
+    #[rustversion::since(1.62)]
+    fn tests_rust_1_62(t: &trybuild::TestCases) {
         t.compile_fail("tests/ui/invalid_pymethod_receiver.rs");
+        t.compile_fail("tests/ui/invalid_result_conversion.rs");
     }
 
-    #[rustversion::before(1.60)]
-    fn tests_rust_1_60(_t: &trybuild::TestCases) {}
+    #[rustversion::before(1.62)]
+    fn tests_rust_1_62(_t: &trybuild::TestCases) {}
 }
 
 #[cfg(feature = "nightly")]

tests/ui/abi3_nativetype_inheritance.stderr

@@ -4,6 +4,7 @@ error[E0277]: the trait bound `PyDict: PyClass` is not satisfied
 5 | #[pyclass(extends=PyDict)]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `PyClass` is not implemented for `PyDict`
   |
+  = help: the trait `PyClass` is implemented for `TestClass`
   = note: required because of the requirements on the impl of `PyClassBaseType` for `PyDict`
   = note: this error originates in the attribute macro `pyclass` (in Nightly builds, run with -Z macro-backtrace for more info)
 
@@ -13,6 +14,7 @@ error[E0277]: the trait bound `PyDict: PyClass` is not satisfied
 5   | #[pyclass(extends=PyDict)]
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `PyClass` is not implemented for `PyDict`
     |
+    = help: the trait `PyClass` is implemented for `TestClass`
     = note: required because of the requirements on the impl of `PyClassBaseType` for `PyDict`
 note: required by a bound in `ThreadCheckerInherited`
    --> src/impl_/pyclass.rs

tests/ui/invalid_pymethod_receiver.stderr

@@ -4,11 +4,15 @@ error[E0277]: the trait bound `i32: From<&PyCell<MyClass>>` is not satisfied
 8 |     fn method_with_invalid_self_type(slf: i32, py: Python<'_>, index: u32) {}
   |                                           ^^^ the trait `From<&PyCell<MyClass>>` is not implemented for `i32`
   |
-  = help: the following implementations were found:
-            <i32 as From<NonZeroI32>>
-            <i32 as From<bool>>
-            <i32 as From<i16>>
-            <i32 as From<i8>>
-          and 71 others
+  = help: the following other types implement trait `From<T>`:
+            <f32 as From<i16>>
+            <f32 as From<i8>>
+            <f32 as From<u16>>
+            <f32 as From<u8>>
+            <f64 as From<f32>>
+            <f64 as From<i16>>
+            <f64 as From<i32>>
+            <f64 as From<i8>>
+          and 67 others
   = note: required because of the requirements on the impl of `Into<i32>` for `&PyCell<MyClass>`
   = note: required because of the requirements on the impl of `TryFrom<&PyCell<MyClass>>` for `i32`

tests/ui/invalid_result_conversion.stderr

@@ -4,8 +4,7 @@ error[E0277]: the trait bound `Result<(), MyError>: IntoPyCallbackOutput<_>` is
 21  | #[pyfunction]
     | ^^^^^^^^^^^^^ the trait `IntoPyCallbackOutput<_>` is not implemented for `Result<(), MyError>`
     |
-    = help: the following implementations were found:
-              <Result<T, E> as IntoPyCallbackOutput<U>>
+    = help: the trait `IntoPyCallbackOutput<U>` is implemented for `Result<T, E>`
 note: required by a bound in `pyo3::callback::convert`
    --> src/callback.rs:182:8
     |