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

[Robert Obkircher <r.obkircher@proxmox.com>]

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