From 526023c0d11aac6888bd6cb9965257ab732798f5 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Tue, 26 Jul 2022 08:25:56 +0100 Subject: [PATCH] capsule: soundness backports --- CHANGELOG.md | 8 +++++++ pytests/src/misc.rs | 34 +++++++++++++++++++++++++++- pytests/tests/test_misc.py | 9 ++++++++ src/types/capsule.rs | 46 +++++++++++++++++++++++++++++++++----- 4 files changed, 91 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39b65d41cd8..9ff6e847bf8 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/pytests/src/misc.rs b/pytests/src/misc.rs index 652f3e82cc0..c14e7b5334a 100644 --- a/pytests/src/misc.rs +++ b/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(()) } diff --git a/pytests/tests/test_misc.py b/pytests/tests/test_misc.py index 406c55ee5be..59646e2dbe4 100644 --- a/pytests/tests/test_misc.py +++ b/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() diff --git a/src/types/capsule.rs b/src/types/capsule.rs index 8add77b4133..340536ee22b 100644 --- a/src/types/capsule.rs +++ b/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::), ) }; @@ -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 { 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); + let CapsuleContents { + value, + destructor, + thread_id, + .. + } = *Box::from_raw(ptr as *mut CapsuleContents); + 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) }