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

[Danilo Krummrich <dakr@redhat.com>]

On Tue, Apr 30, 2024 at 04:25:44PM -0300, Wedson Almeida Filho wrote:
> On Tue, 30 Apr 2024 at 09:13, 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> is created through VecExt<T> extension functions.
> 
> Nice catch, thanks!
> 
> A small nit on the wording above: this applies to any Vec<T>, not just
> those created via VecExt<T>. For example, if we create with

True, I think I wanted to say "whenever a new Vec<T>'s backing storage is
allocated through VecExt<T> extension functions".

> Vec::new(), then use VecExt::push or VecExt::reserve, we'll run into
> this. (IOW, which function is used to create the Vec is not a factor,
> using VecExt to extend it from 0 to >0 is.)
> 
> >
> > 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.
> 
> Perhaps it would make sense to add a sample with such a type?
> 
> It would serve as a test as well when kunit and doctests are enabled.

Yeah, that sounds reasonable. Let me see whether I can get to that.

> 
> >          let (ptr, len, cap) = destructure(self);
> >
> > +        // 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(),
> > +            _ => ptr,
> > +        };
> > +
> 
> nit: why did you choose to use a match here?

I felt like it reads nicely.

> I don't think C
> programmers would use a switch to determine if a value is zero or
> non-zero.

Well, I think for writing Rust code it doesn't matter too much what C
programmers would do? ;-)

What is idiomatic in this case?

> 
> I would have written:
> 
> let ptr = if cap != 0 { ptr } else { ptr::null_mut() };
> 
> >          // SAFETY: `ptr` is valid because it's either NULL or comes from a previous call to
> >          // `krealloc_aligned`. We also verified that the type is not a ZST.
> >          let new_ptr = unsafe { super::allocator::krealloc_aligned(ptr.cast(), layout, flags) };
> 
> In the case when reallocation fails, we need to rebuild with the
> original pointer (the dangling one if cap is zero). This patch is
> rebuilding it with null when cap is zero, which is incorrect.
> 

Indeed, good catch. Gonna fix that up in a v3.