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

[Christian Ebner <c.ebner@proxmox.com>]

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