Lokathor / bytemuck

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

Unsound usages of unsafe implementation from different generics #244

Closed llooFlashooll closed 6 months ago

llooFlashooll commented 6 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:331:23: 331:46

/// Try to convert a `&mut T` into `&mut U`.
///
/// As [`try_cast_ref`], but `mut`.
#[inline]
pub(crate) unsafe fn try_cast_mut<A: Copy, B: Copy>(
  a: &mut A,
) -> Result<&mut B, PodCastError> {
  // Note(Lokathor): everything with `align_of` and `size_of` will optimize away
  // after monomorphization.
  if align_of::<B>() > align_of::<A>()
    && !is_aligned_to(a as *const A as *const (), align_of::<B>())
  {
    Err(PodCastError::TargetAlignmentGreaterAndInputNotAligned)
  } else if size_of::<B>() == size_of::<A>() {
    Ok(unsafe { &mut *(a as *mut A as *mut B) })
  } else {
    Err(PodCastError::SizeMismatch)
  }
}

This unsound implementation would create a misalignment issues if the generics type A and B are not properly handled like it's other random 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 6 months ago

You only mention alignment issues, but there is in fact an explicit check in the code you refer to for alignment. If you believe there is something incorrect about this check, 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 6 months ago

Ok, I understand. Thanks for your reply!