* [PATCH proxmox-backup 1/2] pbs-tape: fix undefined behavior in block header deallocation
2026-08-05 14:13 [PATCH proxmox-backup 0/2] Fix Undefined Behavior in Tape Block Header Deallocation Max R. Carrara
@ 2026-08-05 14:13 ` Max R. Carrara
2026-08-05 14:13 ` [PATCH proxmox-backup 2/2] pbs-tape: rename `buffer` to `header` Max R. Carrara
2026-08-05 14:15 ` [PATCH proxmox-backup 0/2] Fix Undefined Behavior in Tape Block Header Deallocation Max R. Carrara
2 siblings, 0 replies; 4+ messages in thread
From: Max R. Carrara @ 2026-08-05 14:13 UTC (permalink / raw)
To: pve-devel
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()`.
While this should be harmless with the current standard library on
*nix and WASM, there is no guarantee that this won't change in the
future. If we were to target Windows, it would already be a problem [1].
To solve this, promote `BlockHeader` to a smart pointer instead, which
manages the allocation of a new `BlockHeaderData` struct, whose
purpose is to hold the actual data. `BlockHeaderData` has the exact
same field ordering, layout and alignment as the previous
implementation of `BlockHeader`, but is confined to a private module
and then re-exported to prevent accessing its fields directly.
For convenience, move any "metadata" fields into a newly introduced
`BlockHeaderMeta`, which is not exposed publicly. This makes computing
the payload's size more explicit, as we can now subtract
`size_of::<BlockHeaderMeta>()` from the total block size instead of
using a hard-coded numeric constant.
While the data format is unlikely to change -- and if it does, we
would need a new struct with a new magic constant for the anyways --
this nevertheless should make understanding the code easier for future
readers, in particular because dynamically sized types are not that
trivial if one is not used to them.
Furthermore, store the layout used for block header allocations using
a `LazyLock`, which allows us to avoid querying the currently used
page size on each new block allocation. We can do this because the
page size is a constant [2].
Additionally, implement various helper methods on `BlockHeaderData`
that make handling the block header a little less error-prone.
Notably, `unsafe` calls to `slice::from_raw_parts` and
`slice::from_raw_parts_mut` are implemented as methods instead of
being called directly in the reader / writer code.
Finally, sites using `BlockHeader` are adapted corresponding to the
changes above, which mainly means that direct field accesses are
replaced with method calls in most cases.
[0]: https://github.com/rust-lang/miri
[1]: https://github.com/rust-lang/rust/blob/c9ff496891c278ad660bc0ab85c1f0b72059464a/library/std/src/sys/alloc/windows.rs#L182
[2]: `man 3 sysconf`
Reported-by: Robert Obkircher <r.obkircher@proxmox.com>
Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
pbs-tape/src/blocked_reader.rs | 34 ++--
pbs-tape/src/blocked_writer.rs | 35 ++--
pbs-tape/src/lib.rs | 352 +++++++++++++++++++++++++++------
3 files changed, 328 insertions(+), 93 deletions(-)
diff --git a/pbs-tape/src/blocked_reader.rs b/pbs-tape/src/blocked_reader.rs
index 22803371c..33c5db780 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<BlockHeader>,
+ buffer: BlockHeader,
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 = BlockHeader::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(BlockHeaderFlags::INCOMPLETE);
Self::consume_eof_marker(&mut reader)?;
got_eod = true;
}
@@ -61,7 +61,7 @@ impl<R: BlockRead> BlockedReader<R> {
}
fn check_buffer(buffer: &BlockHeader, seq_nr: u32) -> Result<(usize, bool), std::io::Error> {
- if buffer.magic != PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0 {
+ 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,13 +76,13 @@ 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(BlockHeaderFlags::END_OF_STREAM);
- if size > buffer.payload.len() {
+ if size > buffer.payload().len() {
proxmox_lang::io_bail!(
"detected tape block with wrong payload size ({} > {}",
size,
- buffer.payload.len()
+ buffer.payload().len()
);
} else if size == 0 && !found_end_marker {
proxmox_lang::io_bail!("detected tape block with zero payload size");
@@ -92,14 +92,7 @@ impl<R: BlockRead> BlockedReader<R> {
}
fn read_block_frame(buffer: &mut BlockHeader, reader: &mut R) -> Result<(), BlockReadError> {
- let data = unsafe {
- std::slice::from_raw_parts_mut(
- (buffer as *mut BlockHeader) as *mut u8,
- BlockHeader::SIZE,
- )
- };
-
- let bytes = reader.read_block(data)?;
+ let bytes = reader.read_block(buffer.as_bytes_mut())?;
if bytes != BlockHeader::SIZE {
return Err(proxmox_lang::io_format_err!("got wrong block size").into());
@@ -127,7 +120,7 @@ impl<R: BlockRead> BlockedReader<R> {
Ok(()) => { /* ok */ }
Err(BlockReadError::EndOfFile) => {
self.got_eod = true;
- self.read_pos = self.buffer.payload.len();
+ self.read_pos = self.buffer.payload().len();
if !self.found_end_marker && check_end_marker {
proxmox_lang::io_bail!("detected tape stream without end marker");
}
@@ -147,7 +140,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(BlockHeaderFlags::INCOMPLETE);
Self::consume_eof_marker(&mut self.reader)?;
self.got_eod = true;
}
@@ -225,8 +218,11 @@ 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)]);
+
+ let (read_start, read_end) = (self.read_pos, self.read_pos + copy_len);
+ let payload = self.buffer.payload();
+ buffer[..copy_len].copy_from_slice(&payload[read_start..read_end]);
+
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 7380af243..57721ed4a 100644
--- a/pbs-tape/src/blocked_writer.rs
+++ b/pbs-tape/src/blocked_writer.rs
@@ -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: BlockHeader,
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: BlockHeader::new().expect("failed to create block header"),
buffer_pos: 0,
seq_nr: 0,
logical_end_of_media: false,
@@ -46,13 +46,7 @@ impl<W: BlockWrite> BlockedWriter<W> {
}
fn write_block(buffer: &BlockHeader, 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,
- )
- };
- writer.write_block(data)
+ writer.write_block(buffer.as_bytes())
}
fn write_eof(&mut self) -> Result<(), std::io::Error> {
@@ -69,16 +63,20 @@ impl<W: BlockWrite> BlockedWriter<W> {
return Ok(0);
}
- let rest = self.buffer.payload.len() - self.buffer_pos;
+ let rest = self.buffer.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)]
- .copy_from_slice(&data[..bytes]);
+
+ let (write_start, write_end) = (self.buffer_pos, self.buffer_pos + bytes);
+
+ let payload = self.buffer.payload_mut();
+ payload[write_start..write_end].copy_from_slice(&data[..bytes]);
let rest = rest - bytes;
if rest == 0 {
- self.buffer.flags = BlockHeaderFlags::empty();
- self.buffer.set_size(self.buffer.payload.len());
+ self.buffer.set_flags(BlockHeaderFlags::empty());
+ let payload_len = self.buffer.payload().len();
+ self.buffer.set_size(payload_len);
self.buffer.set_seq_nr(self.seq_nr);
self.seq_nr += 1;
let leom = Self::write_block(&self.buffer, &mut self.writer)?;
@@ -115,10 +113,13 @@ 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 = BlockHeaderFlags::END_OF_STREAM;
+ let payload = self.buffer.payload_mut();
+ vec::clear(&mut payload[self.buffer_pos..]);
+
+ self.buffer.set_flags(BlockHeaderFlags::END_OF_STREAM);
if incomplete {
- self.buffer.flags |= BlockHeaderFlags::INCOMPLETE;
+ let flags = self.buffer.flags() | BlockHeaderFlags::INCOMPLETE;
+ self.buffer.set_flags(flags);
}
self.buffer.set_size(self.buffer_pos);
self.buffer.set_seq_nr(self.seq_nr);
diff --git a/pbs-tape/src/lib.rs b/pbs-tape/src/lib.rs
index 0fe55a749..f77d83d37 100644
--- a/pbs-tape/src/lib.rs
+++ b/pbs-tape/src/lib.rs
@@ -1,4 +1,8 @@
+use std::alloc;
use std::collections::HashSet;
+use std::io;
+use std::ptr::NonNull;
+use std::sync::LazyLock;
use anyhow::{Error, bail};
use bitflags::bitflags;
@@ -49,28 +53,148 @@ 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
+static BLOCK_HEADER_ALLOC_LAYOUT: LazyLock<alloc::Layout> = 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 == -1 {
+ panic!("failed to query PAGESIZE");
+ }
+
+ // Aligned to PAGESIZE, so that we can use it with SG_IO
+ let layout = alloc::Layout::from_size_align(BlockHeader::SIZE, page_size as usize)
+ .expect("creating layout with page size alignment failed");
+
+ debug_assert!(layout.size() != 0, "layout has a size of zero!");
+
+ layout
+});
+
+/// 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
+/// Tape data blocks are always read/written with a fixed size
+/// (`PROXMOX_TAPE_BLOCK_SIZE`), but since they may contain less data, the
+/// header also 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)]
+///
+/// This struct acts as a smart pointer to the underlying block header
+/// implementation, [`BlockHeaderData`], which is a dynamically sized type.
+/// [`BlockHeaderData`]'s memory is aligned to `PAGESIZE`, but because it uses a
+/// `packed` layout, the compiler will think its alignment is `1` instead.
+///
+/// Since Rust accounts for the alignment even on deallocation, this
+/// smart pointer therefore ensures that [`BlockHeaderData`] is deallocated
+/// with the same alignment that was used when it was allocated. Using a
+/// different alignment would otherwise be undefined behavior, which [`miri`]
+/// would report as such.
+///
+/// [`miri`]: https://github.com/rust-lang/miri
+#[repr(transparent)]
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],
+ ptr: NonNull<BlockHeaderData>,
+}
+
+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;
+
+ 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",
+ ));
+ };
+
+ // 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.
+ 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.
+ 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
+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
+ self.size[0] = size[0];
+ self.size[1] = size[1];
+ self.size[2] = size[2];
+ }
+
+ const fn set_seq_nr(&mut self, seq_nr: u32) {
+ self.seq_nr = seq_nr.to_le();
+ }
+
+ const fn seq_nr(&self) -> u32 {
+ u32::from_le(self.seq_nr)
+ }
+ }
+}
+
+pub use _impl::BlockHeaderData;
+
#[derive(Endian, Copy, Clone, Debug)]
#[repr(C, packed)]
/// Media Content Header
@@ -152,46 +430,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
--
2.47.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH proxmox-backup 2/2] pbs-tape: rename `buffer` to `header`
2026-08-05 14:13 [PATCH proxmox-backup 0/2] Fix Undefined Behavior in Tape Block Header Deallocation Max R. Carrara
2026-08-05 14:13 ` [PATCH proxmox-backup 1/2] pbs-tape: fix undefined behavior in block header deallocation Max R. Carrara
@ 2026-08-05 14:13 ` Max R. Carrara
2026-08-05 14:15 ` [PATCH proxmox-backup 0/2] Fix Undefined Behavior in Tape Block Header Deallocation Max R. Carrara
2 siblings, 0 replies; 4+ messages in thread
From: Max R. Carrara @ 2026-08-05 14:13 UTC (permalink / raw)
To: pve-devel
... since `BlockHeader` is now a smart pointer.
Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
pbs-tape/src/blocked_reader.rs | 46 +++++++++++++++++-----------------
pbs-tape/src/blocked_writer.rs | 36 +++++++++++++-------------
2 files changed, 41 insertions(+), 41 deletions(-)
diff --git a/pbs-tape/src/blocked_reader.rs b/pbs-tape/src/blocked_reader.rs
index 33c5db780..9ad16c91f 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: BlockHeader,
+ header: BlockHeader,
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 = BlockHeader::new()?;
+ let mut header = BlockHeader::new()?;
- Self::read_block_frame(&mut buffer, &mut reader)?;
+ Self::read_block_frame(&mut header, &mut reader)?;
- let (_size, found_end_marker) = Self::check_buffer(&buffer, 0)?;
+ let (_size, found_end_marker) = Self::check_buffer(&header, 0)?;
let mut incomplete = false;
let mut got_eod = false;
if found_end_marker {
- incomplete = buffer.flags().contains(BlockHeaderFlags::INCOMPLETE);
+ incomplete = header.flags().contains(BlockHeaderFlags::INCOMPLETE);
Self::consume_eof_marker(&mut reader)?;
got_eod = true;
}
Ok(Self {
reader,
- buffer,
+ header,
found_end_marker,
incomplete,
got_eod,
@@ -60,29 +60,29 @@ impl<R: BlockRead> BlockedReader<R> {
})
}
- fn check_buffer(buffer: &BlockHeader, seq_nr: u32) -> Result<(usize, bool), std::io::Error> {
- if buffer.magic() != PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0 {
+ fn check_buffer(header: &BlockHeader, seq_nr: u32) -> Result<(usize, bool), std::io::Error> {
+ if header.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 != header.seq_nr() {
proxmox_lang::io_bail!(
"detected tape block with wrong sequence number ({} != {})",
seq_nr,
- buffer.seq_nr()
+ header.seq_nr()
)
}
- let size = buffer.size();
- let found_end_marker = buffer.flags().contains(BlockHeaderFlags::END_OF_STREAM);
+ let size = header.size();
+ let found_end_marker = header.flags().contains(BlockHeaderFlags::END_OF_STREAM);
- if size > buffer.payload().len() {
+ if size > header.payload().len() {
proxmox_lang::io_bail!(
"detected tape block with wrong payload size ({} > {}",
size,
- buffer.payload().len()
+ header.payload().len()
);
} else if size == 0 && !found_end_marker {
proxmox_lang::io_bail!("detected tape block with zero payload size");
@@ -91,8 +91,8 @@ impl<R: BlockRead> BlockedReader<R> {
Ok((size, found_end_marker))
}
- fn read_block_frame(buffer: &mut BlockHeader, reader: &mut R) -> Result<(), BlockReadError> {
- let bytes = reader.read_block(buffer.as_bytes_mut())?;
+ fn read_block_frame(header: &mut BlockHeader, reader: &mut R) -> Result<(), BlockReadError> {
+ let bytes = reader.read_block(header.as_bytes_mut())?;
if bytes != BlockHeader::SIZE {
return Err(proxmox_lang::io_format_err!("got wrong block size").into());
@@ -116,11 +116,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.header, &mut self.reader) {
Ok(()) => { /* ok */ }
Err(BlockReadError::EndOfFile) => {
self.got_eod = true;
- self.read_pos = self.buffer.payload().len();
+ self.read_pos = self.header.payload().len();
if !self.found_end_marker && check_end_marker {
proxmox_lang::io_bail!("detected tape stream without end marker");
}
@@ -134,13 +134,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_buffer(&self.header, 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(BlockHeaderFlags::INCOMPLETE);
+ self.incomplete = self.header.flags().contains(BlockHeaderFlags::INCOMPLETE);
Self::consume_eof_marker(&mut self.reader)?;
self.got_eod = true;
}
@@ -175,7 +175,7 @@ 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 buffer_size = self.header.size();
let rest = (buffer_size as isize) - (self.read_pos as isize);
if rest > 0 {
bytes = rest as usize;
@@ -195,7 +195,7 @@ 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 buffer_size = self.header.size();
let mut rest = (buffer_size as isize) - (self.read_pos as isize);
if rest <= 0 && !self.got_eod {
@@ -220,7 +220,7 @@ impl<R: BlockRead> Read for BlockedReader<R> {
};
let (read_start, read_end) = (self.read_pos, self.read_pos + copy_len);
- let payload = self.buffer.payload();
+ let payload = self.header.payload();
buffer[..copy_len].copy_from_slice(&payload[read_start..read_end]);
self.read_pos += copy_len;
diff --git a/pbs-tape/src/blocked_writer.rs b/pbs-tape/src/blocked_writer.rs
index 57721ed4a..6ca621716 100644
--- a/pbs-tape/src/blocked_writer.rs
+++ b/pbs-tape/src/blocked_writer.rs
@@ -9,7 +9,7 @@ use crate::{BlockHeader, BlockHeaderFlags, BlockWrite, TapeWrite};
/// to the underlying writer.
pub struct BlockedWriter<W: BlockWrite> {
writer: W,
- buffer: BlockHeader,
+ header: BlockHeader,
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().expect("failed to create block header"),
+ header: BlockHeader::new().expect("failed to create block header"),
buffer_pos: 0,
seq_nr: 0,
logical_end_of_media: false,
@@ -45,8 +45,8 @@ impl<W: BlockWrite> BlockedWriter<W> {
}
}
- fn write_block(buffer: &BlockHeader, writer: &mut W) -> Result<bool, std::io::Error> {
- writer.write_block(buffer.as_bytes())
+ fn write_block(header: &BlockHeader, writer: &mut W) -> Result<bool, std::io::Error> {
+ writer.write_block(header.as_bytes())
}
fn write_eof(&mut self) -> Result<(), std::io::Error> {
@@ -63,23 +63,23 @@ impl<W: BlockWrite> BlockedWriter<W> {
return Ok(0);
}
- let rest = self.buffer.payload().len() - self.buffer_pos;
+ let rest = self.header.payload().len() - self.buffer_pos;
let bytes = if data.len() < rest { data.len() } else { rest };
let (write_start, write_end) = (self.buffer_pos, self.buffer_pos + bytes);
- let payload = self.buffer.payload_mut();
+ let payload = self.header.payload_mut();
payload[write_start..write_end].copy_from_slice(&data[..bytes]);
let rest = rest - bytes;
if rest == 0 {
- self.buffer.set_flags(BlockHeaderFlags::empty());
- let payload_len = self.buffer.payload().len();
- self.buffer.set_size(payload_len);
- self.buffer.set_seq_nr(self.seq_nr);
+ self.header.set_flags(BlockHeaderFlags::empty());
+ let payload_len = self.header.payload().len();
+ self.header.set_size(payload_len);
+ self.header.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.header, &mut self.writer)?;
if leom {
self.logical_end_of_media = true;
}
@@ -113,19 +113,19 @@ 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> {
- let payload = self.buffer.payload_mut();
+ let payload = self.header.payload_mut();
vec::clear(&mut payload[self.buffer_pos..]);
- self.buffer.set_flags(BlockHeaderFlags::END_OF_STREAM);
+ self.header.set_flags(BlockHeaderFlags::END_OF_STREAM);
if incomplete {
- let flags = self.buffer.flags() | BlockHeaderFlags::INCOMPLETE;
- self.buffer.set_flags(flags);
+ let flags = self.header.flags() | BlockHeaderFlags::INCOMPLETE;
+ self.header.set_flags(flags);
}
- self.buffer.set_size(self.buffer_pos);
- self.buffer.set_seq_nr(self.seq_nr);
+ self.header.set_size(self.buffer_pos);
+ self.header.set_seq_nr(self.seq_nr);
self.seq_nr += 1;
self.bytes_written += BlockHeader::SIZE;
- let leom = Self::write_block(&self.buffer, &mut self.writer)?;
+ let leom = Self::write_block(&self.header, &mut self.writer)?;
self.write_eof()?;
Ok(leom)
}
--
2.47.3
^ permalink raw reply related [flat|nested] 4+ messages in thread