[pbs-devel] [PATCH v3 proxmox-backup 0/2] prevent potentially unaligned FixedIndexHeader reference

[pbs-devel] [PATCH v3 proxmox-backup 0/2] prevent potentially unaligned FixedIndexHeader reference

[Robert Obkircher]

Changes since v2:
  - added description to first commit

Changes since [v1]:
   - handle allocation failure
   - reword commit message

[v1] https://lore.proxmox.com/pbs-devel/20251121143255.136721-1-r.obkircher@proxmox.com/

Robert Obkircher (2):
  datastore: check for null pointer when allocating DynamicIndexHeader
  datastore: prevent potentially unaligned FixedIndexHeader reference

 pbs-datastore/src/dynamic_index.rs |  9 +++++++-
 pbs-datastore/src/fixed_index.rs   | 35 +++++++++++++++++++++++-------
 2 files changed, 35 insertions(+), 9 deletions(-)

-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH v3 proxmox-backup 1/2] datastore: check for null pointer when allocating DynamicIndexHeader

[Robert Obkircher]

Check for allocation failures because creating a Box from a null
pointer is undefined behavior. Call handle_alloc_error to cease
execution, because returning an anyhow error would require further
allocations.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/dynamic_index.rs | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/pbs-datastore/src/dynamic_index.rs b/pbs-datastore/src/dynamic_index.rs
index ad49cdf3..12df78b1 100644
--- a/pbs-datastore/src/dynamic_index.rs
+++ b/pbs-datastore/src/dynamic_index.rs
@@ -41,13 +41,20 @@ proxmox_lang::static_assert_size!(DynamicIndexHeader, 4096);
 impl DynamicIndexHeader {
     /// Convenience method to allocate a zero-initialized header struct.
     pub fn zeroed() -> Box<Self> {
+        let layout = std::alloc::Layout::new::<Self>();
         unsafe {
-            Box::from_raw(std::alloc::alloc_zeroed(std::alloc::Layout::new::<Self>()) as *mut Self)
+            let ptr = std::alloc::alloc_zeroed(layout) as *mut Self;
+            if ptr.is_null() {
+                std::alloc::handle_alloc_error(layout);
+            }
+            Box::from_raw(ptr)
         }
     }
 
     pub fn as_bytes(&self) -> &[u8] {
         unsafe {
+            // There can't be any uninitialized padding, because the fields
+            // take up all of the statically asserted total size.
             std::slice::from_raw_parts(
                 self as *const Self as *const u8,
                 std::mem::size_of::<Self>(),
-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH v3 proxmox-backup 2/2] datastore: prevent potentially unaligned FixedIndexHeader reference

[Robert Obkircher]

Continue to avoid the 4 KiB stack allocation in debug builds by
adopting the approach used for the DynamicIndexHeader.

This also avoid mutation through Vec::as_ptr, which is forbidden
according to its documentation.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/fixed_index.rs | 35 ++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 6c3be2d4..7241d77f 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -29,6 +29,31 @@ pub struct FixedIndexHeader {
 }
 proxmox_lang::static_assert_size!(FixedIndexHeader, 4096);
 
+impl FixedIndexHeader {
+    /// Convenience method to allocate a zero-initialized header struct.
+    pub fn zeroed() -> Box<Self> {
+        let layout = std::alloc::Layout::new::<Self>();
+        unsafe {
+            let ptr = std::alloc::alloc_zeroed(layout) as *mut Self;
+            if ptr.is_null() {
+                std::alloc::handle_alloc_error(layout);
+            }
+            Box::from_raw(ptr)
+        }
+    }
+
+    pub fn as_bytes(&self) -> &[u8] {
+        unsafe {
+            // There can't be any uninitialized padding, because the fields
+            // take up all of the statically asserted total size.
+            std::slice::from_raw_parts(
+                self as *const Self as *const u8,
+                std::mem::size_of::<Self>(),
+            )
+        }
+    }
+}
+
 // split image into fixed size chunks
 
 pub struct FixedIndexReader {
@@ -237,7 +262,6 @@ impl Drop for FixedIndexWriter {
 }
 
 impl FixedIndexWriter {
-    #[allow(clippy::cast_ptr_alignment)]
     // Requires obtaining a shared chunk store lock beforehand
     pub fn create(
         store: Arc<ChunkStore>,
@@ -267,18 +291,13 @@ impl FixedIndexWriter {
 
         let uuid = Uuid::generate();
 
-        let buffer = vec![0u8; header_size];
-        let header = unsafe { &mut *(buffer.as_ptr() as *mut FixedIndexHeader) };
-
+        let mut header = FixedIndexHeader::zeroed();
         header.magic = file_formats::FIXED_SIZED_CHUNK_INDEX_1_0;
         header.ctime = i64::to_le(ctime);
         header.size = u64::to_le(size as u64);
         header.chunk_size = u64::to_le(chunk_size as u64);
         header.uuid = *uuid.as_bytes();
-
-        header.index_csum = [0u8; 32];
-
-        file.write_all(&buffer)?;
+        file.write_all(header.as_bytes())?;
 
         let index_length = size.div_ceil(chunk_size);
         let index_size = index_length * 32;
-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH v3 proxmox-backup 0/2] prevent potentially unaligned FixedIndexHeader reference

[Christian Ebner]

Patches look good to me. Tested backup/restore of CT/VM/host backups.

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
Tested-by: Christian Ebner <c.ebner@proxmox.com>


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel