On Tue, Aug 06, 2024 at 08:04:30PM +0000, Benno Lossin wrote:
> >>> + /// instance. The alignment encoded in `layout` must be smaller than or equal to the alignment
> >>> + /// requested in the previous `alloc` or `realloc` call of the same allocation.
> >>> + ///
> >>> + /// Additionally, `ptr` is allowed to be `None`; in this case a new memory allocation is
> >>> + /// created.
> >>> + ///
> >>> + unsafe fn realloc(
> >>> + ptr: Option<NonNull<u8>>,
> >>> + layout: Layout,
> >>> + flags: Flags,
> >>> + ) -> Result<NonNull<[u8]>, AllocError>;
> >>> +
> >>> + /// Free an existing memory allocation.
> >>> + ///
> >>> + /// # Safety
> >>> + ///
> >>> + /// `ptr` must point to an existing and valid memory allocation created by this `Allocator`
> >>> + /// instance.
> >>
> >> Additionally, you need "The memory allocation at `ptr` must never again
> >> be read from or written to.".
> >
> > I'm fine adding it, but I wonder if technically this is really required? The
> > condition whether the pointer is ever accessed again in any way is not relevant
> > in terms of being a precondition for `free` not causing UB, right?
>
> I don't see how else we would find the mistake in the following code:
>
> let ptr = Box::into_raw(Box::<i32, Kmalloc>::new(42));
> // SAFETY: `ptr` came from `Box::into_raw` and thus is pointing to a
> // valid and existing memory allocation allocated by `Kmalloc`.
> unsafe { Kmalloc::free(ptr) };
> // SAFETY: `ptr` came from `Box::into_raw` and thus is pointing at a
> // valid `i32`.
> let v = unsafe { ptr.read() };
Sure, but what I mean is that my understanding is that the "Safety" section in a
comment describes the requirements of the function it documents. I.e. `free`
itself doesn't care whether the pointer is read or writted ever again.
Or in other words, what are the rules where this belongs to? E.g. why not
document this exact aspect in the safety section of `Allocator`?
>
> Also see the `from_raw` for our `Arc`:
>
> /// Recreates an [`Arc`] instance previously deconstructed via [`Arc::into_raw`].
> ///
> /// # Safety
> ///
> /// `ptr` must have been returned by a previous call to [`Arc::into_raw`]. Additionally, it
> /// must not be called more than once for each previous call to [`Arc::into_raw`].
> pub unsafe fn from_raw(ptr: *const T) -> Self {
>
> That also requires that the function must not be called more than once.
> This reminds me, I forgot to say that about `Box::from_raw`.
Indeed, I also wonder if we ever have cases where C code gives us ownership of a
memory allocation of a certain type that fulfills the requirements we have for
a `Box`, such that Rust code is tempted to pass it to `Box::from_raw`.
It sounds a bit scary design wise, but in theory it's possible.
>
> ---
> Cheers,
> Benno
>
