Unsoundness: &T and &mut T to same object

[davidhewitt]

It is currently possible with pyo3 to get &mut T and &T to the same object simultaneously.

This is trivially demonstrated with the following code:

#[pyclass]
struct Foo {
    value: i32
}

#[pyfunction]
fn edit_foo(a: &mut Foo, b: &Foo) {
    let start = b.value;
    a.value += 1;
    assert_eq!(b.value, start);
}

Because of the borrow rules the Rust compiler is safe to assume that a and b are not borrowing the same object. So it may do any number of optimisations to the assert_eq! line using the assumption that b.value is never modified.

This test can be written which breaks these rules by using the same python object for both arguments:

#[test]
fn test_edit_foo() {
    use pyo3::types::IntoPyDict;

    let gil = Python::acquire_gil();
    let py = gil.python();
    let foo = Py::new(py, Foo { value: 0 }).unwrap().to_object(py);

    let locals = [
        ("foo", foo),
        ("edit_foo", wrap_pyfunction!(edit_foo)(py)),
    ].into_py_dict(py);

    py.run("edit_foo(foo, foo)", None, Some(&locals)).unwrap();
}

This is clearly UB. With cargo test, the assert_eq! check panics and the test fails. With cargo test --release, the test passes, presumably because some optimisation has been applied.

A solution is to implement #342 to guard against this unsoundness.

If a user attempted to call edit_foo(foo, foo) from Python, the only choice I see is to raise a Python exception reading something like "TypeError: Attempted to access a Foo object mutably while also accessing it immutably".

I pushed this test on a branch; you can see the test failure here: https://github.com/davidhewitt/pyo3/runs/414604743?check_suite_focus=true

[kngwyu]

Now we have PyClassShell so we can add a borrow flag to it.
Then PyClassCell may a prefferable name, though...

[davidhewitt]

What about calling it PyCell? Even shorter name but still captures the intent.

[konstin]

I think this another case of #342

[kngwyu]

After my first 1 hour trying to resolve this...
Using RefCell(or our customized borrow checker) is OK, but current PyTryFrom really doesn't fit this change. Maybe fixing PyTryFrom is much more difficult than borrow checking.

[davidhewitt]

Ouch. Yes I guess PyTryFrom is technically also unsound as it allows getting &mut T from the same object multiple times?

[davidhewitt]

@kngwyu if you want I'm happy to have a play with this and propose a WIP PR. (But I'll wait if you know how you want to solve it.)

[kngwyu]

@davidhewitt
Thank you, but I have a rough sketch of the design.
The problem is we have to return Ref instead of &T, which requires PyTryFrom to have custom reference type, like

trait PyTryFrom<'a>: PyTypeInfo<'a> {
    fn try_from(obj: &PyAny) -> <Self as PyTypeInfo>::Ref<'a>;
}

[davidhewitt]

👍 this is what I'm thinking too. I was also thinking PyDowncastError might want to be changed for something that can also represent borrow checker errors?

[davidhewitt]

Fixed in #770