Unsoundness without forget

[dtolnay]

I came across the following note in https://docs.rs/rio/0.9.3/rio/struct.Completion.html, which I found surprising and possibly disingenuous:

Safety

Never call std::mem::forget on this value. It can lead to a use-after-free bug. The fact that std::mem::forget is not marked unsafe is a bug in the Rust standard library.

In trying to understand the backstory of your position on this, I found this sequence of tweets from which I think I see the misunderstanding on your part:

• @Lucretiel(https://twitter.com/Lucretiel/status/1256430755808509952)—
  Wait, does this mean that forgetting something can lead to unsoundness in rio? That would imply I can create unsoundness by, say, accidentally creating a reference cycle

• @spacejam(https://twitter.com/sadisticsystems/status/1256519569830670336)—
  A reference cycle is fine and not the same as mem::forget because the compiler still enforces that any buffer passed to rio outlives the kernel's write into the buffer

• @spacejam(https://twitter.com/sadisticsystems/status/1256519873401745413)—
  A reference cycle is immortal. mem::forget allows desired lifetime constraints related to Drop to be violated

• @spacejam(https://twitter.com/sadisticsystems/status/1256520244505411586)—
  Immortality is safe. Violating drop-based lifetime constraints via forget can lead to use after free though

In case it helps see why these tweets are wrong, I wrote up some example code that demonstrates stack corruption (or heap corruption / use after free / however you want it) without any involvement of forget.

The same misunderstanding in rio was called out recently by someone else in https://www.reddit.com/r/rust/comments/hk8lab/ringbahn_ii_the_central_state_machine/fwt0xmt/.

Hopefully the example code will help identify a path to making a sound API for rio; otherwise it would be good to clarify the documentation to make it clear that the safety proposition is "hope that nothing else in your code interacts poorly with rio's assumptions about Rust semantics", rather than anything specific about forget.

---

// [dependencies]
// extreme = "666.666.666666"
// futures-util = "0.3"
// rio = { git = "https://github.com/spacejam/rio" }

#[derive(Debug)]
pub enum Buffer {
    Inline([u8; 32]),
    OutOfLine(Vec<u8>),
}

impl Buffer {
    fn as_mut(&mut self) -> &mut [u8] {
        match self {
            Buffer::Inline(buffer) => buffer,
            Buffer::OutOfLine(buffer) => buffer,
        }
    }
}

fn main() {
    let rio = rio::new().unwrap();
    let stdin = std::io::stdin();

    let mut buf = Buffer::Inline(Default::default());
    let mut slice = buf.as_mut();
    let mut completion = rio.read_at(&stdin, &mut slice, 0);
    let _ = extreme::run(async { futures_util::poll!(&mut completion) });

    struct Oops<'a>(
        rio::Completion<'a, usize>,
        std::cell::Cell<Option<std::sync::Arc<Oops<'a>>>>,
    );
    let oops = std::sync::Arc::new(Oops(completion, Default::default()));
    oops.1.set(Some(oops.clone()));
    drop(oops);

    buf = Buffer::OutOfLine(vec![0; 100]);
    std::thread::sleep(std::time::Duration::from_secs(2));
    println!("{:?}", buf);
}

$ (sleep 1; yes) | cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/repro`
Segmentation fault (core dumped)

[spacejam]

👍

[Lucretiel]

Why closed? Is there an issue with the demonstration of unsoundness without using forget?

[spacejam]

It was closed because it is known, and not a problem. This project is art. At some point it change. Feel free to open a PR.

[spacejam]

Issues like this kill any motivation to improve things. Not helpful. Open a PR instead.

[paulocsanz]

Pointing possible UB in a library described as "misuse resistant" is helpful... Seriously, we don't want another actix like debacle, but we also don't want libraries with known problems that can cause UB in production.

And I say this because it's incredible code and we as a community have evolved a lot in the async world, but it's only with issues like this we can actually make the ecosystem mature. It's new stuff and we are still figuring things out, but pointing possible problems is a great start.

Sure maybe it can be kept open and marked as wont-fix, or waiting-for-pr. But why close it like it's solved? It's important to have that documented, specially because if UB happens it won't point directly to the docs that explain it. The dev will have a better time looking for open issues.

And if it proves to be a problem in production the community might move together to fix. Closing won't help.

I'm not saying you should fix it, just leave it there for posteriority, those issues always prove to be useful.

[nox]

Issues like this kill any motivation to improve things. Not helpful. Open a PR instead.

So you would accept a PR that marks all methods returning a Completion as unsafe?

[elibenporat]

At the risk of speaking on the author's behalf, it sounds like they are asking for help to fix this. Theoretically, keeping an issue tagged as "open" might inspire someone to jump in and help, but that isn't necessarily the case. Keep in mind that the subset of people who understand and could fix (not me!), are likely to be aware of this anyway, so closing it doesn't make it disappear.

It's important to note that the mental toll of having an "open" UB issue is worth considering, especially if the author has to see it all the time. This is especially true in the current state of the world.

Perhaps a more helpful route would be a PR that updates the documentation appropriately, to reference this issue, such that it is visible to interested parties, despite being "closed".

[spacejam]

This ain't actix. Look at the dependent crates. All but 1 are mine, and I aggressively hunt leaks in my code because that is table stakes for a real program. I don't accept this drama. If you see yourself using this crate or an improved version, I'm interested in speaking with you. Otherwise, go raise your pitchforks elsewhere :)