On 08.08.24 01:05, Danilo Krummrich wrote: > On Wed, Aug 07, 2024 at 08:15:41PM +0000, Benno Lossin wrote: >>> So, that's not permitted. `free` can't be called with a dangling pointer. The >>> kernel free functions (*1) can't handle it, and I can't detect it, since a >>> dangling pointer does not have a descrete value. >> >> That is true, but only if we do not have access to the old layout of the >> allocation. If we add `old_layout` as a parameter, then we can handle >> dangling pointers. > > Then we'd need `free` to be `fn free(ptr: NonNull<u8>, layout: Layout)` just to > compare whether `ptr.as_ptr() == layout.align() as _`. Same for `realloc`, but > that's less weird. I don't think that's a problem (though we would check against the size and not compare the address!). `Allocator` from stdlib also has the extra argument. > It's also not that we safe code with that. `Box`, `Vec`, any other user, still > would have to create the `Layout` before they call `A::free`. But for `Box` it would just be `Layout::<T>::new()` and `Vec` needs `Layout::<T>::new().repeat(self.cap)`. Though for `repeat` we need the `alloc_layout_extra` feature, which is an argument against doing this. >>> Surely, we could also let the caller pass the old alignment, but this all sounds >>> complicated for something that is very trivial for the caller to take care of, >>> i.e. just don't try to free something that was never actually allocated. >>> >>> It can also lead to subtle bugs, e.g. what if someone calls `Box::from_raw` for >>> a ZST with some other random pointer? Currently, that doesn't hurt us, which for >>> robustness, seems to be a good thing. >> >> I think we should forbid that. To me it's just plain wrong to take a >> random integer literal and cast it to a ZST. IIRC it even is UB if that >> points to a previously allocated object that has been freed (but I don't >> remember where I read it...). > > I think my argument about robustness still holds even if we forbid it. > > The documentation says "For operations of size zero, every pointer is valid, > including the null pointer." [1] How does this increase robustness? I am not allowed to call `free` with a random pointer, only pointers returned by that allocator. The same should also be true for `Box::from_raw`. That way the ZST dangling pointer stuff leaks into that API. > [1] https://doc.rust-lang.org/std/ptr/index.html > >> >> Also if we use the size of the old layout instead of comparing alignment >> with the address of the pointer, then we avoid this issue. > > That is just another problem when passing the old `Layout` (or maybe just the > old size as usize). Neither do we need the old size, nor do we honor it with any > kernel allocator. This has the following implications: > > (1) We never see any error if the old size that is passed is garbage (unless > it's non-zero, when it should be zero and vice versa), which is bad. We could add debug support for that though? > (2) We'd need `free` to be `fn free(ptr: NonNull<u8>, size: usize)`, which is > confusing, because it implies that an actual free relies on this size for > freeing the memory. I don't think that it is confusing to ask for the old layout. > If we want to avoid (1) and (2), we'd need to make it > `fn free(ptr: NonNull<u8>, zero: bool)` instead, but then also the caller can > just check this boolean and conditionally call `free`. Yeah having `free` with a `zero: bool` sounds like a bad idea. > I don't really see why it's better to let `free` do the `if !zero` check. You did not reply to my last argument: >> I think it's better to just let `Box` and `Vec` figure out if calling `free` is >> the right thing to do. The code for that is simple and obvious, i.e. check if >> `T` is a ZST. > > I don't think that it is as simple as you make it out to be. To me this > is functionality that we can move into one place and never have to think > about again. > Also if we at some point decide to add some sort of debugging allocator > (am I right in assuming that such a thing already exists for C?) then we > might want to tag on extra data in the allocation, in that case it would > be very useful if the allocator also saw zero-sized allocations, since > we should check all allocations. --- Cheers, Benno
