From 427da47f10b84c72acd5dbab616cdd7b0c692d6c Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Fri, 31 Oct 2025 08:49:07 +0100 Subject: [PATCH 1/5] Reduce unsafe code in bump allocator and add tests --- src/bump.rs | 144 +++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 102 insertions(+), 42 deletions(-) diff --git a/src/bump.rs b/src/bump.rs index e767753..5fd2dd2 100644 --- a/src/bump.rs +++ b/src/bump.rs @@ -6,10 +6,9 @@ //! The primary use case of this allocator is reduction of Rust future sizes, due to //! `rustc` not being very intelligent w.r.t. stack usage in async functions. -use core::marker::PhantomData; -use core::mem::MaybeUninit; +use core::mem::{self, MaybeUninit}; use core::pin::Pin; -use core::ptr::NonNull; +use core::slice; use embassy_sync::blocking_mutex::raw::RawMutex; use rs_matter::utils::cell::RefCell; @@ -102,48 +101,45 @@ impl Bump { T: Sized, { self.inner.lock(|inner| { - let mut inner = inner.borrow_mut(); - - let size = core::mem::size_of_val(&object); + // SAFETY: + // The idea is to have a large chunk of memory allocated on the stack, + // with this function one can reserve a chunk of that memory for an object + // of type T. + // + // To reserve the memory, it will move the offset forward by the size required + // for T, and return a **mutable** reference to it. + // + // Given that it returns a mutable reference to it, there cannot be any other + // references to that memory location. This is ensured by the offset. + + let size = mem::size_of_val(&object); + let mut inner = inner.borrow_mut(); let offset = inner.offset; - let memory = unsafe { inner.memory.assume_init_mut() }; info!( "BUMP[{}]: {}b (U:{}b/F:{}b)", location, size, offset, - memory.len() - offset + inner.memory.len() - offset ); - let remaining = &mut memory[offset..]; - let remaining_len = remaining.len(); - - let (t_buf, r_buf) = align_min::(remaining, 1); - - // Safety: We just allocated the memory and it's properly aligned - let ptr = unsafe { - let ptr = t_buf.as_ptr() as *mut T; - ptr.write(object); + // SAFETY: The lifetime of the returned reference is bound to &self -> it will not outlive the data it is borrowing. + let value = unsafe { + let t_buf = inner.allocate_for::(1); - NonNull::new_unchecked(ptr) + t_buf[0].write(object) }; - inner.offset += remaining_len - r_buf.len(); - - BumpBox { - ptr, - _allocator: PhantomData, - } + BumpBox { value } }) } } /// A box-like container that uses bump allocation pub struct BumpBox<'a, T> { - ptr: NonNull, - _allocator: core::marker::PhantomData<&'a ()>, + value: &'a mut T, } impl BumpBox<'_, T> { @@ -160,36 +156,27 @@ impl core::ops::Deref for BumpBox<'_, T> { type Target = T; fn deref(&self) -> &Self::Target { - unsafe { self.ptr.as_ref() } + self.value } } impl core::ops::DerefMut for BumpBox<'_, T> { fn deref_mut(&mut self) -> &mut Self::Target { - unsafe { self.ptr.as_mut() } + self.value } } impl Unpin for BumpBox<'_, T> {} -impl Drop for BumpBox<'_, T> { - fn drop(&mut self) { - // Safety: The pointer is valid and we own the data - unsafe { - self.ptr.as_ptr().drop_in_place(); - } - } -} - struct Inner { - memory: MaybeUninit<[u8; N]>, + memory: [MaybeUninit; N], offset: usize, } impl Inner { const fn new() -> Self { Self { - memory: MaybeUninit::uninit(), + memory: [const { MaybeUninit::uninit() }; N], offset: 0, } } @@ -200,10 +187,47 @@ impl Inner { offset: 0, }) } + + /// Allocate space for `count` objects of type `T` + /// + /// # Panics + /// + /// If there is not enough memory left in the bump allocator to + /// allocate the requested objects. + /// + /// # Safety + /// + /// This function returns a mutable reference to the allocated memory + /// that lives independently of the lifetime of `self`. + /// This could result in undefined behavior where the reference outlives + /// the bump allocator itself. + /// + /// The caller must ensure that the returned reference does not outlive + /// the bump allocator. + unsafe fn allocate_for<'s, 'b, T>(&'s mut self, count: usize) -> &'b mut [MaybeUninit] { + // We can only use the memory from the current offset onwards, because + // the previous memory might be in use by previously allocated objects. + let remaining = &mut self.memory[self.offset..]; + let remaining_len = remaining.len(); + // The t_buf will be where the caller can place their objects, + // and r_buf should be the remaining unused memory. + let (t_buf, r_buf) = align_min::(remaining, count); + self.offset += remaining_len - r_buf.len(); + + // This creates an unbounded lifetime, see the safety section of this function. + // + // It is necessary, because technically only one mutable reference can exist + // to self.memory, but because it is an array, the mutable reference to self.memory + // can be split into multiple mutable references to its parts. + slice::from_raw_parts_mut(t_buf.as_mut_ptr(), t_buf.len()) + } } -fn align_min(buf: &mut [u8], count: usize) -> (&mut [MaybeUninit], &mut [u8]) { - if count == 0 || core::mem::size_of::() == 0 { +fn align_min( + buf: &mut [MaybeUninit], + count: usize, +) -> (&mut [MaybeUninit], &mut [MaybeUninit]) { + if count == 0 || mem::size_of::() == 0 { return (&mut [], buf); } @@ -215,7 +239,7 @@ fn align_min(buf: &mut [u8], count: usize) -> (&mut [MaybeUninit], &mut [u // Shrink `t_buf` to the number of requested items (count) let t_buf = &mut t_buf[..count]; let t_leading_buf0_len = t_leading_buf0.len(); - let t_buf_size = core::mem::size_of_val(t_buf); + let t_buf_size = mem::size_of_val(t_buf); let (buf0, remaining_buf) = buf.split_at_mut(t_leading_buf0_len + t_buf_size); @@ -226,3 +250,39 @@ fn align_min(buf: &mut [u8], count: usize) -> (&mut [MaybeUninit], &mut [u (t_buf, remaining_buf) } + +#[cfg(all(test, feature = "std"))] +mod tests { + use super::*; + + use alloc::vec::Vec; + use rs_matter::utils::sync::blocking::raw::StdRawMutex; + + const BUMP_SIZE: usize = 1024; + const DEFAULT_VALUE: u32 = 0xDEADBEEF; + + #[test] + fn test_one_concurrent_borrow() { + static BUMP: Bump = Bump::new(); + + for _ in 0..(BUMP_SIZE / mem::size_of_val(&DEFAULT_VALUE)) { + let b1 = BUMP.alloc(DEFAULT_VALUE, "test1"); + + assert_eq!(*b1, DEFAULT_VALUE); + } + } + + #[test] + fn test_multiple_concurrent_borrow() { + static BUMP: Bump = Bump::new(); + + let mut all_boxes = Vec::new(); + for i in 0..(BUMP_SIZE / mem::size_of::()) { + all_boxes.push(alloc!(BUMP, i)); + } + + for (i, b) in all_boxes.into_iter().enumerate() { + assert_eq!(*b, i); + } + } +} From 7d8dc70c9b0e79a6f58ead68dd001cefa45b43e7 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Fri, 31 Oct 2025 08:49:24 +0100 Subject: [PATCH 2/5] Add support for running tests and miri in CI --- .github/workflows/ci.yml | 14 +++++++++++++- Cargo.toml | 3 ++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d024ebf..52adda4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,7 +26,7 @@ jobs: uses: dtolnay/rust-toolchain@v1 with: toolchain: ${{ env.RUST_TOOLCHAIN }} - components: rustfmt, clippy, rust-src + components: rustfmt, clippy, rust-src, miri - name: Install libdbus run: sudo apt-get install -y libdbus-1-dev @@ -52,6 +52,18 @@ jobs: - name: Build run: cargo build --no-default-features --features ${{matrix.features}} + - name: Test + if: matrix.features == 'std' + run: cargo test --features ${{matrix.features}} + + - name: Miri Setup + if: matrix.features == 'std' + run: cargo miri setup + + - name: Miri Test + if: matrix.features == 'std' + run: cargo miri test --features ${{matrix.features}} + - name: Examples if: matrix.features == 'os' run: cargo build --release --examples --features ${{matrix.features}},nix,log,examples diff --git a/Cargo.toml b/Cargo.toml index e14173d..c4d7a5b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -163,7 +163,7 @@ rustcrypto = ["rs-matter/rustcrypto"] os = ["backtrace", "rs-matter/os", "rustcrypto", "embassy-time/std"] backtrace = ["std", "rs-matter/backtrace"] async-io-mini = ["std", "edge-nal-std/async-io-mini"] -std = ["alloc", "rs-matter/std", "edge-nal-std"] +std = ["alloc", "rs-matter/std", "edge-nal-std", "critical-section/std"] alloc = ["embedded-svc/alloc"] examples = ["log", "os", "nix", "embassy-time-queue-utils/generic-queue-64", "zeroconf"] @@ -191,6 +191,7 @@ bitflags = "2" nix = { version = "0.27", features = ["net"], optional = true } [dev-dependencies] +critical-section = "1.0" static_cell = "2.1" futures-lite = "1" async-compat = "0.2" From 465ef9262625f2c4dbf9870c99f8994fc8f23b72 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Thu, 13 Nov 2025 14:51:43 +0100 Subject: [PATCH 3/5] Address feedback --- src/bump.rs | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/src/bump.rs b/src/bump.rs index 5fd2dd2..0824b82 100644 --- a/src/bump.rs +++ b/src/bump.rs @@ -6,8 +6,10 @@ //! The primary use case of this allocator is reduction of Rust future sizes, due to //! `rustc` not being very intelligent w.r.t. stack usage in async functions. +use core::marker::PhantomData; use core::mem::{self, MaybeUninit}; use core::pin::Pin; +use core::ptr::NonNull; use core::slice; use embassy_sync::blocking_mutex::raw::RawMutex; @@ -126,20 +128,26 @@ impl Bump { ); // SAFETY: The lifetime of the returned reference is bound to &self -> it will not outlive the data it is borrowing. - let value = unsafe { + let ptr = unsafe { let t_buf = inner.allocate_for::(1); - t_buf[0].write(object) + t_buf[0].write(object); + + NonNull::new_unchecked(t_buf[0].as_mut_ptr()) }; - BumpBox { value } + BumpBox { + ptr, + _allocator: PhantomData, + } }) } } /// A box-like container that uses bump allocation pub struct BumpBox<'a, T> { - value: &'a mut T, + ptr: NonNull, + _allocator: PhantomData<&'a ()>, } impl BumpBox<'_, T> { @@ -156,18 +164,27 @@ impl core::ops::Deref for BumpBox<'_, T> { type Target = T; fn deref(&self) -> &Self::Target { - self.value + unsafe { self.ptr.as_ref() } } } impl core::ops::DerefMut for BumpBox<'_, T> { fn deref_mut(&mut self) -> &mut Self::Target { - self.value + unsafe { self.ptr.as_mut() } } } impl Unpin for BumpBox<'_, T> {} +impl Drop for BumpBox<'_, T> { + fn drop(&mut self) { + // Safety: The pointer is valid and we own the data + unsafe { + self.ptr.as_ptr().drop_in_place(); + } + } +} + struct Inner { memory: [MaybeUninit; N], offset: usize, From db33559f1db514a1774a7d2705bf4ddcc5987fe0 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Sat, 15 Nov 2025 11:52:45 +0100 Subject: [PATCH 4/5] Use nightly toolchain for miri in CI --- .github/workflows/ci.yml | 39 ++++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 52adda4..17dd86f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,7 +26,7 @@ jobs: uses: dtolnay/rust-toolchain@v1 with: toolchain: ${{ env.RUST_TOOLCHAIN }} - components: rustfmt, clippy, rust-src, miri + components: rustfmt, clippy, rust-src - name: Install libdbus run: sudo apt-get install -y libdbus-1-dev @@ -56,14 +56,6 @@ jobs: if: matrix.features == 'std' run: cargo test --features ${{matrix.features}} - - name: Miri Setup - if: matrix.features == 'std' - run: cargo miri setup - - - name: Miri Test - if: matrix.features == 'std' - run: cargo miri test --features ${{matrix.features}} - - name: Examples if: matrix.features == 'os' run: cargo build --release --examples --features ${{matrix.features}},nix,log,examples @@ -91,3 +83,32 @@ jobs: if: ${{ !env.ACT && matrix.features == 'os' }} with: platform-name: cross-platform + + run_miri: + runs-on: ubuntu-latest + strategy: + matrix: + features: ['std'] + + steps: + - name: Rust + uses: dtolnay/rust-toolchain@nightly + with: + components: miri + + - name: Install libdbus + run: sudo apt-get install -y libdbus-1-dev + + - name: Install libavahi-client + run: sudo apt-get install -y libavahi-client-dev + + - name: Checkout + uses: actions/checkout@v3 + + - name: Miri Setup + if: matrix.features == 'std' + run: cargo miri setup + + - name: Miri Test + if: matrix.features == 'std' + run: cargo miri test --features ${{matrix.features}} From 8ccbff4ca398c55a1da618f23ccc89ce33e178c1 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Sat, 15 Nov 2025 11:53:39 +0100 Subject: [PATCH 5/5] Fix concerns mentioned in review --- src/bump.rs | 67 ++++++++++++++++++++++++++++------------------------- 1 file changed, 35 insertions(+), 32 deletions(-) diff --git a/src/bump.rs b/src/bump.rs index 0824b82..516b878 100644 --- a/src/bump.rs +++ b/src/bump.rs @@ -10,7 +10,6 @@ use core::marker::PhantomData; use core::mem::{self, MaybeUninit}; use core::pin::Pin; use core::ptr::NonNull; -use core::slice; use embassy_sync::blocking_mutex::raw::RawMutex; use rs_matter::utils::cell::RefCell; @@ -124,20 +123,16 @@ impl Bump { location, size, offset, - inner.memory.len() - offset + inner.len() - offset ); - // SAFETY: The lifetime of the returned reference is bound to &self -> it will not outlive the data it is borrowing. - let ptr = unsafe { - let t_buf = inner.allocate_for::(1); + let value = inner.allocate::(); - t_buf[0].write(object); - - NonNull::new_unchecked(t_buf[0].as_mut_ptr()) - }; + value.write(object); BumpBox { - ptr, + // SAFETY: The code above wrote to the memory location -> it can not be null + ptr: unsafe { NonNull::new_unchecked(value.as_mut_ptr()) }, _allocator: PhantomData, } }) @@ -186,14 +181,29 @@ impl Drop for BumpBox<'_, T> { } struct Inner { - memory: [MaybeUninit; N], + // It is uncertain whether a [MaybeUninit; N] initialized with [const { MaybeUninit::uninit() }; N] + // would never be temporarily allocated on the stack and then moved to the final destination. + // + // In addition to that, the const { MaybeUninit::uninit() } sometimes results in bad optimizations like + // https://stackoverflow.com/questions/79513440. + // With the 1.77 compiler target, this issue would still be present, and const expressions only got + // stabilized in 1.79 -> not available. + // + // To avoid this, the entire array is wrapped in a MaybeUninit, which should prevent the compiler from + // trying to eagerly initialize the array on the stack. + // + // Technically it is enough to have MaybeUninit<[u8; N]> and then transmute it to [MaybeUninit; N] + // (which is safe to do), but this would require a transmute that is easy to get wrong. + // Using MaybeUninit<[MaybeUninit; N]> does not have any downsides and one can just use assume_init + // or assume_init_mut to get the [MaybeUninit; N] directly. + memory: MaybeUninit<[MaybeUninit; N]>, offset: usize, } impl Inner { const fn new() -> Self { Self { - memory: [const { MaybeUninit::uninit() }; N], + memory: MaybeUninit::uninit(), offset: 0, } } @@ -205,38 +215,31 @@ impl Inner { }) } - /// Allocate space for `count` objects of type `T` + fn len(&self) -> usize { + N + } + + /// Allocate space for an object of type `T` /// /// # Panics /// /// If there is not enough memory left in the bump allocator to /// allocate the requested objects. - /// - /// # Safety - /// - /// This function returns a mutable reference to the allocated memory - /// that lives independently of the lifetime of `self`. - /// This could result in undefined behavior where the reference outlives - /// the bump allocator itself. - /// - /// The caller must ensure that the returned reference does not outlive - /// the bump allocator. - unsafe fn allocate_for<'s, 'b, T>(&'s mut self, count: usize) -> &'b mut [MaybeUninit] { + fn allocate(&mut self) -> &mut MaybeUninit { + // SAFETY: This is safe because the type we are claiming to have initialized, + // is a bunch of `MaybeUninit`s, which do not require initialization. + let data: &mut [MaybeUninit; N] = unsafe { self.memory.assume_init_mut() }; + // We can only use the memory from the current offset onwards, because // the previous memory might be in use by previously allocated objects. - let remaining = &mut self.memory[self.offset..]; + let remaining = &mut data[self.offset..]; let remaining_len = remaining.len(); // The t_buf will be where the caller can place their objects, // and r_buf should be the remaining unused memory. - let (t_buf, r_buf) = align_min::(remaining, count); + let (t_buf, r_buf) = align_min::(remaining, 1); self.offset += remaining_len - r_buf.len(); - // This creates an unbounded lifetime, see the safety section of this function. - // - // It is necessary, because technically only one mutable reference can exist - // to self.memory, but because it is an array, the mutable reference to self.memory - // can be split into multiple mutable references to its parts. - slice::from_raw_parts_mut(t_buf.as_mut_ptr(), t_buf.len()) + &mut t_buf[0] } }