crayon icon indicating copy to clipboard operation
crayon copied to clipboard

Unsound implementation of `DataBuffer::extend` and `extend_from_slice`

Open shinmao opened this issue 1 year ago • 1 comments

The source of unsoundness

https://github.com/shawnscode/crayon/blob/48d4e879996e2502e0faaf36e4dbcebfca9961b0/src/utils/data_buf.rs#L26-L31 Hi, I think these two functions might include unsound implementation. They both allow users to pass arbitrary types and cast to u8 type. To guarantee the safety of using slice::from_raw_parts, callers need to make sure the u8 pointer points to consecutive initialized memory. If users pass the arbitrary types with padding bytes, then it could break the guarantee. I think T should be limited to primitive types. Same issue could happen in extend_from_slice.

shinmao avatar Sep 01 '23 22:09 shinmao

Similar issue could be found in video::assets::mesh::IndexFormat::encode

https://github.com/shawnscode/crayon/blob/48d4e879996e2502e0faaf36e4dbcebfca9961b0/src/video/assets/mesh.rs#L163-L169 At line 168, users could also pass arbitrary types and cast to u8 type.

To reproduce the bug

use crayon::video::assets::mesh::IndexFormat;

#[repr(align(64))]
#[derive(Copy, Clone, Debug)]
struct Padding {
    a: u8,
    b: u16,
    c: u8,
}

fn main() {
    let pd = Padding { a: 10, b: 11, c: 12 };
    let pd_arr: [Padding; 1] = [pd; 1];
    let db = IndexFormat::encode(&pd_arr);
    println!("{:?}", db);
}

to run with miri,

error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory
   --> /home/rafael/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/num.rs:466:5
    |
466 | /     impl_Display!(
467 | |         i8, u8, i16, u16, i32, u32, i64, u64, usize, isize
468 | |             as u64 via to_u64 named fmt_u64
469 | |     );
    | |_____^ using uninitialized data, but this operation requires initialized memory
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior

Note

In extend and extend_from_slice, even though library didn't read/write the slice memory, it still broke the safety guarantee of from_slice_parts. Library should avoid such kind of usage.

shinmao avatar Sep 01 '23 22:09 shinmao