* [PATCH proxmox{,-backup} v2 00/10] Fix Undefined Behavior in Tape Block Header Deallocation
@ 2026-08-21 14:02 Max R. Carrara
2026-08-21 14:02 ` [PATCH proxmox v2 01/10] proxmox-alloc: introduce proxmox-alloc with `LayoutAwareBox<T>` type Max R. Carrara
` (9 more replies)
0 siblings, 10 replies; 11+ messages in thread
From: Max R. Carrara @ 2026-08-21 14:02 UTC (permalink / raw)
To: pbs-devel
Fix Undefined Behavior in Tape Block Header Deallocation - v2
=============================================================
We allocate `BlockHeader` in `pbs-tape` with an alignment equal
to the page size, but because it uses packed / 1-byte alignment
(`#[repr(C, packed)]` to be precise), the compiler will treat it as if
it was allocated with 1-byte alignment.
Miri [0] will flag this as undefined behavior:
error: Undefined Behavior: incorrect layout on deallocation:
alloc46398 has size 262144 and alignment 4096, but gave size 262144
and alignment 1
This is because Rust cares about a memory region's alignment on
deallocation, even if the underlying allocator does not -- hence why
this hasn't actually been a problem for us. Still, there is no guarantee
that this will not change at some point in the future.
The same kind of UB is also caused by the `alloc_page_aligned_buffer()`
function of the `sgutils2` module.
This series addresses both of these issues by introducing a new type
called `LayoutAwareBox`, which keeps track of the layout a given heap
allocation used.
Special thanks to @Robert, who brought this to my attention and totally
managed to nerd-snipe me with this. I have added a respective git
trailer in each of the patches that address the occurrences of UB.
Notable Changes
---------------
This v2 here is a greater rework of v1, so a lot has changed.
- Introduce the `proxmox-alloc` crate together with its `LayoutAwareBox`
type that keeps track of the layout used during allocation and re-uses
it during deallocation.
This is the main type that allows us to prevent UB in this series.
I was first thinking of keeping this type inside of `pbs-tape`, but
after implementing two thirds of it, I figured it's probably better to
introduce it as part of a separate crate together with ample tests and
documentation instead. This documents the problems the type addresses
and also shows how it ought to be used, which might be helpful for
developers who rarely touch lower-level Rust code.
FYI, the docs of all our crates in proxmox.git can be built and opened
using:
cargo doc --workspace --no-deps --open
- Refactor the tape code in smaller pieces to make reviewing all changes
easier.
- Move `BlockHeader` into a separate module and rename it to
`TapeBlock`, since that's what it actually represents.
- Get rid of any `pub` fields in `TapeBlock` and add any necessary
getters and setters.
- Remove haphazard `unsafe` blocks by implementing them as methods
instead.
- Add "SAFETY" comments to all `unsafe` blocks that are touched or
introduced.
- Add tests in `pbs-tape` that allow Miri to easily catch any UB
regarding the two instances this patch series addresses.
Testing
-------
Would be great if somebody with a working tape storage could give this a
spin -- miri does not report any UB anymore and the tests we have pass,
but some smoke-testing would nevertheless be appreciated. My virtual
tape library has unfortunately borked itself, and I haven't come around
to un-borking it yet.
[0]: https://github.com/rust-lang/miri
Previous Versions
-----------------
v1: https://lore.proxmox.com/pbs-devel/20260805141620.4190773-1-m.carrara@proxmox.com/
Summary of Changes
------------------
proxmox:
Max R. Carrara (2):
proxmox-alloc: introduce proxmox-alloc with `LayoutAwareBox<T>` type
proxmox-alloc: document undefined behavior regarding custom allocs
Cargo.toml | 1 +
proxmox-alloc/Cargo.toml | 24 +
proxmox-alloc/examples/ub.rs | 99 ++
proxmox-alloc/src/aware_boxed.rs | 2199 ++++++++++++++++++++++++++++++
proxmox-alloc/src/lib.rs | 19 +
5 files changed, 2342 insertions(+)
create mode 100644 proxmox-alloc/Cargo.toml
create mode 100644 proxmox-alloc/examples/ub.rs
create mode 100644 proxmox-alloc/src/aware_boxed.rs
create mode 100644 proxmox-alloc/src/lib.rs
proxmox-backup:
Max R. Carrara (8):
tape: move tape block structs into separate file module
tape: rename `BlockHeader` and `BlockHeaderFlags`
tape: blocked_{reader,writer}: rename `buffer` to `tape_block`
tape: tape block: represent tape block header with its own struct
tape: tape block: make `payload` field private
tape: blocked_{reader,writer}: remove haphazard `unsafe` blocks
tape: tape block: fix undefined behavior on tape block deallocation
tape: sgutils2: fix undefined behavior in dealloc of buffer
Cargo.toml | 3 +
pbs-tape/Cargo.toml | 1 +
pbs-tape/src/blocked_reader.rs | 72 ++++++-------
pbs-tape/src/blocked_writer.rs | 51 +++++----
pbs-tape/src/lib.rs | 86 +++------------
pbs-tape/src/sgutils2.rs | 49 ++++++---
pbs-tape/src/tape_block.rs | 191 +++++++++++++++++++++++++++++++++
7 files changed, 300 insertions(+), 153 deletions(-)
create mode 100644 pbs-tape/src/tape_block.rs
Summary over all repositories:
12 files changed, 2642 insertions(+), 153 deletions(-)
--
Generated by murpp 0.12.0
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH proxmox v2 01/10] proxmox-alloc: introduce proxmox-alloc with `LayoutAwareBox<T>` type 2026-08-21 14:02 [PATCH proxmox{,-backup} v2 00/10] Fix Undefined Behavior in Tape Block Header Deallocation Max R. Carrara @ 2026-08-21 14:02 ` Max R. Carrara 2026-08-21 14:02 ` [PATCH proxmox v2 02/10] proxmox-alloc: document undefined behavior regarding custom allocs Max R. Carrara ` (8 subsequent siblings) 9 siblings, 0 replies; 11+ messages in thread From: Max R. Carrara @ 2026-08-21 14:02 UTC (permalink / raw) To: pbs-devel Introducte a new crate called `proxmox-alloc`, an allocation and collections library similar to `core::alloc`. Add `LayoutAwareBox<T>`, a type which tracks the `Layout` its memory was allocated with. Rust requires that the alignment [align] an allocation uses matches that its corresponding deallocation uses. It is considered undefined behavior if the alignments do not match [ub]. Additionally, Rust does not (and cannot) track whether a given type's memory was allocated with a custom layout. Instead, it will always assume that the alignment of a given T's memory is align_of::<T>(). `LayoutAwareBox<T>` solves this issue by tracking which layout was used during T's allocation, passing it to `dealloc()` in its `drop` handler. While `LayoutAwareBox<T>` does not provide any convenience methods for creating custom dynamically sized types (DSTs) [dst], it does provide a `from_raw_parts()` method to store the freshly allocated DST and its layout. Additionally, `LayoutAwareBox<T>` provides several convenience methods for creating heap-allocated values and slices with a particular alignment. It also implements many of the standard library's traits wherever applicable. In particular, it implements almost all traits of `Box<T>`, except: - unstable traits or trait impls that require unstable features (as of writing, Rust 1.94.1) - traits that involve strings or string-like types, such as `From<String> for LayoutAwareBox<str>`, `Clone for LayoutAwareBox<CStr>`, `From<PathBuf> for LayoutAwareBox<Path>`, and so on - traits that require non-trivial memory manipulation, such as `Clone for LayoutAwareBox<T> where T: ?Sized` --> cloning the memory inside a `LayoutAwareBox<T>` itself would be trivial, but if `T: ?Sized`, it cannot be `Clone`. Note that `Box<T>` does not provide such an impl either. If we want to clone DSTs in the future, we will most likely need some kind of custom trait that we can rely on. - fn-traits, such as `Fn`, `AsyncFnMut`, etc. - `AsFd`, `AsHandle`, `Future`, and `Unpin` In other words, `LayoutAwareBox<T>` will work fine for DSTs and buffers. Other trait impls can always be added later once we actually need them. Finally, this commit also adds ample documentation and tests for `LayoutAwareBox<T>` in order to make it easier to use for developers who are not as familiar with lower-level Rust. [align]: https://en.wikipedia.org/wiki/Data_structure_alignment [dst]: https://doc.rust-lang.org/nomicon/exotic-sizes.html#dynamically-sized-types-dsts [ub]: https://doc.rust-lang.org/reference/behavior-considered-undefined.html Signed-off-by: Max R. Carrara <m.carrara@proxmox.com> --- Cargo.toml | 1 + proxmox-alloc/Cargo.toml | 20 + proxmox-alloc/src/aware_boxed.rs | 2196 ++++++++++++++++++++++++++++++ proxmox-alloc/src/lib.rs | 19 + 4 files changed, 2236 insertions(+) create mode 100644 proxmox-alloc/Cargo.toml create mode 100644 proxmox-alloc/src/aware_boxed.rs create mode 100644 proxmox-alloc/src/lib.rs diff --git a/Cargo.toml b/Cargo.toml index 16e91c94..c00515e3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,6 +3,7 @@ members = [ "proxmox-access-control", "proxmox-acme", "proxmox-acme-api", + "proxmox-alloc", "proxmox-api-macro", "proxmox-apt", "proxmox-apt-api-types", diff --git a/proxmox-alloc/Cargo.toml b/proxmox-alloc/Cargo.toml new file mode 100644 index 00000000..4eeeb787 --- /dev/null +++ b/proxmox-alloc/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "proxmox-alloc" +version = "0.1.0" +description = "Proxmox allocation and collections library" + +authors.workspace = true +edition.workspace = true +rust-version.workspace = true +homepage.workspace = true +repository.workspace = true +license.workspace = true +exclude.workspace = true + +[dependencies] + +[dev-dependencies] +libc.workspace = true + +[features] +default = [] diff --git a/proxmox-alloc/src/aware_boxed.rs b/proxmox-alloc/src/aware_boxed.rs new file mode 100644 index 00000000..4ec3811e --- /dev/null +++ b/proxmox-alloc/src/aware_boxed.rs @@ -0,0 +1,2196 @@ +//! The `LayoutAwareBox<T>` type for more involved heap allocations. +//! +//! `LayoutAwareBox<T>` is a pointer to a type that uniquely owns a heap +//! allocation of type `T`, while also keeping track of the [`Layout`] that was +//! used for the allocation. +//! +//! This type works similar to the standard library's [`Box`] and is commonly +//! used as a wrapper around [dynamically sized types (DSTs)][DST] and types +//! that are allocated with a particular [alignment]. +//! +//! The main problem this type addresses it that the Rust compiler does not (and +//! cannot) track the alignment that was used to allocate a certain +//! [`Box<T>`](Box). To illustrate, consider this example: +//! +//! ```no_run +//! // For illustration purposes only; do *not* use this in your code. +//! let buf_len = 1024; +//! let align = 128; +//! +//! let layout = std::alloc::Layout::from_size_align(buf_len, align).unwrap(); +//! +//! let thin_ptr: *mut u8 = unsafe { std::alloc::alloc_zeroed(layout) }; +//! +//! let fat_ptr: *mut [u8] = std::ptr::slice_from_raw_parts_mut(thin_ptr, buf_len); +//! assert!(!fat_ptr.is_null()); +//! +//! let boxed: Box<[u8]> = unsafe { Box::from_raw(fat_ptr) }; +//! +//! drop(boxed); +//! ``` +//! +//! Here we allocate a simple buffer containing 1024 bytes, with an alignment of +//! 128. [`alloc_zeroed`] returns a plain `*mut u8`, which we then convert to a +//! pointer to a slice (or "fat pointer") that is aware of its own size. +//! Finally, we pass this new pointer to [`Box::from_raw`] and create a new +//! `Box<[u8]>` which now owns our heap-allocated buffer with our fancy +//! alignment. The [`Box`] will make sure that our buffer is deallocated +//! correctly once dropped. We make this explicit here by using [`drop`] +//! directly. +//! +//! While our logic might seem sound here, this will actually be reported as +//! [undefined behavior] by [Miri]: +//! +//! ```text +//! error: Undefined Behavior: incorrect layout on deallocation: +//! alloc42389 has size 1024 and alignment 128, but gave size 1024 and alignment 1! +//! ``` +//! +//! Therefore, one must always track the [`Layout`] that was used for "exotic" +//! kinds of allocations, which [`LayoutAwareBox`] will do for you. +//! +//! # Examples +//! +//! Move a value from the stack on the heap, using a specific alignment for the +//! allocation: +//! +//! ``` +//! use proxmox_alloc::LayoutAwareBox; +//! +//! struct SomeData { +//! first: u32, +//! second: u32, +//! } +//! +//! let value = SomeData { first: 10, second: 20 }; +//! +//! let aware_box: LayoutAwareBox<SomeData> = LayoutAwareBox::new(value, 64) +//! .expect("infallible"); +//! ``` +//! +//! Create a page-size-aligned, zero-initialized buffer using +//! [`LayoutAwareBox::slice_fill`]: +//! +//! ``` +//! use libc; +//! +//! use proxmox_alloc::LayoutAwareBox; +//! +//! let buf_len = 2usize.pow(14); +//! +//! let page_size: i64 = unsafe { libc::sysconf(libc::_SC_PAGESIZE) }; +//! assert!(page_size > 0, "failed to query PAGESIZE"); +//! +//! let align = page_size as usize; +//! +//! let pagesize_buf = LayoutAwareBox::<[u8]>::slice_fill(buf_len, align, 0).unwrap(); +//! +//! assert_eq!(pagesize_buf.len(), buf_len); +//! ``` +//! +//! The same can also be done by using [`LayoutAwareBox::slice_fill_with`] and +//! passing [`Default::default`]: +//! +//! ``` +//! # use libc; +//! # use proxmox_alloc::LayoutAwareBox; +//! # let buf_len = 2usize.pow(14); +//! # let page_size: i64 = unsafe { libc::sysconf(libc::_SC_PAGESIZE) }; +//! # assert!(page_size > 0, "failed to query PAGESIZE"); +//! # let align = page_size as usize; +//! let pagesize_buf = +//! LayoutAwareBox::<[u8]>::slice_fill_with(buf_len, align, Default::default).unwrap(); +//! ``` +//! +//! Note that if you want to retrieve the layout that was used for the +//! allocation, you should use [`LayoutAwareBox::layout`]: +//! +//! ``` +//! # use libc; +//! # use proxmox_alloc::LayoutAwareBox; +//! # let buf_len = 2usize.pow(14); +//! # let page_size: i64 = unsafe { libc::sysconf(libc::_SC_PAGESIZE) }; +//! # assert!(page_size > 0, "failed to query PAGESIZE"); +//! let align = page_size as usize; +//! let pagesize_buf = LayoutAwareBox::<[u8]>::slice_fill(buf_len, align, 0).unwrap(); +//! +//! // The alignment of the inner T will be align_of::<T>() in all cases, since Rust +//! // does not and cannot track alignment: +//! assert_eq!(align_of_val(&*pagesize_buf), align_of::<u8>()); +//! +//! // Instead, get it from the layout directly: +//! let layout = LayoutAwareBox::layout(&pagesize_buf); +//! assert_eq!(layout.align(), align); +//! ``` +//! +//! Sometimes you might need to create a zero-sized buffer. This can easily be +//! done using [`LayoutAwareBox::slice_empty`], which avoids allocations +//! altogether: +//! +//! ``` +//! # use proxmox_alloc::LayoutAwareBox; +//! let empty_buf = LayoutAwareBox::<[u8]>::slice_empty(); +//! ``` +//! +//! For convenience, you can also convert from a couple existing owned types: +//! +//! ``` +//! # use proxmox_alloc::LayoutAwareBox; +//! let vec: Vec<usize> = vec![0, 1, 2, 3, 4]; +//! let aware_boxed = LayoutAwareBox::<[usize]>::from(vec); +//! +//! assert_eq!(&*aware_boxed, &[0, 1, 2, 3, 4]); +//! ``` +//! ``` +//! # use proxmox_alloc::LayoutAwareBox; +//! let boxed: Box<[usize]> = Box::new([42, 67]); +//! let aware_boxed = LayoutAwareBox::<[usize]>::from(boxed); +//! +//! assert_eq!(&*aware_boxed, &[42, 67]); +//! ``` +//! ``` +//! # use proxmox_alloc::LayoutAwareBox; +//! let array: [i32; 4] = [0; 4]; +//! let aware_boxed = LayoutAwareBox::<[i32]>::from(array); +//! +//! assert_eq!(&*aware_boxed, &[0, 0, 0, 0]); +//! ``` +//! +//! See the [trait implementations](LayoutAwareBox#trait-implementations) for +//! more. +//! +//! For using `LayoutAwareBox<T>` for custom dynamically sized types, see +//! [below](#custom-dynamically-sized-types). +//! +//! # Custom Dynamically Sized Types +//! +//! ***Hint:** If you already know your way around [DSTs][DST] and want to skip +//! the explanation, see [here](#full-examples) for complete examples.* +//! +//! While [`LayoutAwareBox::slice_fill`] and other methods work great in cases +//! where all you need is a buffer allocated with a custom alignment, you +//! sometimes might have data that comes with a structured part, often called +//! the *header*, and a dynamically sized *payload*. This is relatively common +//! in the world of networking and file formats. +//! +//! [`LayoutAwareBox`] cannot help you with the allocation and initialization of +//! such data, but it can however help you ensure that you do not cause any +//! [undefined behavior] during deallcations. +//! +//! In general, you would structure such data as follows: +//! +//! ``` +//! #[repr(C, packed)] +//! struct PacketHeader { +//! src: [u8; 4], +//! dst: [u8; 4], +//! } +//! +//! #[repr(C, packed)] +//! struct Packet { +//! header: PacketHeader, +//! payload: [u8], +//! } +//! ``` +//! +//! In this example here, the `payload` is our dynamically sized field. We +//! cannot have more than one dynamically sized field anywhere in our struct's +//! definition, and it must always be at the very end of our struct. The +//! presence of such a field makes `Packet` a [dynamically sized type +//! (DST)][DST]. +//! +//! We also chose [`#[repr(C, packed)]`][alt-reprs] as our struct's +//! representation to avoid any field reordering by the Rust compiler, and also +//! to maximize FFI compatibility. +//! +//! Note that while it is possible to omit the `PacketHeader` struct entirely +//! and instead place `src` and `dst` in `Packet` directly, having a separate +//! struct to model the header is useful for size calculations later on. +//! +//! If we then want to allocate a `Packet`, we have to do that by hand. +//! +//! First, we need a suitable [`Layout`]. For the purposes of this example, we +//! will choose an arbitrary size and alignment: +//! +//! ``` +//! # #[repr(C, packed)] +//! # struct PacketHeader { +//! # src: [u8; 4], +//! # dst: [u8; 4], +//! # } +//! # +//! # #[repr(C, packed)] +//! # struct Packet { +//! # header: PacketHeader, +//! # payload: [u8], +//! # } +//! # +//! use std::alloc::Layout; +//! +//! let alloc_size = 1024; +//! let align = 128; +//! +//! let layout = Layout::from_size_align(alloc_size, align).expect("infallible"); +//! ``` +//! +//! Next, we need to allocate the memory for our `Packet` using our layout, +//! calling [`handle_alloc_error`](std::alloc::handle_alloc_error) if our +//! allocation failed: +//! +//! ``` +//! # #[repr(C, packed)] +//! # struct PacketHeader { +//! # src: [u8; 4], +//! # dst: [u8; 4], +//! # } +//! # +//! # #[repr(C, packed)] +//! # struct Packet { +//! # header: PacketHeader, +//! # payload: [u8], +//! # } +//! # +//! # use std::alloc::Layout; +//! # +//! # let alloc_size = 1024; +//! # let align = 128; +//! # +//! # let layout = Layout::from_size_align(alloc_size, align).expect("infallible"); +//! use std::alloc::alloc_zeroed; +//! use std::alloc::handle_alloc_error; +//! +//! // SAFETY: layout's size is not 0. +//! let thin_ptr = unsafe { alloc_zeroed(layout) }; +//! if thin_ptr.is_null() { +//! handle_alloc_error(layout); +//! } +//! # unsafe { std::alloc::dealloc(thin_ptr, layout) }; +//! ``` +//! +//! `thin_ptr` here is now a `*mut u8`, a raw pointer that points to some bytes. +//! We cannot simply cast this to a `*mut Packet` however. Rust actually +//! differentiates between two types of pointers: +//! +//! 1. "Thin" pointers that point to types whose size is known at compile time, +//! meaning they implement the [`Sized`] trait. Thin pointers only consist of +//! an address. +//! 2. "Fat" pointers that point to types whose size is *not* known at compile +//! time, or in other words, [dynamically sized types][DST]. Fat pointers +//! consist of an address and a length, and are the reason why Rust knows how +//! long a string slice `&str` is, for example. +//! +//! In fact, the compiler will complain if we try to cast our `thin_ptr`: +//! +//! ```compile_fail +//! # #[repr(C, packed)] +//! # struct PacketHeader { +//! # src: [u8; 4], +//! # dst: [u8; 4], +//! # } +//! # +//! # #[repr(C, packed)] +//! # struct Packet { +//! # header: PacketHeader, +//! # payload: [u8], +//! # } +//! # +//! # use std::alloc::Layout; +//! # +//! # let alloc_size = 1024; +//! # let align = 128; +//! # +//! # let layout = Layout::from_size_align(alloc_size, align).expect("infallible"); +//! # use std::alloc::alloc_zeroed; +//! # use std::alloc::handle_alloc_error; +//! # +//! # // SAFETY: layout's size is not 0. +//! # let thin_ptr = unsafe { alloc_zeroed(layout) }; +//! # if thin_ptr.is_null() { +//! # handle_alloc_error(layout); +//! # } +//! let fat_ptr = thin_ptr as *mut Packet; +//! ``` +//! +//! Instead, we must first convert `thin_ptr` to a fat pointer by associating it +//! with a *length*, which we get by subtracting our header's size from the +//! number of bytes we allocated. This will make our `*mut u8` a `*mut [u8]`, +//! which we then can cast without the compiler complaining: +//! +//! ``` +//! # #[repr(C, packed)] +//! # struct PacketHeader { +//! # src: [u8; 4], +//! # dst: [u8; 4], +//! # } +//! # +//! # #[repr(C, packed)] +//! # struct Packet { +//! # header: PacketHeader, +//! # payload: [u8], +//! # } +//! # +//! # use std::alloc::Layout; +//! # +//! # let alloc_size = 1024; +//! # let align = 128; +//! # +//! # let layout = Layout::from_size_align(alloc_size, align).expect("infallible"); +//! # use std::alloc::alloc_zeroed; +//! # use std::alloc::handle_alloc_error; +//! # +//! # // SAFETY: layout's size is not 0. +//! # let thin_ptr = unsafe { alloc_zeroed(layout) }; +//! # if thin_ptr.is_null() { +//! # handle_alloc_error(layout); +//! # } +//! use std::ptr::slice_from_raw_parts_mut; +//! +//! let payload_len = alloc_size - size_of::<PacketHeader>(); +//! +//! let fat_ptr = slice_from_raw_parts_mut(thin_ptr, payload_len) as *mut Packet; +//! # unsafe { std::alloc::dealloc(fat_ptr as *mut u8, layout) }; +//! ``` +//! +//! Finally, we can construct our `LayoutAwareBox<Packet>` by using +//! [`LayoutAwareBox::from_raw_parts`]: +//! +//! ``` +//! # #[repr(C, packed)] +//! # struct PacketHeader { +//! # src: [u8; 4], +//! # dst: [u8; 4], +//! # } +//! # +//! # #[repr(C, packed)] +//! # struct Packet { +//! # header: PacketHeader, +//! # payload: [u8], +//! # } +//! # +//! # use std::alloc::Layout; +//! # +//! # let alloc_size = 1024; +//! # let align = 128; +//! # +//! # let layout = Layout::from_size_align(alloc_size, align).expect("infallible"); +//! # use std::alloc::alloc_zeroed; +//! # use std::alloc::handle_alloc_error; +//! # +//! # // SAFETY: layout's size is not 0. +//! # let thin_ptr = unsafe { alloc_zeroed(layout) }; +//! # if thin_ptr.is_null() { +//! # handle_alloc_error(layout); +//! # } +//! # use std::ptr::slice_from_raw_parts_mut; +//! # +//! # let payload_len = alloc_size - size_of::<PacketHeader>(); +//! # +//! # let fat_ptr = slice_from_raw_parts_mut(thin_ptr, payload_len) as *mut Packet; +//! # use proxmox_alloc::LayoutAwareBox; +//! // SAFETY: fat_ptr is not aliased or null, is valid for reads and writes, and its pointee was +//! // zero-initialized. Its length is within the bounds of its allocation. +//! let aware_boxed = unsafe { LayoutAwareBox::from_raw_parts(fat_ptr, layout) }; +//! # assert_eq!((&*aware_boxed).payload.len(), payload_len, "payload.len() != payload_len"); +//! ``` +//! +//! For a condensed version of this example, as well as examples on how to deal +//! with payloads that do not consist of plain bytes, see the [full +//! examples](#full-examples) below. +//! +//! ## Full Examples +//! +//! A custom [dynamically sized type] with a payload consisting of bytes: +//! +//! ``` +//! #[repr(C, packed)] +//! struct PacketHeader { +//! src: [u8; 4], +//! dst: [u8; 4], +//! } +//! +//! #[repr(C, packed)] +//! struct Packet { +//! header: PacketHeader, +//! payload: [u8], +//! } +//! +//! # use proxmox_alloc::LayoutAwareBox; +//! use std::alloc::Layout; +//! use std::alloc::alloc_zeroed; +//! use std::alloc::handle_alloc_error; +//! use std::ptr::slice_from_raw_parts_mut; +//! +//! let alloc_size = 1024; +//! let align = 128; +//! +//! let payload_len = alloc_size - size_of::<PacketHeader>(); +//! +//! let layout = Layout::from_size_align(alloc_size, align).expect("infallible"); +//! +//! // SAFETY: layout's size is not 0. +//! let thin_ptr = unsafe { alloc_zeroed(layout) }; +//! if thin_ptr.is_null() { +//! handle_alloc_error(layout); +//! } +//! +//! let fat_ptr = slice_from_raw_parts_mut(thin_ptr, payload_len) as *mut Packet; +//! +//! // SAFETY: fat_ptr is not aliased or null, is valid for reads and writes, +//! // and its pointee was zero-initialized. Its length is within the bounds +//! // of its allocation. +//! let aware_boxed = unsafe { LayoutAwareBox::from_raw_parts(fat_ptr, layout) }; +//! # assert_eq!((&*aware_boxed).payload.len(), payload_len, "payload.len() != payload_len"); +//! ``` +//! +//! For payloads that do not consist of plain bytes and instead store some oddly +//! sized type, you must either +//! +//! 1. shrink the allocation size to the resulting size of your [DST], or +//! 2. manually add padding to your DST's header until it fits your allocation +//! +//! **In either case, it is important that the size your resulting type is equal +//! to that of its allocation.** A mismatch between the data's size and the +//! allocation's size is considered [undefined behavior] and gets caught by +//! [Miri]. +//! +//! Shrinking the allocation size: +//! +//! ``` +//! #[repr(C, packed)] +//! struct PacketHeader { +//! src: [u8; 4], +//! dst: [u8; 4], +//! } +//! +//! #[repr(C, packed)] +//! struct Packet { +//! header: PacketHeader, +//! payload: [[u8; 7]], +//! } +//! +//! # use proxmox_alloc::LayoutAwareBox; +//! use std::alloc::Layout; +//! use std::alloc::alloc_zeroed; +//! use std::alloc::handle_alloc_error; +//! use std::ptr::slice_from_raw_parts_mut; +//! +//! let max_alloc_size = 1024; +//! let align = 128; +//! +//! let header_size = size_of::<PacketHeader>(); +//! let elem_size = size_of::<[u8; 7]>(); +//! +//! let payload_len = (max_alloc_size - header_size) / elem_size; +//! +//! // Allocated size *must* correspond to the actual size that Packet will occupy! +//! let alloc_size = payload_len * elem_size + header_size; +//! +//! let layout = Layout::from_size_align(alloc_size, align).expect("infallible"); +//! +//! // SAFETY: layout's size is not 0. +//! let thin_ptr = unsafe { alloc_zeroed(layout) }; +//! if thin_ptr.is_null() { +//! handle_alloc_error(layout); +//! } +//! +//! let fat_ptr = slice_from_raw_parts_mut(thin_ptr, payload_len) as *mut Packet; +//! +//! // SAFETY: fat_ptr is not aliased or null, is valid for reads and writes, and its pointee was +//! // zero-initialized. Its length is within the bounds of its allocation. +//! let aware_boxed = unsafe { LayoutAwareBox::from_raw_parts(fat_ptr, layout) }; +//! # assert_eq!((&*aware_boxed).payload.len(), payload_len, "payload.len() != payload_len"); +//! ``` +//! +//! Manually adding padding to the header, which you most likely will have to +//! calculate by hand: +//! +//! ``` +//! #[repr(C, packed)] +//! struct PacketHeader { +//! src: [u8; 4], +//! dst: [u8; 4], +//! _padding: [u8; 13], +//! } +//! +//! #[repr(C, packed)] +//! struct Packet { +//! header: PacketHeader, +//! payload: [[u8; 17]], +//! } +//! +//! # use proxmox_alloc::LayoutAwareBox; +//! use std::alloc::Layout; +//! use std::alloc::alloc_zeroed; +//! use std::alloc::handle_alloc_error; +//! use std::ptr::slice_from_raw_parts_mut; +//! +//! let max_alloc_size = 1024; +//! let align = 128; +//! +//! let header_size = size_of::<PacketHeader>(); +//! let elem_size = size_of::<[u8; 17]>(); +//! +//! let payload_len = (max_alloc_size - header_size) / elem_size; +//! +//! // Allocated size *must* correspond to the actual size that Packet will occupy! +//! let alloc_size = payload_len * elem_size + header_size; +//! assert_eq!(max_alloc_size, alloc_size); +//! +//! let layout = Layout::from_size_align(alloc_size, align).expect("infallible"); +//! +//! // SAFETY: layout's size is not 0. +//! let thin_ptr = unsafe { alloc_zeroed(layout) }; +//! if thin_ptr.is_null() { +//! handle_alloc_error(layout); +//! } +//! +//! let fat_ptr = slice_from_raw_parts_mut(thin_ptr, payload_len) as *mut Packet; +//! +//! // SAFETY: fat_ptr is not aliased or null, is valid for reads and writes, and its pointee was +//! // zero-initialized. Its length is within the bounds of its allocation. +//! let aware_boxed = unsafe { LayoutAwareBox::from_raw_parts(fat_ptr, layout) }; +//! # assert_eq!((&*aware_boxed).payload.len(), payload_len, "payload.len() != payload_len"); +//! ``` +//! +//! [`Layout`]: std::alloc::Layout +//! [`alloc_zeroed`]: std::alloc::alloc_zeroed +//! +//! [DST]: https://doc.rust-lang.org/nomicon/exotic-sizes.html#dynamically-sized-types-dsts +//! [Miri]: https://github.com/rust-lang/miri +//! [alignment]: https://en.wikipedia.org/wiki/Data_structure_alignment +//! [array]: std::array +//! [alt-reprs]: https://doc.rust-lang.org/nomicon/other-reprs.html +//! [undefined behavior]: https://doc.rust-lang.org/reference/behavior-considered-undefined.html + +use std::alloc; +use std::mem; +use std::mem::ManuallyDrop; +use std::ptr; + +/// The error type returned by certain methods of [`LayoutAwareBox`]. +#[derive(Debug)] +pub enum LayoutAwareBoxError { + /// Memory allocation failed, which is most likely caused by being out of memory. + OutOfMemory, + /// Alignment is zero or not a power of 2. + InvalidAlignment, + /// The requested amount of elements does not fit into a slice. + TooManyElements, +} + +impl LayoutAwareBoxError { + const fn as_str(&self) -> &'static str { + use LayoutAwareBoxError::*; + + match self { + InvalidAlignment => "invalid alignment - must be a power of 2", + OutOfMemory => "memory allocation failed - out of memory?", + TooManyElements => "too many elements - requested allocation does not fit into slice", + } + } +} + +impl std::error::Error for LayoutAwareBoxError {} + +impl std::fmt::Display for LayoutAwareBoxError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.as_str()) + } +} + +#[derive(Clone)] +enum TypeSize { + /// "Known" here means that we could ensure that `T` is `Sized` and was not + /// allocated with a custom layout. Note that this also applies to + /// zero-sized types. + /// + /// If we have a `[T]` instead, it must have been in a `Box<[T]>` or + /// `Vec<T>` before, or was acquired from a sized source like `&[T]`, + /// `&mut [T]`, `[T; N]`, and so on. Basically anything where we can get the + /// length for the underlying fat pointer. + Known, + /// Anything that is not `Known`, which basically means `T: ?Sized` and / or + /// `T` was allocated with a custom layout. + Dynamic(alloc::Layout), +} + +/// A pointer to a type that uniquely owns a heap allocation of type `T`, while +/// also keeping track of the [`Layout`] that was used for the allocation. +/// +/// See the [module-level documentation][crate::aware_boxed] for more +/// information. +/// +/// [`Layout`]: std::alloc::Layout +pub struct LayoutAwareBox<T: ?Sized> { + inner: ManuallyDrop<Box<T>>, + type_size: TypeSize, +} + +impl<T> Drop for LayoutAwareBox<T> +where + T: ?Sized, +{ + fn drop(&mut self) { + match self.type_size { + TypeSize::Known => { + // SAFETY: `self` being dropped, so `self.inner` is not used + // again afterwards + unsafe { ManuallyDrop::drop(&mut self.inner) } + } + TypeSize::Dynamic(layout) => { + // SAFETY: `self` being dropped, so `self.inner` is not used + // again afterwards + let inner = unsafe { ManuallyDrop::take(&mut self.inner) }; + let ptr = Box::into_raw(inner); + + // SAFETY: The value behind ptr is valid for reading & writing, + // properly aligned, and valid to drop; dealloc is called with + // layout that has been used for allocation + unsafe { + ptr::drop_in_place(ptr); + alloc::dealloc(ptr as *mut u8, layout); + } + } + } + } +} + +impl<T> LayoutAwareBox<T> +where + T: ?Sized, +{ + /// Consumes the passed [`LayoutAwareBox<T>`], returning the underlying + /// pointer and [`Layout`] as a tuple. + /// + /// # Examples + /// + /// ``` + /// # use proxmox_alloc::LayoutAwareBox; + /// let aware_boxed = LayoutAwareBox::<[usize]>::from([42, 67, 1337]); + /// + /// let (ptr, layout) = LayoutAwareBox::into_raw_parts(aware_boxed); + /// + /// // SAFETY: We just got this pointer and layout from into_raw_parts. + /// let aware_boxed = unsafe { LayoutAwareBox::from_raw_parts(ptr, layout) }; + /// ``` + /// + /// [`Layout`]: std::alloc::Layout + pub fn into_raw_parts(mut from: Self) -> (*mut T, alloc::Layout) { + // SAFETY: We consume `from`, so `from.inner` is not used again afterwards + let boxed = unsafe { ManuallyDrop::take(&mut from.inner) }; + let ptr = Box::into_raw(boxed); + + let layout = Self::layout(&from); + + mem::forget(from); // Prevent LayoutAwareBox::drop() from being called + + (ptr, layout) + } + + /// Create a new [`LayoutAwareBox<T>`] from a pointer and a [`Layout`]. + /// + /// Note that if `T` is [zero-sized], no allocation is actually performed. + /// + /// # Safety + /// + /// Improper use of this function can lead to [undefined behavior] and other + /// issues. + /// + /// In general, the same safety requirements as for [`Box::from_raw`] apply + /// for this function. + /// + /// **Additionally,** the caller must also guarantee that the passed pointer + /// points to a `T` which was allocated using the passed layout **and** has + /// the same size as its underlying allocation. + /// + /// The latter in particular is easy to miss: If you allocate `1024` bytes + /// for a struct that only takes up `1000` bytes for some reason, + /// deallocating this struct will *still* be considered undefined behavior + /// and will be spotted by [Miri]. + /// + /// See [Custom Dynamically Sized Types][custom] for a complete walkthrough + /// and examples. + /// + /// [`Layout`]: std::alloc::Layout + /// + /// [Miri]: https://github.com/rust-lang/miri + /// [custom]: crate::aware_boxed#custom-dynamically-sized-types + /// [undefined behavior]: https://doc.rust-lang.org/reference/behavior-considered-undefined.html + /// [zero-sized]: https://doc.rust-lang.org/nomicon/exotic-sizes.html#zero-sized-types-zsts + pub unsafe fn from_raw_parts(ptr: *mut T, layout: alloc::Layout) -> Self { + // SAFETY: The caller guarantees that ptr is non-null, valid for reads, + // and properly aligned. + let pointee_size = unsafe { mem::size_of_val(&mut *ptr) }; + + let type_size = if pointee_size == 0 { + TypeSize::Known + } else { + TypeSize::Dynamic(layout) + }; + + // SAFETY: Caller guarantees that ptr is non-null, valid for reads and + // writes, properly aligned, and valid to drop. + let boxed = unsafe { Box::from_raw(ptr) }; + let inner = ManuallyDrop::new(boxed); + + Self { inner, type_size } + } + + /// Returns a copy of the stored [`Layout`]. + /// + /// # Examples + /// + /// ``` + /// use std::alloc::Layout; + /// # use proxmox_alloc::LayoutAwareBox; + /// + /// let aware_boxed = LayoutAwareBox::<[usize]>::from([42, 67]); + /// let layout = LayoutAwareBox::layout(&aware_boxed); + /// + /// assert_eq!( + /// layout, + /// Layout::from_size_align(size_of::<usize>() * 2, align_of::<usize>()).unwrap() + /// ); + /// ``` + /// + /// [`Layout`]: std::alloc::Layout + pub fn layout(from: &Self) -> alloc::Layout { + match from.type_size { + TypeSize::Known => alloc::Layout::for_value(&**from.inner), + TypeSize::Dynamic(layout) => layout, + } + } +} + +impl<T> LayoutAwareBox<T> { + /// Moves `T` onto the heap and allocate its memory with the passed + /// alignment. + /// + /// The alignment must always be a power of 2. This also includes `2^0`, + /// which is `1`. + /// + /// Should the passed alignment be smaller than that of `T`, the alignment + /// of `T` will be used directly instead. + /// + /// Note that if `T` is [zero-sized], no allocation is actually performed. + /// + /// # Examples + /// + /// ``` + /// # use proxmox_alloc::LayoutAwareBox; + /// let aware_box = LayoutAwareBox::new(67u8, 1).expect("infallible"); + /// ``` + /// + /// [zero-sized]: https://doc.rust-lang.org/nomicon/exotic-sizes.html#zero-sized-types-zsts + pub fn new(value: T, mut alignment: usize) -> Result<Self, LayoutAwareBoxError> { + if alignment < align_of::<T>() { + alignment = align_of::<T>(); + } + + if alignment == 0 { + return Err(LayoutAwareBoxError::InvalidAlignment); + } + + let size = size_of::<T>(); + if size == 0 { + return Ok(Self { + inner: ManuallyDrop::new(Box::new(value)), + type_size: TypeSize::Known, + }); + } + + let layout = alloc::Layout::from_size_align(size, alignment) + .map_err(|_| LayoutAwareBoxError::InvalidAlignment)?; + + // SAFETY: Requirements on layout are enforced by from_size_align + let ptr = unsafe { alloc::alloc(layout) as *mut T }; + if ptr.is_null() { + return Err(LayoutAwareBoxError::OutOfMemory); + } + + // ptr points to uninitialized memory, and is currently not a valid + // instance of T. Therefore, we have to write to it without dropping the + // invalid pointee it currently stores. `value` must also not be + // dropped. + // + // SAFETY: ptr is non-null, valid for writes and properly aligned, + // guaranteed earlier through from_size_align + unsafe { ptr.write(value) }; + + // SAFETY: ptr is non-null, properly sized and aligned, and doesn't + // alias with other pointers -- it is also consumed here, meaning it + // cannot be used elsewhere. + let new = unsafe { Self::from_raw_parts(ptr, layout) }; + + Ok(new) + } +} + +impl<T> LayoutAwareBox<[T]> { + /// Convenience method to return a `LayoutAwareBox<[T]>` with no elements in + /// it. This implies that the stored `[T]` is [zero-sized] and has an + /// alignment equal to [`align_of::<T>()`][align_of]. + /// + /// # Examples + /// + /// ``` + /// # use proxmox_alloc::LayoutAwareBox; + /// let aware_boxed = LayoutAwareBox::<[usize]>::slice_empty(); + /// + /// assert_eq!(size_of_val(&*aware_boxed), 0); + /// assert_eq!(align_of_val(&*aware_boxed), align_of::<usize>()); + /// ``` + /// + /// [zero-sized]: https://doc.rust-lang.org/nomicon/exotic-sizes.html#zero-sized-types-zsts + pub fn slice_empty() -> Self { + LayoutAwareBox { + inner: ManuallyDrop::new(Box::new([])), + type_size: TypeSize::Known, + } + } + + fn slice_zero(len: usize) -> Option<Self> { + if size_of::<T>() == 0 { + let mut v = Vec::new(); + debug_assert_eq!(v.capacity(), usize::MAX); + + // SAFETY: Vec<T> where T is zero-sized always results in a vector + // with a capacity of usize::MAX. Therefore, the resulting length + // can never be outside the reserved capacity here. + unsafe { v.set_len(len) }; + + return Some(Self { + inner: ManuallyDrop::new(Box::from(v)), + type_size: TypeSize::Known, + }); + } + + if len == 0 { + return Some(Self::slice_empty()); + } + + None + } + + fn slice_alloc( + len: usize, + mut alignment: usize, + ) -> Result<(ptr::NonNull<T>, alloc::Layout), LayoutAwareBoxError> { + debug_assert_ne!(len, 0, "len == 0"); + debug_assert_ne!(alignment, 0, "alignment == 0"); + + let max_len = (isize::MAX as usize) / size_of::<T>(); + if len > max_len { + return Err(LayoutAwareBoxError::TooManyElements); + } + + let alloc_size = size_of::<T>() * len; + + if alignment < align_of::<T>() { + alignment = align_of::<T>(); + } + + let layout = alloc::Layout::from_size_align(alloc_size, alignment) + .map_err(|_| LayoutAwareBoxError::InvalidAlignment)?; + + // SAFETY: Layout never has a size of 0. + let thin_ptr = unsafe { alloc::alloc(layout) as *mut T }; + let Some(thin_ptr) = ptr::NonNull::new(thin_ptr) else { + return Err(LayoutAwareBoxError::OutOfMemory); + }; + + Ok((thin_ptr, layout)) + } + + /// Constructs a `LayoutAwareBox<[T]>` using the given length and alignment, + /// and initializes its elements with the provided value. + /// + /// This method otherwise has the same behavior as [`LayoutAwareBox::new`] + /// when it comes to alignment and zero-sized types. + /// + /// # Examples + /// + /// ``` + /// # use proxmox_alloc::LayoutAwareBox; + /// let aware_boxed_buf = LayoutAwareBox::<[u8]>::slice_fill(1024, 128, 0) + /// .expect("infallible"); + /// + /// assert!(aware_boxed_buf.iter().all(|elem| *elem == 0)); + /// ``` + pub fn slice_fill(len: usize, alignment: usize, value: T) -> Result<Self, LayoutAwareBoxError> + where + T: core::clone::Clone, + { + if let Some(new) = Self::slice_zero(len) { + return Ok(new); + } + + let (thin_ptr, layout) = Self::slice_alloc(len, alignment)?; + + for i in 0..len { + // SAFETY: thin_ptr is valid for writing and we never go out of + // bounds during iteration. + unsafe { thin_ptr.add(i).write(value.clone()) }; + } + + // SAFETY: + // * We checked that thin_ptr is not null. + // * The number of elements does not exceed max_len. + // * thin_ptr is not being aliased by any other pointer. + // * The elements of the array behind thin_ptr have been initialized. + let new = unsafe { + let thick_ptr = core::slice::from_raw_parts_mut(thin_ptr.as_ptr(), len); + Self::from_raw_parts(thick_ptr, layout) + }; + + Ok(new) + } + + /// Constructs a `LayoutAwareBox<[T]>` using the given length and alignment, + /// and initializes its elements by calling the provided function. + /// + /// This method otherwise has the same behavior as [`LayoutAwareBox::new`] + /// when it comes to alignment and zero-sized types. + /// + /// # Examples + /// + /// ``` + /// # use proxmox_alloc::LayoutAwareBox; + /// let aware_boxed_buf = LayoutAwareBox::<[u8]>::slice_fill_with(1024, 128, Default::default) + /// .expect("infallible"); + /// + /// assert!(aware_boxed_buf.iter().all(|elem| *elem == 0)); + /// ``` + /// + /// ``` + /// # use proxmox_alloc::LayoutAwareBox; + /// let mut counter: usize = 0; + /// + /// let init_func = || { + /// let current = counter; + /// counter += 1; + /// current + /// }; + /// + /// let aware_boxed = LayoutAwareBox::<[usize]>::slice_fill_with(1024, 128, init_func) + /// .expect("infallible"); + /// + /// assert_eq!(&aware_boxed[0..4], &[0, 1, 2, 3]); + /// assert_eq!(aware_boxed.len(), counter); + /// assert_eq!(aware_boxed[aware_boxed.len() - 1], counter - 1); + /// ``` + pub fn slice_fill_with<F>( + len: usize, + alignment: usize, + mut func: F, + ) -> Result<Self, LayoutAwareBoxError> + where + F: FnMut() -> T, + { + if let Some(new) = Self::slice_zero(len) { + return Ok(new); + } + + let (thin_ptr, layout) = Self::slice_alloc(len, alignment)?; + + for i in 0..len { + // SAFETY: thin_ptr is valid for writing and we never go out of + // bounds during iteration. + unsafe { thin_ptr.add(i).write(func()) }; + } + + // SAFETY: + // * We checked that thin_ptr is not null. + // * The number of elements does not exceed max_len. + // * thin_ptr is not being aliased by any other pointer. + // * The elements of the array behind thin_ptr have been initialized. + let new = unsafe { + let thick_ptr = core::slice::from_raw_parts_mut(thin_ptr.as_ptr(), len); + Self::from_raw_parts(thick_ptr, layout) + }; + + Ok(new) + } +} + +impl<T> Default for LayoutAwareBox<T> +where + T: Default, +{ + #[inline] + fn default() -> Self { + Self { + inner: Default::default(), + type_size: TypeSize::Known, + } + } +} + +impl<T> Default for LayoutAwareBox<[T]> { + #[inline] + fn default() -> Self { + Self::slice_empty() + } +} + +impl<T> Clone for LayoutAwareBox<T> +where + T: Clone, +{ + #[inline] + fn clone(&self) -> Self { + Self { + inner: self.inner.clone(), + type_size: self.type_size.clone(), + } + } + + #[inline] + fn clone_from(&mut self, source: &Self) { + (**self).clone_from(&(**source)) + } +} + +impl<T> Clone for LayoutAwareBox<[T]> +where + T: Clone, +{ + #[inline] + fn clone(&self) -> Self { + Self { + inner: self.inner.clone(), + type_size: self.type_size.clone(), + } + } + + #[inline] + fn clone_from(&mut self, source: &Self) { + *self = source.clone() + } +} + +impl<T> std::ops::Deref for LayoutAwareBox<T> +where + T: ?Sized, +{ + type Target = T; + + fn deref(&self) -> &Self::Target { + &self.inner + } +} + +impl<T> std::ops::DerefMut for LayoutAwareBox<T> +where + T: ?Sized, +{ + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.inner + } +} + +impl<T> PartialEq for LayoutAwareBox<T> +where + T: PartialEq + ?Sized, +{ + #[inline] + fn eq(&self, other: &Self) -> bool { + PartialEq::eq(&**self, &**other) + } + + #[inline] + #[allow(clippy::partialeq_ne_impl)] + fn ne(&self, other: &Self) -> bool { + PartialEq::ne(&**self, &**other) + } +} + +impl<T> Eq for LayoutAwareBox<T> where T: Eq + ?Sized {} + +impl<T> PartialOrd for LayoutAwareBox<T> +where + T: PartialOrd + ?Sized, +{ + #[inline] + fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> { + PartialOrd::partial_cmp(&**self, &**other) + } + + #[inline] + fn lt(&self, other: &Self) -> bool { + PartialOrd::lt(&**self, &**other) + } + + #[inline] + fn le(&self, other: &Self) -> bool { + PartialOrd::le(&**self, &**other) + } + + #[inline] + fn ge(&self, other: &Self) -> bool { + PartialOrd::ge(&**self, &**other) + } + + #[inline] + fn gt(&self, other: &Self) -> bool { + PartialOrd::gt(&**self, &**other) + } +} + +impl<T> Ord for LayoutAwareBox<T> +where + T: Ord + ?Sized, +{ + #[inline] + fn cmp(&self, other: &Self) -> core::cmp::Ordering { + Ord::cmp(&**self, &**other) + } +} + +impl<T> std::hash::Hash for LayoutAwareBox<T> +where + T: std::hash::Hash + ?Sized, +{ + #[inline] + fn hash<H: std::hash::Hasher>(&self, state: &mut H) { + std::hash::Hash::hash(&**self, state) + } +} + +impl<T> std::hash::Hasher for LayoutAwareBox<T> +where + T: std::hash::Hasher + ?Sized, +{ + #[inline] + fn finish(&self) -> u64 { + (**self).finish() + } + + #[inline] + fn write(&mut self, bytes: &[u8]) { + (**self).write(bytes) + } + + #[inline] + fn write_u8(&mut self, i: u8) { + (**self).write_u8(i) + } + + #[inline] + fn write_u16(&mut self, i: u16) { + (**self).write_u16(i) + } + + #[inline] + fn write_u32(&mut self, i: u32) { + (**self).write_u32(i) + } + + #[inline] + fn write_u64(&mut self, i: u64) { + (**self).write_u64(i) + } + + #[inline] + fn write_u128(&mut self, i: u128) { + (**self).write_u128(i) + } + + #[inline] + fn write_usize(&mut self, i: usize) { + (**self).write_usize(i) + } + + #[inline] + fn write_i8(&mut self, i: i8) { + (**self).write_i8(i) + } + + #[inline] + fn write_i16(&mut self, i: i16) { + (**self).write_i16(i) + } + + #[inline] + fn write_i32(&mut self, i: i32) { + (**self).write_i32(i) + } + + #[inline] + fn write_i64(&mut self, i: i64) { + (**self).write_i64(i) + } + + #[inline] + fn write_i128(&mut self, i: i128) { + (**self).write_i128(i) + } + + #[inline] + fn write_isize(&mut self, i: isize) { + (**self).write_isize(i) + } +} + +impl<T> std::fmt::Display for LayoutAwareBox<T> +where + T: std::fmt::Display + ?Sized, +{ + #[inline] + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + std::fmt::Display::fmt(&**self, f) + } +} + +impl<T> std::fmt::Debug for LayoutAwareBox<T> +where + T: std::fmt::Debug + ?Sized, +{ + #[inline] + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + std::fmt::Debug::fmt(&**self, f) + } +} + +impl<T> std::fmt::Pointer for LayoutAwareBox<T> +where + T: std::fmt::Pointer + ?Sized, +{ + #[inline] + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + std::fmt::Pointer::fmt(&**self, f) + } +} + +impl<T> core::borrow::Borrow<T> for LayoutAwareBox<T> +where + T: ?Sized, +{ + #[inline] + fn borrow(&self) -> &T { + self + } +} + +impl<T> core::borrow::BorrowMut<T> for LayoutAwareBox<T> +where + T: ?Sized, +{ + #[inline] + fn borrow_mut(&mut self) -> &mut T { + self + } +} + +impl<T> core::convert::AsRef<T> for LayoutAwareBox<T> +where + T: ?Sized, +{ + #[inline] + fn as_ref(&self) -> &T { + self + } +} + +impl<T> core::convert::AsMut<T> for LayoutAwareBox<T> +where + T: ?Sized, +{ + #[inline] + fn as_mut(&mut self) -> &mut T { + self + } +} + +impl<E> core::error::Error for LayoutAwareBox<E> +where + E: core::error::Error, +{ + #[inline] + fn cause(&self) -> Option<&dyn std::error::Error> { + core::error::Error::source(&**self) + } + + #[inline] + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + core::error::Error::source(&**self) + } +} + +impl<'a, T> IntoIterator for &'a LayoutAwareBox<[T]> { + type Item = &'a T; + + type IntoIter = core::slice::Iter<'a, T>; + + #[inline] + fn into_iter(self) -> Self::IntoIter { + self.iter() + } +} + +impl<I> FromIterator<I> for LayoutAwareBox<[I]> { + #[inline] + fn from_iter<T: IntoIterator<Item = I>>(iter: T) -> Self { + Self { + inner: ManuallyDrop::new(Box::from_iter(iter)), + type_size: TypeSize::Known, + } + } +} + +impl<I> Iterator for LayoutAwareBox<I> +where + I: Iterator + ?Sized, +{ + type Item = I::Item; + + #[inline] + fn next(&mut self) -> Option<I::Item> { + (**self).next() + } + + #[inline] + fn size_hint(&self) -> (usize, Option<usize>) { + (**self).size_hint() + } + + #[inline] + fn nth(&mut self, n: usize) -> Option<I::Item> { + (**self).nth(n) + } +} + +impl<I> DoubleEndedIterator for LayoutAwareBox<I> +where + I: DoubleEndedIterator + ?Sized, +{ + #[inline] + fn next_back(&mut self) -> Option<I::Item> { + (**self).next_back() + } + + #[inline] + fn nth_back(&mut self, n: usize) -> Option<I::Item> { + (**self).nth_back(n) + } +} +impl<I> ExactSizeIterator for LayoutAwareBox<I> +where + I: ExactSizeIterator + ?Sized, +{ + #[inline] + fn len(&self) -> usize { + (**self).len() + } +} + +impl<I> std::iter::FusedIterator for LayoutAwareBox<I> where I: std::iter::FusedIterator + ?Sized {} + +impl<R> std::io::Read for LayoutAwareBox<R> +where + R: std::io::Read + ?Sized, +{ + #[inline] + fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> { + (**self).read(buf) + } + + #[inline] + fn read_vectored(&mut self, bufs: &mut [std::io::IoSliceMut<'_>]) -> std::io::Result<usize> { + (**self).read_vectored(bufs) + } + + #[inline] + fn read_to_end(&mut self, buf: &mut Vec<u8>) -> std::io::Result<usize> { + (**self).read_to_end(buf) + } + + #[inline] + fn read_to_string(&mut self, buf: &mut String) -> std::io::Result<usize> { + (**self).read_to_string(buf) + } + + #[inline] + fn read_exact(&mut self, buf: &mut [u8]) -> std::io::Result<()> { + (**self).read_exact(buf) + } +} + +impl<W> std::io::Write for LayoutAwareBox<W> +where + W: std::io::Write + ?Sized, +{ + #[inline] + fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> { + (**self).write(buf) + } + + #[inline] + fn write_vectored(&mut self, bufs: &[std::io::IoSlice<'_>]) -> std::io::Result<usize> { + (**self).write_vectored(bufs) + } + + #[inline] + fn flush(&mut self) -> std::io::Result<()> { + (**self).flush() + } + + #[inline] + fn write_all(&mut self, buf: &[u8]) -> std::io::Result<()> { + (**self).write_all(buf) + } + + #[inline] + fn write_fmt(&mut self, fmt: std::fmt::Arguments<'_>) -> std::io::Result<()> { + (**self).write_fmt(fmt) + } +} + +impl<S> std::io::Seek for LayoutAwareBox<S> +where + S: std::io::Seek + ?Sized, +{ + #[inline] + fn seek(&mut self, pos: std::io::SeekFrom) -> std::io::Result<u64> { + (**self).seek(pos) + } + + #[inline] + fn rewind(&mut self) -> std::io::Result<()> { + (**self).rewind() + } + + #[inline] + fn stream_position(&mut self) -> std::io::Result<u64> { + (**self).stream_position() + } + + #[inline] + fn seek_relative(&mut self, offset: i64) -> std::io::Result<()> { + (**self).seek_relative(offset) + } +} + +impl<B> std::io::BufRead for LayoutAwareBox<B> +where + B: std::io::BufRead + ?Sized, +{ + #[inline] + fn fill_buf(&mut self) -> std::io::Result<&[u8]> { + (**self).fill_buf() + } + + #[inline] + fn consume(&mut self, amt: usize) { + (**self).consume(amt) + } + + #[inline] + fn read_until(&mut self, byte: u8, buf: &mut Vec<u8>) -> std::io::Result<usize> { + (**self).read_until(byte, buf) + } + + #[inline] + fn skip_until(&mut self, byte: u8) -> std::io::Result<usize> { + (**self).skip_until(byte) + } + + #[inline] + fn read_line(&mut self, buf: &mut String) -> std::io::Result<usize> { + (**self).read_line(buf) + } +} + +// NOTE: The `From` conversion impls below are all "trivial" in the sense that +// each `Box<T>` we store in `Self` can drop and deallocate its inner value just +// fine. This is because we do not use `T: ?Sized`, so therefore we may use +// `TypeSize::Known`. + +impl<T> From<T> for LayoutAwareBox<T> { + #[inline] + fn from(value: T) -> Self { + Self { + inner: ManuallyDrop::new(Box::from(value)), + type_size: TypeSize::Known, + } + } +} + +impl<T> From<Box<T>> for LayoutAwareBox<T> { + #[inline] + fn from(value: Box<T>) -> Self { + Self { + inner: ManuallyDrop::new(value), + type_size: TypeSize::Known, + } + } +} + +impl<T> From<Box<[T]>> for LayoutAwareBox<[T]> { + #[inline] + fn from(value: Box<[T]>) -> Self { + Self { + inner: ManuallyDrop::new(value), + type_size: TypeSize::Known, + } + } +} + +impl<T> From<Vec<T>> for LayoutAwareBox<[T]> { + #[inline] + fn from(value: Vec<T>) -> Self { + Self { + inner: ManuallyDrop::new(Box::from(value)), + type_size: TypeSize::Known, + } + } +} + +impl<T, const N: usize> From<[T; N]> for LayoutAwareBox<[T]> { + #[inline] + fn from(value: [T; N]) -> Self { + Self { + inner: ManuallyDrop::new(Box::from(value)), + type_size: TypeSize::Known, + } + } +} + +impl<T> From<&[T]> for LayoutAwareBox<[T]> +where + T: Clone, +{ + /// This conversion allocates on the heap and performs a copy of `&[T]` and + /// its contents. + #[inline] + fn from(value: &[T]) -> Self { + Self { + inner: ManuallyDrop::new(Box::from(value)), + type_size: TypeSize::Known, + } + } +} +impl<T> From<&mut [T]> for LayoutAwareBox<[T]> +where + T: Clone, +{ + /// This conversion allocates on the heap and performs a copy of `&mut [T]` + /// and its contents. + #[inline] + fn from(value: &mut [T]) -> Self { + Self { + inner: ManuallyDrop::new(Box::from(value)), + type_size: TypeSize::Known, + } + } +} + +impl<T> From<std::borrow::Cow<'_, [T]>> for LayoutAwareBox<[T]> +where + T: Clone, +{ + /// When cow is the [`Cow::Borrowed`](std::borrow::Cow::Borrowed) variant, + /// this conversion allocates on the heap and copies the underlying slice. + /// Otherwise, it will try to reuse the owned [`Vec`]’s allocation. + #[inline] + fn from(value: std::borrow::Cow<'_, [T]>) -> Self { + Self { + inner: ManuallyDrop::new(Box::from(value)), + type_size: TypeSize::Known, + } + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn zero_sized() { + let boxed_unit = LayoutAwareBox::new((), 1).expect("infallible"); + let inner_ref: &() = &*boxed_unit; + + assert_eq!(size_of_val(inner_ref), 0, "size_of_val() != 0"); + assert_eq!(align_of_val(inner_ref), 1, "align_of_val() != 1"); + + drop(boxed_unit); + + let empty_slice = LayoutAwareBox::<[u8]>::slice_empty(); + let inner_ref: &[u8] = &*empty_slice; + + assert_eq!(inner_ref.len(), 0, "len != 0"); + assert_eq!(size_of_val(inner_ref), 0, "size_of_val() != 0"); + assert_eq!(align_of_val(inner_ref), 1, "align_of_val() != 1"); + + drop(empty_slice); + + let empty_unit_slice = LayoutAwareBox::<[()]>::slice_empty(); + let inner_ref: &[()] = &*empty_unit_slice; + + assert_eq!(inner_ref.len(), 0, "len != 0"); + assert_eq!(size_of_val(inner_ref), 0, "size_of_val() != 0"); + assert_eq!(align_of_val(inner_ref), 1, "align_of_val() != 1"); + + drop(empty_unit_slice); + + let filled_unit_slice = LayoutAwareBox::<[()]>::slice_fill(13, 1, ()).expect("infallible"); + let inner_ref: &[()] = &*filled_unit_slice; + + assert_eq!(inner_ref.len(), 13, "len != 13"); + assert_eq!(size_of_val(inner_ref), 0, "size_of_val() != 0"); + assert_eq!(align_of_val(inner_ref), 1, "align_of_val() != 1"); + + for element in &filled_unit_slice { + assert_eq!(*element, ()); + } + + assert_eq!(filled_unit_slice.into_iter().len(), 13); + + drop(filled_unit_slice); + } + + #[test] + fn zero_sized_layout() { + let aware_boxed = LayoutAwareBox::new((), 1).expect("infallible"); + + assert_eq!( + LayoutAwareBox::layout(&aware_boxed), + std::alloc::Layout::for_value(&()), + ); + } + + #[test] + fn conversion() { + let vec: Vec<usize> = vec![0, 1, 2, 3, 4, 5]; + let aware_boxed = LayoutAwareBox::<[usize]>::from(vec); + + assert_eq!(aware_boxed.len(), 6); + + assert_eq!( + LayoutAwareBox::layout(&aware_boxed), + std::alloc::Layout::from_size_align( + size_of::<usize>() * aware_boxed.len(), + align_of::<usize>() + ) + .unwrap(), + "layout of slice from vector should have size = size_of::<T>() * len, align = align_of::<T>()" + ); + + drop(aware_boxed); + + let boxed: Box<usize> = Box::new(42); + let aware_boxed = LayoutAwareBox::from(boxed); + + assert_eq!( + aware_boxed, + LayoutAwareBox::new(42, align_of::<usize>()).expect("infallible"), + ); + + assert_eq!( + LayoutAwareBox::layout(&aware_boxed), + std::alloc::Layout::from_size_align(size_of::<usize>(), align_of::<usize>()).unwrap(), + "layout of value from Box should always have size = size_of::<T>(), align = align_of::<T>()" + ); + + drop(aware_boxed); + + let boxed: Box<[usize]> = Box::new([42, 67]); + let aware_boxed = LayoutAwareBox::<[usize]>::from(boxed); + + assert_eq!(aware_boxed, LayoutAwareBox::from(vec![42usize, 67])); + + assert_eq!( + LayoutAwareBox::layout(&aware_boxed), + std::alloc::Layout::from_size_align( + size_of::<usize>() * aware_boxed.len(), + align_of::<usize>() + ) + .unwrap(), + "layout of slice from Box should always have size = size_of::<T>() * len, align = align_of::<T>()" + ); + + drop(aware_boxed); + + let aware_boxed = LayoutAwareBox::<[usize]>::from([42, 67]); + + assert_eq!(aware_boxed, LayoutAwareBox::from(vec![42usize, 67])); + + assert_eq!( + LayoutAwareBox::layout(&aware_boxed), + std::alloc::Layout::from_size_align( + size_of::<usize>() * aware_boxed.len(), + align_of::<usize>() + ) + .unwrap(), + "layout of slice from [T; N] should always have size = size_of::<T>() * N, align = align_of::<T>()" + ); + + drop(aware_boxed); + } + + #[test] + fn slice_fill_sized_nonempty() { + let aware_boxed = + LayoutAwareBox::slice_fill(7, align_of::<u64>(), 8u32).expect("infallible"); + let inner_ref = &*aware_boxed; + + assert_eq!(inner_ref.len(), 7, "inner_ref.len() != 7"); + + assert_eq!( + align_of_val(inner_ref), + align_of::<u32>(), + concat!( + "for any [T] where T: Sized, align_of_val() is always align_of::<T>(),", + " because the compiler does not track alignment" + ) + ); + + assert_eq!( + size_of_val(inner_ref), + size_of::<u32>() * 7, + "[T] where T: Sized with N elements should always have a size of size_of::<T>() * N" + ); + + assert_eq!( + LayoutAwareBox::layout(&aware_boxed), + std::alloc::Layout::from_size_align( + size_of::<u32>() * aware_boxed.len(), + align_of::<u64>() + ) + .unwrap(), + "[T] where T: Sized with N elements should always have a layout where size = size_of::<T>() * N and the alignment that was specified" + ); + + assert_eq!(inner_ref, &[8u32; 7], "equality sanity check failed"); + + assert_eq!( + aware_boxed, + LayoutAwareBox::slice_fill(7, align_of::<u64>(), 8u32).expect("infallible"), + "equality sanity check failed" + ); + + let aware_boxed_other = + LayoutAwareBox::slice_fill_with(7, align_of::<u64>(), || 8u32).expect("infallible"); + + assert_eq!( + aware_boxed, aware_boxed_other, + "equality check with aware box from slice_fill_with failed" + ); + + assert_eq!( + LayoutAwareBox::layout(&aware_boxed), + LayoutAwareBox::layout(&aware_boxed_other), + "layout equality check with aware box from slice_fill_with failed" + ); + + drop(aware_boxed); + } + + #[test] + fn slice_fill_sized_empty() { + let aware_boxed = LayoutAwareBox::slice_fill(0, 1024, 1337usize).expect("infallible"); + let inner_ref = &*aware_boxed; + + assert_eq!(inner_ref.len(), 0, "inner_ref.len() != 0"); + + assert_eq!( + size_of_val(inner_ref), + 0, + "[T] where T: Sized with 0 elements should always have a size of 0" + ); + + assert_eq!( + align_of_val(inner_ref), + align_of::<usize>(), + "[T] where T: Sized with 0 elements should always have an alignment of align_of::<T>()" + ); + + assert_eq!( + LayoutAwareBox::layout(&aware_boxed), + std::alloc::Layout::from_size_align(0, align_of::<usize>()).unwrap(), + "[T] where T: Sized with 0 elements should always have a layout where size = 0, align = align_of::<T>()" + ); + + assert_eq!(inner_ref, &[], "equality sanity check failed"); + + assert_eq!( + aware_boxed, + LayoutAwareBox::slice_empty(), + "equality sanity check failed" + ); + + let aware_boxed_other = + LayoutAwareBox::slice_fill_with(0, 1024, || 1337usize).expect("infallible"); + + assert_eq!( + aware_boxed, aware_boxed_other, + "equality check with aware box from slice_fill_with failed" + ); + + assert_eq!( + LayoutAwareBox::layout(&aware_boxed), + LayoutAwareBox::layout(&aware_boxed_other), + "layout equality check with aware box from slice_fill_with failed" + ); + + drop(aware_boxed); + } + + #[test] + fn slice_fill_unsized_nonempty() { + let aware_boxed = LayoutAwareBox::slice_fill(13, 1024, ()).expect("infallible"); + let inner_ref = &*aware_boxed; + + assert_eq!(inner_ref.len(), 13, "inner_ref.len() != 13"); + + assert_eq!( + size_of_val(inner_ref), + 0, + "nonempty slice of a zero-sized type should not have any size" + ); + + assert_eq!( + align_of_val(inner_ref), + 1, + "nonempty slice of a zero-sized type should always have an alignment of 1" + ); + + assert_eq!( + LayoutAwareBox::layout(&aware_boxed), + std::alloc::Layout::from_size_align(0, align_of::<()>()).unwrap(), + "layout of nonempty [T] where T is zero-sized should always have size = 0 and align = 1" + ); + + assert_eq!(inner_ref, &[(); 13], "equality sanity check failed"); + + assert_eq!( + aware_boxed, + LayoutAwareBox::slice_fill(13, 32, ()).expect("infallible"), + "equality sanity check failed" + ); + + let aware_boxed_other = + LayoutAwareBox::slice_fill_with(13, 1024, || ()).expect("infallible"); + + assert_eq!( + aware_boxed, aware_boxed_other, + "equality check with aware box from slice_fill_with failed" + ); + + assert_eq!( + LayoutAwareBox::layout(&aware_boxed), + LayoutAwareBox::layout(&aware_boxed_other), + "layout equality check with aware box from slice_fill_with failed" + ); + + drop(aware_boxed); + } + + #[test] + fn slice_fill_unsized_empty() { + let aware_boxed = LayoutAwareBox::slice_fill(0, 1024, ()).expect("infallible"); + let inner_ref = &*aware_boxed; + + assert_eq!(inner_ref.len(), 0, "inner_ref.len() != 0"); + + assert_eq!( + size_of_val(inner_ref), + 0, + "empty slice of a zero-sized type should not have any size" + ); + + assert_eq!( + align_of_val(inner_ref), + 1, + "empty slice of a zero-sized type should always have an alignment of 1" + ); + + assert_eq!( + LayoutAwareBox::layout(&aware_boxed), + std::alloc::Layout::from_size_align(0, align_of::<()>()).unwrap(), + "layout of empty [T] where T is zero-sized should always have size = 0 and align = 1" + ); + + assert_eq!(inner_ref, &[(); 0], "equality sanity check failed"); + + assert_eq!( + aware_boxed, + LayoutAwareBox::slice_fill(0, 32, ()).expect("infallible"), + "equality sanity check failed" + ); + + let aware_boxed_other = + LayoutAwareBox::slice_fill_with(0, 1024, || ()).expect("infallible"); + + assert_eq!( + aware_boxed, aware_boxed_other, + "equality check with aware box from slice_fill_with failed" + ); + + assert_eq!( + LayoutAwareBox::layout(&aware_boxed), + LayoutAwareBox::layout(&aware_boxed_other), + "layout equality check with aware box from slice_fill_with failed" + ); + + drop(aware_boxed); + } + + #[test] + fn from_raw_parts_unsized_slice() { + let len = 11; + let size = size_of::<u8>() * len; + let align = align_of::<u8>(); + + let layout = std::alloc::Layout::from_size_align(size, align).expect("infallible"); + + let thin_ptr = unsafe { std::alloc::alloc_zeroed(layout) }; + assert!(!thin_ptr.is_null(), "thin_ptr is null"); + + let fat_ptr = std::ptr::slice_from_raw_parts_mut(thin_ptr, len); + assert!(!fat_ptr.is_null(), "fat_ptr is null"); + + let aware_box = unsafe { LayoutAwareBox::from_raw_parts(fat_ptr, layout) }; + + let inner_ref = &*aware_box; + + assert_eq!(inner_ref.len(), len, "inner_ref.len() != len"); + assert_eq!(size_of_val(inner_ref), len, "size_of_val(inner_ref) != len"); + } + + #[test] + fn from_raw_parts_unsized_type() { + #[repr(C, packed)] + struct MyDSTHeader { + foo: usize, + bar: i32, + } + + #[repr(C, packed)] + struct MyDST { + header: MyDSTHeader, + data: [u8], + } + + // -- construction + let alloc_size = 1024; + let header_size = size_of::<MyDSTHeader>(); + let data_len = alloc_size - header_size; + + let layout = std::alloc::Layout::from_size_align(alloc_size, align_of::<MyDSTHeader>()) + .expect("infallible"); + + let thin_ptr = unsafe { std::alloc::alloc_zeroed(layout) }; + assert!(!thin_ptr.is_null(), "thin_ptr is null"); + + let fat_ptr = std::ptr::slice_from_raw_parts_mut(thin_ptr, data_len) as *mut MyDST; + assert!(!fat_ptr.is_null(), "fat_ptr is null"); + + let aware_boxed = unsafe { LayoutAwareBox::from_raw_parts(fat_ptr, layout) }; + let inner_ref = &*aware_boxed; + + // -- actual checks start here + assert_eq!(inner_ref.data.len(), data_len, "data.len() != data_len"); + + assert_eq!( + inner_ref.data.iter().count(), + data_len, + "data.iter().count() != data_len" + ); + + let got_size = size_of_val(inner_ref); + let expected_size = header_size + data_len; + assert_eq!( + got_size, expected_size, + "size_of_val() != header_size + data_len ({got_size} != {expected_size})" + ); + + assert_eq!( + LayoutAwareBox::layout(&aware_boxed), + layout, + "layout equality sanity check failed" + ); + } + + #[test] + fn from_raw_parts_unsized_type_weird_data() { + #[repr(C, packed)] + struct MyDSTHeader { + foo: usize, + bar: i32, + } + + #[repr(Rust)] + #[allow(unused)] + struct MyPayloadElement([u8; 7]); + + #[repr(C, packed)] + struct MyDST { + header: MyDSTHeader, + data: [MyPayloadElement], + } + + // -- construction + let max_alloc_size = 1024; + let header_size = size_of::<MyDSTHeader>(); + + let data_element_size = size_of::<MyPayloadElement>(); + let data_len = (max_alloc_size - header_size) / data_element_size; + + // Allocated size *must* correspond to actual size that MyDST will occupy + let alloc_size = data_len * data_element_size + header_size; + + let layout = std::alloc::Layout::from_size_align(alloc_size, align_of::<MyDSTHeader>()) + .expect("infallible"); + + let thin_ptr = unsafe { std::alloc::alloc_zeroed(layout) }; + assert!(!thin_ptr.is_null(), "thin_ptr is null"); + + let fat_ptr = std::ptr::slice_from_raw_parts_mut(thin_ptr, data_len) as *mut MyDST; + assert!(!fat_ptr.is_null(), "fat_ptr is null"); + + let aware_boxed = unsafe { LayoutAwareBox::from_raw_parts(fat_ptr, layout) }; + let inner_ref = &*aware_boxed; + + // -- actual checks start here + assert_eq!(inner_ref.data.len(), data_len, "data.len() != data_len"); + + assert_eq!( + inner_ref.data.iter().count(), + data_len, + "data.iter().count() != data_len" + ); + + let got_size = size_of_val(inner_ref); + let expected_size = header_size + data_element_size * data_len; + assert_eq!( + got_size, expected_size, + "size_of_val() != header_size + data_element_size * data_len \ + ({got_size} != {expected_size})" + ); + + assert_eq!( + LayoutAwareBox::layout(&aware_boxed), + layout, + "layout equality sanity check failed" + ); + } + + #[test] + fn from_raw_parts_unsized_type_weird_data_padded() { + #[repr(C, packed)] + struct MyDSTHeader { + foo: usize, + bar: i32, + _pad1: u8, + _pad2: u8, + _pad3: u8, + _pad4: u8, + } + + #[repr(Rust)] + #[allow(unused)] + struct MyPayloadElement([u8; 7]); + + #[repr(C, packed)] + struct MyDST { + header: MyDSTHeader, + data: [MyPayloadElement], + } + + // -- construction + let max_alloc_size = 1024; + let header_size = size_of::<MyDSTHeader>(); + + let data_element_size = size_of::<MyPayloadElement>(); + let data_len = (max_alloc_size - header_size) / data_element_size; + + // Allocated size *must* correspond to actual size that MyDST will occupy + let alloc_size = data_len * data_element_size + header_size; + + assert_eq!( + max_alloc_size, alloc_size, + "MyDST will not occupy all {max_alloc_size} allocated bytes after construction" + ); + + let layout = std::alloc::Layout::from_size_align(alloc_size, align_of::<MyDSTHeader>()) + .expect("infallible"); + + let thin_ptr = unsafe { std::alloc::alloc_zeroed(layout) }; + assert!(!thin_ptr.is_null(), "thin_ptr is null"); + + let fat_ptr = std::ptr::slice_from_raw_parts_mut(thin_ptr, data_len) as *mut MyDST; + assert!(!fat_ptr.is_null(), "fat_ptr is null"); + + let aware_boxed = unsafe { LayoutAwareBox::from_raw_parts(fat_ptr, layout) }; + let inner_ref = &*aware_boxed; + + // -- actual checks start here + assert_eq!(inner_ref.data.len(), data_len, "data.len() != data_len"); + + assert_eq!( + inner_ref.data.iter().count(), + data_len, + "data.iter().count() != data_len" + ); + + let got_size = size_of_val(inner_ref); + let expected_size = header_size + data_element_size * data_len; + assert_eq!( + got_size, expected_size, + "size_of_val() != header_size + data_element_size * data_len \ + ({got_size} != {expected_size})" + ); + + assert_eq!( + LayoutAwareBox::layout(&aware_boxed), + layout, + "layout equality sanity check failed" + ); + } + + #[test] + fn clone_impl() { + let aware_boxed = LayoutAwareBox::<usize>::default(); + let cloned = aware_boxed.clone(); + + assert_eq!(aware_boxed, cloned); + assert_eq!( + LayoutAwareBox::layout(&aware_boxed), + LayoutAwareBox::layout(&cloned) + ); + assert_ne!(&*aware_boxed as *const _, &*cloned as *const _); + + drop(aware_boxed); + drop(cloned); + + let aware_boxed = LayoutAwareBox::<[usize]>::default(); + let cloned = aware_boxed.clone(); + + assert_eq!(aware_boxed, cloned); + assert_eq!( + LayoutAwareBox::layout(&aware_boxed), + LayoutAwareBox::layout(&cloned) + ); + assert_eq!( + &*aware_boxed as *const _, &*cloned as *const _, + "empty slices are zero-sized types and thus will always have the same pointer" + ); + + drop(aware_boxed); + drop(cloned); + + let aware_boxed = LayoutAwareBox::<[usize]>::slice_fill(4, 64, 1337).expect("infallible"); + let cloned = aware_boxed.clone(); + + assert_eq!(aware_boxed, cloned); + assert_eq!( + LayoutAwareBox::layout(&aware_boxed), + LayoutAwareBox::layout(&cloned) + ); + assert_ne!(&*aware_boxed as *const _, &*cloned as *const _); + + drop(aware_boxed); + drop(cloned); + } +} diff --git a/proxmox-alloc/src/lib.rs b/proxmox-alloc/src/lib.rs new file mode 100644 index 00000000..64c1beb4 --- /dev/null +++ b/proxmox-alloc/src/lib.rs @@ -0,0 +1,19 @@ +//! # The Proxmox allocation and collections library +//! +//! This library provides smart pointers and collections for managing +//! heap-allocated values, similar to [`core::alloc`]. +//! +//! ## Layout-Aware Box +//! +//! The [`LayoutAwareBox`] is a smart pointer type that, unlike the regular +//! [`Box`], also tracks which [`Layout`] was used to allocate its contents. +//! This is useful for managing the allocations of [dynamically sized types +//! (DSTs)][DST] and creating heap-allocated buffers with a certain alignment. +//! +//! [`Layout`]: std::alloc::Layout +//! +//! [DST]: https://doc.rust-lang.org/nomicon/exotic-sizes.html#dynamically-sized-types-dsts + +pub mod aware_boxed; +pub use aware_boxed::LayoutAwareBox; +pub use aware_boxed::LayoutAwareBoxError; -- 2.47.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH proxmox v2 02/10] proxmox-alloc: document undefined behavior regarding custom allocs 2026-08-21 14:02 [PATCH proxmox{,-backup} v2 00/10] Fix Undefined Behavior in Tape Block Header Deallocation Max R. Carrara 2026-08-21 14:02 ` [PATCH proxmox v2 01/10] proxmox-alloc: introduce proxmox-alloc with `LayoutAwareBox<T>` type Max R. Carrara @ 2026-08-21 14:02 ` Max R. Carrara 2026-08-21 14:02 ` [PATCH proxmox-backup v2 03/10] tape: move tape block structs into separate file module Max R. Carrara ` (7 subsequent siblings) 9 siblings, 0 replies; 11+ messages in thread From: Max R. Carrara @ 2026-08-21 14:02 UTC (permalink / raw) To: pbs-devel Add a new example at examples/ub.rs with tests that demonstrate incorrect usage & undefined behavior of custom allocations. Add a hint in the `aware_boxed` module's docs that such an example exists in the crate. Signed-off-by: Max R. Carrara <m.carrara@proxmox.com> --- proxmox-alloc/Cargo.toml | 4 ++ proxmox-alloc/examples/ub.rs | 99 ++++++++++++++++++++++++++++++++ proxmox-alloc/src/aware_boxed.rs | 3 + 3 files changed, 106 insertions(+) create mode 100644 proxmox-alloc/examples/ub.rs diff --git a/proxmox-alloc/Cargo.toml b/proxmox-alloc/Cargo.toml index 4eeeb787..b16496f6 100644 --- a/proxmox-alloc/Cargo.toml +++ b/proxmox-alloc/Cargo.toml @@ -11,6 +11,10 @@ repository.workspace = true license.workspace = true exclude.workspace = true +[[example]] +crate-type = ["staticlib"] +name = "ub" + [dependencies] [dev-dependencies] diff --git a/proxmox-alloc/examples/ub.rs b/proxmox-alloc/examples/ub.rs new file mode 100644 index 00000000..aa97b02b --- /dev/null +++ b/proxmox-alloc/examples/ub.rs @@ -0,0 +1,99 @@ +//! This example contains various tests that demonstrate [undefined behavior]. +//! Needless to say, you should not use what's shown here in your own code. +//! +//! You can run this example's tests using the following command: +//! +//! ```text +//! cargo test --package proxmox-alloc --example ub +//! ``` +//! +//! Alternatively, if you want to see what kinds of [undefined behavior] get +//! caught by [Miri], use the following instead: +//! +//! ```text +//! cargo +nightly miri nextest run --package proxmox-alloc --example ub +//! ``` +//! +//! Note that this requires [`cargo nextest`][nextest] to be installed on your +//! nightly toolchain. This will guarantee that all tests are run, even if they +//! fail (which they do when you use Miri). +//! +//! [Miri]: https://github.com/rust-lang/miri +//! [nextest]: https://nexte.st/ +//! [undefined behavior]: https://doc.rust-lang.org/reference/behavior-considered-undefined.html + +#[cfg(test)] +mod test { + use proxmox_alloc::LayoutAwareBox; + + /// Rust does not and cannot track the alignment that was used for any given + /// allocation and will instead always resort to using the alignment of + /// whatever T it is currently dropping during deallocation. + /// + /// This gets caught by Miri. + #[test] + fn demo_ub_alignment_mismatch() { + let buf_len = 1024; + let align = 128; + + let layout = std::alloc::Layout::from_size_align(buf_len, align).unwrap(); + + let thin_ptr = unsafe { std::alloc::alloc_zeroed(layout) }; + let fat_ptr = std::ptr::slice_from_raw_parts_mut(thin_ptr, buf_len); + assert!(!fat_ptr.is_null()); + + let boxed = unsafe { Box::from_raw(fat_ptr) }; + + drop(boxed); + } + + /// Rust requires that the size of a pointee is equal to the size of its + /// allocation. + /// + /// This gets caught by Miri. + #[test] + fn demo_ub_size_mismatch() { + type PayloadElement = [u8; 7]; + + #[repr(C, packed)] + struct PacketHeader { + src: [u8; 4], + dst: [u8; 4], + } + + #[repr(C, packed)] + struct Packet { + header: PacketHeader, + payload: [PayloadElement], + } + + let alloc_size = 1024; + let align = 128; + + let header_size = size_of::<PacketHeader>(); + let elem_size = size_of::<PayloadElement>(); + + let payload_len = (alloc_size - header_size) / elem_size; + + let layout = std::alloc::Layout::from_size_align(alloc_size, align).expect("infallible"); + + // SAFETY: layout's size is not 0. + let thin_ptr = unsafe { std::alloc::alloc_zeroed(layout) }; + assert!(!thin_ptr.is_null(), "thin_ptr is null -- allocation failed"); + + let fat_ptr = std::ptr::slice_from_raw_parts_mut(thin_ptr, payload_len) as *mut Packet; + + // In this case you can actually check whether the pointee's size + // differs from the allocation's size: + let pointee_size = unsafe { size_of_val(&mut *fat_ptr) }; + assert_ne!(pointee_size, alloc_size); + + // SAFETY: fat ptr is not aliased or null, is valid for reads and + // writes, and its pointee was zero-initialized. Its length is within + // the bounds of its allocation. + // WARNING: This isn't actually safe, as the example implies. + let aware_boxed = unsafe { LayoutAwareBox::from_raw_parts(fat_ptr, layout) }; + + drop(aware_boxed); + } +} diff --git a/proxmox-alloc/src/aware_boxed.rs b/proxmox-alloc/src/aware_boxed.rs index 4ec3811e..ce6005ac 100644 --- a/proxmox-alloc/src/aware_boxed.rs +++ b/proxmox-alloc/src/aware_boxed.rs @@ -49,6 +49,9 @@ //! Therefore, one must always track the [`Layout`] that was used for "exotic" //! kinds of allocations, which [`LayoutAwareBox`] will do for you. //! +//! *Hint: This crate's source code contains extra examples that you can compile +//! and run with [Miri] if you want to see the UB in action.* +//! //! # Examples //! //! Move a value from the stack on the heap, using a specific alignment for the -- 2.47.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH proxmox-backup v2 03/10] tape: move tape block structs into separate file module 2026-08-21 14:02 [PATCH proxmox{,-backup} v2 00/10] Fix Undefined Behavior in Tape Block Header Deallocation Max R. Carrara 2026-08-21 14:02 ` [PATCH proxmox v2 01/10] proxmox-alloc: introduce proxmox-alloc with `LayoutAwareBox<T>` type Max R. Carrara 2026-08-21 14:02 ` [PATCH proxmox v2 02/10] proxmox-alloc: document undefined behavior regarding custom allocs Max R. Carrara @ 2026-08-21 14:02 ` Max R. Carrara 2026-08-21 14:02 ` [PATCH proxmox-backup v2 04/10] tape: rename `BlockHeader` and `BlockHeaderFlags` Max R. Carrara ` (6 subsequent siblings) 9 siblings, 0 replies; 11+ messages in thread From: Max R. Carrara @ 2026-08-21 14:02 UTC (permalink / raw) To: pbs-devel Move `BlockHeader` and `BlockHeaderFlags` into a separate file for overall better organization and to make future changes a little easier to follow. Also, re-export both types as `pub(crate)`, since there is not really any reason for them to be completely public. Signed-off-by: Max R. Carrara <m.carrara@proxmox.com> --- pbs-tape/src/lib.rs | 79 ++------------------------------------ pbs-tape/src/tape_block.rs | 79 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 76 deletions(-) create mode 100644 pbs-tape/src/tape_block.rs diff --git a/pbs-tape/src/lib.rs b/pbs-tape/src/lib.rs index 0fe55a749..6cff175dd 100644 --- a/pbs-tape/src/lib.rs +++ b/pbs-tape/src/lib.rs @@ -1,7 +1,6 @@ use std::collections::HashSet; use anyhow::{Error, bail}; -use bitflags::bitflags; use endian_trait::Endian; use serde::{Deserialize, Serialize}; use serde_json::Value; @@ -20,6 +19,9 @@ pub use blocked_reader::BlockedReader; mod blocked_writer; pub use blocked_writer::BlockedWriter; +mod tape_block; +pub(crate) use tape_block::{BlockHeader, BlockHeaderFlags}; + mod tape_write; pub use tape_write::*; @@ -49,41 +51,6 @@ pub const PROXMOX_BACKUP_MEDIA_LABEL_MAGIC_1_0: [u8; 8] = [42, 5, 191, 60, 176, // openssl::sha::sha256(b"Proxmox Backup MediaSet Label v1.0") pub const PROXMOX_BACKUP_MEDIA_SET_LABEL_MAGIC_1_0: [u8; 8] = [8, 96, 99, 249, 47, 151, 83, 216]; -/// Tape Block Header with data payload -/// -/// All tape files are written as sequence of blocks. -/// -/// Note: this struct is large, never put this on the stack! -/// so we use an unsized type to avoid that. -/// -/// Tape data block are always read/written with a fixed size -/// (`PROXMOX_TAPE_BLOCK_SIZE`). But they may contain less data, so the -/// header has an additional size field. For streams of blocks, there -/// is a sequence number (`seq_nr`) which may be use for additional -/// error checking. -#[repr(C, packed)] -pub struct BlockHeader { - /// fixed value `PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0` - pub magic: [u8; 8], - pub flags: BlockHeaderFlags, - /// size as 3 bytes unsigned, little endian - pub size: [u8; 3], - /// block sequence number - pub seq_nr: u32, - pub payload: [u8], -} - -bitflags! { - /// Header flags (e.g. `END_OF_STREAM` or `INCOMPLETE`) - #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] - pub struct BlockHeaderFlags: u8 { - /// Marks the last block in a stream. - const END_OF_STREAM = 0b00000001; - /// Mark multivolume streams (when set in the last block) - const INCOMPLETE = 0b00000010; - } -} - #[derive(Endian, Copy, Clone, Debug)] #[repr(C, packed)] /// Media Content Header @@ -152,46 +119,6 @@ impl MediaContentHeader { } } -impl BlockHeader { - pub const SIZE: usize = PROXMOX_TAPE_BLOCK_SIZE; - - /// Allocates a new instance on the heap - pub fn new() -> Box<Self> { - use std::alloc::{Layout, alloc_zeroed}; - - // align to PAGESIZE, so that we can use it with SG_IO - let page_size = unsafe { libc::sysconf(libc::_SC_PAGESIZE) } as usize; - - let mut buffer = unsafe { - let ptr = alloc_zeroed(Layout::from_size_align(Self::SIZE, page_size).unwrap()); - Box::from_raw(core::ptr::slice_from_raw_parts_mut(ptr, Self::SIZE - 16) as *mut Self) - }; - buffer.magic = PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0; - buffer - } - - /// Set the `size` field - pub fn set_size(&mut self, size: usize) { - let size = size.to_le_bytes(); - self.size.copy_from_slice(&size[..3]); - } - - /// Returns the `size` field - pub fn size(&self) -> usize { - (self.size[0] as usize) + ((self.size[1] as usize) << 8) + ((self.size[2] as usize) << 16) - } - - /// Set the `seq_nr` field - pub fn set_seq_nr(&mut self, seq_nr: u32) { - self.seq_nr = seq_nr.to_le(); - } - - /// Returns the `seq_nr` field - pub fn seq_nr(&self) -> u32 { - u32::from_le(self.seq_nr) - } -} - /// Changer element status. /// /// Drive and slots may be `Empty`, or contain some media, either diff --git a/pbs-tape/src/tape_block.rs b/pbs-tape/src/tape_block.rs new file mode 100644 index 000000000..4f9c2192f --- /dev/null +++ b/pbs-tape/src/tape_block.rs @@ -0,0 +1,79 @@ +use bitflags::bitflags; + +use crate::PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0; +use crate::PROXMOX_TAPE_BLOCK_SIZE; + +/// Tape Block Header with data payload +/// +/// All tape files are written as sequence of blocks. +/// +/// Note: this struct is large, never put this on the stack! +/// so we use an unsized type to avoid that. +/// +/// Tape data block are always read/written with a fixed size +/// (`PROXMOX_TAPE_BLOCK_SIZE`). But they may contain less data, so the +/// header has an additional size field. For streams of blocks, there +/// is a sequence number (`seq_nr`) which may be use for additional +/// error checking. +#[repr(C, packed)] +pub struct BlockHeader { + /// fixed value `PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0` + pub magic: [u8; 8], + pub flags: BlockHeaderFlags, + /// size as 3 bytes unsigned, little endian + pub size: [u8; 3], + /// block sequence number + pub seq_nr: u32, + pub payload: [u8], +} + +bitflags! { + /// Header flags (e.g. `END_OF_STREAM` or `INCOMPLETE`) + #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] + pub struct BlockHeaderFlags: u8 { + /// Marks the last block in a stream. + const END_OF_STREAM = 0b00000001; + /// Mark multivolume streams (when set in the last block) + const INCOMPLETE = 0b00000010; + } +} + +impl BlockHeader { + pub const SIZE: usize = PROXMOX_TAPE_BLOCK_SIZE; + + /// Allocates a new instance on the heap + pub fn new() -> Box<Self> { + use std::alloc::{Layout, alloc_zeroed}; + + // align to PAGESIZE, so that we can use it with SG_IO + let page_size = unsafe { libc::sysconf(libc::_SC_PAGESIZE) } as usize; + + let mut buffer = unsafe { + let ptr = alloc_zeroed(Layout::from_size_align(Self::SIZE, page_size).unwrap()); + Box::from_raw(core::ptr::slice_from_raw_parts_mut(ptr, Self::SIZE - 16) as *mut Self) + }; + buffer.magic = PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0; + buffer + } + + /// Set the `size` field + pub fn set_size(&mut self, size: usize) { + let size = size.to_le_bytes(); + self.size.copy_from_slice(&size[..3]); + } + + /// Returns the `size` field + pub fn size(&self) -> usize { + (self.size[0] as usize) + ((self.size[1] as usize) << 8) + ((self.size[2] as usize) << 16) + } + + /// Set the `seq_nr` field + pub fn set_seq_nr(&mut self, seq_nr: u32) { + self.seq_nr = seq_nr.to_le(); + } + + /// Returns the `seq_nr` field + pub fn seq_nr(&self) -> u32 { + u32::from_le(self.seq_nr) + } +} -- 2.47.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH proxmox-backup v2 04/10] tape: rename `BlockHeader` and `BlockHeaderFlags` 2026-08-21 14:02 [PATCH proxmox{,-backup} v2 00/10] Fix Undefined Behavior in Tape Block Header Deallocation Max R. Carrara ` (2 preceding siblings ...) 2026-08-21 14:02 ` [PATCH proxmox-backup v2 03/10] tape: move tape block structs into separate file module Max R. Carrara @ 2026-08-21 14:02 ` Max R. Carrara 2026-08-21 14:02 ` [PATCH proxmox-backup v2 05/10] tape: blocked_{reader,writer}: rename `buffer` to `tape_block` Max R. Carrara ` (5 subsequent siblings) 9 siblings, 0 replies; 11+ messages in thread From: Max R. Carrara @ 2026-08-21 14:02 UTC (permalink / raw) To: pbs-devel ... to `TapeBlock` and `TapeBlockFlags`, since `BlockHeader` is a bit of a misnomer -- each tape block has a header followed by a data payload, so it makes sense to just name it after what it is. Also adapt the docstring for `BlockHeader`. Signed-off-by: Max R. Carrara <m.carrara@proxmox.com> --- pbs-tape/src/blocked_reader.rs | 23 ++++++++++------------- pbs-tape/src/blocked_writer.rs | 23 ++++++++++------------- pbs-tape/src/lib.rs | 2 +- pbs-tape/src/tape_block.rs | 23 +++++++++++------------ 4 files changed, 32 insertions(+), 39 deletions(-) diff --git a/pbs-tape/src/blocked_reader.rs b/pbs-tape/src/blocked_reader.rs index 22803371c..3e3f7095c 100644 --- a/pbs-tape/src/blocked_reader.rs +++ b/pbs-tape/src/blocked_reader.rs @@ -1,7 +1,7 @@ use std::io::Read; use crate::{ - BlockHeader, BlockHeaderFlags, BlockRead, BlockReadError, PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0, + BlockRead, BlockReadError, PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0, TapeBlock, TapeBlockFlags, TapeRead, }; @@ -18,7 +18,7 @@ use crate::{ /// the end of the stream). pub struct BlockedReader<R> { reader: R, - buffer: Box<BlockHeader>, + buffer: Box<TapeBlock>, seq_nr: u32, found_end_marker: bool, incomplete: bool, @@ -33,7 +33,7 @@ impl<R: BlockRead> BlockedReader<R> { /// This tries to read the first block. Please inspect the error /// to detect EOF and EOT. pub fn open(mut reader: R) -> Result<Self, BlockReadError> { - let mut buffer = BlockHeader::new(); + let mut buffer = TapeBlock::new(); Self::read_block_frame(&mut buffer, &mut reader)?; @@ -43,7 +43,7 @@ impl<R: BlockRead> BlockedReader<R> { let mut got_eod = false; if found_end_marker { - incomplete = buffer.flags.contains(BlockHeaderFlags::INCOMPLETE); + incomplete = buffer.flags.contains(TapeBlockFlags::INCOMPLETE); Self::consume_eof_marker(&mut reader)?; got_eod = true; } @@ -60,7 +60,7 @@ impl<R: BlockRead> BlockedReader<R> { }) } - fn check_buffer(buffer: &BlockHeader, seq_nr: u32) -> Result<(usize, bool), std::io::Error> { + fn check_buffer(buffer: &TapeBlock, seq_nr: u32) -> Result<(usize, bool), std::io::Error> { if buffer.magic != PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0 { proxmox_lang::io_bail!( "got tape block with unknown magic number - not written by PBS or incompatible LTO version" @@ -76,7 +76,7 @@ impl<R: BlockRead> BlockedReader<R> { } let size = buffer.size(); - let found_end_marker = buffer.flags.contains(BlockHeaderFlags::END_OF_STREAM); + let found_end_marker = buffer.flags.contains(TapeBlockFlags::END_OF_STREAM); if size > buffer.payload.len() { proxmox_lang::io_bail!( @@ -91,17 +91,14 @@ impl<R: BlockRead> BlockedReader<R> { Ok((size, found_end_marker)) } - fn read_block_frame(buffer: &mut BlockHeader, reader: &mut R) -> Result<(), BlockReadError> { + fn read_block_frame(buffer: &mut TapeBlock, reader: &mut R) -> Result<(), BlockReadError> { let data = unsafe { - std::slice::from_raw_parts_mut( - (buffer as *mut BlockHeader) as *mut u8, - BlockHeader::SIZE, - ) + std::slice::from_raw_parts_mut((buffer as *mut TapeBlock) as *mut u8, TapeBlock::SIZE) }; let bytes = reader.read_block(data)?; - if bytes != BlockHeader::SIZE { + if bytes != TapeBlock::SIZE { return Err(proxmox_lang::io_format_err!("got wrong block size").into()); } @@ -147,7 +144,7 @@ impl<R: BlockRead> BlockedReader<R> { if found_end_marker { // consume EOF mark self.found_end_marker = true; - self.incomplete = self.buffer.flags.contains(BlockHeaderFlags::INCOMPLETE); + self.incomplete = self.buffer.flags.contains(TapeBlockFlags::INCOMPLETE); Self::consume_eof_marker(&mut self.reader)?; self.got_eod = true; } diff --git a/pbs-tape/src/blocked_writer.rs b/pbs-tape/src/blocked_writer.rs index 7380af243..9aba7b832 100644 --- a/pbs-tape/src/blocked_writer.rs +++ b/pbs-tape/src/blocked_writer.rs @@ -1,6 +1,6 @@ use proxmox_io::vec; -use crate::{BlockHeader, BlockHeaderFlags, BlockWrite, TapeWrite}; +use crate::{BlockWrite, TapeBlock, TapeBlockFlags, TapeWrite}; /// Assemble and write blocks of data /// @@ -9,7 +9,7 @@ use crate::{BlockHeader, BlockHeaderFlags, BlockWrite, TapeWrite}; /// to the underlying writer. pub struct BlockedWriter<W: BlockWrite> { writer: W, - buffer: Box<BlockHeader>, + buffer: Box<TapeBlock>, buffer_pos: usize, seq_nr: u32, logical_end_of_media: bool, @@ -36,7 +36,7 @@ impl<W: BlockWrite> BlockedWriter<W> { pub fn new(writer: W) -> Self { Self { writer, - buffer: BlockHeader::new(), + buffer: TapeBlock::new(), buffer_pos: 0, seq_nr: 0, logical_end_of_media: false, @@ -45,12 +45,9 @@ impl<W: BlockWrite> BlockedWriter<W> { } } - fn write_block(buffer: &BlockHeader, writer: &mut W) -> Result<bool, std::io::Error> { + fn write_block(buffer: &TapeBlock, writer: &mut W) -> Result<bool, std::io::Error> { let data = unsafe { - std::slice::from_raw_parts( - (buffer as *const BlockHeader) as *const u8, - BlockHeader::SIZE, - ) + std::slice::from_raw_parts((buffer as *const TapeBlock) as *const u8, TapeBlock::SIZE) }; writer.write_block(data) } @@ -77,7 +74,7 @@ impl<W: BlockWrite> BlockedWriter<W> { let rest = rest - bytes; if rest == 0 { - self.buffer.flags = BlockHeaderFlags::empty(); + self.buffer.flags = TapeBlockFlags::empty(); self.buffer.set_size(self.buffer.payload.len()); self.buffer.set_seq_nr(self.seq_nr); self.seq_nr += 1; @@ -86,7 +83,7 @@ impl<W: BlockWrite> BlockedWriter<W> { self.logical_end_of_media = true; } self.buffer_pos = 0; - self.bytes_written += BlockHeader::SIZE; + self.bytes_written += TapeBlock::SIZE; } else { self.buffer_pos += bytes; } @@ -116,14 +113,14 @@ impl<W: BlockWrite> TapeWrite for BlockedWriter<W> { /// END_OF_STREAM flag. fn finish(&mut self, incomplete: bool) -> Result<bool, std::io::Error> { vec::clear(&mut self.buffer.payload[self.buffer_pos..]); - self.buffer.flags = BlockHeaderFlags::END_OF_STREAM; + self.buffer.flags = TapeBlockFlags::END_OF_STREAM; if incomplete { - self.buffer.flags |= BlockHeaderFlags::INCOMPLETE; + self.buffer.flags |= TapeBlockFlags::INCOMPLETE; } self.buffer.set_size(self.buffer_pos); self.buffer.set_seq_nr(self.seq_nr); self.seq_nr += 1; - self.bytes_written += BlockHeader::SIZE; + self.bytes_written += TapeBlock::SIZE; let leom = Self::write_block(&self.buffer, &mut self.writer)?; self.write_eof()?; Ok(leom) diff --git a/pbs-tape/src/lib.rs b/pbs-tape/src/lib.rs index 6cff175dd..1b6dc94cc 100644 --- a/pbs-tape/src/lib.rs +++ b/pbs-tape/src/lib.rs @@ -20,7 +20,7 @@ mod blocked_writer; pub use blocked_writer::BlockedWriter; mod tape_block; -pub(crate) use tape_block::{BlockHeader, BlockHeaderFlags}; +pub(crate) use tape_block::{TapeBlock, TapeBlockFlags}; mod tape_write; pub use tape_write::*; diff --git a/pbs-tape/src/tape_block.rs b/pbs-tape/src/tape_block.rs index 4f9c2192f..4ab1f5ce9 100644 --- a/pbs-tape/src/tape_block.rs +++ b/pbs-tape/src/tape_block.rs @@ -3,23 +3,22 @@ use bitflags::bitflags; use crate::PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0; use crate::PROXMOX_TAPE_BLOCK_SIZE; -/// Tape Block Header with data payload +/// A [`TapeBlock`] consists of a tape header followed by a data payload. /// /// All tape files are written as sequence of blocks. /// -/// Note: this struct is large, never put this on the stack! -/// so we use an unsized type to avoid that. +/// Note: This struct is a dynamically sized type and can therefore only ever +/// exist as a heap-allocated value. /// -/// Tape data block are always read/written with a fixed size -/// (`PROXMOX_TAPE_BLOCK_SIZE`). But they may contain less data, so the -/// header has an additional size field. For streams of blocks, there -/// is a sequence number (`seq_nr`) which may be use for additional -/// error checking. +/// Tape blocks are always read/written with a fixed size +/// (`PROXMOX_TAPE_BLOCK_SIZE`). However, since they may contain less data, the +/// header has an additional size field. For streams of blocks, there is a +/// sequence number (`seq_nr`) which may be used for additional error checking. #[repr(C, packed)] -pub struct BlockHeader { +pub struct TapeBlock { /// fixed value `PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0` pub magic: [u8; 8], - pub flags: BlockHeaderFlags, + pub flags: TapeBlockFlags, /// size as 3 bytes unsigned, little endian pub size: [u8; 3], /// block sequence number @@ -30,7 +29,7 @@ pub struct BlockHeader { bitflags! { /// Header flags (e.g. `END_OF_STREAM` or `INCOMPLETE`) #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] - pub struct BlockHeaderFlags: u8 { + pub struct TapeBlockFlags: u8 { /// Marks the last block in a stream. const END_OF_STREAM = 0b00000001; /// Mark multivolume streams (when set in the last block) @@ -38,7 +37,7 @@ bitflags! { } } -impl BlockHeader { +impl TapeBlock { pub const SIZE: usize = PROXMOX_TAPE_BLOCK_SIZE; /// Allocates a new instance on the heap -- 2.47.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH proxmox-backup v2 05/10] tape: blocked_{reader,writer}: rename `buffer` to `tape_block` 2026-08-21 14:02 [PATCH proxmox{,-backup} v2 00/10] Fix Undefined Behavior in Tape Block Header Deallocation Max R. Carrara ` (3 preceding siblings ...) 2026-08-21 14:02 ` [PATCH proxmox-backup v2 04/10] tape: rename `BlockHeader` and `BlockHeaderFlags` Max R. Carrara @ 2026-08-21 14:02 ` Max R. Carrara 2026-08-21 14:02 ` [PATCH proxmox-backup v2 06/10] tape: tape block: represent tape block header with its own struct Max R. Carrara ` (4 subsequent siblings) 9 siblings, 0 replies; 11+ messages in thread From: Max R. Carrara @ 2026-08-21 14:02 UTC (permalink / raw) To: pbs-devel ... since a tape block does not really represent a plain buffer. Also rename any private functions mentioning `buffer` along the way. Signed-off-by: Max R. Carrara <m.carrara@proxmox.com> --- pbs-tape/src/blocked_reader.rs | 63 +++++++++++++++++++--------------- pbs-tape/src/blocked_writer.rs | 35 ++++++++++--------- 2 files changed, 54 insertions(+), 44 deletions(-) diff --git a/pbs-tape/src/blocked_reader.rs b/pbs-tape/src/blocked_reader.rs index 3e3f7095c..ef3f8d217 100644 --- a/pbs-tape/src/blocked_reader.rs +++ b/pbs-tape/src/blocked_reader.rs @@ -18,7 +18,7 @@ use crate::{ /// the end of the stream). pub struct BlockedReader<R> { reader: R, - buffer: Box<TapeBlock>, + tape_block: Box<TapeBlock>, seq_nr: u32, found_end_marker: bool, incomplete: bool, @@ -33,24 +33,24 @@ impl<R: BlockRead> BlockedReader<R> { /// This tries to read the first block. Please inspect the error /// to detect EOF and EOT. pub fn open(mut reader: R) -> Result<Self, BlockReadError> { - let mut buffer = TapeBlock::new(); + let mut tape_block = TapeBlock::new(); - Self::read_block_frame(&mut buffer, &mut reader)?; + Self::read_block_frame(&mut tape_block, &mut reader)?; - let (_size, found_end_marker) = Self::check_buffer(&buffer, 0)?; + let (_size, found_end_marker) = Self::check_tape_block(&tape_block, 0)?; let mut incomplete = false; let mut got_eod = false; if found_end_marker { - incomplete = buffer.flags.contains(TapeBlockFlags::INCOMPLETE); + incomplete = tape_block.flags.contains(TapeBlockFlags::INCOMPLETE); Self::consume_eof_marker(&mut reader)?; got_eod = true; } Ok(Self { reader, - buffer, + tape_block, found_end_marker, incomplete, got_eod, @@ -60,29 +60,32 @@ impl<R: BlockRead> BlockedReader<R> { }) } - fn check_buffer(buffer: &TapeBlock, seq_nr: u32) -> Result<(usize, bool), std::io::Error> { - if buffer.magic != PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0 { + fn check_tape_block( + tape_block: &TapeBlock, + seq_nr: u32, + ) -> Result<(usize, bool), std::io::Error> { + if tape_block.magic != PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0 { proxmox_lang::io_bail!( "got tape block with unknown magic number - not written by PBS or incompatible LTO version" ); } - if seq_nr != buffer.seq_nr() { + if seq_nr != tape_block.seq_nr() { proxmox_lang::io_bail!( "detected tape block with wrong sequence number ({} != {})", seq_nr, - buffer.seq_nr() + tape_block.seq_nr() ) } - let size = buffer.size(); - let found_end_marker = buffer.flags.contains(TapeBlockFlags::END_OF_STREAM); + let size = tape_block.size(); + let found_end_marker = tape_block.flags.contains(TapeBlockFlags::END_OF_STREAM); - if size > buffer.payload.len() { + if size > tape_block.payload.len() { proxmox_lang::io_bail!( "detected tape block with wrong payload size ({} > {}", size, - buffer.payload.len() + tape_block.payload.len() ); } else if size == 0 && !found_end_marker { proxmox_lang::io_bail!("detected tape block with zero payload size"); @@ -91,9 +94,12 @@ impl<R: BlockRead> BlockedReader<R> { Ok((size, found_end_marker)) } - fn read_block_frame(buffer: &mut TapeBlock, reader: &mut R) -> Result<(), BlockReadError> { + fn read_block_frame(tape_block: &mut TapeBlock, reader: &mut R) -> Result<(), BlockReadError> { let data = unsafe { - std::slice::from_raw_parts_mut((buffer as *mut TapeBlock) as *mut u8, TapeBlock::SIZE) + std::slice::from_raw_parts_mut( + (tape_block as *mut TapeBlock) as *mut u8, + TapeBlock::SIZE, + ) }; let bytes = reader.read_block(data)?; @@ -120,11 +126,11 @@ impl<R: BlockRead> BlockedReader<R> { } fn read_block(&mut self, check_end_marker: bool) -> Result<usize, std::io::Error> { - match Self::read_block_frame(&mut self.buffer, &mut self.reader) { + match Self::read_block_frame(&mut self.tape_block, &mut self.reader) { Ok(()) => { /* ok */ } Err(BlockReadError::EndOfFile) => { self.got_eod = true; - self.read_pos = self.buffer.payload.len(); + self.read_pos = self.tape_block.payload.len(); if !self.found_end_marker && check_end_marker { proxmox_lang::io_bail!("detected tape stream without end marker"); } @@ -138,13 +144,13 @@ impl<R: BlockRead> BlockedReader<R> { } } - let (size, found_end_marker) = Self::check_buffer(&self.buffer, self.seq_nr)?; + let (size, found_end_marker) = Self::check_tape_block(&self.tape_block, self.seq_nr)?; self.seq_nr += 1; if found_end_marker { // consume EOF mark self.found_end_marker = true; - self.incomplete = self.buffer.flags.contains(TapeBlockFlags::INCOMPLETE); + self.incomplete = self.tape_block.flags.contains(TapeBlockFlags::INCOMPLETE); Self::consume_eof_marker(&mut self.reader)?; self.got_eod = true; } @@ -179,8 +185,8 @@ impl<R: BlockRead> TapeRead for BlockedReader<R> { // stream has no end marker. fn skip_data(&mut self) -> Result<usize, std::io::Error> { let mut bytes = 0; - let buffer_size = self.buffer.size(); - let rest = (buffer_size as isize) - (self.read_pos as isize); + let tape_block_size = self.tape_block.size(); + let rest = (tape_block_size as isize) - (self.read_pos as isize); if rest > 0 { bytes = rest as usize; } @@ -199,19 +205,19 @@ impl<R: BlockRead> Read for BlockedReader<R> { proxmox_lang::io_bail!("detected read after error - internal error"); } - let mut buffer_size = self.buffer.size(); - let mut rest = (buffer_size as isize) - (self.read_pos as isize); + let mut tape_block_size = self.tape_block.size(); + let mut rest = (tape_block_size as isize) - (self.read_pos as isize); if rest <= 0 && !self.got_eod { // try to refill buffer - buffer_size = match self.read_block(true) { + tape_block_size = match self.read_block(true) { Ok(len) => len, err => { self.read_error = true; return err; } }; - rest = buffer_size as isize; + rest = tape_block_size as isize; } if rest <= 0 { @@ -222,8 +228,9 @@ impl<R: BlockRead> Read for BlockedReader<R> { } else { rest as usize }; - buffer[..copy_len] - .copy_from_slice(&self.buffer.payload[self.read_pos..(self.read_pos + copy_len)]); + buffer[..copy_len].copy_from_slice( + &self.tape_block.payload[self.read_pos..(self.read_pos + copy_len)], + ); self.read_pos += copy_len; Ok(copy_len) } diff --git a/pbs-tape/src/blocked_writer.rs b/pbs-tape/src/blocked_writer.rs index 9aba7b832..9aab0cbb0 100644 --- a/pbs-tape/src/blocked_writer.rs +++ b/pbs-tape/src/blocked_writer.rs @@ -9,7 +9,7 @@ use crate::{BlockWrite, TapeBlock, TapeBlockFlags, TapeWrite}; /// to the underlying writer. pub struct BlockedWriter<W: BlockWrite> { writer: W, - buffer: Box<TapeBlock>, + tape_block: Box<TapeBlock>, buffer_pos: usize, seq_nr: u32, logical_end_of_media: bool, @@ -36,7 +36,7 @@ impl<W: BlockWrite> BlockedWriter<W> { pub fn new(writer: W) -> Self { Self { writer, - buffer: TapeBlock::new(), + tape_block: TapeBlock::new(), buffer_pos: 0, seq_nr: 0, logical_end_of_media: false, @@ -45,9 +45,12 @@ impl<W: BlockWrite> BlockedWriter<W> { } } - fn write_block(buffer: &TapeBlock, writer: &mut W) -> Result<bool, std::io::Error> { + fn write_block(tape_block: &TapeBlock, writer: &mut W) -> Result<bool, std::io::Error> { let data = unsafe { - std::slice::from_raw_parts((buffer as *const TapeBlock) as *const u8, TapeBlock::SIZE) + std::slice::from_raw_parts( + (tape_block as *const TapeBlock) as *const u8, + TapeBlock::SIZE, + ) }; writer.write_block(data) } @@ -66,19 +69,19 @@ impl<W: BlockWrite> BlockedWriter<W> { return Ok(0); } - let rest = self.buffer.payload.len() - self.buffer_pos; + let rest = self.tape_block.payload.len() - self.buffer_pos; let bytes = if data.len() < rest { data.len() } else { rest }; - self.buffer.payload[self.buffer_pos..(self.buffer_pos + bytes)] + self.tape_block.payload[self.buffer_pos..(self.buffer_pos + bytes)] .copy_from_slice(&data[..bytes]); let rest = rest - bytes; if rest == 0 { - self.buffer.flags = TapeBlockFlags::empty(); - self.buffer.set_size(self.buffer.payload.len()); - self.buffer.set_seq_nr(self.seq_nr); + self.tape_block.flags = TapeBlockFlags::empty(); + self.tape_block.set_size(self.tape_block.payload.len()); + self.tape_block.set_seq_nr(self.seq_nr); self.seq_nr += 1; - let leom = Self::write_block(&self.buffer, &mut self.writer)?; + let leom = Self::write_block(&self.tape_block, &mut self.writer)?; if leom { self.logical_end_of_media = true; } @@ -112,16 +115,16 @@ impl<W: BlockWrite> TapeWrite for BlockedWriter<W> { /// Note: This may write an empty block just including the /// END_OF_STREAM flag. fn finish(&mut self, incomplete: bool) -> Result<bool, std::io::Error> { - vec::clear(&mut self.buffer.payload[self.buffer_pos..]); - self.buffer.flags = TapeBlockFlags::END_OF_STREAM; + vec::clear(&mut self.tape_block.payload[self.buffer_pos..]); + self.tape_block.flags = TapeBlockFlags::END_OF_STREAM; if incomplete { - self.buffer.flags |= TapeBlockFlags::INCOMPLETE; + self.tape_block.flags |= TapeBlockFlags::INCOMPLETE; } - self.buffer.set_size(self.buffer_pos); - self.buffer.set_seq_nr(self.seq_nr); + self.tape_block.set_size(self.buffer_pos); + self.tape_block.set_seq_nr(self.seq_nr); self.seq_nr += 1; self.bytes_written += TapeBlock::SIZE; - let leom = Self::write_block(&self.buffer, &mut self.writer)?; + let leom = Self::write_block(&self.tape_block, &mut self.writer)?; self.write_eof()?; Ok(leom) } -- 2.47.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH proxmox-backup v2 06/10] tape: tape block: represent tape block header with its own struct 2026-08-21 14:02 [PATCH proxmox{,-backup} v2 00/10] Fix Undefined Behavior in Tape Block Header Deallocation Max R. Carrara ` (4 preceding siblings ...) 2026-08-21 14:02 ` [PATCH proxmox-backup v2 05/10] tape: blocked_{reader,writer}: rename `buffer` to `tape_block` Max R. Carrara @ 2026-08-21 14:02 ` Max R. Carrara 2026-08-21 14:02 ` [PATCH proxmox-backup v2 07/10] tape: tape block: make `payload` field private Max R. Carrara ` (3 subsequent siblings) 9 siblings, 0 replies; 11+ messages in thread From: Max R. Carrara @ 2026-08-21 14:02 UTC (permalink / raw) To: pbs-devel Instead of defining the tape block header's fields inline, move them into a new struct called `TapeBlockHeader` and adapt existing methods correspondingly. Introduce methods for any fields that lacked them in the first place so that they can be accessed again. Add a static assertion that ensures that `TapeBlockHeader` always has a size of 16, since its size is not allowed to change. Finally, adapt sites that used direct field access for any header fields to use each field's respective method. Signed-off-by: Max R. Carrara <m.carrara@proxmox.com> --- pbs-tape/src/blocked_reader.rs | 8 ++-- pbs-tape/src/blocked_writer.rs | 7 ++-- pbs-tape/src/tape_block.rs | 76 ++++++++++++++++++++++++---------- 3 files changed, 62 insertions(+), 29 deletions(-) diff --git a/pbs-tape/src/blocked_reader.rs b/pbs-tape/src/blocked_reader.rs index ef3f8d217..cf8150ba3 100644 --- a/pbs-tape/src/blocked_reader.rs +++ b/pbs-tape/src/blocked_reader.rs @@ -43,7 +43,7 @@ impl<R: BlockRead> BlockedReader<R> { let mut got_eod = false; if found_end_marker { - incomplete = tape_block.flags.contains(TapeBlockFlags::INCOMPLETE); + incomplete = tape_block.flags().contains(TapeBlockFlags::INCOMPLETE); Self::consume_eof_marker(&mut reader)?; got_eod = true; } @@ -64,7 +64,7 @@ impl<R: BlockRead> BlockedReader<R> { tape_block: &TapeBlock, seq_nr: u32, ) -> Result<(usize, bool), std::io::Error> { - if tape_block.magic != PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0 { + if tape_block.magic() != PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0 { proxmox_lang::io_bail!( "got tape block with unknown magic number - not written by PBS or incompatible LTO version" ); @@ -79,7 +79,7 @@ impl<R: BlockRead> BlockedReader<R> { } let size = tape_block.size(); - let found_end_marker = tape_block.flags.contains(TapeBlockFlags::END_OF_STREAM); + let found_end_marker = tape_block.flags().contains(TapeBlockFlags::END_OF_STREAM); if size > tape_block.payload.len() { proxmox_lang::io_bail!( @@ -150,7 +150,7 @@ impl<R: BlockRead> BlockedReader<R> { if found_end_marker { // consume EOF mark self.found_end_marker = true; - self.incomplete = self.tape_block.flags.contains(TapeBlockFlags::INCOMPLETE); + self.incomplete = self.tape_block.flags().contains(TapeBlockFlags::INCOMPLETE); Self::consume_eof_marker(&mut self.reader)?; self.got_eod = true; } diff --git a/pbs-tape/src/blocked_writer.rs b/pbs-tape/src/blocked_writer.rs index 9aab0cbb0..5e94f54b8 100644 --- a/pbs-tape/src/blocked_writer.rs +++ b/pbs-tape/src/blocked_writer.rs @@ -77,7 +77,7 @@ impl<W: BlockWrite> BlockedWriter<W> { let rest = rest - bytes; if rest == 0 { - self.tape_block.flags = TapeBlockFlags::empty(); + self.tape_block.set_flags(TapeBlockFlags::empty()); self.tape_block.set_size(self.tape_block.payload.len()); self.tape_block.set_seq_nr(self.seq_nr); self.seq_nr += 1; @@ -116,10 +116,11 @@ impl<W: BlockWrite> TapeWrite for BlockedWriter<W> { /// END_OF_STREAM flag. fn finish(&mut self, incomplete: bool) -> Result<bool, std::io::Error> { vec::clear(&mut self.tape_block.payload[self.buffer_pos..]); - self.tape_block.flags = TapeBlockFlags::END_OF_STREAM; + let mut flags = TapeBlockFlags::END_OF_STREAM; if incomplete { - self.tape_block.flags |= TapeBlockFlags::INCOMPLETE; + flags |= TapeBlockFlags::INCOMPLETE; } + self.tape_block.set_flags(flags); self.tape_block.set_size(self.buffer_pos); self.tape_block.set_seq_nr(self.seq_nr); self.seq_nr += 1; diff --git a/pbs-tape/src/tape_block.rs b/pbs-tape/src/tape_block.rs index 4ab1f5ce9..2db5ed137 100644 --- a/pbs-tape/src/tape_block.rs +++ b/pbs-tape/src/tape_block.rs @@ -1,8 +1,24 @@ use bitflags::bitflags; +use proxmox_lang::static_assert_size; + use crate::PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0; use crate::PROXMOX_TAPE_BLOCK_SIZE; +#[repr(C, packed)] +struct TapeBlockHeader { + /// Fixed value: `PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0` + magic: [u8; 8], + /// See [`TapeBlockFlags`]. + flags: TapeBlockFlags, + /// Size as 3 bytes unsigned, little endian. + size: [u8; 3], + /// Block sequence number. + seq_nr: u32, +} + +static_assert_size!(TapeBlockHeader, 16); + /// A [`TapeBlock`] consists of a tape header followed by a data payload. /// /// All tape files are written as sequence of blocks. @@ -16,13 +32,7 @@ use crate::PROXMOX_TAPE_BLOCK_SIZE; /// sequence number (`seq_nr`) which may be used for additional error checking. #[repr(C, packed)] pub struct TapeBlock { - /// fixed value `PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0` - pub magic: [u8; 8], - pub flags: TapeBlockFlags, - /// size as 3 bytes unsigned, little endian - pub size: [u8; 3], - /// block sequence number - pub seq_nr: u32, + header: TapeBlockHeader, pub payload: [u8], } @@ -51,28 +61,50 @@ impl TapeBlock { let ptr = alloc_zeroed(Layout::from_size_align(Self::SIZE, page_size).unwrap()); Box::from_raw(core::ptr::slice_from_raw_parts_mut(ptr, Self::SIZE - 16) as *mut Self) }; - buffer.magic = PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0; + buffer.header.magic = PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0; buffer } - /// Set the `size` field + /// Returns the magic value of this tape block's header. + pub fn magic(&self) -> [u8; 8] { + self.header.magic + } + + /// Returns the [`TapeBlockFlags`] currently set. + pub fn flags(&self) -> TapeBlockFlags { + self.header.flags + } + + /// Sets new [`TapeBlockFlags`]. + pub fn set_flags(&mut self, flags: TapeBlockFlags) { + self.header.flags = flags; + } + + /// Returns the size of the tape block's data. + /// + /// Note that this value is at most `2^24 - 1`, since the size is + /// represented using 24 bits under the hood. + pub fn size(&self) -> usize { + let raw_size = self.header.size; + (raw_size[0] as usize) + ((raw_size[1] as usize) << 8) + ((raw_size[2] as usize) << 16) + } + + /// Sets the size of the tape block's data. + /// + /// Note that the passed value will be truncated to 24 bits, since the size + /// is represented using 24 bits under the hood. pub fn set_size(&mut self, size: usize) { let size = size.to_le_bytes(); - self.size.copy_from_slice(&size[..3]); + self.header.size.copy_from_slice(&size[..3]); } - /// Returns the `size` field - pub fn size(&self) -> usize { - (self.size[0] as usize) + ((self.size[1] as usize) << 8) + ((self.size[2] as usize) << 16) - } - - /// Set the `seq_nr` field - pub fn set_seq_nr(&mut self, seq_nr: u32) { - self.seq_nr = seq_nr.to_le(); - } - - /// Returns the `seq_nr` field + /// Returns the sequence number of the tape block. pub fn seq_nr(&self) -> u32 { - u32::from_le(self.seq_nr) + u32::from_le(self.header.seq_nr) + } + + /// Sets the tape block's sequence number. + pub fn set_seq_nr(&mut self, seq_nr: u32) { + self.header.seq_nr = seq_nr.to_le(); } } -- 2.47.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH proxmox-backup v2 07/10] tape: tape block: make `payload` field private 2026-08-21 14:02 [PATCH proxmox{,-backup} v2 00/10] Fix Undefined Behavior in Tape Block Header Deallocation Max R. Carrara ` (5 preceding siblings ...) 2026-08-21 14:02 ` [PATCH proxmox-backup v2 06/10] tape: tape block: represent tape block header with its own struct Max R. Carrara @ 2026-08-21 14:02 ` Max R. Carrara 2026-08-21 14:02 ` [PATCH proxmox-backup v2 08/10] tape: blocked_{reader,writer}: remove haphazard `unsafe` blocks Max R. Carrara ` (2 subsequent siblings) 9 siblings, 0 replies; 11+ messages in thread From: Max R. Carrara @ 2026-08-21 14:02 UTC (permalink / raw) To: pbs-devel While all cases where we directly use the `payload` field are fine, direct field access should IMO be used as sparingly as possible, as the field *itself* is otherwise part of a type's public API. Therefore, make the `payload` field of `TapeBlock` private and add corresponding getters to allow obtaining it as a (mutable) slice. Signed-off-by: Max R. Carrara <m.carrara@proxmox.com> --- pbs-tape/src/blocked_reader.rs | 11 +++++------ pbs-tape/src/blocked_writer.rs | 13 ++++++++----- pbs-tape/src/tape_block.rs | 12 +++++++++++- 3 files changed, 24 insertions(+), 12 deletions(-) diff --git a/pbs-tape/src/blocked_reader.rs b/pbs-tape/src/blocked_reader.rs index cf8150ba3..6b1e3c692 100644 --- a/pbs-tape/src/blocked_reader.rs +++ b/pbs-tape/src/blocked_reader.rs @@ -81,11 +81,11 @@ impl<R: BlockRead> BlockedReader<R> { let size = tape_block.size(); let found_end_marker = tape_block.flags().contains(TapeBlockFlags::END_OF_STREAM); - if size > tape_block.payload.len() { + if size > tape_block.payload().len() { proxmox_lang::io_bail!( "detected tape block with wrong payload size ({} > {}", size, - tape_block.payload.len() + tape_block.payload().len() ); } else if size == 0 && !found_end_marker { proxmox_lang::io_bail!("detected tape block with zero payload size"); @@ -130,7 +130,7 @@ impl<R: BlockRead> BlockedReader<R> { Ok(()) => { /* ok */ } Err(BlockReadError::EndOfFile) => { self.got_eod = true; - self.read_pos = self.tape_block.payload.len(); + self.read_pos = self.tape_block.payload().len(); if !self.found_end_marker && check_end_marker { proxmox_lang::io_bail!("detected tape stream without end marker"); } @@ -228,9 +228,8 @@ impl<R: BlockRead> Read for BlockedReader<R> { } else { rest as usize }; - buffer[..copy_len].copy_from_slice( - &self.tape_block.payload[self.read_pos..(self.read_pos + copy_len)], - ); + let payload = self.tape_block.payload(); + buffer[..copy_len].copy_from_slice(&payload[self.read_pos..(self.read_pos + copy_len)]); self.read_pos += copy_len; Ok(copy_len) } diff --git a/pbs-tape/src/blocked_writer.rs b/pbs-tape/src/blocked_writer.rs index 5e94f54b8..0d5d10147 100644 --- a/pbs-tape/src/blocked_writer.rs +++ b/pbs-tape/src/blocked_writer.rs @@ -69,16 +69,18 @@ impl<W: BlockWrite> BlockedWriter<W> { return Ok(0); } - let rest = self.tape_block.payload.len() - self.buffer_pos; + let payload = self.tape_block.payload_mut(); + let payload_len = payload.len(); + + let rest = payload_len - self.buffer_pos; let bytes = if data.len() < rest { data.len() } else { rest }; - self.tape_block.payload[self.buffer_pos..(self.buffer_pos + bytes)] - .copy_from_slice(&data[..bytes]); + payload[self.buffer_pos..(self.buffer_pos + bytes)].copy_from_slice(&data[..bytes]); let rest = rest - bytes; if rest == 0 { self.tape_block.set_flags(TapeBlockFlags::empty()); - self.tape_block.set_size(self.tape_block.payload.len()); + self.tape_block.set_size(payload_len); self.tape_block.set_seq_nr(self.seq_nr); self.seq_nr += 1; let leom = Self::write_block(&self.tape_block, &mut self.writer)?; @@ -115,7 +117,8 @@ impl<W: BlockWrite> TapeWrite for BlockedWriter<W> { /// Note: This may write an empty block just including the /// END_OF_STREAM flag. fn finish(&mut self, incomplete: bool) -> Result<bool, std::io::Error> { - vec::clear(&mut self.tape_block.payload[self.buffer_pos..]); + let payload = self.tape_block.payload_mut(); + vec::clear(&mut payload[self.buffer_pos..]); let mut flags = TapeBlockFlags::END_OF_STREAM; if incomplete { flags |= TapeBlockFlags::INCOMPLETE; diff --git a/pbs-tape/src/tape_block.rs b/pbs-tape/src/tape_block.rs index 2db5ed137..c73efafa7 100644 --- a/pbs-tape/src/tape_block.rs +++ b/pbs-tape/src/tape_block.rs @@ -33,7 +33,7 @@ static_assert_size!(TapeBlockHeader, 16); #[repr(C, packed)] pub struct TapeBlock { header: TapeBlockHeader, - pub payload: [u8], + payload: [u8], } bitflags! { @@ -107,4 +107,14 @@ impl TapeBlock { pub fn set_seq_nr(&mut self, seq_nr: u32) { self.header.seq_nr = seq_nr.to_le(); } + + /// Returns the tape block's data payload as a byte slice. + pub fn payload(&self) -> &[u8] { + &self.payload + } + + /// Returns the tape block's data payload as a mutable byte slice. + pub fn payload_mut(&mut self) -> &mut [u8] { + &mut self.payload + } } -- 2.47.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH proxmox-backup v2 08/10] tape: blocked_{reader,writer}: remove haphazard `unsafe` blocks 2026-08-21 14:02 [PATCH proxmox{,-backup} v2 00/10] Fix Undefined Behavior in Tape Block Header Deallocation Max R. Carrara ` (6 preceding siblings ...) 2026-08-21 14:02 ` [PATCH proxmox-backup v2 07/10] tape: tape block: make `payload` field private Max R. Carrara @ 2026-08-21 14:02 ` Max R. Carrara 2026-08-21 14:02 ` [PATCH proxmox-backup v2 09/10] tape: tape block: fix undefined behavior on tape block deallocation Max R. Carrara 2026-08-21 14:02 ` [PATCH proxmox-backup v2 10/10] tape: sgutils2: fix undefined behavior in dealloc of buffer Max R. Carrara 9 siblings, 0 replies; 11+ messages in thread From: Max R. Carrara @ 2026-08-21 14:02 UTC (permalink / raw) To: pbs-devel Both the `BlockedReader<R>` and `BlockedWriter<W>` structs use private helpers that cast their `TapeBlock` to a (mutable) byte slice using an `unsafe` block for reading and writing a tape block, respectively. Neither `unsafe` block is prefixed with a "// SAFETY: ..." comment, nor should these casts be done inline in the first place. Instead, implement these casts as methods on `TapeBlock` directly, with either `unsafe` block being preceded with a SAFETY comment. Signed-off-by: Max R. Carrara <m.carrara@proxmox.com> --- pbs-tape/src/blocked_reader.rs | 9 +-------- pbs-tape/src/blocked_writer.rs | 8 +------- pbs-tape/src/tape_block.rs | 31 +++++++++++++++++++++++++++++++ 3 files changed, 33 insertions(+), 15 deletions(-) diff --git a/pbs-tape/src/blocked_reader.rs b/pbs-tape/src/blocked_reader.rs index 6b1e3c692..6f5f87aa7 100644 --- a/pbs-tape/src/blocked_reader.rs +++ b/pbs-tape/src/blocked_reader.rs @@ -95,14 +95,7 @@ impl<R: BlockRead> BlockedReader<R> { } fn read_block_frame(tape_block: &mut TapeBlock, reader: &mut R) -> Result<(), BlockReadError> { - let data = unsafe { - std::slice::from_raw_parts_mut( - (tape_block as *mut TapeBlock) as *mut u8, - TapeBlock::SIZE, - ) - }; - - let bytes = reader.read_block(data)?; + let bytes = reader.read_block(tape_block.as_bytes_mut())?; if bytes != TapeBlock::SIZE { return Err(proxmox_lang::io_format_err!("got wrong block size").into()); diff --git a/pbs-tape/src/blocked_writer.rs b/pbs-tape/src/blocked_writer.rs index 0d5d10147..44ff15ae0 100644 --- a/pbs-tape/src/blocked_writer.rs +++ b/pbs-tape/src/blocked_writer.rs @@ -46,13 +46,7 @@ impl<W: BlockWrite> BlockedWriter<W> { } fn write_block(tape_block: &TapeBlock, writer: &mut W) -> Result<bool, std::io::Error> { - let data = unsafe { - std::slice::from_raw_parts( - (tape_block as *const TapeBlock) as *const u8, - TapeBlock::SIZE, - ) - }; - writer.write_block(data) + writer.write_block(tape_block.as_bytes()) } fn write_eof(&mut self) -> Result<(), std::io::Error> { diff --git a/pbs-tape/src/tape_block.rs b/pbs-tape/src/tape_block.rs index c73efafa7..c6ff98395 100644 --- a/pbs-tape/src/tape_block.rs +++ b/pbs-tape/src/tape_block.rs @@ -117,4 +117,35 @@ impl TapeBlock { pub fn payload_mut(&mut self) -> &mut [u8] { &mut self.payload } + + /// Returns the entirety of the tape block, meaning both its header and data + /// payload, as a byte slice. + pub fn as_bytes(&self) -> &[u8] { + // SAFETY: + // - Since `self` is a reference, we can convert it to a pointer without + // any concerns. The resulting pointer is always valid and non-null. + // - The pointer used here is not used or aliased anywhere else. + // - We allocated `*self` with a total size of `Self::SIZE` earlier, + // meaning that the resulting slice never goes out of bounds. + // - The resulting slice never outlives `self`. + unsafe { std::slice::from_raw_parts((self as *const _) as *const u8, Self::SIZE) } + } + + /// Returns the entirety of the tape block, meaning both its header and data + /// payload, as a mutable byte slice. + /// + /// While this method in itself is safe, be aware that it nevertheless + /// allows you to overwrite the tape block's header fields. + pub fn as_bytes_mut(&mut self) -> &mut [u8] { + // SAFETY: + // - Since `self` is a reference, we can convert it to a pointer without + // any concerns. The resulting pointer is always valid and non-null. + // - The pointer used here is not used or aliased anywhere else. + // - Since we uniquely borrow `self`, we may cast `self` to a `* mut` + // and use it to acquire a mutable slice. + // - We allocated `*self` with a total size of `Self::SIZE` earlier, + // meaning that the resulting slice never goes out of bounds. + // - The resulting slice never outlives `self`. + unsafe { std::slice::from_raw_parts_mut((self as *mut _) as *mut u8, Self::SIZE) } + } } -- 2.47.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH proxmox-backup v2 09/10] tape: tape block: fix undefined behavior on tape block deallocation 2026-08-21 14:02 [PATCH proxmox{,-backup} v2 00/10] Fix Undefined Behavior in Tape Block Header Deallocation Max R. Carrara ` (7 preceding siblings ...) 2026-08-21 14:02 ` [PATCH proxmox-backup v2 08/10] tape: blocked_{reader,writer}: remove haphazard `unsafe` blocks Max R. Carrara @ 2026-08-21 14:02 ` Max R. Carrara 2026-08-21 14:02 ` [PATCH proxmox-backup v2 10/10] tape: sgutils2: fix undefined behavior in dealloc of buffer Max R. Carrara 9 siblings, 0 replies; 11+ messages in thread From: Max R. Carrara @ 2026-08-21 14:02 UTC (permalink / raw) To: pbs-devel The `TapeBlock` struct in `pbs-tape` is a dynamically sized type whose data we allocate with an alignment [align] equal to the page size. However, since `TapeBlock` uses `#[repr(C, packed)]`, the Rust compiler will always treat the type as having an alignment of 1, since it cannot (ever) track which memory layout was used for any given heap allocation of `T`. This means that on deallocation, Rust will deallocate a `TapeBlock` with an alignment of 1 instead of the page size alignment that was used to allocate it. This mismatch in alignment on alloc / dealloc is undefined behavior [ub] as reported by Miri [miri]. Note that for *nix and WASM, this does currently not have any known impact, since allocators on these targets do not actually care about alignment on deallocation. There is however no guarantee that this will not change in the future. If we were to target Windows, it would already be a problem [alloc-win]. To solve this, use the newly introduced `LayoutAwareBox<T>` type of `proxmox-alloc` and return a `LayoutAwareBox<TapeBlock>` instead of a plain `Box<TapeBlock>` from `TapeBlock::new()`. `LayoutAwareBox` tracks the layout that was used during allocation and uses it for deallocation as well, which fixes the aforementioned undefined behavior occurring here. Besides that, rework the implementation of `TapeBlock::new()` and call `std::alloc::handle_alloc_error` if allocating a new tape block fails. Add a SAFETY comment to each `unsafe` block as well. Use `size_of::<TapeBlockHeader>()` instead of hard-coding its (expected) size. Update the two instances where we allocate tape blocks. Since `LayoutAwareBox<T>` works similar to `Box<T>`, only the type signature needs to be changed. Also, use a crate-public `LazyLock`ed static for keeping track of the page size. Since the page size is a system constant [sysconf], this is safe to do. Finally, add a simple test for `TapeBlock` that allows Miri to check whether there is any UB on deallocation. [align]: https://en.wikipedia.org/wiki/Data_structure_alignment [alloc-win]: https://github.com/rust-lang/rust/blob/c9ff496891c278ad660bc0ab85c1f0b72059464a/library/std/src/sys/alloc/windows.rs#L182 [miri]: https://github.com/rust-lang/miri [sysconf]: `man 3 sysconf` [ub]: https://doc.rust-lang.org/reference/behavior-considered-undefined.html Reported-by: Robert Obkircher <r.obkircher@proxmox.com> Signed-off-by: Max R. Carrara <m.carrara@proxmox.com> --- Cargo.toml | 3 ++ pbs-tape/Cargo.toml | 1 + pbs-tape/src/blocked_reader.rs | 4 ++- pbs-tape/src/blocked_writer.rs | 3 +- pbs-tape/src/lib.rs | 13 ++++++++ pbs-tape/src/tape_block.rs | 60 ++++++++++++++++++++++++++++------ 6 files changed, 72 insertions(+), 12 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index f3b67ba79..933c9afd0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -53,6 +53,7 @@ path = "src/lib.rs" [workspace.dependencies] # proxmox workspace +proxmox-alloc = "0.1.0" proxmox-apt = { version = "1.0", features = [ "cache" ] } proxmox-apt-api-types = "3.0" proxmox-async = "0.5" @@ -217,6 +218,7 @@ zstd.workspace = true #valgrind_request = { git = "https://github.com/edef1c/libvalgrind_request", version = "1.1.0", optional = true } # proxmox workspace +proxmox-alloc.workspace = true proxmox-apt.workspace = true proxmox-apt-api-types.workspace = true proxmox-async.workspace = true @@ -282,6 +284,7 @@ proxmox-rrd-api-types.workspace = true #pbs-api-types = { path = "../proxmox/pbs-api-types" } #proxmox-acme = { path = "../proxmox/proxmox-acme" } #proxmox-acme-api = { path = "../proxmox/proxmox-acme-api" } +#proxmox-alloc = { path = "../proxmox/proxmox-alloc" } #proxmox-api-macro = { path = "../proxmox/proxmox-api-macro" } #proxmox-apt = { path = "../proxmox/proxmox-apt" } #proxmox-apt-api-types = { path = "../proxmox/proxmox-apt-api-types" } diff --git a/pbs-tape/Cargo.toml b/pbs-tape/Cargo.toml index 4f153feda..6007b683f 100644 --- a/pbs-tape/Cargo.toml +++ b/pbs-tape/Cargo.toml @@ -21,6 +21,7 @@ serde_json.workspace = true thiserror.workspace = true udev.workspace = true +proxmox-alloc.workspace = true proxmox-io.workspace = true proxmox-lang.workspace=true proxmox-log.workspace=true diff --git a/pbs-tape/src/blocked_reader.rs b/pbs-tape/src/blocked_reader.rs index 6f5f87aa7..3d5cc8f12 100644 --- a/pbs-tape/src/blocked_reader.rs +++ b/pbs-tape/src/blocked_reader.rs @@ -1,5 +1,7 @@ use std::io::Read; +use proxmox_alloc::LayoutAwareBox; + use crate::{ BlockRead, BlockReadError, PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0, TapeBlock, TapeBlockFlags, TapeRead, @@ -18,7 +20,7 @@ use crate::{ /// the end of the stream). pub struct BlockedReader<R> { reader: R, - tape_block: Box<TapeBlock>, + tape_block: LayoutAwareBox<TapeBlock>, seq_nr: u32, found_end_marker: bool, incomplete: bool, diff --git a/pbs-tape/src/blocked_writer.rs b/pbs-tape/src/blocked_writer.rs index 44ff15ae0..39ac22a10 100644 --- a/pbs-tape/src/blocked_writer.rs +++ b/pbs-tape/src/blocked_writer.rs @@ -1,3 +1,4 @@ +use proxmox_alloc::LayoutAwareBox; use proxmox_io::vec; use crate::{BlockWrite, TapeBlock, TapeBlockFlags, TapeWrite}; @@ -9,7 +10,7 @@ use crate::{BlockWrite, TapeBlock, TapeBlockFlags, TapeWrite}; /// to the underlying writer. pub struct BlockedWriter<W: BlockWrite> { writer: W, - tape_block: Box<TapeBlock>, + tape_block: LayoutAwareBox<TapeBlock>, buffer_pos: usize, seq_nr: u32, logical_end_of_media: bool, diff --git a/pbs-tape/src/lib.rs b/pbs-tape/src/lib.rs index 1b6dc94cc..d4a03af6b 100644 --- a/pbs-tape/src/lib.rs +++ b/pbs-tape/src/lib.rs @@ -51,6 +51,19 @@ pub const PROXMOX_BACKUP_MEDIA_LABEL_MAGIC_1_0: [u8; 8] = [42, 5, 191, 60, 176, // openssl::sha::sha256(b"Proxmox Backup MediaSet Label v1.0") pub const PROXMOX_BACKUP_MEDIA_SET_LABEL_MAGIC_1_0: [u8; 8] = [8, 96, 99, 249, 47, 151, 83, 216]; +pub(crate) static PAGE_SIZE: std::sync::LazyLock<usize> = std::sync::LazyLock::new(|| { + // See `man 3 sysconf` -- this is a constant and does therefore not change + // during the lifetime of a process. + // SAFETY: Should always be safe to call, and we check for errors afterwards. + let page_size = unsafe { libc::sysconf(libc::_SC_PAGESIZE) }; + + if page_size <= 0 { + panic!("failed to query PAGESIZE ({page_size})"); + } + + page_size as usize +}); + #[derive(Endian, Copy, Clone, Debug)] #[repr(C, packed)] /// Media Content Header diff --git a/pbs-tape/src/tape_block.rs b/pbs-tape/src/tape_block.rs index c6ff98395..4ff91bdc0 100644 --- a/pbs-tape/src/tape_block.rs +++ b/pbs-tape/src/tape_block.rs @@ -1,7 +1,9 @@ use bitflags::bitflags; +use proxmox_alloc::LayoutAwareBox; use proxmox_lang::static_assert_size; +use crate::PAGE_SIZE; use crate::PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0; use crate::PROXMOX_TAPE_BLOCK_SIZE; @@ -50,19 +52,44 @@ bitflags! { impl TapeBlock { pub const SIZE: usize = PROXMOX_TAPE_BLOCK_SIZE; - /// Allocates a new instance on the heap - pub fn new() -> Box<Self> { - use std::alloc::{Layout, alloc_zeroed}; + /// Allocates a new [`TapeBlock`] on the heap. + /// + /// [`LayoutAwareBox`] ensures that the layout that was used for allocation + /// is also used for dealloction. + pub fn new() -> LayoutAwareBox<Self> { + use std::alloc::Layout; + use std::alloc::alloc_zeroed; + use std::alloc::handle_alloc_error; // align to PAGESIZE, so that we can use it with SG_IO - let page_size = unsafe { libc::sysconf(libc::_SC_PAGESIZE) } as usize; + let layout = Layout::from_size_align(Self::SIZE, *PAGE_SIZE).expect("infallible"); - let mut buffer = unsafe { - let ptr = alloc_zeroed(Layout::from_size_align(Self::SIZE, page_size).unwrap()); - Box::from_raw(core::ptr::slice_from_raw_parts_mut(ptr, Self::SIZE - 16) as *mut Self) - }; - buffer.header.magic = PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0; - buffer + // SAFETY: layout always has a size > 0. Other size and alignment checks + // were performed by from_size_align above. + let thin_ptr = unsafe { alloc_zeroed(layout) }; + if thin_ptr.is_null() { + handle_alloc_error(layout); + } + + let payload_len = Self::SIZE - size_of::<TapeBlockHeader>(); + let fat_ptr = core::ptr::slice_from_raw_parts_mut(thin_ptr, payload_len) as *mut Self; + + // SAFETY: + // - We checked that `thin_ptr` isn't null above, so `fat_ptr` isn't + // null either. + // - We converted `thin_ptr` to `fat_ptr` with the correct payload + // length, which is the total size minus the header's size. + // - `fat_ptr` is valid for reading and writing. + // - `fat_ptr` is not aliased anywhere. + // - The memory `fat_ptr` points to was zero-initialized by + // `alloc_zeroed` earlier. + // - The value that `fat_ptr` points to has the same size as its + // allocation. + let mut tape_block = unsafe { LayoutAwareBox::from_raw_parts(fat_ptr, layout) }; + + tape_block.header.magic = PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0; + + tape_block } /// Returns the magic value of this tape block's header. @@ -149,3 +176,16 @@ impl TapeBlock { unsafe { std::slice::from_raw_parts_mut((self as *mut _) as *mut u8, Self::SIZE) } } } + +#[cfg(test)] +mod test { + use super::*; + + // Test for Miri to check whether there's any memory layout mismatch during + // the deallocation of a `LayoutAwareBox<TapeBlock>`. + #[test] + fn miri_check_dealloc() { + let tape_block = TapeBlock::new(); + drop(tape_block); + } +} -- 2.47.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH proxmox-backup v2 10/10] tape: sgutils2: fix undefined behavior in dealloc of buffer 2026-08-21 14:02 [PATCH proxmox{,-backup} v2 00/10] Fix Undefined Behavior in Tape Block Header Deallocation Max R. Carrara ` (8 preceding siblings ...) 2026-08-21 14:02 ` [PATCH proxmox-backup v2 09/10] tape: tape block: fix undefined behavior on tape block deallocation Max R. Carrara @ 2026-08-21 14:02 ` Max R. Carrara 9 siblings, 0 replies; 11+ messages in thread From: Max R. Carrara @ 2026-08-21 14:02 UTC (permalink / raw) To: pbs-devel The `alloc_page_aligned_buffer()` function in the `sgutils2` module manually allocates a page size aligned buffer. However, there is no way for Rust to track the alignment that was used for any given heap allocation. This means that on deallocation, Rust will deallocate any such buffer using its element type's alignment, which in this case is `align_of::<u8>()` (1). This mismatch in alignment on alloc / dealloc is undefined behavior [ub] as reported by Miri [miri]. Note that for *nix and WASM, this does currently not have any known impact, since allocators on these targets do not actually care about alignment on deallocation. There is however no guarantee that this will not change in the future. If we were to target Windows, it would already be a problem [alloc-win]. Solve this by using the `LayoutAwareBox` from `proxmox-alloc` and returning it instead of the plain `Box`. Since `LayoutAwareBox` also handles requests to allocate zero-length slices, pre-emptive checks whether the requested buffer size is 0 are not necessary anymore. In other words, `alloc_page_aligned_buffer()` will now correctly return a zero-length buffer if the requested size is 0, instead of causing even more undefined behavior -- passing a `Layout` with size 0 to `alloc()` & Co. is also UB [alloc-safety]! Also use the newly introduced `PAGE_SIZE` static to fetch the page size where we are currently querying the page size via `libc::sysconf()`. [align]: https://en.wikipedia.org/wiki/Data_structure_alignment [alloc-safety]: https://doc.rust-lang.org/alloc/alloc/trait.GlobalAlloc.html#safety-3 [alloc-win]: https://github.com/rust-lang/rust/blob/c9ff496891c278ad660bc0ab85c1f0b72059464a/library/std/src/sys/alloc/windows.rs#L182 [miri]: https://github.com/rust-lang/miri [ub]: https://doc.rust-lang.org/reference/behavior-considered-undefined.html Reported-by: Robert Obkircher <r.obkircher@proxmox.com> Signed-off-by: Max R. Carrara <m.carrara@proxmox.com> --- pbs-tape/src/sgutils2.rs | 49 ++++++++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 17 deletions(-) diff --git a/pbs-tape/src/sgutils2.rs b/pbs-tape/src/sgutils2.rs index 340616c73..ab02c6ad2 100644 --- a/pbs-tape/src/sgutils2.rs +++ b/pbs-tape/src/sgutils2.rs @@ -15,8 +15,11 @@ use endian_trait::Endian; use libc::{c_char, c_int}; use serde::{Deserialize, Serialize}; +use proxmox_alloc::LayoutAwareBox; use proxmox_io::ReadExt; +use crate::PAGE_SIZE; + #[derive(thiserror::Error, Debug)] pub struct SenseInfo { pub sense_key: u8, @@ -405,7 +408,7 @@ extern "C" { /// Safe interface to run RAW SCSI commands pub struct SgRaw<'a, F> { file: &'a mut F, - buffer: Box<[u8]>, + buffer: LayoutAwareBox<[u8]>, sense_buffer: [u8; 32], timeout: i32, } @@ -435,16 +438,8 @@ pub fn get_asc_ascq_string(asc: u8, ascq: u8) -> String { /// Allocate a page aligned buffer /// /// SG RAWIO commands needs page aligned transfer buffers. -pub fn alloc_page_aligned_buffer(buffer_size: usize) -> Result<Box<[u8]>, Error> { - let page_size = unsafe { libc::sysconf(libc::_SC_PAGESIZE) } as usize; - let layout = std::alloc::Layout::from_size_align(buffer_size, page_size)?; - let dinp = unsafe { std::alloc::alloc_zeroed(layout) }; - if dinp.is_null() { - bail!("alloc SCSI output buffer failed"); - } - - let buffer_slice = unsafe { std::slice::from_raw_parts_mut(dinp, buffer_size) }; - Ok(unsafe { Box::from_raw(buffer_slice) }) +pub fn alloc_page_aligned_buffer(buffer_size: usize) -> Result<LayoutAwareBox<[u8]>, Error> { + LayoutAwareBox::slice_fill(buffer_size, *PAGE_SIZE, 0).map_err(Error::from) } impl<'a, F: AsRawFd> SgRaw<'a, F> { @@ -452,11 +447,7 @@ impl<'a, F: AsRawFd> SgRaw<'a, F> { /// /// The file must be a handle to a SCSI device. pub fn new(file: &'a mut F, buffer_size: usize) -> Result<Self, Error> { - let buffer = if buffer_size > 0 { - alloc_page_aligned_buffer(buffer_size)? - } else { - Box::new([]) - }; + let buffer = alloc_page_aligned_buffer(buffer_size)?; let sense_buffer = [0u8; 32]; @@ -674,7 +665,7 @@ impl<'a, F: AsRawFd> SgRaw<'a, F> { return Err(format_err!("no valid SCSI command").into()); } - let page_size = unsafe { libc::sysconf(libc::_SC_PAGESIZE) } as usize; + let page_size = *PAGE_SIZE; if ((data.as_ptr() as usize) & (page_size - 1)) != 0 { return Err(format_err!("wrong transfer buffer alignment").into()); } @@ -1017,8 +1008,32 @@ pub fn scsi_request_sense<F: AsRawFd>(file: &mut F) -> Result<RequestSenseFixed, #[cfg(test)] mod test { + use crate::PAGE_SIZE; + use crate::sgutils2::alloc_page_aligned_buffer; use crate::sgutils2::scsi_ascii_to_string; + #[test] + fn page_aligned_buffer_zero_length() { + let buf = alloc_page_aligned_buffer(0).expect("infallible"); + + assert_eq!(buf.len(), 0); + } + + #[test] + fn page_aligned_buffer_alignment() { + let len = 8 * 1024; + let buf = alloc_page_aligned_buffer(len).expect("infallible"); + + assert_eq!(buf.len(), len); + + let page_size = *PAGE_SIZE; + assert_eq!( + (buf.as_ptr() as usize) & (page_size - 1), + 0, + "not actually aligned to page size" + ); + } + #[test] fn test_scsi_ascii_to_string() { fn test(input: &'static str, expected: &'static str) { -- 2.47.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2026-08-21 14:03 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-21 14:02 [PATCH proxmox{,-backup} v2 00/10] Fix Undefined Behavior in Tape Block Header Deallocation Max R. Carrara
2026-08-21 14:02 ` [PATCH proxmox v2 01/10] proxmox-alloc: introduce proxmox-alloc with `LayoutAwareBox<T>` type Max R. Carrara
2026-08-21 14:02 ` [PATCH proxmox v2 02/10] proxmox-alloc: document undefined behavior regarding custom allocs Max R. Carrara
2026-08-21 14:02 ` [PATCH proxmox-backup v2 03/10] tape: move tape block structs into separate file module Max R. Carrara
2026-08-21 14:02 ` [PATCH proxmox-backup v2 04/10] tape: rename `BlockHeader` and `BlockHeaderFlags` Max R. Carrara
2026-08-21 14:02 ` [PATCH proxmox-backup v2 05/10] tape: blocked_{reader,writer}: rename `buffer` to `tape_block` Max R. Carrara
2026-08-21 14:02 ` [PATCH proxmox-backup v2 06/10] tape: tape block: represent tape block header with its own struct Max R. Carrara
2026-08-21 14:02 ` [PATCH proxmox-backup v2 07/10] tape: tape block: make `payload` field private Max R. Carrara
2026-08-21 14:02 ` [PATCH proxmox-backup v2 08/10] tape: blocked_{reader,writer}: remove haphazard `unsafe` blocks Max R. Carrara
2026-08-21 14:02 ` [PATCH proxmox-backup v2 09/10] tape: tape block: fix undefined behavior on tape block deallocation Max R. Carrara
2026-08-21 14:02 ` [PATCH proxmox-backup v2 10/10] tape: sgutils2: fix undefined behavior in dealloc of buffer Max R. Carrara
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.