Skip to content

Commit

Permalink
Fix a soundness bug with PyClassInitializer (#4454)
Browse files Browse the repository at this point in the history
From now you cannot initialize a `PyClassInitializer<SubClass>` with `PyClassInitializer::from(Py<BaseClass>).add_subclass(SubClass)`.

This was out of bounds write. Now it panics. See details at #4452.
  • Loading branch information
ChayimFriedman2 committed Aug 21, 2024
1 parent 7879d57 commit 8413890
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 0 deletions.
1 change: 1 addition & 0 deletions newsfragments/4454.fixed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a soundness bug with `PyClassInitializer`: from now you cannot initialize a `PyClassInitializer<SubClass>` with `PyClassInitializer::from(Py<BaseClass>).add_subclass(SubClass)`.
49 changes: 49 additions & 0 deletions src/pyclass_init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ pub trait PyObjectInit<T>: Sized {
py: Python<'_>,
subtype: *mut PyTypeObject,
) -> PyResult<*mut ffi::PyObject>;

#[doc(hidden)]
fn can_be_subclassed(&self) -> bool;

private_decl! {}
}

Expand Down Expand Up @@ -81,6 +85,11 @@ impl<T: PyTypeInfo> PyObjectInit<T> for PyNativeTypeInitializer<T> {
inner(py, type_object, subtype)
}

#[inline]
fn can_be_subclassed(&self) -> bool {
true
}

private_impl! {}
}

Expand Down Expand Up @@ -147,7 +156,14 @@ impl<T: PyClass> PyClassInitializer<T> {
/// Constructs a new initializer from value `T` and base class' initializer.
///
/// It is recommended to use `add_subclass` instead of this method for most usage.
#[track_caller]
#[inline]
pub fn new(init: T, super_init: <T::BaseType as PyClassBaseType>::Initializer) -> Self {
// This is unsound; see https://github.com/PyO3/pyo3/issues/4452.
assert!(
super_init.can_be_subclassed(),
"you cannot add a subclass to an existing value",
);
Self(PyClassInitializerImpl::New { init, super_init })
}

Expand Down Expand Up @@ -197,6 +213,8 @@ impl<T: PyClass> PyClassInitializer<T> {
/// })
/// }
/// ```
#[track_caller]
#[inline]
pub fn add_subclass<S>(self, subclass_value: S) -> PyClassInitializer<S>
where
S: PyClass<BaseType = T>,
Expand Down Expand Up @@ -268,6 +286,11 @@ impl<T: PyClass> PyObjectInit<T> for PyClassInitializer<T> {
.map(Bound::into_ptr)
}

#[inline]
fn can_be_subclassed(&self) -> bool {
!matches!(self.0, PyClassInitializerImpl::Existing(..))
}

private_impl! {}
}

Expand All @@ -288,6 +311,8 @@ where
B: PyClass,
B::BaseType: PyClassBaseType<Initializer = PyNativeTypeInitializer<B::BaseType>>,
{
#[track_caller]
#[inline]
fn from(sub_and_base: (S, B)) -> PyClassInitializer<S> {
let (sub, base) = sub_and_base;
PyClassInitializer::from(base).add_subclass(sub)
Expand Down Expand Up @@ -320,3 +345,27 @@ where
Ok(self.into())
}
}

#[cfg(all(test, feature = "macros"))]
mod tests {
//! See https://github.com/PyO3/pyo3/issues/4452.

use crate::prelude::*;

#[pyclass(crate = "crate", subclass)]
struct BaseClass {}

#[pyclass(crate = "crate", extends=BaseClass)]
struct SubClass {
_data: i32,
}

#[test]
#[should_panic]
fn add_subclass_to_py_is_unsound() {
Python::with_gil(|py| {
let base = Py::new(py, BaseClass {}).unwrap();
let _subclass = PyClassInitializer::from(base).add_subclass(SubClass { _data: 42 });
});
}
}

0 comments on commit 8413890

Please sign in to comment.