Skip to content

Fix read_pixels_of_type UB: use bytemuck::cast_slice#918

Merged
mrDIMAS merged 2 commits into
FyroxEngine:masterfrom
kimjune01:fix/read-pixels-manually-drop
May 14, 2026
Merged

Fix read_pixels_of_type UB: use bytemuck::cast_slice#918
mrDIMAS merged 2 commits into
FyroxEngine:masterfrom
kimjune01:fix/read-pixels-manually-drop

Conversation

@kimjune01
Copy link
Copy Markdown
Contributor

Summary

  • Replaces unsafe Vec::from_raw_parts + mem::forget with safe bytemuck::cast_slice + to_vec()
  • The previous implementation had two sources of UB:
    1. mem::forget received a Vec<u8> whose memory was already transferred to another Vec
    2. Vec<T> would deallocate with align_of::<T>() but the memory was allocated with align_of::<u8>() = 1
  • The new implementation safely casts the byte slice and copies into a properly-aligned Vec<T>
  • bytemuck is already a dependency of fyrox-graphics

Test plan

  • cargo check -p fyrox-graphics passes
  • CI should verify no regressions across the full test suite

Closes #827

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 FyroxEngine#827
Comment thread fyrox-graphics/src/framebuffer.rs Outdated
// Use bytemuck for a safe, correctly-aligned conversion. This copies into a new
// Vec<T> with proper alignment, avoiding the UB from reinterpreting a Vec<u8>
// allocation as Vec<T> (which would deallocate with the wrong layout).
Some(bytemuck::cast_slice::<u8, T>(&bytes).to_vec())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.to_vec call is kinda concerning here, I'm pretty sure that this can be done without calling it. Probably by creating a proxy struct that keeps the memory layout like in here - https://github.com/FyroxEngine/Fyrox/blob/master/fyrox-impl/src/scene/mesh/buffer.rs#L269-L277

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.
@kimjune01
Copy link
Copy Markdown
Contributor Author

@mrDIMAS thanks for the pointer to BytesStorage — addressed in 1d26960.

Introduced a TypedPixels<T: Pod> proxy struct that owns the original Vec<u8> and exposes &[T] via Deref (using bytemuck::cast_slice). The byte buffer is kept intact, so it's freed with the same Layout::array::<u8>(cap) it was allocated with. No .to_vec(), no copy.

I didn't carry an explicit Layout field like BytesStorage does, because that field is only needed when BytesStorage later decomposes back via from_raw_parts and deallocates manually. Here the Vec<u8> is never decomposed — dropping it as Vec<u8> already uses the right layout. Happy to add the field for symmetry if you'd prefer.

The single caller (fyrox-impl/src/renderer/hdr/mod.rs:207, evaluator.average_luminance(&pixels)) compiles unchanged via deref coercion. cargo check -p fyrox-graphics and cargo check -p fyrox-impl both pass.

@mrDIMAS mrDIMAS merged commit e8740b6 into FyroxEngine:master May 14, 2026
@du82
Copy link
Copy Markdown

du82 commented May 16, 2026

This "Kim June" guy is using AI to generate and submit pull requests. I suggest blocking him.

@mrDIMAS
Copy link
Copy Markdown
Member

mrDIMAS commented May 17, 2026

Yeah, but these changes were super obvious and I'm kinda ok with that. Anything more complex might not pass the review, because it would require real knowledge :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GpuFrameBufferTrait::read_pixels_of_type contains UB

3 participants