Reference to ReleasePool can be broken

[kngwyu]

Please see #159 (comment) for detail.
Currently, register_owned/borrowed/pointer methods returns a reference to an element in Vec, like

pub unsafe fn register_owned(_py: Python, obj: *mut ffi::PyObject) -> &PyObjectRef {
    let pool: &'static mut ReleasePool = &mut *POOL;
    pool.owned.push(obj);
    &*(&pool.owned[pool.owned.len() - 1] as *const *mut ffi::PyObject as *const PyObjectRef)
}

.
But, since Vec's inner address can be changed by realloc, this reference can be invalid one, and causes segfault like #158.
There can be some solutions for this.

1. Wrap by box

Current definition of release pool

struct ReleasePool {
    owned: Vec<*mut ffi::PyObject>,
    borrowed: Vec<*mut ffi::PyObject>,
    pointers: *mut Vec<*mut ffi::PyObject>,
    obj: Vec<Box<any::Any>>,
    p: spin::Mutex<*mut Vec<*mut ffi::PyObject>>,
}

to

struct ReleasePool {
    owned: Vec<PinBox<*mut ffi::PyObject>>,
    borrowed: Vec<PinBox<*mut ffi::PyObject>>,
    pointers: *mut Vec<PinBox<*mut ffi::PyObject>>,
    obj: Vec<PinBox<any::Any>>,
    p: spin::Mutex<*mut Vec<*mut ffi::PyObject>>,
}

so that each objects have a consistent address.
Then we can get a consistent reference by

&*(&*pool.owned[pool.owned.len() - 1] as *const *mut ffi::PyObject as *const PyObjectRef)

advantage

Easy

disadvantage

Allocation overhead

2. Remove ReleasePool

Remove release pool and rewrite all PyObject wrapper like PyDict and PyList to have a lifetime'py.
E.g.

#[repr(transparent)]
pub struct PyDict<'py>(PyObject, Python<'py>);

And also rewrite all constructors to return owned type instead of returning &'py.
E.g.

impl<'py> PyDict<'py> {
    /// Creates a new empty dictionary.
    pub fn new(py: Python<'py>) -> PyDict<'py> {
        unsafe {
            let obj = crate::PyObject::from_owned_ptr(py, ffi::PyDict_New());
            PyDict(obj, py)
        }
    }
}

advantages

Ergonomics improves
No overhead

disadvantage

Really big breaking change

[konstin]

Regarding Option 2: Would this still support wrapping object pub struct Py<T>(NonNull<ffi::PyObject>, std::marker::PhantomData<T>) to store them without owning the GIL?

Also Option 3: LinkedList. This is similar to option 1, but interesting regarding performance. (You could make a science out of this an try linked fixed size slices, but for now it's definitely safety and simplicity over performance)

[kngwyu]

Would this still support wrapping object pub struct Py

I can't imagine any problem around that...
But, anyway it's a very breaking change so there's many points to be considered.

Also Option 3: LinkedList.

Sounds good to me, though it's slower than Vec.
I'm going to try and bench it.

[fdoyon]

Option 3 can be amortized with a linked-list of preallocated buffers using the indexed crate

[konstin]

Would this still support wrapping object pub struct Py

I can't imagine any problem around that...

Py<T> must be given a T. If we have a PyDict<'p>, we'd need to save a Py<PyDict<'p>>, therefore Py<T> could outlive the gil.

[kngwyu]

Ah I understand, thanks.
But I'm still skeptical about our &PyDict approach 🤔

[Shnatsel]

This seems to be a case of a use-after-free, which is an exploitable security vulnerability. Please add this issue to the Rust security advisory database so people could check if they're running a vulnerable version and upgrade.

I see you've fixed a bunch of segfaults lately, so you might want to just specify the range of affected versions and declare that those shouldn't be used instead of filing each and every one as a separate vulnerability.