public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [PATCH proxmox{,-backup} v4 0/9] Fix Undefined Behavior in Tape Block Header Deallocation
@ 2026-09-10 15:23 Max R. Carrara
  2026-09-10 15:23 ` [PATCH proxmox v4 1/9] proxmox-alloc: introduce proxmox-alloc with `LayoutAwareBox<T>` type Max R. Carrara
                   ` (8 more replies)
  0 siblings, 9 replies; 10+ messages in thread
From: Max R. Carrara @ 2026-09-10 15:23 UTC (permalink / raw)
  To: pbs-devel

Fix Undefined Behavior in Tape Block Header Deallocation - v4
=============================================================

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 is a quick refresh of v3, aiming to fix a couple of cases where
`LayoutAwareBox<T>` leaked memory when certain methods panicked.

The *only* instance where we still may leak memory is if a drop handler
of an element of a slice panics while another panic is already
unwinding. In that case, there is not really much we can do without
overengineering the data structure.

In particular:

- In the regular (non-slice) `Clone` impl, a panic when calling
  `clone()` would leak memory, since the ownership of the pointer would
  be transferred to the aware-box only after the write.

  To fix this, add an inline `DropGuard<T>` struct whose drop handler
  calls `dealloc`. If the write is successful, we `mem::forget()` the
  drop guard; otherwise, `DropGuard::drop()` frees the allocation before
  continuing to unwind.

  Respective tests for this are added to ensure this doesn't happen again.

- For sized types, when allocating a new slice using
  `LayoutAwareBox::<[T]>::slice_fill_with()` and related methods that
  use it under the hood, a panic in the initialization function would
  cause the fat pointer tracking the number of initialized elements to
  be used for the deallocation as well.

  To be more precise, if 5 out of 10 elements were initialized before
  panicking, the local `LayoutAwareBox<[T]>` would store a fat pointer
  with a length of 5. During `LayoutAwareBox::<[T]>::drop()`, this
  pointer would then be used in the call to `dealloc()`.

  This is actually undefined behavior once again (according to Miri),
  since the length of the pointer does not match up with the
  allocation's size.

  To fix this, use another inline `DropGuard<T>` struct that tracks the
  number of initialized elements *in addition to the allocation*. If the
  initialization loop panics, `DropGuard<T>` creates a fat pointer with
  the number of initialized elements and passes it to
  `core::ptr::drop_in_place()`, but passes the *original* pointer (and
  layout) to `dealloc()`.

  This prevents UB and also lets Miri correctly reason about our
  allocations.

  Like for the previous point, respective tests for this are added here
  too. Note that these tests need to be run using Miri to actually check
  for UB.

- Another fix for `LayoutAwareBox::<[T]>::slice_fill_with()` makes the
  method actually call the initializer function for zero-sized types.
  Add a test case to ensure this is actually done.

- Use the size of the stored layout in `LayoutAwareBox<T>` to check
  whether we have a zero-sized type instead of calling `size_of_val()`.

- Toss out `LayoutAwareBox::<[T]>::zero_sized_slice()`, since that's
  become redundant (and was just confusing anyways).

Testing
-------

As before, 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/
v3: https://lore.proxmox.com/pbs-devel/20260909154027.595374-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   | 2821 ++++++++++++++++++++++++++++
 proxmox-alloc/src/lib.rs           |   21 +
 8 files changed, 2923 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, 3217 insertions(+), 153 deletions(-)

-- 
Generated by murpp 0.12.0




^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2026-09-11 11:39 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-10 15:23 [PATCH proxmox{,-backup} v4 0/9] Fix Undefined Behavior in Tape Block Header Deallocation Max R. Carrara
2026-09-10 15:23 ` [PATCH proxmox v4 1/9] proxmox-alloc: introduce proxmox-alloc with `LayoutAwareBox<T>` type Max R. Carrara
2026-09-10 15:23 ` [PATCH proxmox-backup v4 2/9] tape: move tape block structs into separate file module Max R. Carrara
2026-09-10 15:23 ` [PATCH proxmox-backup v4 3/9] tape: rename `BlockHeader` and `BlockHeaderFlags` Max R. Carrara
2026-09-10 15:23 ` [PATCH proxmox-backup v4 4/9] tape: blocked_{reader,writer}: rename `buffer` to `tape_block` Max R. Carrara
2026-09-10 15:23 ` [PATCH proxmox-backup v4 5/9] tape: tape block: represent tape block header with its own struct Max R. Carrara
2026-09-10 15:23 ` [PATCH proxmox-backup v4 6/9] tape: tape block: make `payload` field private Max R. Carrara
2026-09-10 15:23 ` [PATCH proxmox-backup v4 7/9] tape: blocked_{reader,writer}: remove haphazard `unsafe` blocks Max R. Carrara
2026-09-10 15:23 ` [PATCH proxmox-backup v4 8/9] tape: tape block: fix undefined behavior on tape block deallocation Max R. Carrara
2026-09-10 15:23 ` [PATCH proxmox-backup v4 9/9] tape: sgutils2: fix undefined behavior in dealloc of buffer Max R. Carrara

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal