Fails with alignment errors when run under miri

[ids1024]

Running with cargo +nightly miri test --no-default-features --features std,png-format, I see alignment errors like this:

---- pipeline::blend_tests::clear_highp stdout ----
thread 'pipeline::blend_tests::clear_highp' panicked at 'cast_slice_mut>TargetAlignmentGreaterAndInputNotAligned', /home/ian/.cargo/registry/src/github.com-1ecc6299db9ec823/bytemuck-1.13.0/src/internal.rs:32:3
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

So it's not unsound since it's checked by bytemuck, but crashes. Miri presumably is giving us allocations that are no more aligned than required, while typically allocations are more aligned than this.

[ids1024]

Things like Pixmap::pixels are incorrect. If one wants data to be aligned appropriately for an array of u32s, it can't be allocated in a Vec<u8>.

I presume SIMD code may have even stronger alignment requirements, but I'm not too familiar with SIMD and haven't looked at how it's handled in tiny-skia. Miri isn't able to handle the SIMD calls so it can't test that.

[RazrFalcon]

I'm not sure I understand the issue. Pixmap is guarantee to store u8 * 4, therefore alignment isn't an issue.

PixmapRef::pixels, on the other hand, when created from an unaligned store may cause issues. But I'm not sure it can be validated and should simply be clarified in docs.

Otherwise the code works and does not panic. It's hypothetically incorrect, but it works just fine in our case. Am I missing something?

[ids1024]

Pixmap contains a Vec<u8>. u8 has an alignment of 1, so the contents of the vec must have an alignment of 1, which get passed in the layout to alloc::alloc. In practice, the default system allocator may consistently provide something more aligned than this (malloc() doesn't take an alignment argument), but this could break with a custom global allocator, on a different platform, or technically in a future version of Rust.

PremultipliedColorU8 contains a u32, so on most platforms it requires a 4 byte alignment. (If the alignment requirement is not desirable/needed, it could instead contain [u8; 4]).

[CryZe]

I guess it would make sense to change that to a Vec<u32> then. Can easily be bytemucked to &mut [u8] when needed.

[ids1024]

Yeah, though this also impacts {PixmapRef, PixmapMut}::from_bytes.

If alignment isn't needed than representing pixels with [u8; 4] rather than u32 may make sense.

[CryZe]

Yeah, though this also impacts {PixmapRef, PixmapMut}::from_bytes.

Not necessarily, they can also use bytemuck to cast and then either panic like tiny-skia currently does, or possibly return Result / Option.

Although yeah maybe it makes sense to change the piece of code that even turns it into a [u32] and instead make it use a [[u8; 4]].

[ids1024]

Yeah, it's perfectly sound to panic or return an error when a given &[u8] slice isn't aligned, but it doesn't seem like a great API. (Guaranteeing your own u8 slice is aligned is non-trivial, if you didn't just cast it from a u32 slice or something.) It should either take a &[u32] that must already be aligned, or not require alignment.

[RazrFalcon]

I see. Will take a look.

Not sure if the compiler is smart enough to treat [[u8; 4]] the same as [u32]. Will check.

And I'm not sure how much we rely on u32 alignment anyway. Will see.