Re: [PATCH proxmox-backup 1/2] pbs-tape: fix undefined behavior in block header deallocation

["Max R. Carrara" <m.carrara@proxmox.com>]

On Mon Aug 10, 2026 at 10:34 AM CEST, Robert Obkircher wrote:
>
> On 07.08.26 14:47, Max R. Carrara wrote:
> > On Thu Aug 6, 2026 at 12:57 PM CEST, Robert Obkircher wrote:
> >>> The `BlockHeader` struct in `pbs-tape` is a dynamically sized type
> >>> whose data we allocate with an alignment equal to the page size.
> >>> However, since `BlockHeader` uses `#[repr(C, packed)]`, the Rust
> >>> compiler will always treat the type as having an alignment of 1
> >>> instead. This is undefined behavior as reported by miri [0], since
> >>> Rust requires the alignment used during deallocation to match that
> >>> used during allocation, even if the underlying allocator doesn't
> >>> actually care about alignment on `free()`.
> >>> [...]
> >>> +
> >>> +impl BlockHeader {
> >>> +    pub const SIZE: usize = PROXMOX_TAPE_BLOCK_SIZE;
> >>> +
> >>> +    // We are subtracting the metadata size first before dividing by the size of the
> >>> +    // payload's element type, since we need to ensure that we don't mix units.
> >>> +    //
> >>> +    // Note that the division at the end here is actually redundant, since
> >>> +    // size_of::<PayloadElement>() == 1, but it's better to be explicit about the arithmetic.
> >>> +    const PAYLOAD_LEN: usize =
> >>> +        (Self::SIZE - _impl::BLOCK_HEADER_META_SIZE) / _impl::PAYLOAD_ELEMENT_SIZE;
> >> Imo having constants like this all over the place makes it much harder
> >> to reason about the correctness of new.
> > How so exactly?
> Just that previously you had to keep 1 type and 1 constant in mind and
> now it's four of each.

I mean, fair, but before you also had to be aware that the fields before
the payload had a total size of 16 due to their layout + alignment,
which isn't really something that obvious if you're not used to working
with such types.

I mean, I don't mind changing this per se; I'm just not sure how this
makes it harder to reason about the correctness of `::new()` in
particular.

Could you perhaps show what would make it easier to reason about for you
instead? Maybe I'm just missing something here.

> >>> +
> >>> +    pub fn new() -> Result<Self, io::Error> {
> >>> +        // SAFETY: layout is non-zero by construction
> >>> +        let raw_thin_ptr = unsafe { alloc::alloc_zeroed(*BLOCK_HEADER_ALLOC_LAYOUT) };
> >>> +
> >>> +        let Some(nonnull_thin) = NonNull::new(raw_thin_ptr) else {
> >>> +            // NOTE: Getting a null pointer here means that we either ran out of memory, or that
> >>> +            // our allocator doesn't support the given size or alignment. Since the latter is very,
> >>> +            // very unlikely, we assume that we only ever hit this branch if we're OOM.
> >>> +            return Err(io::Error::new(
> >>> +                io::ErrorKind::OutOfMemory,
> >>> +                "failed to allocate memory for block header",
> >>> +            ));
> >> io::Error::new allocates, so it might be better to just call
> >> handle_alloc_error.
> > ACK, good point.
> >
> >>> +        };
> >>> +
> >>> +        // A bit of weirdness regarding dynamically sized types:
> >>> +        //
> >>> +        // Since BlockHeaderData is dynamically sized, we need to explicitly construct a fat
> >>> +        // pointer for it. A fat pointer consists of a memory address plus the number of elements
> >>> +        // that the pointee stores, called the length. Therefore, size_of::<*mut u8>() (thin) is
> >>> +        // not the same as size_of::<*mut [u8]>() (fat), usually the latter is twice as large.
> >>> +        //
> >>> +        // The length that we specify here is the number of elements our trailing payload can
> >>> +        // store, which we defined earlier as PAYLOAD_LEN. We then cast our fat *mut [u8] to
> >>> +        // *mut BlockHeaderData -- since the length stored in a fat pointer is *not* converted or
> >>> +        // altered when casting, we successfully gave the trailing payload slice in BlockHeaderData
> >>> +        // a concrete size of PAYLOAD_LEN.
> >>> +        //
> >>> +        // This only works because the unsized payload slice is at the end of the BlockHeaderData
> >>> +        // struct. The compiler will know what the length in the fat pointer corresponds to that
> >>> +        // slice. (Note that a different position for the payload in the struct wouldn't work, and
> >>> +        // neither would multiple slices; the compiler prevents us from doing either.)
> >>> +        //
> >>> +        // SAFETY: We ensured that PAYLOAD_LEN is the total size minus the size of the metadata by
> >>> +        // construction. We also ensured that our thin pointer is not null.
> >>> +        let raw_fat_ptr =
> >>> +            core::ptr::slice_from_raw_parts_mut(nonnull_thin.as_ptr(), Self::PAYLOAD_LEN)
> >>> +                as *mut _impl::BlockHeaderData;
> >>> +
> >>> +        // SAFETY: The pointer returned by slice_from_raw_parts_mut() is never null.
> >>> +        let mut ptr = unsafe { NonNull::new_unchecked(raw_fat_ptr) };
> >>> +
> >>> +        // SAFETY: We are allowed to convert the pointer to a reference. See impl Deref.
> >> Why not just construct Self and write the field via DerefMut then?
> > Oh, true... missed that one here. Thanks for spotting this!
> >
> >>> +        unsafe { ptr.as_mut().set_magic(PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0) };
> >>> +
> >>> +        debug_assert_eq!(unsafe { ptr.as_ref().payload().len() }, Self::PAYLOAD_LEN);
> >>> +
> >>> +        Ok(Self { ptr })
> >>> +    }
> >>> +}
> >>> +
> >>> +impl Drop for BlockHeader {
> >>> +    fn drop(&mut self) {
> >>> +        // SAFETY: We allocated ptr with the given layout earlier.
> >>> +        unsafe { alloc::dealloc(self.ptr.as_ptr() as *mut u8, *BLOCK_HEADER_ALLOC_LAYOUT) };
> >>> +    }
> >>> +}
> >>> +
> >>> +impl std::ops::Deref for BlockHeader {
> >>> +    type Target = _impl::BlockHeaderData;
> >>> +
> >>> +    fn deref(&self) -> &Self::Target {
> >>> +        // SAFETY: Safe to convert ptr to a shared reference, since it's aligned, the data
> >>> +        // initialized, dereferenceable, and explicitly non-null.
> >>> +        unsafe { self.ptr.as_ref() }
> >>> +    }
> >>> +}
> >>> +
> >>> +impl std::ops::DerefMut for BlockHeader {
> >>> +    fn deref_mut(&mut self) -> &mut Self::Target {
> >>> +        // SAFETY: Safe to convert ptr to a unique reference, since it's aligned, the data
> >>> +        // initialized, dereferenceable, and explicitly non-null.
> >> The important thing is that &mut self provides mutual exclusion with
> >> other places that dereference.
> > Agree with this as well, will add this in v2 too.
> You could even get rid of unsafe by storing ManuallyDrop Box like the
> aligned_box crate.

I'll check it out, thanks!

> >
> >>> +        unsafe { self.ptr.as_mut() }
> >>> +    }
> >>> +}
> >>> +
> >>> +impl AsRef<BlockHeaderData> for BlockHeader {
> >>> +    fn as_ref(&self) -> &BlockHeaderData {
> >>> +        &*self
> >>> +    }
> >>> +}
> >>> +
> >>> +impl AsMut<BlockHeaderData> for BlockHeader {
> >>> +    fn as_mut(&mut self) -> &mut BlockHeaderData {
> >>> +        &mut *self
> >>> +    }
> >>>  }
> >>>
> >>>  bitflags! {
> >>> @@ -84,6 +208,160 @@ bitflags! {
> >>>      }
> >>>  }
> >>>
> >>> +// Putting underlying implementation in here to prevent access to private fields
> >> This seems like a confusing, unrelated change with no benefit.
> > The main benefit here is that private fields cannot be accessed anymore.
> I'm not fully convinced that the definition of an extern format needs
> to be hidden from someone willing to work at that level.

What do you mean with hidden? All fields are still accessible through
their respective methods, it's just that *directly* modifying those
fields is not as easy as before anymore. There wasn't any point in
exposing the fields as `pub` in the first place IMO, and I also think
that doing that encourages bad patterns ...

For example, in both the [reader] and [writer] implementations we are
casting the entire struct to a byte slice directly, even though that
could've been a method each instead, keeping the implementation of such
an operation local to the struct. That's why I introduced `as_bytes()`
and `as_bytes_mut()`, which do exactly that.

Don't get me wrong, if a struct really only stores plain old data, then
I don't mind `pub` fields, but as soon as there's any kind of state
involved, it's vastly easier to reason about a type if you know what can
be done with it, IMO.

[reader] https://git.proxmox.com/?p=proxmox-backup.git;a=blob;f=pbs-tape/src/blocked_reader.rs;h=22803371c3e36753c7fe987312713bb98ca0d1a1;hb=refs/heads/master#l94
[writer] https://git.proxmox.com/?p=proxmox-backup.git;a=blob;f=pbs-tape/src/blocked_writer.rs;h=7380af2435d62ee11fbab47188f782360ebab474;hb=refs/heads/master#l48

> >
> > I was also thinking of moving this out of `lib.rs` and instead making a
> > separate module like e.g. `header.rs` (or something), since private
> > fields should then remain private throughout the crate.
> That seems nicer than manual multiple inheritance of accessors. Just
> do that in a separate commit please.

ACK, will do that.

> >
> >> Leaving the original code as-is and simply replacing Box<T> with a new
> >> PageAlignedBox<T> would have been so much easier to review.
> > I didn't want to introduce a new generic type for a one-off smart
> > pointer -- and moreover, what in particular made it harder to review, if
> > I may ask? I'm simply moving the implementation into separate structs
> > here.
> There is a [2]nd one that I had posted on Zulip, and the third could
> be a unit test that checks an Rc count.
>
> I just prefer mechanical changes like moving code to be in separate
> commits.
>
> [2]
> https://git.proxmox.com/?p=proxmox-backup.git;a=blob;f=pbs-tape/src/sgutils2.rs;h=340616c73675b994c8761a6981fd41e2e249f953;hb=HEAD#l447

Hmm, fair point! I'll see if I can fix the second instance in v2 too,
then.

Which third one are you referring to exactly?

>
> >
> >>> +mod _impl {
> >>> +    use super::BlockHeaderFlags;
> >>> +
> >>> +    type PayloadType = u8;
> >>> +
> >>> +    pub(super) const BLOCK_HEADER_META_SIZE: usize = size_of::<BlockHeaderMeta>();
> >>> +    pub(super) const PAYLOAD_ELEMENT_SIZE: usize = size_of::<PayloadType>();
> >>> +
> >>> +    /// Dynamically sized type that represents a tape block header.
> >>> +    ///
> >>> +    /// This struct should never be created manually and is instead accessed through
> >>> +    /// its associated [`BlockHeader`] smart pointer.
> >>> +    #[repr(C, packed)]
> >>> +    pub struct BlockHeaderData {
> >>> +        meta: BlockHeaderMeta,
> >>> +        payload: [PayloadType],
> >>> +    }
> >>> +
> >>> +    impl BlockHeaderData {
> >>> +        /// Returns the magic value for the block header.
> >>> +        pub const fn magic(&self) -> [u8; 8] {
> >>> +            self.meta.magic()
> >>> +        }
> >>> +
> >>> +        pub(super) const fn set_magic(&mut self, magic: [u8; 8]) {
> >>> +            self.meta.magic = magic;
> >>> +        }
> >>> +
> >>> +        /// Returns the currently used [`BlockHeaderFlags`].
> >>> +        pub const fn flags(&self) -> BlockHeaderFlags {
> >>> +            self.meta.flags()
> >>> +        }
> >>> +
> >>> +        /// Sets the [`BlockHeaderFlags`] for the header.
> >>> +        pub const fn set_flags(&mut self, flags: BlockHeaderFlags) {
> >>> +            self.meta.set_flags(flags)
> >>> +        }
> >>> +
> >>> +        /// Returns the size of the header.
> >>> +        ///
> >>> +        /// Note that this value is at most `2^24 - 1`, since the size is represented using 24 bits
> >>> +        /// under the hood.
> >>> +        pub const fn size(&self) -> usize {
> >>> +            self.meta.size()
> >>> +        }
> >>> +
> >>> +        /// Sets the size of the header.
> >>> +        ///
> >>> +        /// Note that the passed value will be truncated to 24 bits, since the size is represented
> >>> +        /// using 24 bits under the hood.
> >>> +        pub const fn set_size(&mut self, size: usize) {
> >>> +            self.meta.set_size(size)
> >>> +        }
> >>> +
> >>> +        /// Sets the sequence number of the header.
> >>> +        pub const fn set_seq_nr(&mut self, seq_nr: u32) {
> >>> +            self.meta.set_seq_nr(seq_nr);
> >>> +        }
> >>> +
> >>> +        /// Returns the sequence number of the header.
> >>> +        pub const fn seq_nr(&self) -> u32 {
> >>> +            self.meta.seq_nr()
> >>> +        }
> >>> +
> >>> +        /// Returns the underlying payload as a slice.
> >>> +        pub const fn payload(&self) -> &[PayloadType] {
> >>> +            &self.payload
> >>> +        }
> >>> +
> >>> +        /// Returns the underlying payload as a mutable slice.
> >>> +        pub const fn payload_mut(&mut self) -> &mut [PayloadType] {
> >>> +            &mut self.payload
> >>> +        }
> >>> +
> >>> +        /// Returns the entirety of [`BlockHeaderData`], meaning both metadata
> >>> +        /// and payload, as a byte slice.
> >>> +        pub const fn as_bytes(&self) -> &[u8] {
> >>> +            unsafe {
> >>> +                std::slice::from_raw_parts(
> >>> +                    (self as *const BlockHeaderData) as *const u8,
> >>> +                    super::BlockHeader::SIZE,
> >>> +                )
> >>> +            }
> >>> +        }
> >>> +
> >>> +        /// Returns the entirety of [`BlockHeaderData`], meaning both metadata
> >>> +        /// and payload, as a mutable byte slice.
> >>> +        ///
> >>> +        /// While this method in itself is safe, be aware that it nevertheless
> >>> +        /// allows you to overwrite metadata in this struct.
> >>> +        pub const fn as_bytes_mut(&mut self) -> &mut [u8] {
> >>> +            unsafe {
> >>> +                std::slice::from_raw_parts_mut(
> >>> +                    (self as *mut BlockHeaderData) as *mut u8,
> >>> +                    super::BlockHeader::SIZE,
> >>> +                )
> >>> +            }
> >>> +        }
> >>> +    }
> >>> +
> >>> +    /// Metadata for tape block headers, which precedes the header's payload.
> >>> +    #[repr(C, packed)]
> >>> +    struct BlockHeaderMeta {
> >>> +        /// Fixed value, set to `PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0`.
> >>> +        magic: [u8; 8],
> >>> +        flags: BlockHeaderFlags,
> >>> +        /// Size as 3 bytes unsigned, little endian.
> >>> +        size: [u8; 3],
> >>> +        /// Block sequence number.
> >>> +        seq_nr: u32,
> >>> +    }
> >>> +
> >>> +    impl BlockHeaderMeta {
> >>> +        const fn magic(&self) -> [u8; 8] {
> >>> +            self.magic
> >>> +        }
> >>> +
> >>> +        const fn flags(&self) -> BlockHeaderFlags {
> >>> +            self.flags
> >>> +        }
> >>> +
> >>> +        const fn set_flags(&mut self, flags: BlockHeaderFlags) {
> >>> +            self.flags = flags;
> >>> +        }
> >>> +
> >>> +        const fn size(&self) -> usize {
> >>> +            (self.size[0] as usize)
> >>> +                + ((self.size[1] as usize) << 8)
> >>> +                + ((self.size[2] as usize) << 16)
> >>> +        }
> >>> +
> >>> +        const fn set_size(&mut self, size: usize) {
> >>> +            let size = size.to_le_bytes();
> >>> +            // Since we only need three bytes, we can copy them manually
> >>> +            // instead --> allows this function to be const, since range index
> >>> +            // is not yet const stable
> >> Seems kind of pointless to make these functions const if you cannot
> >> allocatate a value at compile time in the first place.
> > Good point, but there's also no downside in declaring them as const at
> > the same time. IMO it's just good hygiene to make things const if they
> > can be made const, but I don't mind dropping it if you insist.
> I was just confused about the rewrite.
>
> (In general I'd also be a bit careful about promising that a public
> function is const before there is a use for it, because that turns
> adding a side-effect like logging into a breaking change.)

That's a fair point, actually. I'll make these functions non-const for
now, since there isn't really any benefit in having them be const
anyway. (Though I do think that for small getters / setters it's
perfectly fine to make them const from the beginning, since it's very
rare to add logging inside those kinds of methods anyway, IMO.)

>
> >