From 3a5e38f8ff82dce0290969f2dc10937de4407f92 Mon Sep 17 00:00:00 2001 From: James Munns Date: Mon, 14 Nov 2022 13:07:46 +0100 Subject: [PATCH 1/5] Minimal fix - just leak allocations --- .github/workflows/build.yml | 2 +- src/test.rs | 13 ++++++------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 9b3115b..392731c 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -135,7 +135,7 @@ jobs: name: "Miri tests" runs-on: ubuntu-latest env: - MIRIFLAGS: "-Zmiri-disable-isolation -Zmiri-strict-provenance -Zmiri-tag-raw-pointers" + MIRIFLAGS: "-Zmiri-disable-isolation -Zmiri-strict-provenance -Zmiri-tag-raw-pointers -Zmiri-ignore-leaks" steps: - uses: actions/checkout@v1 - run: rustup toolchain install nightly --profile minimal --component rust-src miri diff --git a/src/test.rs b/src/test.rs index 1e88c19..b5e209e 100644 --- a/src/test.rs +++ b/src/test.rs @@ -43,15 +43,14 @@ impl DerefMut for OwnedHeap { pub fn new_heap() -> OwnedHeap { const HEAP_SIZE: usize = 1000; let mut heap_space = Box::new(Chonk::::new()); - let data = &mut heap_space.data; + let data = &mut Box::leak(heap_space).data; let assumed_location = data.as_mut_ptr().cast(); let heap = unsafe { Heap::new(data.as_mut_ptr().cast(), data.len()) }; assert_eq!(heap.bottom(), assumed_location); assert_eq!(heap.size(), align_down_size(HEAP_SIZE, size_of::())); - let drop = move || { - let _ = heap_space; + // let _ = heap_space; }; OwnedHeap { heap, _drop: drop } } @@ -60,7 +59,7 @@ fn new_max_heap() -> OwnedHeap { const HEAP_SIZE: usize = 1024; const HEAP_SIZE_MAX: usize = 2048; let mut heap_space = Box::new(Chonk::::new()); - let data = &mut heap_space.data; + let data = &mut Box::leak(heap_space).data; let start_ptr = data.as_mut_ptr().cast(); // Unsafe so that we have provenance over the whole allocation. @@ -69,7 +68,7 @@ fn new_max_heap() -> OwnedHeap { assert_eq!(heap.size(), HEAP_SIZE); let drop = move || { - let _ = heap_space; + // let _ = heap_space; }; OwnedHeap { heap, _drop: drop } } @@ -77,11 +76,11 @@ fn new_max_heap() -> OwnedHeap { fn new_heap_skip(ct: usize) -> OwnedHeap { const HEAP_SIZE: usize = 1000; let mut heap_space = Box::new(Chonk::::new()); - let data = &mut heap_space.data[ct..]; + let data = &mut Box::leak(heap_space).data[ct..]; let heap = unsafe { Heap::new(data.as_mut_ptr().cast(), data.len()) }; let drop = move || { - let _ = heap_space; + // let _ = heap_space; }; OwnedHeap { heap, _drop: drop } } From 5f3ba041e0cf6555035054adc8e5d9abc1b4e2b6 Mon Sep 17 00:00:00 2001 From: James Munns Date: Mon, 14 Nov 2022 13:33:59 +0100 Subject: [PATCH 2/5] Revert leaking, track POINTER instead of REFERENCE for leaking --- .github/workflows/build.yml | 2 +- src/test.rs | 63 +++++++++++++++++++++++-------------- 2 files changed, 41 insertions(+), 24 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 392731c..9b3115b 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -135,7 +135,7 @@ jobs: name: "Miri tests" runs-on: ubuntu-latest env: - MIRIFLAGS: "-Zmiri-disable-isolation -Zmiri-strict-provenance -Zmiri-tag-raw-pointers -Zmiri-ignore-leaks" + MIRIFLAGS: "-Zmiri-disable-isolation -Zmiri-strict-provenance -Zmiri-tag-raw-pointers" steps: - uses: actions/checkout@v1 - run: rustup toolchain install nightly --profile minimal --component rust-src miri diff --git a/src/test.rs b/src/test.rs index b5e209e..179f478 100644 --- a/src/test.rs +++ b/src/test.rs @@ -10,13 +10,13 @@ use std::{ #[repr(align(128))] struct Chonk { - data: [MaybeUninit; N], + data: MaybeUninit<[u8; N]>, } impl Chonk { pub fn new() -> Self { Self { - data: [MaybeUninit::uninit(); N], + data: MaybeUninit::uninit(), } } } @@ -42,15 +42,18 @@ impl DerefMut for OwnedHeap { pub fn new_heap() -> OwnedHeap { const HEAP_SIZE: usize = 1000; - let mut heap_space = Box::new(Chonk::::new()); - let data = &mut Box::leak(heap_space).data; - let assumed_location = data.as_mut_ptr().cast(); + let heap_space_ptr: *mut Chonk = { + let owned_box = Box::new(Chonk::::new()); + let mutref = Box::leak(owned_box); + mutref + }; + let data_ptr: *mut u8 = unsafe { core::ptr::addr_of_mut!((*heap_space_ptr).data).cast() }; - let heap = unsafe { Heap::new(data.as_mut_ptr().cast(), data.len()) }; - assert_eq!(heap.bottom(), assumed_location); + let heap = unsafe { Heap::new(data_ptr, HEAP_SIZE) }; + assert_eq!(heap.bottom(), data_ptr); assert_eq!(heap.size(), align_down_size(HEAP_SIZE, size_of::())); let drop = move || { - // let _ = heap_space; + let _ = unsafe { Box::from_raw(heap_space_ptr) }; }; OwnedHeap { heap, _drop: drop } } @@ -58,29 +61,37 @@ pub fn new_heap() -> OwnedHeap { fn new_max_heap() -> OwnedHeap { const HEAP_SIZE: usize = 1024; const HEAP_SIZE_MAX: usize = 2048; - let mut heap_space = Box::new(Chonk::::new()); - let data = &mut Box::leak(heap_space).data; - let start_ptr = data.as_mut_ptr().cast(); + let heap_space_ptr: *mut Chonk = { + let owned_box = Box::new(Chonk::::new()); + let mutref = Box::leak(owned_box); + mutref + }; + let data_ptr: *mut u8 = unsafe { core::ptr::addr_of_mut!((*heap_space_ptr).data).cast() }; // Unsafe so that we have provenance over the whole allocation. - let heap = unsafe { Heap::new(start_ptr, HEAP_SIZE) }; - assert_eq!(heap.bottom(), start_ptr); + let heap = unsafe { Heap::new(data_ptr, HEAP_SIZE) }; + assert_eq!(heap.bottom(), data_ptr); assert_eq!(heap.size(), HEAP_SIZE); let drop = move || { - // let _ = heap_space; + let _ = unsafe { Box::from_raw(heap_space_ptr) }; }; OwnedHeap { heap, _drop: drop } } fn new_heap_skip(ct: usize) -> OwnedHeap { const HEAP_SIZE: usize = 1000; - let mut heap_space = Box::new(Chonk::::new()); - let data = &mut Box::leak(heap_space).data[ct..]; - let heap = unsafe { Heap::new(data.as_mut_ptr().cast(), data.len()) }; + let heap_space_ptr: *mut Chonk = { + let owned_box = Box::new(Chonk::::new()); + let mutref = Box::leak(owned_box); + mutref + }; + let data_ptr: *mut u8 = unsafe { core::ptr::addr_of_mut!((*heap_space_ptr).data).cast() }; + + let heap = unsafe { Heap::new(data_ptr.add(ct), HEAP_SIZE - ct) }; let drop = move || { - // let _ = heap_space; + let _ = unsafe { Box::from_raw(heap_space_ptr) }; }; OwnedHeap { heap, _drop: drop } } @@ -95,17 +106,23 @@ fn empty() { #[test] fn oom() { const HEAP_SIZE: usize = 1000; - let mut heap_space = Box::new(Chonk::::new()); - let data = &mut heap_space.data; - let assumed_location = data.as_mut_ptr().cast(); + let heap_space_ptr: *mut Chonk = { + let owned_box = Box::new(Chonk::::new()); + let mutref = Box::leak(owned_box); + mutref + }; + let data_ptr: *mut u8 = unsafe { core::ptr::addr_of_mut!((*heap_space_ptr).data).cast() }; - let mut heap = unsafe { Heap::new(data.as_mut_ptr().cast(), data.len()) }; - assert_eq!(heap.bottom(), assumed_location); + let mut heap = unsafe { Heap::new(data_ptr, HEAP_SIZE) }; + assert_eq!(heap.bottom(), data_ptr); assert_eq!(heap.size(), align_down_size(HEAP_SIZE, size_of::())); let layout = Layout::from_size_align(heap.size() + 1, align_of::()); let addr = heap.allocate_first_fit(layout.unwrap()); assert!(addr.is_err()); + + // Explicitly unleak the heap allocation + let _ = unsafe { Box::from_raw(heap_space_ptr) }; } #[test] From 3f8f9fc96c9978e2758b311ba4a001cc646c849a Mon Sep 17 00:00:00 2001 From: James Munns Date: Mon, 14 Nov 2022 13:40:49 +0100 Subject: [PATCH 3/5] Move repeated code into the Chonk type --- src/test.rs | 66 ++++++++++++++++++++++------------------------------- 1 file changed, 27 insertions(+), 39 deletions(-) diff --git a/src/test.rs b/src/test.rs index 179f478..73b62e0 100644 --- a/src/test.rs +++ b/src/test.rs @@ -14,10 +14,24 @@ struct Chonk { } impl Chonk { - pub fn new() -> Self { - Self { - data: MaybeUninit::uninit(), - } + /// Returns (almost certainly aliasing) pointers to the Chonk + /// as well as the data payload. + /// + /// MUST be freed with a matching call to `Chonk::unleak` + pub fn new() -> (*mut Chonk, *mut u8) { + let heap_space_ptr: *mut Chonk = { + let owned_box = Box::new(Self { + data: MaybeUninit::uninit(), + }); + let mutref = Box::leak(owned_box); + mutref + }; + let data_ptr: *mut u8 = unsafe { core::ptr::addr_of_mut!((*heap_space_ptr).data).cast() }; + (heap_space_ptr, data_ptr) + } + + pub unsafe fn unleak(putter: *mut Chonk) { + drop(Box::from_raw(putter)) } } @@ -42,57 +56,36 @@ impl DerefMut for OwnedHeap { pub fn new_heap() -> OwnedHeap { const HEAP_SIZE: usize = 1000; - let heap_space_ptr: *mut Chonk = { - let owned_box = Box::new(Chonk::::new()); - let mutref = Box::leak(owned_box); - mutref - }; - let data_ptr: *mut u8 = unsafe { core::ptr::addr_of_mut!((*heap_space_ptr).data).cast() }; + let (heap_space_ptr, data_ptr) = Chonk::::new(); let heap = unsafe { Heap::new(data_ptr, HEAP_SIZE) }; assert_eq!(heap.bottom(), data_ptr); assert_eq!(heap.size(), align_down_size(HEAP_SIZE, size_of::())); - let drop = move || { - let _ = unsafe { Box::from_raw(heap_space_ptr) }; - }; + let drop = move || unsafe { Chonk::unleak(heap_space_ptr) }; OwnedHeap { heap, _drop: drop } } fn new_max_heap() -> OwnedHeap { const HEAP_SIZE: usize = 1024; const HEAP_SIZE_MAX: usize = 2048; - let heap_space_ptr: *mut Chonk = { - let owned_box = Box::new(Chonk::::new()); - let mutref = Box::leak(owned_box); - mutref - }; - let data_ptr: *mut u8 = unsafe { core::ptr::addr_of_mut!((*heap_space_ptr).data).cast() }; + let (heap_space_ptr, data_ptr) = Chonk::::new(); // Unsafe so that we have provenance over the whole allocation. let heap = unsafe { Heap::new(data_ptr, HEAP_SIZE) }; assert_eq!(heap.bottom(), data_ptr); assert_eq!(heap.size(), HEAP_SIZE); - let drop = move || { - let _ = unsafe { Box::from_raw(heap_space_ptr) }; - }; + let drop = move || unsafe { Chonk::unleak(heap_space_ptr) }; OwnedHeap { heap, _drop: drop } } fn new_heap_skip(ct: usize) -> OwnedHeap { const HEAP_SIZE: usize = 1000; - let heap_space_ptr: *mut Chonk = { - let owned_box = Box::new(Chonk::::new()); - let mutref = Box::leak(owned_box); - mutref - }; - let data_ptr: *mut u8 = unsafe { core::ptr::addr_of_mut!((*heap_space_ptr).data).cast() }; + let (heap_space_ptr, data_ptr) = Chonk::::new(); let heap = unsafe { Heap::new(data_ptr.add(ct), HEAP_SIZE - ct) }; - let drop = move || { - let _ = unsafe { Box::from_raw(heap_space_ptr) }; - }; + let drop = move || unsafe { Chonk::unleak(heap_space_ptr) }; OwnedHeap { heap, _drop: drop } } @@ -106,12 +99,7 @@ fn empty() { #[test] fn oom() { const HEAP_SIZE: usize = 1000; - let heap_space_ptr: *mut Chonk = { - let owned_box = Box::new(Chonk::::new()); - let mutref = Box::leak(owned_box); - mutref - }; - let data_ptr: *mut u8 = unsafe { core::ptr::addr_of_mut!((*heap_space_ptr).data).cast() }; + let (heap_space_ptr, data_ptr) = Chonk::::new(); let mut heap = unsafe { Heap::new(data_ptr, HEAP_SIZE) }; assert_eq!(heap.bottom(), data_ptr); @@ -120,9 +108,9 @@ fn oom() { let layout = Layout::from_size_align(heap.size() + 1, align_of::()); let addr = heap.allocate_first_fit(layout.unwrap()); assert!(addr.is_err()); - + // Explicitly unleak the heap allocation - let _ = unsafe { Box::from_raw(heap_space_ptr) }; + unsafe { Chonk::unleak(heap_space_ptr) }; } #[test] From de40bb120b59a9469478cfb84b8cdc9eba846bd9 Mon Sep 17 00:00:00 2001 From: James Munns Date: Mon, 14 Nov 2022 13:53:50 +0100 Subject: [PATCH 4/5] Actually fix leaks I was running with the ignore-leaks MIRIFLAGS set locally previously. --- src/test.rs | 49 +++++++++++++++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/src/test.rs b/src/test.rs index 73b62e0..00da541 100644 --- a/src/test.rs +++ b/src/test.rs @@ -35,12 +35,28 @@ impl Chonk { } } -pub struct OwnedHeap { +pub struct Dropper { + putter: *mut Chonk, +} + +impl Dropper { + fn new(putter: *mut Chonk) -> Self { + Self { putter } + } +} + +impl Drop for Dropper { + fn drop(&mut self) { + unsafe { Chonk::unleak(self.putter) } + } +} + +pub struct OwnedHeap { heap: Heap, - _drop: F, + _drop: Dropper, } -impl Deref for OwnedHeap { +impl Deref for OwnedHeap { type Target = Heap; fn deref(&self) -> &Self::Target { @@ -48,24 +64,26 @@ impl Deref for OwnedHeap { } } -impl DerefMut for OwnedHeap { +impl DerefMut for OwnedHeap { fn deref_mut(&mut self) -> &mut Self::Target { &mut self.heap } } -pub fn new_heap() -> OwnedHeap { +pub fn new_heap() -> OwnedHeap<1000> { const HEAP_SIZE: usize = 1000; let (heap_space_ptr, data_ptr) = Chonk::::new(); let heap = unsafe { Heap::new(data_ptr, HEAP_SIZE) }; assert_eq!(heap.bottom(), data_ptr); assert_eq!(heap.size(), align_down_size(HEAP_SIZE, size_of::())); - let drop = move || unsafe { Chonk::unleak(heap_space_ptr) }; - OwnedHeap { heap, _drop: drop } + OwnedHeap { + heap, + _drop: Dropper::new(heap_space_ptr), + } } -fn new_max_heap() -> OwnedHeap { +fn new_max_heap() -> OwnedHeap<2048> { const HEAP_SIZE: usize = 1024; const HEAP_SIZE_MAX: usize = 2048; let (heap_space_ptr, data_ptr) = Chonk::::new(); @@ -75,18 +93,21 @@ fn new_max_heap() -> OwnedHeap { assert_eq!(heap.bottom(), data_ptr); assert_eq!(heap.size(), HEAP_SIZE); - let drop = move || unsafe { Chonk::unleak(heap_space_ptr) }; - OwnedHeap { heap, _drop: drop } + OwnedHeap { + heap, + _drop: Dropper::new(heap_space_ptr), + } } -fn new_heap_skip(ct: usize) -> OwnedHeap { +fn new_heap_skip(ct: usize) -> OwnedHeap<1000> { const HEAP_SIZE: usize = 1000; let (heap_space_ptr, data_ptr) = Chonk::::new(); let heap = unsafe { Heap::new(data_ptr.add(ct), HEAP_SIZE - ct) }; - - let drop = move || unsafe { Chonk::unleak(heap_space_ptr) }; - OwnedHeap { heap, _drop: drop } + OwnedHeap { + heap, + _drop: Dropper::new(heap_space_ptr), + } } #[test] From 14bf9c5bc369b1d6d4a9fc40a4ab83cf91bdcc8b Mon Sep 17 00:00:00 2001 From: James Munns Date: Mon, 14 Nov 2022 13:58:43 +0100 Subject: [PATCH 5/5] Add comment regarding load-bearing drop order --- src/test.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/test.rs b/src/test.rs index 00da541..3ff0514 100644 --- a/src/test.rs +++ b/src/test.rs @@ -53,6 +53,9 @@ impl Drop for Dropper { pub struct OwnedHeap { heap: Heap, + // /!\ SAFETY /!\: Load bearing drop order! `_drop` MUST be dropped AFTER + // `heap` is dropped. This is enforced by rust's built-in drop ordering, as + // long as `_drop` is declared after `heap`. _drop: Dropper, }