Re: [PATCH v4 01/28] rust: alloc: add `Allocator` trait

[Benno Lossin <benno.lossin@proton.me>]

On 07.08.24 11:36, Danilo Krummrich wrote:
> 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.

So if you have an `unsafe` function, the safety requirements are not
limited to ensuring that that function does not exhibit UB. But in
general any correct usage of that function must not exhibit UB in
combination with any other correct usage of any other `unsafe`
functions.

> 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`?

That doesn't work, since the Safety section of `Allocator` is meant for
safety requirements for implementing `Allocator`. They should not be
relevant for using it.

>> 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.

I don't think it's scary, in fact we are already doing it and have a
trait for that: `ForeignOwnable`. But that means that the pointer
originates from Rust, so it might not exactly be what you meant ie C
allocates the memory. But that case also doesn't seem too scary for me.
C just needs to respect alignment and then it should be equivalent to
Rust calling the allocator.

---
Cheers,
Benno