From e8740b64526f73adb681a309e94694966306b13e Mon Sep 17 00:00:00 2001
From: June <kimjune01@gmail.com>
Date: Thu, 14 May 2026 00:48:13 -0700
Subject: [PATCH] Fix read_pixels_of_type UB: use bytemuck::cast_slice (#918)

* fix: eliminate UB in read_pixels_of_type via bytemuck::cast_slice

The previous implementation used Vec::from_raw_parts to reinterpret a
Vec<u8> as Vec<T>, then mem::forget to suppress the original Vec's
destructor. This was UB for two reasons:
1. mem::forget receives an invalid value (already-transferred ownership)
2. Vec<T> deallocates with align_of::<T>(), but the memory was allocated
   with align_of::<u8>() = 1, violating alloc/dealloc layout invariants

Replace with bytemuck::cast_slice + to_vec(), which safely casts the
byte slice and copies into a properly-aligned Vec<T>. The Pod bound
already guarantees valid bit patterns for any byte sequence.

Closes #827

* fix: zero-copy TypedPixels<T> wrapper preserves original byte layout

Per @mrDIMAS review: avoid the .to_vec() copy by introducing a
TypedPixels<T> proxy struct that owns the original Vec<u8> (preserving
its allocation layout for correct deallocation) and exposes &[T] via
Deref + bytemuck::cast_slice. Mirrors the BytesStorage pattern used in
fyrox-impl/src/scene/mesh/buffer.rs.

The single caller (HDR luminance histogram) uses &pixels as a slice,
which works unchanged via deref coercion.
---
 fyrox-graphics/src/framebuffer.rs | 63 +++++++++++++++++++++++++------
 1 file changed, 52 insertions(+), 11 deletions(-)

diff --git a/fyrox-graphics/src/framebuffer.rs b/fyrox-graphics/src/framebuffer.rs
index ee8dc3a93..f9fafd3cc 100644
--- a/fyrox-graphics/src/framebuffer.rs
+++ b/fyrox-graphics/src/framebuffer.rs
@@ -322,22 +322,63 @@ pub trait GpuFrameBufferTrait: GpuFrameBufferAsAny {
     ) -> Result<DrawCallStatistics, FrameworkError>;
 }
 
+/// Owned, typed view of pixels read back from a [`GpuFrameBufferTrait`]. Preserves the
+/// original byte allocation (and therefore its memory layout) so it is dropped with the
+/// same `Layout` it was allocated with, while exposing a `&[T]` view of the contents.
+///
+/// This avoids the UB of reinterpreting a `Vec<u8>` allocation directly as `Vec<T>`
+/// (which would deallocate with `Layout::array::<T>(cap)` instead of the original
+/// `Layout::array::<u8>(cap)`) without copying the data.
+pub struct TypedPixels<T: Pod> {
+    bytes: Vec<u8>,
+    _marker: std::marker::PhantomData<T>,
+}
+
+impl<T: Pod> TypedPixels<T> {
+    /// Returns the pixels as a typed slice.
+    #[inline]
+    pub fn as_slice(&self) -> &[T] {
+        bytemuck::cast_slice(&self.bytes)
+    }
+
+    /// Number of `T` elements in the readback.
+    #[inline]
+    pub fn len(&self) -> usize {
+        self.bytes.len() / std::mem::size_of::<T>()
+    }
+
+    /// Whether the readback contains zero elements.
+    #[inline]
+    pub fn is_empty(&self) -> bool {
+        self.len() == 0
+    }
+}
+
+impl<T: Pod> std::ops::Deref for TypedPixels<T> {
+    type Target = [T];
+
+    #[inline]
+    fn deref(&self) -> &[T] {
+        self.as_slice()
+    }
+}
+
 impl dyn GpuFrameBufferTrait {
     /// Reads the pixels and reinterprets them using the given type.
-    pub fn read_pixels_of_type<T>(&self, read_target: ReadTarget) -> Option<Vec<T>>
+    ///
+    /// Returns an owning [`TypedPixels<T>`] that derefs to `&[T]`. The underlying byte
+    /// buffer is preserved as-is, so it is freed with the same memory layout it was
+    /// allocated with (avoiding the UB of dropping a `Vec<u8>` allocation as a `Vec<T>`),
+    /// and no extra copy is made when constructing the typed view.
+    pub fn read_pixels_of_type<T>(&self, read_target: ReadTarget) -> Option<TypedPixels<T>>
     where
         T: Pod,
     {
-        let mut bytes = self.read_pixels(read_target)?;
-        let typed = unsafe {
-            Vec::<T>::from_raw_parts(
-                bytes.as_mut_ptr() as *mut T,
-                bytes.len() / size_of::<T>(),
-                bytes.capacity() / size_of::<T>(),
-            )
-        };
-        std::mem::forget(bytes);
-        Some(typed)
+        let bytes = self.read_pixels(read_target)?;
+        Some(TypedPixels {
+            bytes,
+            _marker: std::marker::PhantomData,
+        })
     }
 }