FyroxEngine / Fyrox

3D and 2D game engine written in Rust
https://fyrox.rs
MIT License
7.7k stars 346 forks source link

Type erasure in `VertexBuffer`'s data storage is unsound #481

Closed Nemo157 closed 1 year ago

Nemo157 commented 1 year ago

It is a requirement of Vec that it is dropped with a type that has the same alignment as it was allocated with, inside VertexBuffer::new the passed Vec<T> is type-erased into a Vec<u8>, with most of the T types being passed having alignment 4 rather than u8's alignment 1, resulting in undefined behavior when the value is deallocated.

This can be observed by running a minimal testcase under Miri:

use fyrox::{
    core::algebra::Vector3,
    scene::mesh::{buffer::VertexBuffer, vertex::SimpleVertex},
};

fn main() {
    VertexBuffer::new(
        1,
        vec![SimpleVertex {
            position: Vector3::new(0.0, 0.0, 0.0),
        }],
    )
    .unwrap();
}
> cargo miri run
...
error: Undefined Behavior: incorrect layout on deallocation: alloc1451 has size 12 and alignment 4, but gave size 12 and alignment 1
   --> /home/nemo157/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:121:14
    |
121 |     unsafe { __rust_dealloc(ptr, layout.size(), layout.align()) }
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ incorrect layout on deallocation: alloc1451 has size 12 and alignment 4, but gave size 12 and alignment 1
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
    = note: BACKTRACE:
    = note: inside `std::alloc::dealloc` at /home/nemo157/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:121:14: 121:64
    = note: inside `<std::alloc::Global as std::alloc::Allocator>::deallocate` at /home/nemo157/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:258:22: 258:51
    = note: inside `<alloc::raw_vec::RawVec<u8> as std::ops::Drop>::drop` at /home/nemo157/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/raw_vec.rs:485:22: 485:56
    = note: inside `std::ptr::drop_in_place::<alloc::raw_vec::RawVec<u8>> - shim(Some(alloc::raw_vec::RawVec<u8>))` at /home/nemo157/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:497:1: 497:56
    = note: inside `std::ptr::drop_in_place::<std::vec::Vec<u8>> - shim(Some(std::vec::Vec<u8>))` at /home/nemo157/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:497:1: 497:56
    = note: inside `std::ptr::drop_in_place::<fyrox::scene::mesh::buffer::VertexBuffer> - shim(Some(fyrox::scene::mesh::buffer::VertexBuffer))` at /home/nemo157/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:497:1: 497:56
note: inside `main`
   --> src/main.rs:51:14
    |
51  |     .unwrap();
    |              ^
...
mrDIMAS commented 1 year ago

Could you please re-check the issue? I committed the potential fix for it.

Nemo157 commented 1 year ago

The DerefMut<Target = Vec<u8>> still makes me a bit wary, if any reallocating function is called on that there will be similar UB when it realloc's the storage. Skimming through I think this is reachable via push_vertex.

EDIT: Yeah, reproducer depending on the git repo:

use fyrox::{
    core::algebra::Vector3,
    scene::mesh::{buffer::VertexBuffer, vertex::SimpleVertex},
};

fn main() {
    let vertex = SimpleVertex {
        position: Vector3::new(0.0, 0.0, 0.0),
    };
    let mut buf = VertexBuffer::new(1, vec![vertex]).unwrap();
    buf.modify().push_vertex(&vertex).unwrap();
}
error: Undefined Behavior: incorrect layout on deallocation: alloc1459 has size 12 and alignment 4, but gave size 12 and alignment 1
   --> /home/nemo157/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:140:14
    |
140 |     unsafe { __rust_realloc(ptr, layout.size(), layout.align(), new_size) }
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ incorrect layout on deallocation: alloc1459 has size 12 and alignment 4, but gave size 12 and alignment 1
    |
...
    = note: inside `std::vec::Vec::<u8>::extend_from_slice` at /home/nemo157/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/vec/mod.rs:2386:9: 2386:39
    = note: inside `fyrox::scene::mesh::buffer::VertexBufferRefMut::<'_>::push_vertex::<fyrox::scene::mesh::vertex::SimpleVertex>` at cargo/git/checkouts/fyrox-3b28ccbadb9683b5/a85ad50/src/scene/mesh/buffer.rs:347:13: 349:62
note: inside `main`
   --> src/main.rs:11:5
    |
11  |     buf.modify().push_vertex(&vertex).unwrap();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
mrDIMAS commented 1 year ago

Yeah, you absolutely right, I did a potential fix, could you please re-check it?