search

mooman219 / fontdue

The fastest font renderer in the world, written in pure rust.
Apache License 2.0
1.38k stars 70 forks source link

Misaligned pointer dereference in `get_bitmap` #142

Closed shinmao closed 7 months ago

shinmao commented 9 months ago

The source of unsoundness

Hi, we found that the safe function get_bitmap might include unsound implementation. https://github.com/mooman219/fontdue/blob/9e7bacff105089a5a7904c99ff58044303cf5a10/src/platform/float/get_bitmap.rs#L59-L60 At line 59, the mutable pointer from output (aligned to 1 byte) is transmuted to &mut i32 (aligned to 4 bytes), and this created a misaligned pointer. Misaligned pointer dereference at line 60 can leads to undefined behavior.

mooman219 commented 9 months ago

You're right that it looks like unaligned reads, but x86_64 implementations from AMD and Intel create memory allocations aligned to usually at least 16 byte boundaries, and we always read in 4 byte chunks, so we never actually end up with an unaligned read since we're reading 4 byte chunks from a 16 byte aligned allocation.

Probably undefined behavior as far as rust is concerned, I'd have to revisit the docs to see if they guarantee some sort of alignment on the initial allocation in writing. If not I can explicitly request the alignment on allocation which should be a no-op

shinmao commented 9 months ago

@mooman219 Yeah. I know what you mean. However, the allocated vec is not always aligned to >= 16 bytes as I know. Please see the link: https://github.com/rust-lang/unsafe-code-guidelines/issues/38#issuecomment-439690567 It still depends on the alignment of elements, and it could be greater than it.

Here is the official unsafe code reference to talk about the layout of packed SIMD vector: https://rust-lang.github.io/unsafe-code-guidelines/layout/packed-simd-vectors.html

mooman219 commented 9 months ago

Fair enough, I couldn't actually get it to allocate unaligned after fiddling with compiler settings for a little bit, so it's not likely to be an issue, but it's definitely compiler implementation defined. That being said, unaligned reads are benign security wise on the targets this codepath is enabled for at least.

I'll get this resolved, thanks for the heads up!

mooman219 commented 7 months ago

Published a new version and re-benched. The change is within noise. Thanks for the analysis!