pyclass: fix reference count issue in subclass new

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

src/freelist.rs

@@ -76,6 +76,11 @@ where
  if subtype == Self::type_object_raw(py) {
  if let Some(obj) = <Self as PyClassWithFreeList>::get_free_list(py).pop() {
  ffi::PyObject_Init(obj, subtype);
+ // Workaround for Python issue 35810; no longer necessary in Python 3.8
+ #[cfg(not(Py_3_8))]
+ if ffi::PyType_HasFeature(subtype, ffi::Py_TPFLAGS_HEAPTYPE) != 0 {
+ ffi::Py_INCREF(subtype as *mut ffi::PyObject);
+ }
  return obj as _;
  }
  }
@@ -87,13 +92,12 @@ where
  let obj = PyAny::from_borrowed_ptr_or_panic(py, self_ as _);
 
  if let Some(obj) = <Self as PyClassWithFreeList>::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);
+
+ if ffi::PyType_HasFeature(ty, ffi::Py_TPFLAGS_HEAPTYPE) != 0 {
+ ffi::Py_DECREF(ty as *mut ffi::PyObject);
  }
  }
  }

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;
@@ -44,8 +42,17 @@ pub(crate) unsafe fn default_new<T: PyTypeInfo>(
  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 _
+ let ptr = alloc(subtype, 0);
+
+ // Workaround for Python issue 35810; no longer necessary in Python 3.8
+ #[cfg(not(Py_3_8))]
+ if ffi::PyType_HasFeature(subtype, ffi::Py_TPFLAGS_HEAPTYPE) != 0 {
+ ffi::Py_INCREF(subtype as *mut ffi::PyObject);
+ }
+
+ ptr
 }
 
 /// This trait enables custom `tp_new`/`tp_dealloc` implementations for `T: PyClass`.
@@ -64,15 +71,14 @@ 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);
+
+ if ffi::PyType_HasFeature(ty, ffi::Py_TPFLAGS_HEAPTYPE) != 0 {
+ ffi::Py_DECREF(ty as *mut ffi::PyObject);
  }
  }
 }
@@ -89,18 +95,11 @@ fn tp_dealloc<T: PyClassAlloc>() -> Option<ffi::destructor> {
  Some(dealloc::<T>)
 }
 
-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
  }
 }
 

tests/test_inheritance.rs

@@ -209,3 +209,46 @@ 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)
+ assert after == count, f"{after} vs {count}"
+ "#
+ );
+ })
+}