tracel-ai / burn

Burn is a new comprehensive dynamic Deep Learning Framework built using Rust with extreme flexibility, compute efficiency and portability as its primary goals.
https://burn.dev
Apache License 2.0
8.67k stars 430 forks source link

Wrong usage of unsafe `Vec::from_raw_parts` in TensorData #2375

Open WorldSEnder opened 1 week ago

WorldSEnder commented 1 week ago

https://github.com/tracel-ai/burn/blob/b29367c1b8763f0e7d81a32c574259d21b33a1f5/crates/burn-tensor/src/tensor/data.rs#L84

This unsafe line has an incorrect usage of Vec::from_raw_parts due to re-interpreting a pointer to some E as a pointer to u8. Note that the docs state in the safety requirements:

T needs to have the same alignment as what ptr was allocated with.

This is because some allocators care about the alignment of these types, hence the buffer that was initially allocated with alignment for the arbitrary (even possibly user-defined) type E will be deallocated with alignment of 1. Some allocators do not care about alignment where this bug does not surface. In any case, miri will complain about this minimal test case:

#[cfg(test)]
#[test]
fn bug() {
    let _ = burn::tensor::TensorData::new(vec![0xdeadu32], [1]);
}

incorrect layout on deallocation: alloc80436 has size 4 and alignment 4, but gave size 4 and alignment 1


Comment: The asserts at the top of the function is pointless, since size_of::<u8>() is guaranteed to return 1. Comment2: The unconditional truncation of the input vector was a bit surprising - instead of an error due to length mismatch as in the case when the input vector is too short - and made me look closer into the function in the first place ;)

WorldSEnder commented 1 week ago

bytemuck has a BoxBytes struct that could contain the allocation in an erased form, but it doesn't implement Clone and Debug.

nathanielsimard commented 1 week ago

For now I think all Element types are valid in terms of alignment, making the checks quite useless and confusing.