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,8 @@ 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);
+                #[cfg(not(Py_3_8))]
+                crate::pyclass::bpo_35810_workaround(py, subtype);
                 return obj as _;
             }
         }
@@ -87,13 +89,13 @@ 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);
+
+            #[cfg(Py_3_8)]
+            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;
@@ -23,6 +21,26 @@ pub(crate) unsafe fn get_type_free(tp: *mut ffi::PyTypeObject) -> Option<ffi::fr
     mem::transmute(ffi::PyType_GetSlot(tp, ffi::Py_tp_free))
 }
 
+/// Workaround for Python issue 35810; no longer necessary in Python 3.8
+#[inline]
+#[cfg(not(Py_3_8))]
+pub(crate) unsafe fn bpo_35810_workaround(_py: Python, ty: *mut ffi::PyTypeObject) {
+    #[cfg(Py_LIMITED_API)]
+    {
+        // Must check version at runtime for abi3 wheels - they could run against a higher version
+        // than the build config suggests.
+        use crate::once_cell::GILOnceCell;
+        static IS_PYTHON_3_8: GILOnceCell<bool> = GILOnceCell::new();
+
+        if *IS_PYTHON_3_8.get_or_init(_py, || _py.version_info() >= (3, 8)) {
+            // No fix needed - the wheel is running on a sufficiently new interpreter.
+            return;
+        }
+    }
+
+    ffi::Py_INCREF(ty as *mut ffi::PyObject);
+}
+
 #[inline]
 pub(crate) unsafe fn default_new<T: PyTypeInfo>(
     py: Python,
@@ -44,8 +62,13 @@ 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 _
+
+    #[cfg(not(Py_3_8))]
+    bpo_35810_workaround(py, 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<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,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}"
+            "#
+        );
+    })
+}