[T; $x]::read_bytes_default_le can drop uninitialized memory on panic

[ammaraskar]

Hi there, we (Rust group @sslab-gatech) are scanning crates on crates.io for potential soundness bugs. We noticed that in the read_bytes_default_le impl for [T; $x]

byte-struct-rs/byte_struct/src/lib.rs

Lines 410 to 422 in 9c41996

	fn read_bytes_default_le(bytes: &[u8]) -> Self {
	let mut pos = 0;
	let len = T::BYTE_LEN;
	let mut result: Self;
	unsafe {
	result = std::mem::uninitialized();
	for i in 0 .. ($x) {
	std::ptr::write(&mut result[i], <T>::read_bytes_default_le(&bytes[pos .. pos + len]));
	pos += len;
	}
	}
	result
	}

Memory is created to hold the unpacked type T using std::mem::uninitialized(); and then <T>::read_bytes_default_le is called, which in the case of custom types could potentially panic. This means that the uninitialized memory would be dropped as if it were an instance of T, causing soundness problems. See this example:

#![forbid(unsafe_code)]

use byte_struct::*;

// Custom type that panics when reading bytes.
struct CustomByteStruct(u8);

impl ByteStructLen for CustomByteStruct {
    const BYTE_LEN: usize = 1;
}

impl ByteStruct for CustomByteStruct {
    fn write_bytes(&self, bytes: &mut [u8]) { }
    fn read_bytes(bytes: &[u8]) -> Self { panic!("Panic when reading") }
}

impl Drop for CustomByteStruct {
    fn drop(&mut self) { println!("Dropping {}", self.0) }
}

// Wrapper around the type above so we can use the
// `ByteStructUnspecifiedByteOrder for [T; $x]` impl.
#[derive(ByteStruct)]
#[byte_struct_le]
struct ArrayOfCustomByteStruct {
    custom_structs: [CustomByteStruct; 2]
}

fn main() {
    let bytes = [0x01, 0x02];
    let deserialized = ArrayOfCustomByteStruct::read_bytes(&bytes[..]);
}

It outputs:

thread 'main' panicked at 'Panic when reading', src/main.rs:31:43
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Dropping 166
Dropping 179

Return code 101

Notice that the drop is printing out uninitialized memory.

This code should probably use MaybeUninit to avoid this problem.

[wwylele]

Thanks for the report! There was an attempt of rewriting this part with MaybeUninit in the past, but it didn't quite work out as transmute couldn't prove the size equality that involves associated constant for types involving generics, or something like that. I will try that again today and see if I can resolve it

Edit: this was the problem with MaybeUninit: rust-lang/rust#47966

[wwylele]

I decided to think less and just replace it with safe code that is possible today in a535678

[ammaraskar]

That looks great, thank you for the quick fix!