Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

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

On Mon Mar 17, 2025 at 4:57 PM CET, Danilo Krummrich wrote:
> On Mon, Mar 17, 2025 at 02:57:51PM +0000, Benno Lossin wrote:
>> On Mon Mar 17, 2025 at 12:12 PM CET, Danilo Krummrich wrote:
>> > On Mon, Mar 17, 2025 at 09:52:07AM +0000, Benno Lossin wrote:
>> >> On Sun Mar 16, 2025 at 8:09 PM CET, Danilo Krummrich wrote:
>> >> > On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
>> >> >> But let's define it then; what about:
>> >> >> 
>> >> >> "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
>> >> >> [0; `new_len`] and abandons ownership of all values outside of this range, if
>> >> >> any."
>> >> >> 
>> >> >> The caller may take ownership of the abandoned elements."
>> >> >> 
>> >> >> I'd argue that giving up ownership, while offering someone else to take it means
>> >> >> that it implies that otherwise we'll just end up forgetting about the value.
>> >> >
>> >> > Btw. I'd still prefer if we could enforce that the caller has to document what
>> >> > should happen to the abandoned value. But I acknowledge that the safety comment
>> >> > isn't the scope for it.
>> >> >
>> >> > It'd be great if e.g. clippy would give us a tool to do something analogous to
>> >> > safety comments.
>> >> >
>> >> > It think it would be useful to enfoce some additional safety documentation. For
>> >> > instance, I think the kernel would much benefit if we could enforce that
>> >> > mem::forget() must be justified with a comment, since as mentioned ina previous
>> >> > mail, it can cause fatal bugs, for instance when used on lock guards.
>> >> 
>> >> I get where you're coming from, but this probably will very quickly get
>> >> out of hand.
>> >> 
>> >> For example, I can define `forget` safely:
>> >> 
>> >>     fn forget<T>(value: T) {
>> >>         struct Cycle<T> {
>> >>             this: RefCell<Option<Arc<Self>>>,
>> >>             value: T,
>> >>         }
>> >>         let cycle = Arc::new(Cycle { this: RefCell::new(None), value });
>> >>         *cycle.this.borrow_mut() = Some(cycle.clone());
>> >>     }
>> >> 
>> >> How would you ensure that this kind of pattern doesn't get written
>> >> accidentally (or with many indirections)?
>> >
>> > I don't think that the possibility of writing safe (but yet buggy) code is an
>> > argument against having the possibility of enforcing that a caller must write a
>> > comment for justification on certain things, such as mem::forget().
>> 
>> My argument is that the problem of forgetting a value is not
>> self-contained like `unsafe` code is. Even if we were to document all
>> `forget` or `ManuallyDrop::new` invocations (which we definitely should)
>> we wouldn't get the security that one can't accidentally forget a lock
>> guard. I'm totally in favor of mandating an explaining comment above
>> `forget` calls (but not as a `SAFETY` comment).
>
> Oh, I see where the misunderstanding might lie.
>
> Let's take a look at FileDescriptorReservation::fd_install(). My proposal is to
> have something like:
>
> 	// SANITY: `fd_install` consumed file descriptor
> 	core::mem::forget(self);
> 	// SANITY: `fd_install` consumed file reference
> 	core::mem::forget(file);
>
> Where we have e.g. clippy to complain if there is no "SANITY" (or whatever we
> call it) comment for mem::forget().
>
> I'm not proposing a SAFETY comment.

Ok, that's good :)

As for having a dedicated `SANITY` comment, I'm not sold yet. I'll think
a bit more.

>> > But there's another reason I think having something like this could be
>> > problematic: It might set the wrong incentive, as in "hey, I can just use a
>> > "sanity requirement" in my function rather figuring out how to ensure it through
>> > the type system, etc.".
>> 
>> I don't understand your point here, can you explain it more?
>
> Does the explanation above make this concern clear for you?

Do you mean having sanity requirements would lead to people putting less
effort into designing a type system solution?

---
Cheers,
Benno