Affine2 (f32) Can’t Implement bytemuck::Pod

[JoeOsborn]

Since Mat2 is 16 byte aligned and Vec2 only takes up 8 bytes, I believe there are 8 following padding bytes in Affine2. According to bytemuck’s documentation it’s illegal to implement Pod for structs with padding bytes (this causes UB when converting from a slice and bad data when converting to a slice).

This could be fixed by removing the unsafe impl Pod or by adding an explicit 8 byte _padding field to Affine2.

There may be other types affected by this too. Anything containing a 16 byte aligned type like Mat2 or Vec4 and then some other stuff may be suspect.

I’m not sure, but does the bytemuck Pod derive check that constraint? If so could it be conditionally applied on structs if the bytemuck feature is active rather than having these unsafe impls?

[bitshifter]

Yes, that does sound like it would violate Bytemuck's Pod constraints. I don't believe there are any checks in Bytemuck, I'm not sure it would be possible for bytemuck itself to check. Vec3A, Mat3A and Affine3A are the only other types to my knowledge which should not be Pod, they use AnyBitPattern instead of Pod.

[JoeOsborn]

The bytemuck derive macro is supposed to check the constraints: https://docs.rs/bytemuck/latest/bytemuck/derive.Pod.html

Manual unsafe implementations of Pod of course don’t get such checks.

[bitshifter]

Ah thanks, good to know.

[bitshifter]

I think I didn't use derive because at the moment most features are fairly self contained implementation wise, e.g. all the bytemuck changes are in https://github.com/bitshifter/glam-rs/blob/main/src/features/impl_bytemuck.rs. I could use derive, it can just get a bit messy with lots of cfg checks everywhere. Perhaps there is a way I could make use of the macro in testing somehow without needing to annotate all the structs in glam with it.

[JoeOsborn]

That's an interesting idea. I can imagine two solutions off the top of my head:

1. Just to check the existence of invisible padding bytes, you could (for a test) write some templates that generate the structs as normal and also with #[attr(packed)] and statically assert that their sizes are the same. I don't know if there's a way to statically check whether a struct has padding... if you had an idea of how big each struct was supposed to be you could use static asserts to make sure they're all the right size. (e.g., Affine2 "should" be 24, but that test would fail because of alignment, so we would know to add the padding field and check that it's 32).
2. You could also have a set of templates or parameters that generate the struct definitions with the bytemuck Pod derive on them.

But since you can't add derives to a struct after it's defined, there's no easy way I can see to do these things without either duplicating some templates or adding more parameters to the templates, at which point you may as well generate the cfg-bounded Pod derives.

You can get close to (1) with something like this for each type in each template (e.g., this one is for Affine2):

            const _: fn() = || {
                std::mem::transmute::<
                    _,
                    [u8; std::mem::size_of::<Mat2>() + std::mem::size_of::<Vec2>()],
                >(Affine2::IDENTITY);
            };

If that throws a compile error it will be because the sum of the field sizes is different from the size of Affine2. I borrowed the idea from bytemuck. But if you're not using the derive and have to copy the field types into the assertion, it's potentially error-prone.

Other things can go wrong with Pod too, e.g. if generics are used the rules are a little different or if any field is not Pod it is not valid.

[JoeOsborn]

Miri can catch some of these types of issues. I modified the unit tests in impl_bytemuck like so to add a loop that checks each byte against itself:

                let t = <$t>::default();
                let b = bytemuck::bytes_of(&t);
                for bi in b {
                    assert_eq!(bi, bi);
                }
                // should be the same address
                assert_eq!(&t as *const $t as usize, b.as_ptr() as usize);
                // should be the same size
                assert_eq!(b.len(), mem::size_of_val(&t));

Now making bytemuck part of the default features and a cargo run +nightly miri will flag the access of uninitialized bytes for Affine2. If you could modify this test in this way and run miri on all combinations of feature flags (at least scalar/non-scalar) then maybe you'd have some assurance that this won't bite anyone again.

[bitshifter]

Fix is committed, I am keeping this open as a reminder to look into ways of testing it, e.g. using miri as described above.