Unsoundness in (decode/encode)_variable_primitive_vec

[shinmao]

Bug and PoC

Hi, we consider the following function has the unsound implementation:

rosrust/rosrust/src/rosmsg.rs

Lines 217 to 235 in c46a7e6

	pub fn decode_variable_primitive_vec<R: io::Read, T: RosMsg>(mut r: R) -> io::Result<Vec<T>> {
	let num_elements = u32::decode(r.by_ref())? as usize;
	let num_bytes = num_elements * std::mem::size_of::<T>();
	// Allocate the memory w/o initializing because we will later fill
	// all the memory.
	let mut buf = Vec::<T>::with_capacity(num_elements);
	let buf_ptr = buf.as_mut_ptr();
	// Fill the Vec to full capacity with the stream data.
	let read_buf = unsafe { std::slice::from_raw_parts_mut(buf_ptr as *mut u8, num_bytes) };
	r.read_exact(read_buf)?;
	// Do not drop the memory
	std::mem::forget(buf);
	// Return a new, completely full Vec using the now initialized memory.
	Ok(unsafe { Vec::from_raw_parts(buf_ptr, num_elements, num_elements) })

The function first defines the type of buffer it wants to fill up; then read from the input data. This could break the validity in Rust. For example, we use bool as the generic type T, then this function allows us to write some invalid bits into the buffer while Rust only allows 0x00 or 0x01 to be boolean type.

Following is the PoC:

use rosrust::rosmsg::decode_variable_primitive_vec;
use rosrust::RosMsg;
use std::io::{self, Cursor, Read, Write};

fn main() -> io::Result<()> {
    let invalid_data = vec![0x02, 0x03, 0xFF, 0x00, 0x01];

    // Encode the number of elements
    let mut encoded_data = Vec::new();
    (invalid_data.len() as u32).encode(&mut encoded_data)?;
    encoded_data.extend_from_slice(&invalid_data);

    // Use a Cursor to simulate reading from a stream
    let mut cursor = Cursor::new(encoded_data);

    let decoded_vec = decode_variable_primitive_vec::<_, bool>(&mut cursor)?;
    println!("Decoded vector: {:?}", decoded_vec);

    for (i, &b) in decoded_vec.iter().enumerate() {
        println!("Boolean at index {}: {:?}", i, b);
    }

    Ok(())
}

run with miri then you can get,

error: Undefined Behavior: constructing invalid value: encountered 0x02, but expected a boolean
    --> /{user}/.rustup/toolchains/nightly-2023-06-01-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:2307:25
     |
2307 |         Display::fmt(if *self { "true" } else { "false" }, f)
     |                         ^^^^^ constructing invalid value: encountered 0x02, but expected a boolean

At the same time, we also consider the both functions should inherit to the supertrait Pod rather than Sized. The reason is that slice::from_raw_parts requires the raw pointer to be initialized. However, we can implement our own repr(Rust) struct, which could contains padding bytes, and cast to u8 pointer at line 228. It could break the safety requirement of from_raw_parts.

Please consider to use Pod for scalar types!!