tcdi / postgrestd

The most peculiar std you have ever seen
Other
37 stars 4 forks source link

`pallocator` does not respect `layout.align()` #17

Closed thomcc closed 1 year ago

thomcc commented 1 year ago

I noticed that pallocator does not respect the requested alignment -- it totally ignores it -- palloc only guarantees word-alignment (e.g. size_of::<usize>()), so this is unsound, as rustc tells LLVM about the alignment, so which can actually cause problems (crashes on aarch64 for sure, but even x86_64 if SIMD gets used... and given our -Ctarget-cpu=native use, it might).

Fixing it basically looks like:

  1. if layout.align() <= WORD_ALIGN use palloc/pfree/etc, since they're fine.
  2. Allocate size + align bytes for overaligned requests, and store the real pointer that you need to free immediately before the aligned pointer.
  3. When freeing, if the request is overaligned: offset the pointer we're given by -size_of::<*mut u8>(), read() the result, and pass it to pfree.

There's some trickiness to how you offset the pointer to get the aligned version (even if it didn't have the "do not rely on this for correctness" caveat, align_offset is not quite right -- we need to ensure we have space for the real pointer).

A totally rough/untested sketch that punts on realloc: https://gist.github.com/thomcc/d3ce63c4f592cc5be6f894234c4e220b.

workingjubilee commented 1 year ago

Yeaaaah. Postgres is fairly pedantic about alignment and I suspect we've been effectively coasting on the Postgres inclination to be pedantic about alignment... but notably it does not really care about alignments greater than 8, whereas Rust supports alignments up to at least a page size, and indeed many types are aligned to 16 and greater, so we were due for getting busted on this the moment we tried to use SIMD stuff.

thomcc commented 1 year ago

We no longer use this allocator so I'm closing this.