diff --git a/CHANGELOG.md b/CHANGELOG.md index 86ae80ea399..43619bd19c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Remove `#[deny(warnings)]` attribute (and instead refuse warnings only in CI). [#1340](https://github.com/PyO3/pyo3/pull/1340) - Fix deprecation warning for missing `__module__` with `#[pyclass]`. [#1343](https://github.com/PyO3/pyo3/pull/1343) - Correct return type of `PyFrozenSet::empty` to `&PyFrozenSet` (was incorrectly `&PySet`). [#1351](https://github.com/PyO3/pyo3/pull/1351) +- Fix missing `Py_INCREF` on heap type objects on Python versions before 3.8. [#1365](https://github.com/PyO3/pyo3/pull/1365) ## [0.13.0] - 2020-12-22 ### Packaging diff --git a/src/freelist.rs b/src/freelist.rs index 98a18cb51eb..80494092e4b 100644 --- a/src/freelist.rs +++ b/src/freelist.rs @@ -76,6 +76,8 @@ where if subtype == Self::type_object_raw(py) { if let Some(obj) = ::get_free_list(py).pop() { ffi::PyObject_Init(obj, subtype); + #[cfg(not(Py_3_8))] + crate::pyclass::bpo_35810_workaround(subtype); return obj as _; } } @@ -87,13 +89,13 @@ where let obj = PyAny::from_borrowed_ptr_or_panic(py, self_ as _); if let Some(obj) = ::get_free_list(py).insert(obj.as_ptr()) { - match get_type_free(ffi::Py_TYPE(obj)) { - Some(free) => { - let ty = ffi::Py_TYPE(obj); - free(obj as *mut c_void); - ffi::Py_DECREF(ty as *mut ffi::PyObject); - } - None => tp_free_fallback(obj), + let ty = ffi::Py_TYPE(obj); + let free = get_type_free(ty).unwrap_or_else(|| tp_free_fallback(ty)); + free(obj as *mut c_void); + + #[cfg(Py_3_8)] + if ffi::PyType_HasFeature(ty, ffi::Py_TPFLAGS_HEAPTYPE) != 0 { + ffi::Py_DECREF(ty as *mut ffi::PyObject); } } } diff --git a/src/pyclass.rs b/src/pyclass.rs index 22287a522f0..908dd70bc35 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -1,11 +1,9 @@ //! `PyClass` and related traits. use crate::class::methods::{PyClassAttributeDef, PyMethodDefType, PyMethods}; use crate::class::proto_methods::PyProtoMethods; -use crate::conversion::{AsPyPointer, FromPyPointer}; use crate::derive_utils::PyBaseTypeUtils; use crate::pyclass_slots::{PyClassDict, PyClassWeakRef}; use crate::type_object::{type_flags, PyLayout}; -use crate::types::PyAny; use crate::{ffi, PyCell, PyErr, PyNativeType, PyResult, PyTypeInfo, Python}; use std::convert::TryInto; use std::ffi::CString; @@ -23,6 +21,26 @@ pub(crate) unsafe fn get_type_free(tp: *mut ffi::PyTypeObject) -> Option = GILOnceCell::new(); + if *IS_NOT_PYTHON_3_8.get_or_init(py, || py.version_info() < (3, 8)) { + ffi::Py_INCREF(ty as *mut ffi::PyObject); + } + } else { + ffi::Py_INCREF(ty as *mut ffi::PyObject); + } + } +} + #[inline] pub(crate) unsafe fn default_new( py: Python, @@ -44,8 +62,13 @@ pub(crate) unsafe fn default_new( unreachable!("Subclassing native types isn't support in limited API mode"); } } + let alloc = get_type_alloc(subtype).unwrap_or(ffi::PyType_GenericAlloc); - alloc(subtype, 0) as _ + + #[cfg(not(Py_3_8))] + bpo_35810_workaround(subtype); + + alloc(subtype, 0) } /// This trait enables custom `tp_new`/`tp_dealloc` implementations for `T: PyClass`. @@ -64,15 +87,15 @@ pub trait PyClassAlloc: PyTypeInfo + Sized { /// `self_` must be a valid pointer to the Python heap. unsafe fn dealloc(py: Python, self_: *mut Self::Layout) { (*self_).py_drop(py); - let obj = PyAny::from_borrowed_ptr_or_panic(py, self_ as _); + let obj = self_ as *mut ffi::PyObject; - match get_type_free(ffi::Py_TYPE(obj.as_ptr())) { - Some(free) => { - let ty = ffi::Py_TYPE(obj.as_ptr()); - free(obj.as_ptr() as *mut c_void); - ffi::Py_DECREF(ty as *mut ffi::PyObject); - } - None => tp_free_fallback(obj.as_ptr()), + let ty = ffi::Py_TYPE(obj); + let free = get_type_free(ty).unwrap_or_else(|| tp_free_fallback(ty)); + free(obj as *mut c_void); + + #[cfg(Py_3_8)] + if ffi::PyType_HasFeature(ty, ffi::Py_TPFLAGS_HEAPTYPE) != 0 { + ffi::Py_DECREF(ty as *mut ffi::PyObject); } } } @@ -89,18 +112,11 @@ fn tp_dealloc() -> Option { Some(dealloc::) } -pub(crate) unsafe fn tp_free_fallback(obj: *mut ffi::PyObject) { - let ty = ffi::Py_TYPE(obj); +pub(crate) unsafe fn tp_free_fallback(ty: *mut ffi::PyTypeObject) -> ffi::freefunc { if ffi::PyType_IS_GC(ty) != 0 { - ffi::PyObject_GC_Del(obj as *mut c_void); + ffi::PyObject_GC_Del } else { - ffi::PyObject_Free(obj as *mut c_void); - } - - // For heap types, PyType_GenericAlloc calls INCREF on the type objects, - // so we need to call DECREF here: - if ffi::PyType_HasFeature(ty, ffi::Py_TPFLAGS_HEAPTYPE) != 0 { - ffi::Py_DECREF(ty as *mut ffi::PyObject); + ffi::PyObject_Free } } diff --git a/tests/test_inheritance.rs b/tests/test_inheritance.rs index d2d019a91f0..056e3d8ccdf 100644 --- a/tests/test_inheritance.rs +++ b/tests/test_inheritance.rs @@ -209,3 +209,50 @@ mod inheriting_native_type { ); } } + +#[pyclass(subclass)] +struct SimpleClass {} + +#[pymethods] +impl SimpleClass { + #[new] + fn new() -> Self { + Self {} + } +} + +#[test] +fn test_subclass_ref_counts() { + // regression test for issue #1363 + use pyo3::type_object::PyTypeObject; + Python::with_gil(|py| { + #[allow(non_snake_case)] + let SimpleClass = SimpleClass::type_object(py); + py_run!( + py, + SimpleClass, + r#" + import gc + import sys + + class SubClass(SimpleClass): + pass + + gc.collect() + count = sys.getrefcount(SubClass) + + for i in range(1000): + c = SubClass() + del c + + gc.collect() + after = sys.getrefcount(SubClass) + # depending on Python's GC the count may be either identical or exactly 1000 higher, + # both are expected values that are not representative of the issue. + # + # (With issue #1363 the count will be decreased.) + assert after == count or (after == count + 1000), f"{after} vs {count}" + "# + ); + }) +}