Fix UB in *_offset functions

[mejrs]

If the addr_of macro is not available, this created a zeroed PyCell<T>, but arbitrary T's do not permit zero-initialization.

[adamreichold]

I suspect it had been considered when this code was written, but isn't memoffset the central place where these workarounds are collected?

[mejrs]

I suspect it had been considered when this code was written, but isn't memoffset the central place where these workarounds are collected?

memoffset just chooses the least UB way to do this, and it cannot avoid UB if addr_of! is unavailable. The fallback as written here is not UB.

FWIW I suspect the real reason that memoffset was not used is to avoid adding a dependency.

[danielhenrymantilla]

memoffset shall never reach exploited UB using cargo + rustc:

• before addr_of!, such fixed list of rustc implementations did not exploit its UB;
• after addr_of!, it no longer has UB.

It is thus as fine as it could possibly be.

The current PR duplicates a structure's layout, which means these could theoretically end up out of sync as the codebase is updated, which in a vacuum (I don't know the constraints of PyO3), seems orders of magnitude more error-prone than using an unofficial Rust implementation with no addr_of! which would exploit that UB

---

One approach to make the code robust to original struct updates, while keeping the current interesting strategy, would be:

type Apply<Wrapper, T> = <Wrapper as CanWrap<T>>::T;

trait CanWrap<T> : 'static { type T; }

/// For any `T`, `Apply<Identity, T> = T`.
enum Identity {}
impl<T> CanWrap<T> for Identity { type T = T; }

/// For any `T`, `Apply<InManuallyDrop, T> = ManuallyDrop<T>`
enum InManuallyDrop {}
impl<T> CanWrap<T> for InManuallyDrop { type T = ManuallyDrop<T>; }

#[repr(C)]
pub(crate) struct PyCellContents<T: PyClassImpl, Wrapper = Identity>
where
    // this would not be needed if we had (type) GATs
    Wrapper : CanWrap< ManuallyDrop<UnsafeCell<T>> >,
    Wrapper : CanWrap< <T::PyClassMutability as PyClassMutability>::Storage >,
    Wrapper : CanWrap< T::ThreadChecker >,
    Wrapper : CanWrap< T::Dict >,
    Wrapper : CanWrap< T::WeakRef >,
{
    pub(crate) value: Apply<Wrapper, ManuallyDrop<UnsafeCell<T> >,
    pub(crate) borrow_checker: Apply<Wrapper, <T::PyClassMutability as PyClassMutability>::Storage >,
    pub(crate) thread_checker: Apply<Wrapper, T::ThreadChecker >,
    pub(crate) dict: Apply<Wrapper, T::Dict >,
    pub(crate) weakref: Apply<Wrapper, T::WeakRef >,
}

• (cc Stabilize generic associated types rust-lang/rust#96709 btw)

that way, the MaybeUninit-projected struct could be queried by using PyCellContents<T, InManuallyDrop>, and would be guaranteed to be kept in sync.

• (Sadly, without (type) GATs, this is a mouthful, so I still think that using memoffset (or inlining its logic) is the least error-prone approach)

• (another approach would be to define a helper derive to get the …Dummy def auto-generated alongside the non-Dummy one, which with a macro_rules! macro is not that annoying to write)

[mejrs]

Hmm, I like simple, and just using memoffset sounds simple.

[davidhewitt]

Thanks, using memoffset sounds reasonable to me. Please add a CHANGELOG entry. Not sure whether this classes as Packaging or Fixed category.

Thanks for cleaning up the tests with flaky type inference issues too! 😁

[davidhewitt]

Should we add a note here that it's to support safe offset calculations on versions predating addr_of? I imagine in the future we might consider removing again.

[adamreichold]

I think we are using it to compute offsets in all cases since even when addr_of is available, using memoffset enables us to avoid writing out its usage ourselves.

Personally, I think as it takes care of what is available where and how to use it, memoffset does carry its weight as a dependency.

[davidhewitt]

Agree with this, I was thinking more that after a future MSRV bump that may change as addr_of would be trivial for us to implement safely. I'll worry about that in the future, perhaps 😄