cgmath icon indicating copy to clipboard operation
cgmath copied to clipboard

Unspecified behavior in `impl_tuple_conversions`

Open Mokuzzai opened this issue 4 years ago • 2 comments

The conversion between a vector and a tuple is defined as follows:

https://docs.rs/cgmath/0.18.0/src/cgmath/macros.rs.html#226-238

impl<$S> AsRef<$Tuple> for $ArrayN<$S> {
    #[inline]
    fn as_ref(&self) -> &$Tuple {
        unsafe { mem::transmute(self) }
    }
}

impl<$S> AsMut<$Tuple> for $ArrayN<$S> {
    #[inline]
    fn as_mut(&mut self) -> &mut $Tuple {
        unsafe { mem::transmute(self) }
    }
}

which is effectively a cast between &[T; 2] and &(T, T) and contains unspecified behavior since the layout of tuples (exept the unit ()) is not defined.

https://doc.rust-lang.org/reference/type-layout.html#tuple-layout

Tuples do not have any guarantees about their layout.

Mokuzzai avatar Aug 18 '21 18:08 Mokuzzai

I noticed a few more impls which suffer from the same bug:

AsRef<(S..)>, AsMut<(S..)>, From<&(S..)> and From<&mut (S..)>

For each vector, point and the quaternion.

A possible solution I came up with is to check that the layout of the Vectors and Tuples match either at compile- or runtime.

This would mean that code that currently works stays the same, and if the layout of tuples happens to be different (possible field reordering) either a compile-error or a panic occurs.

EXAMPLE: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=f9b78fab51bdd60e35f4e29606a3eb03

Mokuzzai avatar Aug 19 '21 15:08 Mokuzzai

Those impls should be deprecated and then later removed in a subsequent release.

aloucks avatar Oct 23 '21 02:10 aloucks