RFC: I/O Safety

[sunfishcode]

Pre-RFC on IRLO

Raw OS handles such as RawFd and RawHandle have hazards similar to raw pointers; they may be bogus or may dangle, leading to broken encapsulation boundaries and code whose behavior is impossible to bound in general.

Introduce a concept of I/O safety, and introduce a new set of types and traits, led by OwnedFd and BorrowedFd, to support it.

[Edited to reflect changes to the RFC].

Rendered

[SOF3]

Either way, creating an IO-safe random-access pointer to the mmap device file can mess up the memory safety of rust, right?

[sunfishcode]

Either way, creating an IO-safe random-access pointer to the mmap device file can mess up the memory safety of rust, right?

A safe mmap wrapper requires preventing writes to the file (if you hand out &Ts or &mut Ts) and truncations (which can lead to SIGBUSs). This is difficult in general (and making it easier is out of scope here) but there are OS-specific ways to do it in some cases. Some of the ways work by making the file inaccessible to other processes, however they don't always prevent access from the same process. Any code in the process holding a bogus or dangling file descriptor can bypass encapsulation boundaries and operate on the file with the rights of the process. I/O safety prevents safe Rust from doing that.

[sunfishcode]

Either way, creating an IO-safe random-access pointer to the mmap device file can mess up the memory safety of rust, right?

Ah, if you're talking about Linux's /proc/self/mem, yes, that can also break memory safety. Spawning a debugger process can too. These are independent concerns that don't cancel the usefulness of memory safety or I/O safety.

[the8472]

Any code in the process holding a bogus or dangling file descriptor can bypass encapsulation boundaries and operate on the file with the rights of the process. I/O safety prevents safe Rust from doing that.

File::open("/proc/self/fd/5")

[RalfJung]

File::open("/proc/self/fd/5")

I view that like /proc/self/mem: we have to basically ignore its existence to be able to say anything reasonable about what Rust programs can and cannot do. Ideally we'd be able to ask the Linux kernel to just turn off these files for our process... that's a different discussion.

[the8472]

Ideally we'd be able to ask the Linux kernel to just turn off these files for our process... that's a different discussion.

That could be done by passing a flag to openat2, at least for the fd magic symlinks, but not for mem. But that would break legit uses of the fd directory such as bash's process substitution feature.

[workingjubilee]

It's true that one can reasonably say that anything to do with File and OpenOptions is arguably unsafe because we can access things like the proc filesystem with it and thus edit a Rust program's runtime memory. But while marking those as unsafe in response might be humorous, the joke would be short lived, and I am not entirely sure the laughs would last much longer if we asked the OS to block a bunch of file descriptors. Most OS allow a programmer to invoke ambient authority early and often, and accordingly so does Rust. But that does not mean we should give up on this concern about I/O safety.

I believe the correct focus for unsafe semantics is on anything that projects into, and can be fully crystallized within, Rust logical control flow. File manipulation by necessity projects both ways: Rust extends requests to the OS, and the OS extends authority to Rust. With that, we can focus our concern on making sure the program does not violate Rust programmatic I/O safety.

That means it's not about whether the OS allows Rust to edit its own runtime memory, or even its binary code, but making sure that the authority to do so is obtained in a lawful manner obtained via File APIs. With an appropriate extension to the language, such as IoSafe or some other means, we could make sure sure that a Rust program can, following the explicit request of the programmer, soundly invalidate each and every last one of Rust's safety guarantees (and itself as a Rust program!). That's much better than unsoundly invalidating all of Rust's safety guarantees, I think.

[sunfishcode]

Several folks have expressed interest in seeing what a less minimalist change to std might look like, both in comments above and in offline discussion with @joshtriplett.

I've now put together a prototype of such an API. The high-level summary is that it defines OwnedFd and BorrowedFd and related things. BorrowedFd looks like it'll be nicer than &OwnedFd because it can use repr(transparent) and participate in FFI directly. This means users can avoid touching Raw* values entirely.

The prototype repo, with an example that supports Unix and Windows, is here:

https://github.com/sunfishcode/io-experiment

Feedback welcome!

[RalfJung]

Yeah that looks great. :) Though I am a bit surprised by OptionFd... for this to be useful in FFI, I think the "sentry value" needs to be documented. But I am also not sure if it is truly needed -- since rust-lang/rust#74699 landed, we could probably arrange for Option<OwnedFd> to have the desired representation.

[sunfishcode]

Option<OwnedFd> instead of OptionFd would be nice, if feasible. It looks like that would require special-casing it within rustc in places like this and this and possibly others. Does that sound like what you're thinking about?

Also, my prototype doesn't yet deal with this yet, but Windows has multiple representations for optional handles. I'm not a Windows expert, but if I understand correctly, we may still be able to have Option<Handle> with NULL as the sentry value, along with separate types for file handles with INVALID_HANDLE_VALUE as the sentry value.

[RalfJung]

Option instead of OptionFd would be nice, if feasible. It looks like that would require special-casing it within rustc in places like this and this and possibly others. Does that sound like what you're thinking about?

I think it would just require setting the right rustc_layout_scalar_valid_range_start/rustc_layout_scalar_valid_range_end like rust-lang/rust#74699 did. No rustc changes should be necessary, only library changes (and documenting that we guarantee the appropriate ABI).

EDIT: Oh, you are pointing to the FFI-compat lints. Yeah those would also need to be adjusted, indeed. (This is the codification of "documenting that we guarantee the appropriate ABI".)

[pickfire]

Users could still do unsafe { File::from_raw_fd(5) } right? How can using IoSafe helps here? Isn't it the same?

[RalfJung]

@pickfire that's unsafe.
Consider this: users can also do unsafe { &*(42 as *const i32) }, and yet Rust has memory safety. The same works for I/O safety.

[FloLo100]

Users could still do unsafe { File::from_raw_fd(5) } right? How can using IoSafe helps here? Isn't it the same?

"Dereferencing" filedescriptors not obtained by functions implementing IoSafe would be considered UB aka "Dont do that".

[sunfishcode]

I've now removed the OptionFd as @RalfJung suggested, and it works well using Option.

To support this, I added niche optimizations to the Windows types, so that Option<OwnedHandle> and Option<OwnedSocket> have the right representation for FFI in the general case, and added an OptionFileHandle to cover the INVALID_HANDLE_VALUE case.

Seeing how well repr(transparent) works out, and seeing how tricky IoSafe is turning out to be, I'm becoming much more optimistic about this approach in general.

[programmerjake]

To support this, I added niche optimizations to the Windows types, so that Option<OwnedHandle> and Option<OwnedSocket> have the right representation for FFI in the general case, and added an OptionFileHandle to cover the INVALID_HANDLE_VALUE case.

iirc Windows has two different invalid values it uses inconsistently for handles: NULL and INVALID_HANDLE_VALUE. I can't remember which one(s) are used for file, socket, and other handles we care about, this will require extra care to get right, at the minimum the types should be explicitly documented which values they have for niches since that could be a major foot-gun.

[sunfishcode]

iirc Windows has two different invalid values it uses inconsistently for handles: NULL and INVALID_HANDLE_VALUE. Icr which one(s) are used for file, socket, and other handles we care about, this will require extra care to get right, at the minimum the types should be explicitly documented which values they have for niches since that could be a major foot-gun.

That's right; the blog post I linked to above has more info. io-experiment now deals with this, and documents which values are valid in each type, such as here for OptionFileHandle, the type where INVALID_HANDLE_VALUE is disallowed.

[programmerjake]

If possible, I think putting the IoSafe (or equivalent) traits in core is a good idea, allowing no_std crates to handle file descriptors or handles in an abstracted manner, or otherwise use IoSafe without needing to depend on std.

[matklad]

I would like to air concern about churn. As far as I understand, when this RFC is implemented, more or less every current user of RawFd needs to change by either making the functions unsafe or migrating to the new types. It seems like the change will be required in the libraries and in the consumers of the libraries. As a specific example, nix::dup would have to change in semver-incompatible way.

What eases the transition is the fact that RawFds are rarely a part of contagious "vocabulary types" of the interfaces between the crates. Getting duplicate major versions of nix or mio in the Cargo.lock is not too costly.

There are some aspects that could make the transition troublesome though:

First, there's little concrete, tangible benefit in migrating to the new APIs, as it mostly closes a loophole. Some library authors might see this as a needless churn, and refuse to migrate. If this happens, we will have both old and new style fd APIs in the ecosystem forever, sort of how we have both mod.rs and no mod.rs codebases.

Second, my understanding is that, under this RFC, the dup method above becomes unsound (it needs to be unsafe). So, formally, all nix releases should get a CVE, as they have a public function that should be, but is not, unsafe.

Third, library crates usually have some sort of MSRV commitment and often support at least a couple of Rust versions: they'll have a choice between keeping API as is (and becoming unsound in Rust X + 1) or migrating to the new API (breaking compatibility with Rust X).

The combination of the last two points feels especially concerning -- it seems like we might end up in a situation where we have a significant number of published API which "misuse" unsafe, but can't be immediately fixed.

Several specific questions:

• Am I correct that the current signature of dup and the like is unsound under this RFC and requires unsafe keyword?
• What is the expected migration story here? I've painted a doom and gloom picture, but maybe I am just not seeing a way to play out the migration in a nicer way?
• Are we sure that the benefits we get here are worth the churn for already published APIs?

[the8472]

First, there's little concrete, tangible benefit in migrating to the new APIs, as it mostly closes a loophole.

It's not just about closing a loophole but also ergonomics. Having lifetimes for fds means you can hand out an fd without having to manually ensure that it doesn't get closed prematurely. Currently when you're converting between different views on a file descriptor you have to ensure that all but one of the views flows into mem::forget, ManuallyDrop or into_raw_fd so it doesn't get closed prematurely.

Plus it reduces the need for unsafe blocks since FromRawFd is unsafe.

[Thomasdezeeuw]

There are some aspects that could make the transition troublesome though:

First, there's little concrete, tangible benefit in migrating to the new APIs, as it mostly closes a loophole. Some library authors might see this as a needless churn, and refuse to migrate. If this happens, we will have both old and new style fd APIs in the ecosystem forever, sort of how we have both mod.rs and no mod.rs codebases.

I think you're really underselling the benefits here. You're basically saying references and lifetimes over raw pointers aren't worth it. But I would disagree, and since you're using Rust I think/hope would disagree too. The whole point of this RFC is to bring (some of) the same safety guarantees we have for pointers and memory accesses to the I/O and file descriptors space.

Yes churn will be an issue, it always is. But I would say the benefits outweigh the costs here.

Second, my understanding is that, under this RFC, the dup method above becomes unsound (it needs to be unsafe). So, formally, all nix releases should get a CVE, as they have a public function that should be, but is not, unsafe.

All methods that use AsRawFd and are safe are already unsound, regardless of this RFC, because we can pass any integer to it.

I'm not a security expert, but I don't think that warrants an CVE as there is no evidence that any of the API were actually used incorrect such that it can lead to an exploit, but again not a security expect.

Third, library crates usually have some sort of MSRV commitment and often support at least a couple of Rust versions: they'll have a choice between keeping API as is (and becoming unsound in Rust X + 1) or migrating to the new API (breaking compatibility with Rust X).

The solution to this is of course library dependent. Speaking for myself, and thus the socket2 and mio crates, we'll likely first document the unsoundness without breaking the API (i.e. not marking functions as unsafe). Then once this RFC is in stable long enough we can release a breaking change, luckily both crates are still pre version 1.

The combination of the last two points feels especially concerning -- it seems like we might end up in a situation where we have a significant number of published API which "misuse" unsafe, but can't be immediately fixed.

We're already in that situation, this RFC proposes a fix.

[matklad]

See https://internals.rust-lang.org/t/pre-rfc-i-o-safety/14585/7 for the "loophole" context.

You're basically saying references and lifetimes over raw pointers aren't worth it.

This might be a false equivalence. We empirically know that raw pointers cause a lot of CVEs, even if you have RAII. To me at least, its unclear if IO-unsafety actually leads to CVEs in Rust. I don't remember ever seeing a bug in Rust due to fd re-use. The RFC text doesn't provide a list of the specific bugs that wouldn't have happened, had we shipped I/O safety in Rust 1.0.

To me at least, in terms of impact IO-unsafety seems closer to File::open("/proc/self/mem") than to raw pointers. I haven't seen a conclusive evidence to the contrary (which doesn't mean that such evidence doesn't exist).

[RalfJung]

First, there's little concrete, tangible benefit in migrating to the new APIs, as it mostly closes a loophole.

I would say that the current situation around AsRawFd (using this just as a representative of the entire family of traits) is not coherent -- we ned to change something, since our current story in this space just makes no sense.

• Either AsRawFd is actually useful as a trait in generic code, so that one can expose safe fn foo<T: AsRawFd> that do things with the file descriptor. Then Rust has no I/O safety and we should make FromRawFd safe. We gain nothing by making that trait unsafe.
• Or we acknowledge that the AsRawFd trait was a mistake, and replace it by something better. This will indeed cause churn in the I/O parts of the ecosystem.

I am solidly in camp "option 2" here. The AsRawFd trait is equivalent to a trait like

fn AsRawPtr {
  fn as_raw_ptr(&self) -> *const i32
}

I hope we all agree that there is no reasonable safe fn foo<T: AsRawPtr> that can be written against such a trait.

Why do I claim that "raw FD" is akin to "raw pointer"? Two points:

• I don't think it is an accident that the term "raw" appears in "raw FD"
• Raw FDs are safe to construct from integers (like raw pointers), are safe to construct from "safe" I/O wrappers such as File (like raw pointers, which are safe to construct from reference), and are unsafe to turn into "safe" I/O wrappers such as File (like raw pointers, which are unsafe to turn into references).

I don't know why the AsRawFd trait was added, but it goes against everything else in how this part of libstd was designed.

This might be a false equivalence. We empirically know that raw pointers cause a lot of CVEs, even if you have RAII. To me at least, its unclear if IO-unsafety actually leads to CVEs in Rust.

I think it is a pretty good equivalence, see above. Regarding actual evidence for bugs, Android had enough trouble with incorrect use of FDs that they invented a 'tagging'/'cookie' system to combat that.

[programmerjake]

fn AsRawPtr {
  fn as_raw_ptr(&self) -> *const i32
}

I hope we all agree that there is no reasonable safe fn foo<T: AsRawPtr> that can be written against such a trait.

I'd say there are reasonable safe uses for AsRawPtr, however they are limited to things like comparing the returned pointer (no dereferencing):

struct AssociatedData<T: AsRawPtr> {
    map: HashMap<*const i32, Data>,
}

impl<T: AsRawPtr> AssociatedData<T> {
    fn get(&self, key: &T) -> Option<&Data> {
        self.map.get(&key.as_raw_ptr())
    }
}

[RalfJung]

I'd say there are reasonable safe uses for AsRawPtr, however they are limited to things like comparing the returned pointer (no dereferencing):

Okay, yes, there is a tiny list of things you can do. But that is an extremely niche case.

To my knowledge, the overwhelming majority of code that abstracts over AsRawFd actually performs syscalls on those FDs. This is in direct contradiction to its equivalence to AsRawPtr. In other words, it shows that something in our current story is broken.

[m-ou-se]

I personally think that using Into and From (and potentially TryInto and TryFrom for fallible cases) would be completely reasonable, rather than adding a new set of traits.

@rust-lang/libs Thoughts?

Having the option of fallible conversions is definitely a nice effect of that. Using the existing traits for also reduces the api surface significantly. It also seems consistent with std::process::Stdio implementing From<File>, From<ChildStderr> etc. (instead of having a IntoSubprocessStdio trait).

A potential downside is that we can't add extra methods (with a default implementation) to the trait. Maybe at some point we'd want to add a is_guaranteed_close_on_exec() -> bool method or something. But probably not.

If only we had a language feature that would allow us to make &'a Fd an alias for BorrowedFd<'a>, then we could also use AsRef<Fd>. ^^

cc @rust-lang/libs-api

[the8472]

Maybe at some point we'd want to add a is_guaranteed_close_on_exec() -> bool method or something

APIs that make sense for all descriptors (there aren't many) could in principle be implemented on OwnedFd and BorrowedFd directly. Or extension traits if they're platform-specific. So we wouldn't need the IntoFd or FromFd traits there.
And I think helpers like as_filelike_view::<T>() can still be expressed with where T: From<OwnedFd> + Into<OwnedFd>.

But once we have AsFd::as_filelike_view we don't really need any syscalls (other than close on drop) in OwnedFd because you can always convert to a view that provides them.

But the question is where would from_into_fd go if we did away with IntoFd? I think the From/Into blanket implementations can't do two-step .into() conversion automatically, right?
The user would have to write something like <File as From<OwnedFd>>::from(stdin.into()).

[m-ou-se]

The user would have to write something like <File as From<OwnedFd>>::from(stdin.into()).

Or File::from(Fd::from(stdin))

[the8472]

That's better but doesn't answer the question where such a helper would live. I guess we could make it an associated method on OwnedFd?

Or we could remove that future possibility if chained froms are good enough.

[sunfishcode]

I expect we could reintroduce from_into_fd like this if we wanted to:

trait FromFd: From<OwnedFd> {
    fn from_into_fd<T: Into<OwnedFd>>(t: T) -> Self where Self: Sized {
        let tmp: OwnedFd = t.into();
        tmp.into()
    }
}

impl<T: From<OwnedFd>> FromFd for T {}

The only downside is that users would need to use FromFd; to use it, but that seems worth the overall upside of reusing From.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

[joshtriplett]

Merging...

[joshtriplett]

The RFC has been merged! 🎉

Further discussion and ongoing work should take place in the tracking issue.