Unsound implementation of `transmute_vec_as_bytes` in `fyrox-core`

[shinmao]

Hi, we are researchers from SunLab. We consider the safe function transmute_vec_as_bytes includes the unsound implementation.

transmute_vec_as_bytes

Fyrox/fyrox-core/src/lib.rs

Lines 325 to 331 in 3b03ea7

	pub fn transmute_vec_as_bytes<T: Copy>(vec: Vec<T>) -> Vec<u8> {
	unsafe {
	let mut vec = std::mem::ManuallyDrop::new(vec);
	Vec::from_raw_parts(
	vec.as_mut_ptr() as *mut u8,
	vec.len() * std::mem::size_of::<T>(),
	vec.capacity() * std::mem::size_of::<T>(),

When casting type to u8 slice, we also need to make sure whether the type won't contain padding bytes. Otherwise, it could lead to uninitialized memory access or break the reliability of program. Based on the usages of the function, we consider the generic type T should also implement Pod trait.

PoC

use fyrox_core::transmute_vec_as_bytes;

#[derive(Copy, Clone)]
struct Pad {
    a: u8,
    b: u32,
    c: u8
}

fn main() {
    let pd = Pad { a: 0x1, b: 0x2, c: 0x3 };
    let mut v = Vec::new();
    v.push(pd);
    let fv = transmute_vec_as_bytes(v);
    println!("{:?}", fv);
}

In the program above, we passed the struct that might contain padding bytes as the generic type T. First, when we run with miri, we can get the following results:

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:471:5
    |
471 | /     impl_Display!(
472 | |         i8, u8, i16, u16, i32, u32, i64, u64, usize, isize
473 | |             as u64 via to_u64 named fmt_u64
474 | |     );
    | |_____^ using uninitialized data, but this operation requires initialized memory

In the library, we found that this function uses Vec as input as internal usages, which will not trigger the bug. However, since this function is provided as public safe API, we should consider the constraints on the generic type T. (It is required also because the function name is transmute_"vec"_as_bytes :)

The consequence of UB

We could find that the results of fv break the reliability of program under different environment.
Compiled with x86_64, the results would be:

[2, 0, 0, 0, 1, 3, 0, 0]

While compiled with x86, the results would be:

[2, 0, 0, 0, 1, 3, 233, 247]

Take the following usages for example,

Fyrox/fyrox-impl/src/scene/terrain/mod.rs

Lines 101 to 112 in 25a2296

	fn make_height_map_texture_internal(
	height_map: Vec<f32>,
	size: Vector2<u32>,
	) -> Option<TextureResource> {
	let mut data = Texture::from_bytes(
	TextureKind::Rectangle {
	width: size.x,
	height: size.y,
	},
	TexturePixelKind::R32F,
	crate::core::transmute_vec_as_bytes(height_map),
	)?;

The Texture built on this bytes could have different results...

[shinmao]

We also considered that array_as_u8_slice and value_as_u8_slice unsound with usages of casting. The trait of Sized doesn't pose the limitation on composite types.

• Fyrox/fyrox-core/src/lib.rs

  Line 302 in 3b03ea7

  pub fn array_as_u8_slice<T: Sized>(v: &[T]) -> &'_ [u8] {

• Fyrox/fyrox-core/src/lib.rs

  Line 319 in 3b03ea7

  pub fn value_as_u8_slice<T: Sized>(v: &T) -> &'_ [u8] {

[orzogc]

IMO something like NoUninit should be added in the trait bounds.

[NathanaelG1]

Noticed this was tagged as a good first issue, I have made some changes to the function referenced by @shinmao. Instead of adding a dependency on bytemuck, I think I was able to handle the padding issue without the use of any unsafe blocks. Ran into a lot of issues trying to handle any possible value, but noticed it is only referenced three times. Out of each case the vec is either a usize or an f32. If other types need to be supported I can add support.

I have run a modified version of the FPS tutorial as well as the RPG tutorial and all the logs appear the same as running off the latest release. All tests are passing, and I added two tests to cover the transmute_vec_as_bytes.

If I am barking up the wrong tree with this approach let me know and I can follow the suggestion by @orzogc.

I have this playground showing the current function as is and its output as well as the updated function.

I linked my PR to my forked repo and it is in the history above, should I open a PR to the main fork to receive feedback? I haven't made any documentation updates as I wanted to check with everyone if this is a valid solution. Once everything looks good to people more familiar with the project I will add a documentation update to the PR.

functions downstream of transmute_vec_as_bytes:

[mrDIMAS]

@shinmao the issue should be fixed by the recent commits, could you please re-check this?

[shinmao]

Yes. It could fix the issue.

[shinmao]

Hi, @mrDIMAS I saw that the issue has been patched. However, it is not included in the latest release (0.28.1). Do you plan to release this patch?

[mrDIMAS]

Yes, this change will be released as part of Fyrox 0.35 which should be released in a month or so.