`PyRefMut::as_super` and `PyClassGuardMut::as_super` enable type confusion

[davidhewitt]

Both PyRefMut::as_super and PyClassGuardMut::as_super return &mut references to the base-type guard.

e.g. PyRefMut<T>::as_super() -> &mut PyRefMut<T::BaseType>.

Unfortunately because of the &mut reference, they can be swapped with other guards to T::BaseType which are from base type objects, i.e. not the same subclass. After such a swap and the .as_super() borrow ends, the original PyRefMut<T> is not sound because it allows access to fields of T on a value which isn't actually T, i.e. creates an out of bounds read.

Here's a quick repro which shows an out of bounds read:

#[crate::pyclass]
#[pyo3(crate = "crate", subclass)]
struct BaseClass {
    val1: usize,
}

#[crate::pyclass]
#[pyo3(crate = "crate", extends=BaseClass, subclass)]
struct SubClass {
    val2: usize,
}

#[test]
fn test_pyrefmut_as_super() {
    Python::attach(|py| {
        let obj = Bound::new(py, PyClassInitializer::from(BaseClass { val1: 10 }).add_subclass(SubClass { val2: 20 })).unwrap();

        let base_obj = BaseClass { val1: 10 }.into_pyobject(py).unwrap();
        let mut obj_guard_mut = obj.borrow_mut();

        // after the swap, `obj_guard_mut` actually points to an instance of the base type
        std::mem::swap(
            obj_guard_mut.as_super(),
            &mut base_obj.borrow_mut(),
        );

        // XXX: Read of uninitialized memory, this panic message is basically arbitrary
        panic!("{}", obj_guard_mut.val2);
    });
}

---

cc @Icxolu (not your API but you most recently worked on these types with me 😄)

I think the only reasonable thing we can do is replace these APIs with types such as PyRefMutSuper<'_, T::BaseType> and PyClassGuardMutSuper<'_, T::BaseType> which don't allow for this invalid swap.

I think for PyClassGuardMut it makes sense to just fix the API now, similar to #6073. For PyRefMut I am less sure, it will be a bigger change. One option is that we deprecate PyRefMut::as_super with a note why the API should be used with caution, and aim to land PyClassGuardMut as the more complete replacement for PyRefMut in 0.30 or 0.31 (as we'd like to do that anyway).

(Credit to Codex security scanning for this discovery.)

[Icxolu]

Uff, yes this is a problem...

I think the only reasonable thing we can do is replace these APIs with types such as PyRefMutSuper<'_, T::BaseType> and PyClassGuardMutSuper<'_, T::BaseType> which don't allow for this invalid swap.

This is definitely an option. What I could also see is using an additional type parameter which keeps track of the original subtype. That way it could only be swapped with the same subtype (which I think would be fine?). Just an idea and I'm not sure it that works, but I have some time in the later half of the week to play around with this. Based on what we came up with for PyClassGuardMut, maybe it gives us a better feeling on how to proceed with PyRefMut

aim to land PyClassGuardMut as the more complete replacement for PyRefMut in 0.30 or 0.31 (as we'd like to do that anyway).

I think it would be great if we could move forward with this. I'm gonna look into that as well.

[davidhewitt]

That way it could only be swapped with the same subtype (which I think would be fine?)

I think that'd be sound, such a swap would be no weirder than two existing PyRefMut<T> values being swapped (for two actual T instances).

Based on what we came up with for PyClassGuardMut, maybe it gives us a better feeling on how to proceed with PyRefMut.

Agreed we can feel our way forwards with the new guard types first, good idea 👍

[Icxolu]

With #6104 now landed: We could do the same PyRefMut and I would not expect it to be a huge problem given that type inference and auto deref makes this pretty seamless. But given that the hole API is on it's way out anyway there is also little gain. Would we be ok with starting to deprecate the whole PyRef / PyRefMut API in 0.29? I've had a quick look through our test suite and found a few code paths that still used PyRef (which I can also push independently, #6111), otherwise it is mostly about replacing Bound::(try_)borrow(_mut). I guess we could go for try_borrow_guard(_mut), deprecate the old ones and in the future move to the old names again.

[davidhewitt]

I think if we don't feel there's benefit in changing / deprecating PyRefMut::as_super on its own in 0.29, let's wait until 0.30 and give ourselves time to get the whole migration correct in the next release. This API has theoretically been a problem since 0.22.