Enforce minimum alignment on entries to fix tagged pointer unsoundness #74
Conversation
|
|
|
I just saw there's a build failure on an older version of Rust, my bad for not checking the MSRV. I just pushed a workaround for that (using an older method for static asserts), let me know if you'd prefer me to squash it with my first commit. |
src/raw/utils/tagged.rs
Outdated
| const fn static_assert_align_of<T: Unpack>() { | ||
| struct Dummy<T>(T); | ||
| impl<T: Unpack> Dummy<T> { | ||
| const ASSERT: () = assert!(align_of::<T>() > !T::MASK); |
There was a problem hiding this comment.
Just a bit, can we put this const in the Unpack trait to avoid the dummy struct?
There was a problem hiding this comment.
Sure, just pushed.
I didn't do that originally since a trait can only supply a "default" implementation, so an impl could in theory override it with e.g. const ASSERT_ALIGNMENT: () = (); and defeat the check. But I don't think that's going to happen by accident!
|
Thanks! |
2 participants
The code currently assumes that it's safe to store tags in the low 3 bits of pointers to
raw::Entry<K, V>values, which is only valid if those pointers are 8-byte aligned (otherwise the tags will overwrite real bits). But theraw::Entrytype itself doesn't enforce any particular alignment besides those of its fields, and for exampleraw::Entry<i32, i32>only has 4-byte alignment on most systems.However, most memory allocators have a larger minimum alignment, which hides the issue. On Linux, GNU libc documents a minimum 8-byte alignment on all architectures, for example. I'm pretty sure this is why none of the tests in this repo caught it.
I found this bug after a long debugging process. I maintain a Zstandard library for Node.js that includes a native add-on, and I supply pre-built binaries which are tested in CI before publication. After upgrading to the recently-released version 30 of the Jest JS testing framework, I found that tests for the 32-bit Windows binaries crashed with an invalid memory access before my native code was even loaded. It turns out that Jest started using the Rust library unrs-resolver, which uses papaya and mimalloc. On 32-bit platforms, mimalloc can return 4-byte-aligned pointers if the type alignment allows it, and if that happens the process pretty quickly goes up in flames.
The fix is easy enough: I added a
align(8)flag to theEntrystructreprto enforce a minimum 8-byte alignment. This should have no effect on platforms where the allocator already enforces this alignment, and has no effect if the types already had 8+-byte alignment to start (it can't reduce alignment).I also added an assertion to the
taggedutilities that blows up at compile time if the code would try to mask out more bits than the type alignment permits. If you add this assert without the above fix, the test suite triggers it in several files.