rustgd / cgmath

A linear algebra and mathematics library for computer graphics.
https://docs.rs/cgmath
Apache License 2.0
1.12k stars 155 forks source link

Unspecified behavior in `impl_tuple_conversions` #538

Open Mokuzzai opened 3 years ago

Mokuzzai commented 3 years ago

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 commented 3 years ago

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

aloucks commented 2 years ago

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