Re: [PATCH v3] rust: alloc: fix dangling pointer in VecExt<T>::reserve()

[Danilo Krummrich <dakr@redhat.com>]

On 5/6/24 16:11, Wedson Almeida Filho wrote:
> On Wed, 1 May 2024 at 10:48, Danilo Krummrich <dakr@redhat.com> wrote:
>>
>> Currently, a Vec<T>'s ptr value, after calling Vec<T>::new(), is
>> initialized to Unique::dangling(). Hence, in VecExt<T>::reserve(), we're
>> passing a dangling pointer (instead of NULL) to krealloc() whenever a new
>> Vec<T>'s backing storage is allocated through VecExt<T> extension
>> functions.
>>
>> This only works as long as align_of::<T>(), used by Unique::dangling() to
>> derive the dangling pointer, resolves to a value between 0x0 and
>> ZERO_SIZE_PTR (0x10) and krealloc() hence treats it the same as a NULL
>> pointer however.
>>
>> This isn't a case we should rely on, since there may be types whose
>> alignment may exceed the range still covered by krealloc(), plus other
>> kernel allocators are not as tolerant either.
>>
>> Instead, pass a real NULL pointer to krealloc_aligned() if Vec<T>'s
>> capacity is zero.
>>
>> Fixes: 5ab560ce12ed ("rust: alloc: update `VecExt` to take allocation flags")
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
>> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
>> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> 
> Reviewed-by: Wedson Almeida Filho <walmeida@microsoft.com>
> 
>> +
>> +        // We need to make sure that `ptr` is either NULL or comes from a previous call to
>> +        // `krealloc_aligned`. A `Vec<T>`'s `ptr` value is not guaranteed to be NULL and might be
>> +        // dangling after being created with `Vec::new`. Instead, we can rely on `Vec<T>`'s capacity
>> +        // to be zero if no memory has been allocated yet.
>> +        let ptr = match cap {
>> +            0 => ptr::null_mut(),
>> +            _ => old_ptr,
>> +        };
> 
> I still think this should be an `if`.

Is there any benefit using an if here, or is that your personal preference?

> 
> What happened to adding the sample/test? It should be straightforward
> to do it...

As mentioned, I can send a patch for that soon.

> 
> Cheers,
> -Wedson
>