Segfault adding class object to module.

[derekdreery]

🌍 Environment

• Your operating system and version: Linux: 4.18.16-arch1-1-ARCH
• Your python version: 3.7.1
• How did you install python (e.g. apt or pyenv)? Did you use a virtualenv?: arch-linux (no venv)
• Your rust version (rustc --version):
• Are you using the latest pyo3 version?: git-master

💥 Reproducing

#![feature(specialization)]

use pyo3::prelude::*;

/// Main entry point - set up modules, functions, etc.
#[pymodule]
fn libbug(_: Python, m: &PyModule) -> PyResult<()> {
    m.add("Foo", A { kind: 0 })?;
    Ok(())
}

#[pyclass]
#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
pub struct A {
    kind: u32,
}

Import the .so -> segfault (somewhat intermittent).

[kngwyu]

Thanks for reporting!
I think it's a design mistake...
This implementation is used when m.add(A {}) is called, but we can't use as_ptr before we call into_object.
So to fix this bug is relatively easy, but I think we have to rethink pyclass design.

1. We can't know a struct is in a python heap(and has ob_refcnt and ob_type), or not. Is it proper design? Ideally we should use different types for python-allocated items, though it's far from current design.
2. For pyclass, currently IntoPyObject actually allocates in python heap and copies its data. So I think Into prefix is not suitable.

[athre0z]

I think you're on the right track there @kngwyu. This design issue is probably also closely related to #308. The ToPyObject implementation generated by the proc macro has the same issue.

[derekdreery]

Do you have any ideas of how to fix this? I'm happy to help implement if possible.

[konstin]

I fear it's a bit complex to fix. Basically, we need to properly differentiate between instances of pyclass structs that can be wrapped in a python object, but aren't, and types that are python objects (i.e. they are/have a *mut PyObject). I think the bug is we assume that our instance is the latter while it is the former in the the pyclass implementation:

pyo3/pyo3-derive-backend/src/py_class.rs

Lines 174 to 195 in 27ef337

	impl ::pyo3::ToPyObject for #cls {
	fn to_object(&self, py: ::pyo3::Python) -> ::pyo3::PyObject {
	use ::pyo3::python::ToPyPointer;
	unsafe { ::pyo3::PyObject::from_borrowed_ptr(py, self.as_ptr()) }
	}
	}
	impl ::pyo3::ToPyPointer for #cls {
	fn as_ptr(&self) -> *mut ::pyo3::ffi::PyObject {
	unsafe {
	{self as *const _ as *mut u8}
	.offset(-<#cls as ::pyo3::typeob::PyTypeInfo>::OFFSET) as *mut ::pyo3::ffi::PyObject
	}
	}
	}
	impl<'a> ::pyo3::ToPyObject for &'a mut #cls {
	fn to_object(&self, py: ::pyo3::Python) -> ::pyo3::PyObject {
	use ::pyo3::python::ToPyPointer;
	unsafe { ::pyo3::PyObject::from_borrowed_ptr(py, self.as_ptr()) }
	}
	}

If I'm not wrong, we need to remove those &self methods on the pyclass instances and instead start to allow IntoPyObject where currently ToPyObject is required.

[kngwyu]

@konstin
I'm planning to implement reference wrappers PyRef and PyRefMut.
They're just wrappers of &T and &mut T, but we can retrieve *mut ffi::PyObject from them.
Just &T where T: PyTypeInfo doesn't mean we can retrieve python object, since we can't know they're already copied to python heap or not.
What do you think about this idea?