Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Alter mem::forget to be safe #1066

Merged
merged 1 commit into from
May 7, 2015
+124 −0
Merged

RFC: Alter mem::forget to be safe #1066

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 124 additions & 0 deletions text/0000-safe-mem-forget.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
- Feature Name: N/A
- Start Date: 2015-04-15
- RFC PR: (leave this empty)
- Rust Issue: (leave this empty)

# 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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I (and others on IRC) think the priority is backwards here. The RFC should first and foremost be about declaring leaks safe, or more generally (in the Alternative section) addressing the ability to leak resources/prevent drop from being called. Making mem::forget safe is just a corollary, not the core issue.


# Motivation

It was [recently discovered][scoped-bug] 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.

[scoped-bug]: https://github.com/rust-lang/rust/issues/24292

The [underlying issue][forget-bug] 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][dtor-comment]
that `Rc` is not the only vector of leaking contents today as there are
[known][dtor-bug1] [bugs][dtor-bug2] where `panic!` may fail to run
destructors. Furthermore, it has [also been pointed out][drain-bug] that not
running destructors can affect the safety of APIs like `Vec::drain_range` in
addition to `thread::scoped`.

[forget-bug]: https://github.com/rust-lang/rust/issues/24456
[dtor-comment]: https://github.com/rust-lang/rust/issues/24292#issuecomment-93505374
[dtor-bug1]: https://github.com/rust-lang/rust/issues/14875
[dtor-bug2]: https://github.com/rust-lang/rust/issues/16135
[drain-bug]: https://github.com/rust-lang/rust/issues/24292#issuecomment-93513451

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: “may not” sounds wrong here. “might not” would be better, I think.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can guarantee much more. Let's use ownership as the core concept.

"The owner of a value has the final influence on whether the destructors run or not."

  • You must own the value to forget it
  • Your own owned values that you keep in a scope as guards will be guaranteed to drop as usual when they leave the scope
  • Shared ownership leaves you at mercy of all the owners together.
  • If you want to be sure the destructor runs, don't give away ownership (like thread::scoped would return a value, relinquishing ownership).

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very, very hard thing to do, because there’s process::exit, intrinsics::abort, stack overflow etc which circumvent unwinding the stack and terminate the process immediately.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a useful distinction can be made between "the destructor will always be run" and "the destructor will always be run if the program itself continues to run". I.e., what we'd be interested in preventing is Rust code being able to observe the fact of a destructor having not run when it "should have".1 Put another way, actual Rust code shouldn't need to guard against the possibility of a destructor having not run. Of course if the process aborts (the world ends), then that possibility is avoided (and in fact, ending the world can be a reasonable strategy for avoiding it). As language designers it's also unfortunately not within our power to prevent the power going out or a sudden gamma ray burst destroying all life and computation on Earth, but that's also not what we're concerned with.

1 If an invariant is violated in the forest but nobody can observe it, might demons fly out of your nose? (No - they mightn't.)

A tricky question, though, is to what extent panics can be considered to end the world to an adequate degree. From the perspective of the code that thread was executing, the world has indeed ended - but other threads and (notably!) destructors can still potentially observe what the end was like.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what we'd be interested in preventing is Rust code being able to observe the fact of a destructor having not run when it "should have".

This is pretty easy, actually, unless you declare destructors to be “pure”. A nice example would be not running destructor for PID-like files that are only supposed to exist for the lifetime of process. Killing and running the same Rust code again could somewhat reliably tell you that the destructor was not run and PID-like file was not deleted.

What I’m trying to say here: this alternative would require marking all of these prematurely-kills-process functions unsafe as well. I believe this makes the alternative unviable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Point noted. I'm not quite sure of the correct way to formulate the guarantee we'd want (if we'd want it, which this RFC says we wouldn't), but I still very much feel that ruling out abort() (or loop { }) shouldn't have to be a part of it. The motivation for the guarantee would be to be able to depend on it for memory safety. Things external to the process such as PID files do not impact on memory safety. This is similar to how Rust prevents you from having two &mut references to the same object but not from doing rm -rf /.

require a number of pieces to work together:

* Panicking destructors not running other locals' destructors would [need to be
fixed][dtor-bug1]
* Panics in the elements of containers would [need to be fixed][dtor-bug2] 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][leak] 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 treak `Leak` more similarly to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/treak/treat/

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

[leak]: https://github.com/rust-lang/rust/issues/24292#issuecomment-91646130

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?