Advisory for rio

[meithecatte]

No description provided.

[Shnatsel]

Thanks for reporting this!

We are currently soliciting community feedback on how to handle such cases. If you have any particular stance on them, please chime in here. We will act on this PR after a rough consensus is reached by the community - at least sufficient for agreeing on a sane default.

[meithecatte]

Oh, wow. The timing makes it seem my PR was prompted by that Reddit post...

Anyway, my position is already represented in the discussion, so I will proceed to upvote accordingly.

[RalfJung]

There was also a lot of prior discussion in #275 and some consensus for having a dedicated category/severity/whatever for soundness issues.

[tarcieri]

I've been somewhat torn on what to do with this one. I think it might sense to track as an unsound informational advisory per #300, however if someone wants to make the case that this is a security vulnerability in and of itself, I'm open to that as well.

[oherrala]

if someone wants to make the case that this is a security vulnerability in and of itself, I'm open to that as well.

The issue linked in advisory has been deleted (https://github.com/spacejam/rio/issues/11), but it's available in caches. No idea why it was deleted, because it was a good civil discussion.

• https://webcache.googleusercontent.com/search?q=cache:https://github.com/spacejam/rio/issues/11
• http://archive.md/VGJiW

I have a feeling that I'm misunderstanding something in all this, so please correct me if I'm wrong.

After reading the issue I can't see a problem in rio itself. If user of rio does something like mem::forget, then it's not rio's fault if bad things happen, is it? Rio could probably do things to prevent that, but like @spacejam (the author of rio) writes in the issue:

personally I'm not sure we should significantly reduce the usability of an API due to an issue that you can only encounter if you do something kind of foolish to begin with.

Documentation of mem::forget says:

Takes ownership and "forgets" about the value without running its destructor. Any resources the value manages, such as heap memory or a file handle, will linger forever in an unreachable state.

So rio is working as intended from mem::forget perspective. Someone calls it and disables the destructor, bad things happen.

If we consider this an worthy advisory I feel we probably open a can of worms and we need to make every possible crate work with users calling mem::forget on all the things and every possible crate which doesn't work with mem::forget has unsound or other advisories listed.

Maybe using mem::forget should be linter warning (if it's not already).

My 2¢ is that this is not unsoundness issue nor vulnerability.

[meithecatte]

My 2¢ is that this is not unsoundness issue nor vulnerability.

It is not a matter of opinion whether it is unsound. By using only rio and safe code, one can induce undefined behavior. This is, by definition, unsound.

[dtolnay]

The issue linked in advisory has been deleted (https://github.com/spacejam/rio/issues/11), but it's available in caches. No idea why it was deleted, because it was a good civil discussion.

spacejam/rio#11.pdf

[oherrala]

It is not a matter of opinion whether it is unsound. By using only rio and safe code, one can induce undefined behavior. This is, by definition, unsound.

I think the topic here is rio and the use of mem::forget. mem::forget is safe code, but is there any other safe function causing unsoundness?

If every possible type implementing Drop combined with mem::forget is unsound we need to write a lot of advisories for unsoundness. For example mem::forget a Vec<T> or mem::forget a fs::File or mem::forget a net::TcpStream is unsound. If anything plus mem::forget is unsound then what's the point of even trying to fix soundness issues?

My opinion here is based on practicality and not theory. In theory the whole Rust as a language is unsound because mem::forget is safe function. In practice, there's a lot of advisories to be written if we follow that theory.

Crates.io has 41472 published crates. Instead of harassing all the awesome authors of published crates because of a safe Rust standard library function mem::forget we should spend our time fixing the Rust standard library or the definition of unsound.

[dtolnay]

For example mem::forget a Vec<T> or mem::forget a fs::File or mem::forget a net::TcpStream is unsound.

They aren't; none of those make it possible to have undefined behavior in safe code.

[Shnatsel]

Instead of harassing all the awesome authors of published crates because of a safe Rust standard library function mem::forget we should spend our time fixing the Rust standard library or the definition of unsound.

The root of the problem is not mem::forget (which would be easily fixable) but numerous other ways to leak values, which are impossible to statically rule out without making Rust overwhelmingly restrictive. The most pertinent of them is Rc or Arc cycles.

The definition of unsoundness laid down in the Nomicon and has not been arrived to lightly. Rust language designers have thoroughly investigated this exact memory unsafety pattern in relation to std scoped threads before. The issue is both real and fundamental; sadly there is no feasible technical solution outside rio to make its current pattern memory-safe.

With the issue being technically impossible to fix outside rio and organizationally impossible to fix in rio, we're at a stalemate for getting a technical fix. What we're trying to do here is figure out how to inform interested parties about the issue while minimizing harassment.

[spacejam]

I've viewed rio as a piece of art to encourage some conversations. One of the conversations I've watched unfold is the controversial licensing I've used with it, which has been useful to encourage awareness of the virality of licenses and support of open source etc... I see this issue as another thing that has caused some back-and-forth to educate people about Rust. Never dropping would not be an issue. The issue is specifically what happens when the borrow checker is disabled.

Consider this code:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=0857f0072ebf17a0c3bae773cc791cf1

It does not compile when the std::mem::forget is removed because even though there's a leak (the panic in Drop never happens), the compiler is still able to see that the Drop might influence the borrowed buffer. This is infinitely preferable to what happens when using std::mem::forget where the buffer is able to be mutated freely (this is the double-free opportunity). forget seems to disable the borrow checker in ways that a leak does not, at least in some cases.

I don't think it's correct to equate std::mem::forget with a leak, as illustrated with that code.

In rio's case, immortality for a completion is totally fine. It's the edges where the borrow checker seems to be disabled that I hope people will talk about more. I'd also like to better understand if there are other cases where the borrow checker seems to be disabled.

[bjorn3]

If you use std::mem::drop in that example, the borrow checker is still fine with it, despite the fact that Completion doesn't get dropped. Unlike std::mem::forget, std::mem::drop doesn't use any unsafe code anywhere. It is literally implemented as pub fn drop<T>(_x: T) {}: https://github.com/rust-lang/rust/blob/bb8674837a9cc5225020e07fc3f164762bb4c11c/src/libcore/mem/mod.rs#L873

[spacejam]

@bjorn3 it does not seem to compile when using drop. Did you mean something else than the drop I added at the end? https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=a088dbbc067538d824a2b7caefe9c8c8

[bjorn3]

I meant drop(cycle);:

use std::rc::Rc;
use std::cell::RefCell;

struct Completion<'a> {
    dst: &'a mut [u8],
}

impl<'a> Drop for Completion<'a> {
    fn drop(&mut self) {
        // the kernel does some write in the background
        for item in self.dst.iter_mut() {
            *item = 66;
        }
        panic!("this doesn't run because of the leak");
    }
}

struct Cycle<'a> {
    completion: Completion<'a>,
    cycle: RefCell<Option<Rc<Cycle<'a>>>>,
}

fn main() {
    let mut buf = [0; 1024];
    let completion = Completion {
        dst: &mut buf,
    };
    let cycle = Rc::new(Cycle { completion, cycle: RefCell::new(None)});
    cycle.cycle.replace(Some(cycle.clone()));

    std::mem::drop(cycle); // <---- drop cycle instead of forget
    
    buf[4] = 42;
}

Otherwise cycle is still alive when you try to access buf.

[RalfJung]

@oherrala

In theory the whole Rust as a language is unsound because mem::forget is safe function. In practice, there's a lot of advisories to be written if we follow that theory.

That's not true, and I was involved in formally proving this. It seems you are not entirely familiar with the involved terminology of "soundness" and/or "Undefined Behavior" here; a good starting point for this is the UCG glossary. Modulo the known and unknown soundness bugs, Rust is a perfectly sound language. This includes mem::forget. Bugs can and will always happen, but they are bugs and treated as such.

Also note that mem::forget is not even needed to cause UB with rio; as @bjorn3 pointed out, we can achieve the same effect using just Rc.

The "unsound" advisory category is meant for exactly those cases -- has it already been officially introduced by the WG? Once that category exists, rio ticks all the boxes to justify a corresponding advisory. Whether or not the developer treats a soundness violation as a bug is their choice, but has no impact on whether or not it is a soundness violation.

[RalfJung]

@spacejam

Here's a way to re-implement mem::forget using entirely safe code, based on @bjorn3's code:

use std::cell::Cell;
use std::rc::Rc;


fn my_forget<T>(x: T) {
    #[allow(dead_code)]
    struct Cycle<T> {
        x: T,
        cycle: Cell<Option<Rc<Cycle<T>>>>,
    }
    let cycle = Rc::new(Cycle { x, cycle: Cell::new(None)});
    cycle.cycle.replace(Some(cycle.clone()));
}

fn main() {
    // Just test that this works as expected.
    struct Bomb;
    impl Drop for Bomb {
        fn drop(&mut self) {
            println!("BOOM!");
        }
    }
    
    let b = Bomb;
    // Note how the bomb never goes off.
    my_forget(b);
}

This conclusively demonstrates that mem::forget is not the problem here. We just use it as a convenient short-hand to demonstrate that data can be forgotten in Rust. Somehow people then seem to think that without mem::forget, data could not be forgotten -- but that is not the case.

I also don't know what you mean by "disabling the borrow checker", nothing of the sort is happening in any of these examples.

[spacejam]

Thanks. @RalfJung ahh yeah I've hit the same thing before, still waking up. Was this one of the things that changed with NLL? Looking back at history to the flurry of activity around leakopalypse, just a few days remaining before 1.0, the proposal to make mem::forget safe, and frantic attempts to avoid unsafe drop, despite it being a decision that was made intentionally, I can't help but wonder what approaches simply didn't have time to be considered given that 1.0 was about to drop in a few hours. In that last issue, it was clear that the people making decisions didn't have enough time to pay attention to the people outside of the room due to the impending release.

[RalfJung]

@spacejam
First of all, this is the wrong place to re-open that RFC'd decision. The right place would be another RFC.

Secondly, I think you are misrepresenting the discussion back then. As my_forget shows, if we say mem::forget is unsound, we have to say that Rc is unsound. Everyone agreed that Rust should have some form of (safe and sound) reference-counted pointer in the standard library. So the challenge is: can you design a version of Rc that statically guarantees absence of memory leaks (by somehow ruling out cycles), but is still useful? The consensus back then was that ruling out cycles might be possible by somehow using lifetimes as "markers" and strictly nested scopes to ensure absence of cycles, but the resulting API would be very complicated (to the point of being unusable), and it would rule out many useful applications of Rc.

It is correct that no effort was made to spend 3 months on designing such an API. This is because the people involved were sure that the resulting API is bespoke and too limiting. That is still the consensus today, so I think those people were right. To prove them wrong, someone should present an Rc API that rules out cycles and is actually ergonomic. Having Rc is non-optional, so as far as I can tell that is literally the only alternative to a safe mem::forget.

Was this one of the things that changed with NLL?

No, this works all the way back to Rust 1.0, as you can check on godbolt.

[RalfJung]

@spacejam

I've viewed rio as a piece of art to encourage some conversations.

I should also add that I appreciate this crate. :) It's always nice to see more of the design space explored, including the design space that could have been had Rust made different decisions around memory leaks. io_uring is an important API that Rust should have best-in-class safe+sound support for, and your crate certainly pushed that discussion forward.

Our disagreement is on the treatment of the unsoundness, not on the publication of the crate in the first place.

[spacejam]

Well, as rio is art that I hope will encourage conversations, I hope that whatever decision you come to results in the loudest ruckus possible around it :) The unsoundness is currently a feature to that end.

When that goal loses its lustre, the methods will just flip to unsafe fn's. Long-term, rio 2 is intended to be sound and may accept a IO topology structure that can be intelligently scheduled in prioritized ways, taking ideas from scoped threads and maybe tricks from typed arena. The benefits of io_uring really come about through chained concurrent operations in the kernel that can execute without userspace code stomping into the middle of the data plane, and that's the next space for exploration.

[Shnatsel]

Following the discussion above and a demonstration of memory unsafety in presence of an Rc cycle I'm inclined to merge the advisory as-is. Given that Rc/Arc cycles are not uncommon in async code the required conditions are not contrived, and the resulting memory unsafety is significant. That will make dependency on rio a hard error in cargo-audit instead of a warning.

If there is any opposition to that, I'd like to hear it. I'm particularly interested in hearing from @spacejam

[RalfJung]

The alternative would be to start with an informational = "unsound" advisory, which are a thing now, and then maybe upgrade to a vulnerability advisory later. I do not have a strong opinion on that.

This is also related to #313.

[Shnatsel]

No objections have been voiced in 10 days, so I'm going to go ahead and merge this.

Thanks to everyone in this thread for the constructive, insightful and respectful discussion!