ratel-rust / toolshed

Arena allocator and a handful of useful data structures
Apache License 2.0
39 stars 6 forks source link

allocated value is incorrectly aligned #11

Open Kogia-sima opened 3 years ago

Kogia-sima commented 3 years ago

Description

According to Rust official reference (https://doc.rust-lang.org/reference/type-layout.html):

A value of alignment n must only be stored at an address that is a multiple of n.

However, if the value with more than 8 bytes is allocated using toolshed, it will be incorrectly aligned. That means toolshed may cause undefined behaviour without unsafe block.

Example

// main.rs

use toolshed::Arena;

#[repr(align(4096))]
#[derive(Clone, Copy, Default)]
struct U64Array {
    values: [u64; 16],
}

fn main() {
    println!("allocated on stack: {:p}", &U64Array::default() as *const _);

    let arena = Arena::new();
    let array = arena.alloc(U64Array::default());
    println!("allocated using arena: {:p}", array as *mut _);
}
$ cargo run
allocated on stack: 0x7ffde7138000
allocated using arena: 0x564cf4988ca0

Possible solution

bumpalo crate has more heauristic way to correctly align the value.

https://github.com/fitzgen/bumpalo/blob/a1f663217f93b79b25c9580db33c54e19d022e9e/src/lib.rs#L920-L921

Or, forbit types with more than 8 byte alignement.

kitten commented 1 year ago

I'm not sure if this is related or whether it’s a coincidence — but with Rust 1.70.0 targeting the wasm32-wasi target I’m currently seeing a misalignment that leads to a runtime panic and failing toolshed tests.

Specifically, running cargo wasi test -- --nocapture in Rust 1.70.0 surfaces a misalignment. Currently, I was able to fix this test and resolve the panic by changing the aligment in this section: https://github.com/ratel-rust/toolshed/blob/8cff0c97ab34a70f1d45f39d854489637a0215c5/src/arena.rs#L318-L321

Swapping size_of::<usize>() out with size_of::<u64>() fixes it, but is obviously not acceptable. However, using std::mem::align_of_val(&5i64) also fixes the issue.

+        let align = std::mem::align_of_val(&5i64);
-        let size = match size % size_of::<usize>() {
+        let size = match size % align {
             0 => size,
-            n => size + (size_of::<usize>() - n),
+            n => size + (align - n),
         };

However, I’m really not confident enough in the codebase or in my memory alignment knowledge to know whether this is correct.