From: "Max R. Carrara" <m.carrara@proxmox.com>
To: pbs-devel@lists.proxmox.com
Subject: [PATCH proxmox{,-backup} v3 0/9] Fix Undefined Behavior in Tape Block Header Deallocation
Date: Wed, 9 Sep 2026 17:40:14 +0200 [thread overview]
Message-ID: <20260909154027.595374-1-m.carrara@proxmox.com> (raw)
Fix Undefined Behavior in Tape Block Header Deallocation - v3
=============================================================
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 refresh incorporates the feedback @Robert has given on v2. Many
thanks for the review, it's really appreciated! The most noteworthy
changes are found below.
proxmox:
- Drop the inner enum that was used to differ between "known" and
"dynamic" layouts -- always store a `std::alloc::Layout` and handle
zero-sized types explicitly instead
- Fix the `Clone` impls of `LayoutAwareBox<T>` by allocating the new
memory with the same layout as that of `self`, then clone and write
directly to that memory
- Introduce a new method to `LayoutAwareBox<T>` called
`from_slice_align()` that allows the caller to easily create an aware
box from a given `&[T]` with a particular alignment
--> This also happens to be useful for the `Clone` impl for
`LayoutAwareBox<[T]>`
- Prevent leaking / observing (uninitialized) memory on panic in
`slice_fill()` and `slice_fill_with()` of `LayoutAwareBox<T>` by
constructing the aware-boxed slice with a length of 0 first, and then
updating the length of the allocated slice at the end of every
iteration.
This also means that drop-handlers of already initialized elements
will get called.
- Add initial Debian packaging for the proxmox-alloc crate
- Drop the `examples/` directory
- Adapt SAFETY comments as per @Robert's feedback
- Adapt existing tests and add additional ones, such as for `From` and
`Clone` impls
proxmox-backup:
- Adapt docstring of the `TapeBlock` type
- Adapt SAFETY comments as per @Robert's feedback
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/
v2: https://lore.proxmox.com/pbs-devel/20260821140238.615302-1-m.carrara@proxmox.com/
Summary of Changes
------------------
proxmox:
Max R. Carrara (1):
proxmox-alloc: introduce proxmox-alloc with `LayoutAwareBox<T>` type
Cargo.toml | 1 +
proxmox-alloc/Cargo.toml | 20 +
proxmox-alloc/debian/changelog | 5 +
proxmox-alloc/debian/control | 30 +
proxmox-alloc/debian/copyright | 18 +
proxmox-alloc/debian/debcargo.toml | 7 +
proxmox-alloc/src/aware_boxed.rs | 2548 ++++++++++++++++++++++++++++
proxmox-alloc/src/lib.rs | 21 +
8 files changed, 2650 insertions(+)
create mode 100644 proxmox-alloc/Cargo.toml
create mode 100644 proxmox-alloc/debian/changelog
create mode 100644 proxmox-alloc/debian/control
create mode 100644 proxmox-alloc/debian/copyright
create mode 100644 proxmox-alloc/debian/debcargo.toml
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 | 185 +++++++++++++++++++++++++++++++++
7 files changed, 294 insertions(+), 153 deletions(-)
create mode 100644 pbs-tape/src/tape_block.rs
Summary over all repositories:
15 files changed, 2944 insertions(+), 153 deletions(-)
--
Generated by murpp 0.12.0
next reply other threads:[~2026-09-09 15:40 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-09 15:40 Max R. Carrara [this message]
2026-09-09 15:40 ` [PATCH proxmox v3 1/9] proxmox-alloc: introduce proxmox-alloc with `LayoutAwareBox<T>` type Max R. Carrara
2026-09-09 15:40 ` [PATCH proxmox-backup v3 2/9] tape: move tape block structs into separate file module Max R. Carrara
2026-09-09 15:40 ` [PATCH proxmox-backup v3 3/9] tape: rename `BlockHeader` and `BlockHeaderFlags` Max R. Carrara
2026-09-09 15:40 ` [PATCH proxmox-backup v3 4/9] tape: blocked_{reader,writer}: rename `buffer` to `tape_block` Max R. Carrara
2026-09-09 15:40 ` [PATCH proxmox-backup v3 5/9] tape: tape block: represent tape block header with its own struct Max R. Carrara
2026-09-09 15:40 ` [PATCH proxmox-backup v3 6/9] tape: tape block: make `payload` field private Max R. Carrara
2026-09-09 15:40 ` [PATCH proxmox-backup v3 7/9] tape: blocked_{reader,writer}: remove haphazard `unsafe` blocks Max R. Carrara
2026-09-09 15:40 ` [PATCH proxmox-backup v3 8/9] tape: tape block: fix undefined behavior on tape block deallocation Max R. Carrara
2026-09-09 15:40 ` [PATCH proxmox-backup v3 9/9] tape: sgutils2: fix undefined behavior in dealloc of buffer Max R. Carrara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260909154027.595374-1-m.carrara@proxmox.com \
--to=m.carrara@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox