{u32, u64}::from_reg_value is unsound

[CAD97]

winreg-rs/src/types.rs

Lines 110 to 128 in fc6521e

	impl FromRegValue for u32 {
	fn from_reg_value(val: &RegValue) -> io::Result<u32> {
	match val.vtype {
	#[allow(clippy::cast_ptr_alignment)]
	REG_DWORD => Ok(unsafe { *(val.bytes.as_ptr() as *const u32) }),
	_ => werr!(Foundation::ERROR_BAD_FILE_TYPE),
	}
	}
	}
	impl FromRegValue for u64 {
	fn from_reg_value(val: &RegValue) -> io::Result<u64> {
	match val.vtype {
	#[allow(clippy::cast_ptr_alignment)]
	REG_QWORD => Ok(unsafe { *(val.bytes.as_ptr() as *const u64) }),
	_ => werr!(Foundation::ERROR_BAD_FILE_TYPE),
	}
	}
	}

RegValue.bytes is a Rust Vec<u8>. This implementation assumes that the vec pointer is sufficiently aligned for u32/u64. This will usually be true (e.g. the Windows System allocator I believe always sufficiently aligns), but this is not guaranteed; an alternative #[global_allocator] could provide allocations at odd addresses. x86 bytecode doesn't care about alignment, but Rust/LLVM does, and an unaligned access is UB even if it works fine on the target.

Significantly more problematic, as RegValue's fields are public, it is possible to provide an empty vector to this function, which will happily try to read from a fully invalid pointer.

let val = RegValue {
    bytes: vec![],
    vtype: REG_QWORD,
};
u64::from_reg_value(&val); // 💣💥‼️

A correct and maximally performant implementation can be written in pure safe code fairly easily:

impl FromRegValue for u32 {
    fn from_reg_value(val: &RegValue) -> io::Result<u32> {
        let bytes = val.bytes.as_slice().try_into();
        match val.vtype {
            REG_DWORD => bytes
                .map(u64::from_ne_bytes)
                .map_err(|_| werr!(ERROR_INVALID_DATA)),
            REG_DWORD_BIG_ENDIAN => bytes
                .map(u64::from_be_bytes)
                .map_err(|_| werr!(ERROR_INVALID_DATA)),
            _ => werr!(Foundation::ERROR_BAD_FILE_TYPE),
        }
    }
}

impl FromRegValue for u64 {
    fn from_reg_value(val: &RegValue) -> io::Result<u64> {
        let bytes = val.bytes.as_slice().try_into();
        match val.vtype {
            REG_QWORD => bytes
                .map(u64::from_ne_bytes)
                .map_err(|_| werr!(ERROR_INVALID_DATA)),
            _ => werr!(Foundation::ERROR_BAD_FILE_TYPE),
        }
    }
}

Any cost imposed by the bounds check and unaligned access is trivially dominated by the cost of reading from the registry.

[CAD97]

"Fun" is any downstream which has copied the unsound pointer access code. Which I believe includes std 🙃

clippy::cast_ptr_alignment may be an aggressive lint, but it's often correct that your code is incorrect unless you are the one to have manually allocated that pointer.

[gentoo90]

Fixed in 0.14.0(winapi)/0.52.0(windows-rs)