[Unsoundness] Out-of-bounds read in public API parse_unk_size_null_utf16_string due to unsafe loop condition order

Hello, While working on a static analysis tool designed to detect unsoundness and safety violations in Rust projects, we identified a potential vulnerability in the etw-reader crate within this repository. Location: The issue is located in the function parse_unk_size_null_utf16_string in etw-reader/src/utils.rs. https://github.com/mstange/samply/blob/c947d6339a9bea6c5659c6317796c5061d0fd7e6/etw-reader/src/utils.rs#L8 Since utils is a public module and parse_unk_size_null_utf16_string is a public function, this unsafe API is accessible to external users, allowing Safe Rust code to trigger Undefined Behavior (UB). Description: The function accepts a safe slice &[u8] and attempts to interpret it as a null-terminated UTF-16 string. However, the while loop used to find the null terminator has a flawed condition order: ``` // Current implementation while unsafe { *ptr } != 0 && ptr < end { len += 1; ptr = unsafe { ptr.offset(1) }; } ``` The logical AND (&&) operator evaluates from left to right. This means unsafe { *ptr } is executed before checking if ptr < end. If the input slice contains valid aligned data but does not contain a null terminator (e.g., [0xFF, 0xFF, 0xFF, 0xFF]), the pointer ptr will eventually advance to equal end. In the final iteration check, the code will attempt to read *end (which is out of bounds) before the ptr < end check can stop the loop. This results in an out-of-bounds read. Proof of Concept (PoC): We constructed a reproduction case where a safe call with a specifically crafted slice triggers UB. ``` use std::alloc::{alloc, Layout}; // Code extracted from etw-reader/src/utils.rs for reproduction fn is_aligned<T>(ptr: *const T) -> bool where T: Sized, { ptr as usize & (std::mem::align_of::<T>() - 1) == 0 } pub fn parse_unk_size_null_utf16_string(v: &[u8]) -> String { let start: *const u16 = v.as_ptr().cast(); if !is_aligned(start) { panic!("Not aligned"); } // safe because we not going past the end of the slice let end: *const u16 = unsafe { v.as_ptr().add(v.len()) }.cast(); // find the null termination let mut len = 0; let mut ptr = start; // BUG HERE: *ptr is dereferenced before checking ptr < end while unsafe { *ptr } != 0 && ptr < end { len += 1; ptr = unsafe { ptr.offset(1) }; } let slice = unsafe { std::slice::from_raw_parts(start, len) }; String::from_utf16_lossy(slice) } #[repr(align(2))] struct AlignedData { // 4 bytes, corresponding to 2 u16s. // Intentionally no 0x0000 (null terminator). bytes: [u8; 4], } fn main() { // 1. Construct data: [0xFF, 0xFF, 0xFF, 0xFF] // Interpreted as two u16::MAX. let v = AlignedData { bytes: [0xFF, 0xFF, 0xFF, 0xFF], }; // 2. Call the vulnerable function with a safe slice. // If the function were sound, this should panic or return a string, but not UB. let _ = parse_unk_size_null_utf16_string(&v.bytes); } ``` Miri Output: Running this with Miri (cargo miri run) confirms the out-of-bounds memory access: ``` error: Undefined Behavior: memory access failed: expected a pointer to 2 bytes of memory, but got alloc472+0x4 which is at or beyond the end of the allocation of size 4 bytes --> src\main.rs:25:20 | 25 | while unsafe { *ptr } != 0 && ptr < end { | ^^^^ memory access failed: expected a pointer to 2 bytes of memory, but got alloc472+0x4 which is at or beyond the end of the allocation of size 4 bytes ``` Recommendation: To fix this, the order of the checks in the while loop condition must be reversed. You must verify that the pointer is within bounds before dereferencing it. ``` // Check bounds first, then dereference while ptr < end && unsafe { *ptr } != 0 { len += 1; ptr = unsafe { ptr.offset(1) }; } ``` Alternatively, utilizing safer iterators or cast_slice from crates like bytemuck (as hinted in the code comments) would avoid manual pointer arithmetic entirely.