Using Drop for safety in Rust | Hacker News Reader

Using Drop for safety in Rust

v5.chriskrycho.com

116 pointsthunderbong27 comments

From the article:

> That means that impl Drop for Drain is responsible for making sure that Drain is sound.

This is _wrong_ the Drop impl _must not_ be responsible for making sure Drain is Sound. Drop is not guaranteed to run it must _never_ be relied on for soundness.

I emphasizes the wrong because that is one of the biggest pitfalls in current rust outside of doing some very clever/magic things.

In std the `drain` function guarantees soundness by setting len=0, i.e. making the vector forget about all it's content, i.e. semantically moves all of it's elements from the Vec into the Drain.

The Drop impl. semantically just moves the parts it didn't drain back into the Vec.

I.e. for Drain Drop it's isn't responsible for soundness but for not leaking the non drained elements.

EDIT: To be fair: Otherwise still a good article. Just misses one crucial paragraph.

Any link to that magic? Sounds interesting.

The code the blog presents copied from the std, ironically it does include the line where the len is set to 0 with a in code comment (but the article fails to go into it and seems to not be quite aware of the issue, it's otherwise quite good).

Also:

- up to date explanation in the nomicon: https://doc.rust-lang.org/nightly/nomicon/leaking.html

- from 2015 when it was realized leak must be sound: https://faultlore.com/blah/everyone-poops/

Ah, you’re totally right; I badly misstated that. I’ll update it tomorrow accordingly. Thank you for the correction. Your last comment before the edit is one part of what I was trying to get at; the other was that Drop itself must uphold the relevant constraints in its own operation. Like I said, great clarification/ correction and I’ll be glad to fix it!

I just published an update to the post which corrected the title, URL, and content, and also published an additional post calling attention to the error and explaining it (https://v5.chriskrycho.com/notes/corrections-using-drop-safe...). Thanks again for flagging it up, @dathinab!

The pattern that this relies on is called "pre-pooping" because of this blog post: https://cglab.ca/~abeinges/blah/everyone-poops/

Canonical URL: https://faultlore.com/blah/everyone-poops/

And here's the more straightforward documentation from the Nomicon that takes the initial blog post and explains it thoroughly and straightforwardly. (It doesn't mention pooping your pants though)

https://doc.rust-lang.org/nightly/nomicon/leaking.html

What impresses me with Rust isn’t that it has highly efficient and idiomatically written standard library features. It’s that the footguns are consistently encapsulated.

One thing that fascinates me about Rust is how, in many ways, the language has become easier over time, because a significant share of the "new" features is shaped like "made an existing construct work across more contexts and smoothed out an irregularity in the language".

yes in general when it comes to rust it tries to make sure that any new abstraction of features don't cause unknown unknowns, at least wrt. safety/soundness.

Like make abstractions only leaky in ways where either the compiler will yell at you if you mess up or it involves unsafe functions with clear definitions about under which conditions they are safe to use. With a strong style bias to not use unsafe.

Like e.g. there are _ton_ of complexity in the exact rules of the borrow checker, but once you know the basics that's most times all you need especially. Or if you don't know async you don't need to know anything about async at all even if you provide a library with closures/callbacks.

and that is lovely, and also where C++ fails (hard IMHO), and where C kinda somewhat doesn't fail (it fails in other places tho IMHO)

Haven't read the post, but I think there must be something wrong if a type's safety depends on the Drop trait due to the existence of std::mem::forget.

That would make the lifetime last forever, preventing you from ever getting the mutable reference back and causing any safety issues. (Your intuition serves you well, though: graphics APIs designed to commit on a guard type's Drop::drop are prone to panicking, since the GPU driver does not care about Rust lifetimes. To implement that properly, you usually need ersatz typestates.)

The crucial bit for Vec::drain is in these two lines of code, which the article lists but does not discuss:

        // set self.vec length's to start, to be safe in case Drain is leaked
        self.set_len(start);

You cannot rely on Drop alone for safety, you need a fallback strategy in case of a leak (mem::forget or Rc cycle). Rust lifetimes only ensure that there is no use after free; there is no guarantee that Drop is called before the end of the lifetime. It's possible to mutably access the original Vec after leaking a Drain.

Yeah that's the "pre" in "pre-pooping your pants"

Thanks to the feedback in this discussion, the article now does discuss 5at line; much obliged to all the folks who helped me fix a substantial error.

This is misleading. `std::mem::forget(&'a mut v)` does not imply `'a: 'static`.

That is precisely what the post is about.

and fails to properly convey outright stating both in the title and places in the article that drop can be responsible for safety (it can't, in drain it's responsible for not leaking non drained elements, not safety)

yes drop can't be responsible for safety

in Vec::drain() by setting the Vec len to 0, std semantically moves all elements from the Vec to the Drain and in in Drain::drop it semantically moves the non drained elements back

i.e. vec::Drain::drop is responsible for not leaking non drained elements, not for safety

and sure not leaking means positioning them so in the vec that the len can be set to a value where all elements from 0..len have not been moved out etc. but that doesn't change that semantically it's responsibility is "not leaking" instead "making sure it's sound".

some random additional information:

- initially it was believed you can rely Drop for soundness

- then the rust community realized that this is an oversight -- the so called leakocalipyse

- there also was no easy straight forward way to fix that, in the end it was decided that leaking must be sound and `std::mem::forget` was added

- while there was some initial fallout (e.g. scoped threads and some &mut iterators now leaking collections if leaked) it wasn't to bad as you still can use function scopes to enforce some non leaking (i.e. if you accept a closure you can make sure something runs both before and after it)

- but with async functions a lot of the workarounds don't work anymore as the async function could be leaked mid execution by calling async functions ... so by now some people which rust had taken a different turn back then. But then to be fair this is the kind of "hypothetically wishing" because making leak sound was with the knowledge and constraints available back then very clearly the right choice.

One nitpick, `std::mem::forget` already existed it was just marked as unsafe before.

Not related to the content of this post, but resizing my browser window on that site is kind of trippy.

Yeah, a thing I implemented years ago to handle the ability to hide the side nav on wide screens never worked quite the way I expected. Making it not do that except when specifically toggling the nav has always been on the “to-do”, just never made it to the top of my priority list.

TL;DR: YOU MUST NOT

(and as I haven't yet time to read the article I'm not sure if they convey that correctly)

But for various (complicated) reasons Drop isn't guaranteed to run on a "safety" level. That means that means whatever you do you must not rely on Drop for safety.

Through what you can do is rely on it for _correctness_, leading to safe but not very correct outcomes if it's not uphold.

What that means is that while you can (and should) use drop for cleanup patterns with patterns like drop guards, but you need to make sure that if drop isn't run it's still safe, i.e. sound.

It's totally fine to abort the process if a drop which _really_ needed to be run isn't run.

One example from `std` would be `vec::Drain` which if dropped leaks all elements of the `Vec` (not just the drained ones) but is safe i.e. sound to leak (also the mem allocation of the vec itself is not leaked just any allocation from elements in the vector).

On exception is if the drop guard itself is part of a unsound block or similar, i.e. if you can limit the scope of unsafe code enough to place the "drop-must" run part into a unsafe contract. E.g. you create drop guard at the beginning of a (non-async) unsafe block and can guarantee as part of the contract for that unsafe block that it's run at the end of it. This is commonly used in compact unsafe code blocks where in go you would use defer.

This is exactly the example that the article uses.

I know, that was intentional.

It doesn't change that you _must not_ use Drop for safety.

You only can use it for cleanup, and that can involve unsafe code and that can make things easier, but you can't rely on drop running for anything safety i.e. soundness related.

And the article fails to highlight this problem but IMHO you really have to highlight it every time you speak about safety+drop, as its one of the biggest pitfalls of current rust and not doing so is irresponsible. There had been too much unsound code due to people overlooking that drop isn't guaranteed to run.

Like to quote:

> What about after you finish with the draining iterator, though? How does Rust guarantee that part of the contract?

> This is where it gets interesting.

> When the iterator is dropped — either because you hit the end of a for loop over it or because you drop it after iterating over some subset of elements — the Drain type’s implementation of the Drop trait takes over. That means that impl Drop for Drain is responsible for making sure that Drain is sound.

And that is _wrong_ the Drop impl _must not_ be responsible for making sure Drain is Sound.

The `drain` function call does so by setting len==0, i.e. making the vector forget about all it's content, i.e. semantically moves out all of it's elements. The Drop impl. semantically just moves the parts it didn't drain back into the Vec. I.e. it's isn't responsible for soundness but for not leaking the non drained elements.

Reply on news.ycombinator.com