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

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

[Robert Obkircher]

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 v2 proxmox-backup 1/2] datastore: check for null pointer when allocating DynamicIndexHeader

[Robert Obkircher]

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 v2 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 v2 proxmox-backup 1/2] datastore: check for null pointer when allocating DynamicIndexHeader

[Christian Ebner]

Please provide a short commit message on why this is done.

As far as I see this now panics in handle_alloc_error() if either memory 
is exhausted or the layout does not fit the allocator constraints, while 
previously this failed on Box::from_raw() for the null pointer?

So maybe better to propagate the allocation error to the call site of 
zeroed() in DynamicIndexWriter::create() and return it there as well?

On 12/30/25 1:42 PM, Robert Obkircher wrote:
> 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>(),



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

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

[Robert Obkircher]

On 1/7/26 14:09, Christian Ebner wrote:
> Please provide a short commit message on why this is done.
Ok, I'll do that tomorrow, after I've finished the next version of "pipe 
to stdin".

>
> As far as I see this now panics in handle_alloc_error() if either 
> memory is exhausted or the layout does not fit the allocator 
> constraints, while previously this failed on Box::from_raw() for the 
> null pointer?

No, in the previous version this was undefined behavior, which is why I 
assumed it should be ok to panic. For example, constructing a box from a 
null can result in `Some(reference) == None` being true [1].

I've also seen the same issue in multiple other places. For example this 
`uninitialized` function [2] is copy-pasted twice. That one is also 
super dangerous, because if you accidentally read from it the compiler 
will most likely delete your code [3]. Creating an uninitialized &mut 
slice itself currently seems to be ok with a lot of asterisks though [4].

[1] https://godbolt.org/z/Ezfqbhqz3
[2] https://godbolt.org/z/ahx6vs8qz
[3] https://godbolt.org/z/4MePEKf79
[4] https://github.com/rust-lang/unsafe-code-guidelines/issues/346

>
>
> So maybe better to propagate the allocation error to the call site of 
> zeroed() in DynamicIndexWriter::create() and return it there as well?
I don't think we can reasonably deal with that situation, because 
returning an anyhow error most likely also requires allocation.

On a Linux system with overcommit enabled, allocations will probably 
never fail anyway, unless the size is something huge like isize::MAX, 
which is more likely to happen for an incorrectly sized Vec than for a Box.

>
> On 12/30/25 1:42 PM, Robert Obkircher wrote:
>> 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>(),
>


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

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

[Christian Ebner]

On 1/7/26 3:28 PM, Robert Obkircher wrote:
> 
> On 1/7/26 14:09, Christian Ebner wrote:
>> Please provide a short commit message on why this is done.
> Ok, I'll do that tomorrow, after I've finished the next version of "pipe 
> to stdin".
> 
>>
>> As far as I see this now panics in handle_alloc_error() if either 
>> memory is exhausted or the layout does not fit the allocator 
>> constraints, while previously this failed on Box::from_raw() for the 
>> null pointer?

Okay, thanks for clarification!

> 
> No, in the previous version this was undefined behavior, which is why I 
> assumed it should be ok to panic. For example, constructing a box from a 
> null can result in `Some(reference) == None` being true [1].
> 
> I've also seen the same issue in multiple other places. For example this 
> `uninitialized` function [2] is copy-pasted twice. That one is also 
> super dangerous, because if you accidentally read from it the compiler 
> will most likely delete your code [3]. Creating an uninitialized &mut 
> slice itself currently seems to be ok with a lot of asterisks though [4].
> 
> [1] https://godbolt.org/z/Ezfqbhqz3
> [2] https://godbolt.org/z/ahx6vs8qz
> [3] https://godbolt.org/z/4MePEKf79
> [4] https://github.com/rust-lang/unsafe-code-guidelines/issues/346
> 
>>
>>
>> So maybe better to propagate the allocation error to the call site of 
>> zeroed() in DynamicIndexWriter::create() and return it there as well?
> I don't think we can reasonably deal with that situation, because 
> returning an anyhow error most likely also requires allocation.
> 
> On a Linux system with overcommit enabled, allocations will probably 
> never fail anyway, unless the size is something huge like isize::MAX, 
> which is more likely to happen for an incorrectly sized Vec than for a Box.

True, also allocation here is 4K only.

> 
>>
>> On 12/30/25 1:42 PM, Robert Obkircher wrote:
>>> 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>(),
>>



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