Skip to content

Commit a59ba94

Browse files
aturonaturon
committed
Merge branch 'alexcrichton-safe-mem-forget' into HEAD
2 parents 5e8ff55 + 68d4f12 commit a59ba94

File tree

2 files changed

+125
-0
lines changed

2 files changed

+125
-0
lines changed

‎README.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ the direction the language is evolving in.
5454
* [1023-rebalancing-coherence.md](text/1023-rebalancing-coherence.md)
5555
* [1040-duration-reform.md](text/1040-duration-reform.md)
5656
* [1044-io-fs-2.1.md](text/1044-io-fs-2.1.md)
57+
* [1066-safe-mem-forget.md](text/1066-safe-mem-forget.md)
5758

5859
## Table of Contents
5960
[Table of Contents]: #table-of-contents

‎text/1066-safe-mem-forget.md

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
- Feature Name: N/A
2+
- Start Date: 2015-04-15
3+
- RFC PR: https://github.com/rust-lang/rfcs/pull/1066
4+
- Rust Issue: https://github.com/rust-lang/rust/issues/25186
5+
6+
# Summary
7+
8+
Alter the signature of the `std::mem::forget` function to remove `unsafe`.
9+
Explicitly state that it is not considered unsafe behavior to not run
10+
destructors.
11+
12+
# Motivation
13+
14+
It was [recently discovered][scoped-bug] by @arielb1 that the `thread::scoped`
15+
API was unsound. To recap, this API previously allowed spawning a child thread
16+
sharing the parent's stack, returning an RAII guard which `join`'d the child
17+
thread when it fell out of scope. The join-on-drop behavior here is critical to
18+
the safety of the API to ensure that the parent does not pop the stack frames
19+
the child is referencing. Put another way, the safety of `thread::scoped` relied
20+
on the fact that the `Drop` implementation for `JoinGuard` was *always* run.
21+
22+
[scoped-bug]: https://github.com/rust-lang/rust/issues/24292
23+
24+
The [underlying issue][forget-bug] for this safety hole was that it is possible
25+
to write a version of `mem::forget` without using `unsafe` code (which drops a
26+
value without running its destructor). This is done by creating a cycle of `Rc`
27+
pointers, leaking the actual contents. It [has been pointed out][dtor-comment]
28+
that `Rc` is not the only vector of leaking contents today as there are
29+
[known][dtor-bug1] [bugs][dtor-bug2] where `panic!` may fail to run
30+
destructors. Furthermore, it has [also been pointed out][drain-bug] that not
31+
running destructors can affect the safety of APIs like `Vec::drain_range` in
32+
addition to `thread::scoped`.
33+
34+
[forget-bug]: https://github.com/rust-lang/rust/issues/24456
35+
[dtor-comment]: https://github.com/rust-lang/rust/issues/24292#issuecomment-93505374
36+
[dtor-bug1]: https://github.com/rust-lang/rust/issues/14875
37+
[dtor-bug2]: https://github.com/rust-lang/rust/issues/16135
38+
[drain-bug]: https://github.com/rust-lang/rust/issues/24292#issuecomment-93513451
39+
40+
It has never been a guarantee of Rust that destructors for a type will run, and
41+
this aspect was overlooked with the `thread::scoped` API which requires that its
42+
destructor be run! Reconciling these two desires has lead to a good deal of
43+
discussion of possible mitigation strategies for various aspects of this
44+
problem. This strategy proposed in this RFC aims to fit uninvasively into the
45+
standard library to avoid large overhauls or destabilizations of APIs.
46+
47+
# Detailed design
48+
49+
Primarily, the `unsafe` annotation on the `mem::forget` function will be
50+
removed, allowing it to be called from safe Rust. This transition will be made
51+
possible by stating that destructors **may not run** in all circumstances (from
52+
both the language and library level). The standard library and the primitives it
53+
provides will always attempt to run destructors, but will not provide a
54+
guarantee that destructors will be run.
55+
56+
It is still likely to be a footgun to call `mem::forget` as memory leaks are
57+
almost always undesirable, but the purpose of the `unsafe` keyword in Rust is to
58+
indicate **memory unsafety** instead of being a general deterrent for "should be
59+
avoided" APIs. Given the premise that types must be written assuming that their
60+
destructor may not run, it is the fault of the type in question if `mem::forget`
61+
would trigger memory unsafety, hence allowing `mem::forget` to be a safe
62+
function.
63+
64+
Note that this modification to `mem::forget` is a breaking change due to the
65+
signature of the function being altered, but it is expected that most code will
66+
not break in practice and this would be an acceptable change to cherry-pick into
67+
the 1.0 release.
68+
69+
# Drawbacks
70+
71+
It is clearly a very nice feature of Rust to be able to rely on the fact that a
72+
destructor for a type is always run (e.g. the `thread::scoped` API). Admitting
73+
that destructors may not be run can lead to difficult API decisions later on and
74+
even accidental unsafety. This route, however, is the least invasive for the
75+
standard library and does not require radically changing types like `Rc` or
76+
fast-tracking bug fixes to panicking destructors.
77+
78+
# Alternatives
79+
80+
The main alternative this proposal is to provide the guarantee that a destructor
81+
for a type is always run and that it is memory unsafe to not do so. This would
82+
require a number of pieces to work together:
83+
84+
* Panicking destructors not running other locals' destructors would [need to be
85+
fixed][dtor-bug1]
86+
* Panics in the elements of containers would [need to be fixed][dtor-bug2] to
87+
continue running other elements' destructors.
88+
* The `Rc` and `Arc` types would need be reevaluated somehow. One option would
89+
be to statically prevent cycles, and another option would be to disallow types
90+
that are unsafe to leak from being placed in `Rc` and `Arc` (more details
91+
below).
92+
* An audit would need to be performed to ensure that there are no other known
93+
locations of leaks for types. There are likely more than one location than
94+
those listed here which would need to be addressed, and it's also likely that
95+
there would continue to be locations where destructors were not run.
96+
97+
There has been quite a bit of discussion specifically on the topic of `Rc` and
98+
`Arc` as they may be tricky cases to fix. Specifically, the compiler could
99+
perform some form of analysis could to forbid *all* cycles or just those that
100+
would cause memory unsafety. Unfortunately, forbidding all cycles is likely to
101+
be too limiting for `Rc` to be useful. Forbidding only "bad" cycles, however, is
102+
a more plausible option.
103+
104+
Another alternative, as proposed by @arielb1, would be [a `Leak` marker
105+
trait][leak] to indicate that a type is "safe to leak". Types like `Rc` would
106+
require that their contents are `Leak`, and the `JoinGuard` type would opt-out
107+
of it. This marker trait could work similarly to `Send` where all types are
108+
considered leakable by default, but types could opt-out of `Leak`. This
109+
approach, however, requires `Rc` and `Arc` to have a `Leak` bound on their type
110+
parameter which can often leak unfortunately into many generic contexts (e.g.
111+
trait objects). Another option would be to treak `Leak` more similarly to
112+
`Sized` where all type parameters have a `Leak` bound by default. This change
113+
may also cause confusion, however, by being unnecessarily restrictive (e.g. all
114+
collections may want to take `T: ?Leak`).
115+
116+
[leak]: https://github.com/rust-lang/rust/issues/24292#issuecomment-91646130
117+
118+
Overall the changes necessary for this strategy are more invasive than admitting
119+
destructors may not run, so this alternative is not proposed in this RFC.
120+
121+
# Unresolved questions
122+
123+
Are there remaining APIs in the standard library which rely on destructors being
124+
run for memory safety?

0 commit comments

Comments
 (0)