Look into drop powered panic guards

[SUPERCILEX]

Origin: #102023 (comment)

It's a bit weird to have forget and ManuallyDrop, especially since forget is just a let _ = ManuallyDrop::new(t); under the hood. The primary usage of forget that still makes sense is for panic guards, but using forget has the disadvantage of being unclear in its intent. Removing forget would probably make panic guards clearer as they would encourage writing a comment or seperate method explaining what's happening.

[thomcc]

Strong -1 from me. That's a lot of churn and there are still totally reasonable cases to use mem::forget, although I do agree that in many cases where you can either use ManuallyDrop or mem::forget, ManuallyDrop is better.

More concretely, I think that let _ = ManuallyDrop::new(t); is a much less clear way to accomplish the same thing as mem::forget.

[RalfJung]

The point that came up in #102023 was that forget is often used for guards, and maybe it'd be more clear if guards had a method to 'defuse' them rather than people having to se a method in std::mem? In fact when talking about memory, you almost always want ManuallyDrop and not forget, as the docs indicate. mem::forget is only good for non-memory uses, which is unfortunate given the module it lives in.

[thomcc]

It's also reasonable to use if you want to run a function that returns an RAII type and just prevent its destructor from running. That is, cases like forget(something.lock().unwrap()) are reasonable.

I agree that perhaps it's in the wrong module, but I don't think it's worth the degree of ecosystem churn that would be required to change it (I also don't know where else would be better).

[RalfJung]

That is exactly the guard case I mentioned, which maybe should be something.lock().unwrap().defuse() or something.lock().unwrap().forget() or so.

I am not suggesting to deprecate mem::forget, so this issue should be closed IMO, but we should have an issue tracking whether we want such defuse/forget methods.

[thomcc]

I think it would be pretty confusing for a MutexGuard, and I don't really think a dedicated method makes sense in all cases (it's quite rare that you want to leave a lock locked).

[ChrisDenton]

See also #62553. It's true that mem::forget can be a subtle footgun in unsafe code because of the uniqueness assertion which is easy to overlook. But I'm not sure if that warrants deprecation.

I'd also note that a large part of the docs for mem::forget are devoted to describing alternatives.

[thomcc]

I think a better solution there is to not make Vec/String/CString/Box assert uniqueness on typed move/copy. It doesn't help much for optimizations (reads and writes to these types still go through &/&mut respectively), and is a huge footgun for a number of reasons.

[RalfJung]

That's rust-lang/unsafe-code-guidelines#326.

Are you suggesting we entirely remove the Unique here and the noalias annotations in codegen, or some middle-ground between 'just a boring NonNull pointer' and 'full uniqueness'?

[SUPERCILEX]

For the MutexGuard case, I can see forget kind of making sense? Then again, keeping a mutex locked forever is a super weird thing to do, so maybe the code that does so should look weird enough to wake people up during review.

---

Regarding the PanicGuards of the stdlib, I thought about this some more and I'm now extremely opposed to the current approach. It is fundamentally flawed in that it misattributes the unsafety of the program. The current approach is as follows:

PanicGuard = unsafe { deallocate memory on drop }

f() {
  let guard = PanicGuard(memory);
  do_stuff();
  // Yay, no panic
  forget(guard)
}

The unsafe is in the PanicGuard, but this is wrong!!! The unsafety is actually in the lack of code. If a developer forgets to forget (lol), they will UAF. This means that it should be unsafe to not call forget which can't be expressed in the language AFAIK. I haven't dug around, but I'd be kind of surprised if there hasn't been some bug caused by a missing forget.

Proposal: rewrite all panic guards to always drop and then check for thread::panicking to decide whether or not to deallocate. Any reason this wasn't done in the first place?

[RalfJung]

Checking thread::panicking seems rather fragile and IMO is much less clear than the current approach.

It seems better to abstract this pattern into a helper so not each and every user needs to get the forget right.

[SUPERCILEX]

Checking thread::panicking seems rather fragile and IMO is much less clear than the current approach.

I don't know how thread::panicking works: why would it be fragile? Also saying thread::panicking is more fragile seems wrong. The current approach is: let me think really hard about all the places I return stuff to the user and where I could panic and then make sure I forget in all the right places but not before I could panic. The new approach would be: just check for panicking inside Drop and deallocate if so. To me, that's way easier and clearer.

It seems better to abstract this pattern into a helper so not each and every user needs to get the forget right.

I would prefer that to checking panicking, but I don't think it's possible to express "drop when panicking but not under normal operation"? Or are you thinking of a defuse wrapper? I don't like that for the reasons above: way too easy to UAF.

[RalfJung]

I am thinking of a function that takes 2 closures, the 2nd one to be executed only if a panic occurs while the first is running.