rust-osdev / linked-list-allocator

Apache License 2.0
219 stars 53 forks source link

fuzz: remove potential undefined behavior in chaos harness #80

Closed 00xc closed 11 months ago

00xc commented 11 months ago

The chaos harness has a potential UB bug reported by Miri due to mutable pointer aliasing. The heap object has a mutable reference to HEAP_MEM, which gets invalidated when calculating remaining_space, as it does so through a mut pointer. Thus, using heap after using the pointer is technically undefined behavior under Rust's aliasing rules.

https://github.com/rust-osdev/linked-list-allocator/blob/3c9bafaf88918a77615e6692c6fffbc7ab3428b5/fuzz/fuzz_targets/chaos.rs#L36

https://github.com/rust-osdev/linked-list-allocator/blob/3c9bafaf88918a77615e6692c6fffbc7ab3428b5/fuzz/fuzz_targets/chaos.rs#L84-L87

https://github.com/rust-osdev/linked-list-allocator/blob/3c9bafaf88918a77615e6692c6fffbc7ab3428b5/fuzz/fuzz_targets/chaos.rs#L94

Fix this by taking a const pointer.

Note that it is very unlikely this caused any actual issues under the current state of the compiler.

This can be tested by running the following reproducer (a simplified version of the chaos harness) under Miri (cargo +nightly miri run).

Reproducer ```rust use linked_list_allocator::Heap; use std::alloc::Layout; use std::ptr::NonNull; #[derive(Debug)] enum Action { // allocate a chunk with the size specified Alloc { size: u16, align_bit: u8 }, // free the pointer at the index specified Free { index: u8 }, // extend the heap by amount specified Extend { additional: u16 }, } use Action::*; const MAX_HEAP_SIZE: usize = 5000; static mut HEAP_MEM: [u8; MAX_HEAP_SIZE] = [0; MAX_HEAP_SIZE]; fn main() { let actions = vec![ Alloc { size: 25, align_bit: 1, }, Extend { additional: 255 }, ]; let size = 100; fuzz(size, actions); } fn fuzz(size: u16, actions: Vec) { // init heap let mut heap = unsafe { let size = size as usize; if size > MAX_HEAP_SIZE || size < 3 * core::mem::size_of::() { return; } Heap::new(HEAP_MEM.as_mut_ptr(), size) }; let mut ptrs: Vec<(NonNull, Layout)> = Vec::new(); // process operations for action in actions { match action { Alloc { size, align_bit } => { let layout = { let align = 1_usize.rotate_left(align_bit as u32); if align == 1 << 63 { return; } Layout::from_size_align(size as usize, align).unwrap() }; if let Ok(ptr) = heap.allocate_first_fit(layout) { ptrs.push((ptr, layout)); } else { return; } } Free { index } => { if index as usize >= ptrs.len() { return; } let (ptr, layout) = ptrs.swap_remove(index as usize); unsafe { heap.deallocate(ptr, layout); } } Extend { additional } => // safety: new heap size never exceeds MAX_HEAP_SIZE unsafe { let remaining_space = HEAP_MEM .as_mut_ptr() .add(MAX_HEAP_SIZE) .offset_from(heap.top()); assert!(remaining_space >= 0); if additional as isize > remaining_space { return; } heap.extend(additional as usize); }, } } // free the remaining allocations for (ptr, layout) in ptrs { unsafe { heap.deallocate(ptr, layout); } } // make sure we can allocate the full heap (no fragmentation) let full = Layout::from_size_align(heap.size(), 1).unwrap(); assert!(heap.allocate_first_fit(full).is_ok()); } ```
Freax13 commented 11 months ago

The heap object has a mutable reference to HEAP_MEM, which gets invalidated when calculating remaining_space, as it does so through a mut pointer. Thus, using heap after using the pointer is technically undefined behavior under Rust's aliasing rules.

Wouldn't get the mutable reference in heap also get invalidated by the creation of a shared reference? Note that you can also create a pointer to the HEAP_MEM global without creating a reference by using the addr_of! macro.

00xc commented 11 months ago

Wouldn't get the mutable reference in heap also get invalidated by the creation of a shared reference?

Technically we are not creating any reference, right? Miri does not seem to complain either.

Note that you can also create a pointer to the HEAP_MEM global without creating a reference by using the addr_of! macro.

Yeah maybe this is cleaner, thanks!

Freax13 commented 11 months ago

Wouldn't get the mutable reference in heap also get invalidated by the creation of a shared reference?

Technically we are not creating any reference, right? Miri does not seem to complain either.

The call to slice::as_ptr creates a reference to the slice. I'm not sure why Miri doesn't complain.