Summary

Alter the signature of the std::mem::forget function to remove unsafe. Explicitly state that it is not considered unsafe behavior to not run destructors.

Motivation

It was recently discovered by @arielb1 that the thread::scoped API was unsound. To recap, this API previously allowed spawning a child thread sharing the parent's stack, returning an RAII guard which join'd the child thread when it fell out of scope. The join-on-drop behavior here is critical to the safety of the API to ensure that the parent does not pop the stack frames the child is referencing. Put another way, the safety of thread::scoped relied on the fact that the Drop implementation for JoinGuard was always run.

The underlying issue for this safety hole was that it is possible to write a version of mem::forget without using unsafe code (which drops a value without running its destructor). This is done by creating a cycle of Rc pointers, leaking the actual contents. It has been pointed out that Rc is not the only vector of leaking contents today as there are known bugs where panic! may fail to run destructors. Furthermore, it has also been pointed out that not running destructors can affect the safety of APIs like Vec::drain_range in addition to thread::scoped.

It has never been a guarantee of Rust that destructors for a type will run, and this aspect was overlooked with the thread::scoped API which requires that its destructor be run! Reconciling these two desires has lead to a good deal of discussion of possible mitigation strategies for various aspects of this problem. This strategy proposed in this RFC aims to fit uninvasively into the standard library to avoid large overhauls or destabilizations of APIs.

Detailed design

Primarily, the unsafe annotation on the mem::forget function will be removed, allowing it to be called from safe Rust. This transition will be made possible by stating that destructors may not run in all circumstances (from both the language and library level). The standard library and the primitives it provides will always attempt to run destructors, but will not provide a guarantee that destructors will be run.

It is still likely to be a footgun to call mem::forget as memory leaks are almost always undesirable, but the purpose of the unsafe keyword in Rust is to indicate memory unsafety instead of being a general deterrent for "should be avoided" APIs. Given the premise that types must be written assuming that their destructor may not run, it is the fault of the type in question if mem::forget would trigger memory unsafety, hence allowing mem::forget to be a safe function.

Note that this modification to mem::forget is a breaking change due to the signature of the function being altered, but it is expected that most code will not break in practice and this would be an acceptable change to cherry-pick into the 1.0 release.

Drawbacks

It is clearly a very nice feature of Rust to be able to rely on the fact that a destructor for a type is always run (e.g. the thread::scoped API). Admitting that destructors may not be run can lead to difficult API decisions later on and even accidental unsafety. This route, however, is the least invasive for the standard library and does not require radically changing types like Rc or fast-tracking bug fixes to panicking destructors.

Alternatives

The main alternative this proposal is to provide the guarantee that a destructor for a type is always run and that it is memory unsafe to not do so. This would require a number of pieces to work together:

Panicking destructors not running other locals' destructors would need to be fixed
Panics in the elements of containers would need to be fixed to continue running other elements' destructors.
The Rc and Arc types would need be reevaluated somehow. One option would be to statically prevent cycles, and another option would be to disallow types that are unsafe to leak from being placed in Rc and Arc (more details below).
An audit would need to be performed to ensure that there are no other known locations of leaks for types. There are likely more than one location than those listed here which would need to be addressed, and it's also likely that there would continue to be locations where destructors were not run.

There has been quite a bit of discussion specifically on the topic of Rc and Arc as they may be tricky cases to fix. Specifically, the compiler could perform some form of analysis could to forbid all cycles or just those that would cause memory unsafety. Unfortunately, forbidding all cycles is likely to be too limiting for Rc to be useful. Forbidding only "bad" cycles, however, is a more plausible option.

Another alternative, as proposed by @arielb1, would be a Leak marker trait to indicate that a type is "safe to leak". Types like Rc would require that their contents are Leak, and the JoinGuard type would opt-out of it. This marker trait could work similarly to Send where all types are considered leakable by default, but types could opt-out of Leak. This approach, however, requires Rc and Arc to have a Leak bound on their type parameter which can often leak unfortunately into many generic contexts (e.g. trait objects). Another option would be to treat Leak more similarly to Sized where all type parameters have a Leak bound by default. This change may also cause confusion, however, by being unnecessarily restrictive (e.g. all collections may want to take T: ?Leak).

Overall the changes necessary for this strategy are more invasive than admitting destructors may not run, so this alternative is not proposed in this RFC.

Unresolved questions

Are there remaining APIs in the standard library which rely on destructors being run for memory safety?

6.5 KiB Raw Permalink Blame History