A soundness issue with GIL-bound references

[nagisa]

I think @pganssle’s comment in #679 ultimately reveals a soundness issue in PyO3.

Remember, that Rust has very strict invariants for its references:

• There must ever be one and only one live &mut reference to the same object (not too relevant to as, but do note that object here is not the python object);
• There may be multiple & references to the same object but only the UnsafeCell fields (interior mutability) of the object may change while there any live references to the object.

The existence of &PyNativeType potentially violates the 2nd invariant as soon as the GIL is unlocked as part of some FFI or allow_threads(|| ...) call. Consider this example and potential way things could go wrong (presume some super-optimised implementation of the functions used here)

let gil = Python::acquire_gil();
let list = PyList::new(gil.python(), [1, 2, 3]);
allow_threads(|| { /*
    Some code here mutates the list object, possibly reallocating or clearing it
    after they obtain their own GIL lock.
*/ } );
// compiler is allowed to reorder this statement to before `allow_threads`
// or constant propagate the statically known length of 3 into here.
//
// After all the invariant does not permit for the underlying data to change
// while a reference to the list is live.
let length = list.len();
// can optimise out the bounds check to use `length` variable
// rather than reading length again.
list[2] // => OOB access

We already solved an soundness bug by making these references !Send and thus preventing invalid Python::assume_gil_acquired() calls, but what we really need is a prohibition to unlock the GIL at all. At least while these references exist.

An alternative, and more pragmatic, option would be to make all these types have interior mutability by adding an UnsafeCell into them.

[nagisa]

NB: I don’t actually know if UnsafeCell + whatever GIL setup we have currently is sufficient and it seems hard to prove too.

That being said, the issue here is fairly theoretical at this point due to the fact that each FFI call is acting as a compiler barrier of sorts, preventing any dangerous optimisations, I think...

[programmerjake]

A related possible issue with PyO3 giving out &mut references to objects:
I didn't take the time to check if this actually works (should be run with compiler optimizations disabled to avoid having code optimized out), but here goes:

#[pyclass]
struct MyClass {
    lock: std::Mutex<String>,
}

fn my_fn(value: &PyAny) -> PyResult<()> {
    let mut_ref1: &mut MyClass = FromPyObject::extract(value)?;
    let mut_ref2: &mut MyClass = FromPyObject::extract(value)?;
    assert!(std::ptr::eq(mut_ref1, mut_ref2),
            "we shouldn't have two &mut references to the same object!!!");
    // cause unsynchronized access:
    rayon::join(
        || mut_ref1.get_mut().unwrap().clear(),
        || println!("{}", mut_ref2.get_mut().unwrap()),
    );
    Ok(())
}

[davidhewitt]

Rather than UnsafeCell, perhaps we just should store *mut PyObject in these types?

If we take the design proposal of PyDict<'py> from #679, then the definition could be:

struct PyDict<'py> {
    ptr: *mut PyObject,
    phantom PhantomData<&'py PyObject>
}

Storing *mut PyObject would be enough to add the !Send bound, which is why I don't think the phantom has to be a &mut. (We just need the phantom for the lifetime)

[davidhewitt]

@kngwyu I think with #770 and #887 merged we have solved the soundness issues raised here? 🎉

[davidhewitt]

Closing unless anyone disagrees with me.