Lokathor / bytemuck

A crate for mucking around with piles of bytes
https://docs.rs/bytemuck
Apache License 2.0
697 stars 77 forks source link

Unsound usages of unsafe implementation from `[u8]` to `T` #245

Closed llooFlashooll closed 3 months ago

llooFlashooll commented 3 months ago

Hi, I am scanning the bytemuck in the latest version with my own static analyzer tool.

Unsafe conversion found at: src/internal.rs:117:17: 117:45

pub(crate) unsafe fn try_pod_read_unaligned<T: Copy>(
  bytes: &[u8],
) -> Result<T, PodCastError> {
  if bytes.len() != size_of::<T>() {
    Err(PodCastError::SizeMismatch)
  } else {
    Ok(unsafe { (bytes.as_ptr() as *const T).read_unaligned() })
  }
}

This unsound implementation would create a misalignment issues. The [u8] and T are different types.

This would potentially cause undefined behaviors in Rust. If we further manipulate the problematic converted types, it would potentially lead to different consequences. I am reporting this issue for your attention.

zachs18 commented 3 months ago

You only mention alignment issues, but the code here uses read_unaligned which is explicitly allowed to read unaligned data. If you believe there is something else incorrect about this code, please say what you think the issue is, specifically.


When using static analysis tools that may have false positives, generally you need to look at the code being flagged to ensure that it is not a false positive, as in this case.

llooFlashooll commented 3 months ago

Ok, thank you very much for your report. That's my bad. I will try to only report TP.