Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking Issue for RFC 3128: I/O Safety #87074

Closed
1 of 5 tasks
joshtriplett opened this issue Jul 12, 2021 · 88 comments · Fixed by #95118
Closed
1 of 5 tasks

Tracking Issue for RFC 3128: I/O Safety #87074

joshtriplett opened this issue Jul 12, 2021 · 88 comments · Fixed by #95118
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@joshtriplett
Copy link
Member

joshtriplett commented Jul 12, 2021

Feature gate: #![feature(io_safety)]

This is a tracking issue for RFC 3128: I/O Safety.

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.

Public API

The public API on UNIX platforms consists of the types OwnedFd and BorrowedFd, the trait AsFd, and implementations of AsFd, Into<OwnedFd>, and From<OwnedFd> for various types (such as files and sockets).

The public API on Windows platforms consists of two sets of parallel types and traits and impls for OwnedHandle, OwnedSocket, BorrowedHandle, BorrowedSocket, etc.

Steps / History

Unresolved Questions

  • This RFC doesn't define a formal model for raw handle ownership and lifetimes. The rules for raw handles in this RFC are vague about their identity. What does it mean for a resource lifetime to be associated with a handle if the handle is just an integer type? Do all integer types with the same value share that association?
  • The Rust reference defines undefined behavior for memory in terms of LLVM's pointer aliasing rules; I/O could conceivably need a similar concept of handle aliasing rules. This doesn't seem necessary for present practical needs, but it could be explored in the future.
@joshtriplett joshtriplett added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Jul 12, 2021
@inquisitivecrystal inquisitivecrystal added the A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` label Jul 18, 2021
@sunfishcode
Copy link
Member

I've now posted an initial PR implementing the proposal in the RFC: #87329

Thomasdezeeuw added a commit to Thomasdezeeuw/socket2 that referenced this issue Jul 28, 2021
Since we now use the niche feature on Unix it's unsound to use
SockRef::from(-1), but it can be done without any unsafe. This change
adds an assertion to ensure we hit this soundness issue.

Still need to wait on the I/O safety RFC:
https://github.com/rust-lang/rfcs/blob/master/text/3128-io-safety.md
Tracking issue: rust-lang/rust#87074
Implementation pr: rust-lang/rust#87329
Thomasdezeeuw added a commit to Thomasdezeeuw/socket2 that referenced this issue Jul 28, 2021
Since we now use the niche feature on Unix it's unsound to use
SockRef::from(-1), but it can be done without any unsafe. This change
adds an assertion to ensure we hit this soundness issue.

Still need to wait on the I/O safety RFC:
https://github.com/rust-lang/rfcs/blob/master/text/3128-io-safety.md
Tracking issue: rust-lang/rust#87074
Implementation pr: rust-lang/rust#87329
Thomasdezeeuw added a commit to Thomasdezeeuw/socket2 that referenced this issue Jul 28, 2021
Since we now use the niche feature on Unix it's unsound to use
SockRef::from(-1), but it can be done without any unsafe. This change
adds an assertion to ensure we hit this soundness issue.

Still need to wait on the I/O safety RFC:
https://github.com/rust-lang/rfcs/blob/master/text/3128-io-safety.md
Tracking issue: rust-lang/rust#87074
Implementation pr: rust-lang/rust#87329
Thomasdezeeuw added a commit to rust-lang/socket2 that referenced this issue Jul 28, 2021
Since we now use the niche feature on Unix it's unsound to use
SockRef::from(-1), but it can be done without any unsafe. This change
adds an assertion to ensure we hit this soundness issue.

Still need to wait on the I/O safety RFC:
https://github.com/rust-lang/rfcs/blob/master/text/3128-io-safety.md
Tracking issue: rust-lang/rust#87074
Implementation pr: rust-lang/rust#87329
sunfishcode added a commit to sunfishcode/rust that referenced this issue Aug 19, 2021
Introduce `OwnedFd` and `BorrowedFd`, and the `AsFd` trait, and
implementations of `AsFd`, `From<OwnedFd>` and `From<T> for OwnedFd`
for relevant types, along with Windows counterparts for handles and
sockets.

Tracking issue:
 - <rust-lang#87074>

RFC:
 - <https://github.com/rust-lang/rfcs/blob/master/text/3128-io-safety.md>
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 20, 2021
…joshtriplett

I/O safety.

Introduce `OwnedFd` and `BorrowedFd`, and the `AsFd` trait, and
implementations of `AsFd`, `From<OwnedFd>` and `From<T> for OwnedFd`
for relevant types, along with Windows counterparts for handles and
sockets.

Tracking issue: <rust-lang#87074>

RFC: <https://github.com/rust-lang/rfcs/blob/master/text/3128-io-safety.md>

Highlights:
 - The doc comments at the top of library/std/src/os/unix/io/mod.rs and library/std/src/os/windows/io/mod.rs
 - The new types and traits in library/std/src/os/unix/io/fd.rs and library/std/src/os/windows/io/handle.rs
 - The removal of the `RawHandle` struct the Windows impl, which had the same name as the `RawHandle` type alias, and its functionality is now folded into `Handle`.

Managing five levels of wrapping (File wraps sys::fs::File wraps sys::fs::FileDesc wraps OwnedFd wraps RawFd, etc.) made for a fair amount of churn and verbose as/into/from sequences in some places. I've managed to simplify some of them, but I'm open to ideas here.

r? `@joshtriplett`
@sunfishcode
Copy link
Member

The implementation PR is now merged!

@sunfishcode
Copy link
Member

Examples of using the I/O safety types and traits in Rust Nightly in real-world codebases, with all tests passing:

@jstarks
Copy link

jstarks commented Aug 23, 2021

Hi, I'm on the Windows team at Microsoft. Perhaps this deserves a separate issue, but posting here for now...

OwnedHandle claims that INVALID_HANDLE_VALUE is a valid value:

Note that it may have the value INVALID_HANDLE_VALUE (-1), which is
sometimes a valid handle value. See [here] for the full story. For APIs
like CreateFileW which report errors with INVALID_HANDLE_VALUE instead
of null, use [HandleOrInvalid] instead of Option<OwnedHandle>.

But this doesn't make sense for OwnedHandle. It's true that INVALID_HANDLE_VALUE can sometimes act as a valid handle for inputs. Specifically, the same value, -1, is used for the current process pseudo handle, which you can pass to functions such as DuplicateHandle and (less usefully) WaitForSingleObject. But it's never the case that INVALID_HANDLE_VALUE is a valid owned handle--you can never legally call CloseHandle on it.

In fact, in user mode (which is all we care about for this target), the only valid owned handle values are > 0--negative handle values are reserved for kernel handles (which are inaccessible to user mode) and pseudo handles, neither of which can be closed.

So the documentation should be updated. But I also think the use of OwnedHandle will be less error prone if you assert (in from_raw_handle and similar) that the value is > 0 instead of != null, so that users do not accidentally store INVALID_HANDLE_VALUE or a pseudo handle in it.

This also gives you a much bigger niche if you choose to use it. It would be reasonable to stick with NonNull for now and optimize as a second step, even after stabilization. But you can't strengthen the assertions after stabilization.

BorrowedHandle, OTOH, is correct, since it would be reasonable to store a pseudo handle in a BorrowedHandle<'static>.

@jstarks
Copy link

jstarks commented Aug 23, 2021

Related question: does HandleOrInvalid belong in std? I like the type--it's a clever approach for dealing with CreateFileW and related APIs that return INVALID_HANDLE_VALUE on error. But I'm not sure that it really needs to be in std, since it really only needs to be used locally at FFI edges before converting to OwnedHandle. I would be very surprised to see it in a function signature in any public Rust API.

Just my 2c.

@sunfishcode
Copy link
Member

sunfishcode commented Aug 23, 2021

That makes sense, however there's another dimension: it's a somewhat established convention to use ManuallyDrop<File> and similar to construct temporary views to otherwise borrowed handles—with some care, the idiom can be done reasonably safely. In those situations, an OwnedHandle can hold a pseudo handle.

One thing we could easily do though would be to assert!(handle > 0) just before libstd's call to CloseHandle. Does that sound like a worthwhile change?

While you're here 😄 would you be able to authoritatively state that NULL is never returned, as an error or as a valid handle, from functions that use INVALID_HANDLE_VALUE? The Windows documentation isn't clear on this point.

Related question: does HandleOrInvalid belong in std? I like the type--it's a clever approach for dealing with CreateFileW and related APIs that return INVALID_HANDLE_VALUE on error. But I'm not sure that it really needs to be in std, since it really only needs to be used locally at FFI edges before converting to OwnedHandle. I would be very surprised to see it in a function signature in any public Rust API.

Indeed, HandleOrInvalid is limited to that exact use case. It's of course subjective whether HandleOrInvalid should be in std; I favor including it because on the Unix side, OwnedFd and BorrowedFd support this kind of FFI use case, and HandleOrInvalid allows std to provide a roughly similar level of functionality for Windows.

@sunfishcode
Copy link
Member

Ah, yes. I'm not familiar with the V4L2 APIs myself, but this sounds similar to the case with mmap. APIs exposing such functions must be unsafe, unless they fully encapsulate the ability to write to arbitrary memory addresses. This isn't specific to OwnedFd, and should already be the case in Rust today.

@sunfishcode
Copy link
Member

sunfishcode commented Jun 7, 2022

For example, rustix::io::read is a wrapper around the read system call. The system call takes a raw pointer, so it could write to any address. The API encapsulates the unsafety by only allowing users to pass in a Rust slice, rather than an arbitrary pointer value, so it can be a safe API.

rustix::io::mmap on the other hand doesn't fully encapsulate the ability to work with raw memory, so it is an unsafe API.

Both of these functions use AsFd to accept their file descriptors. That says is "the file descriptor is not closed, and not held after the call". From<OwnedFd> and Into<OwnedFd> are similar; they encapsulate the "who's responsible for calling close" and "make sure no one uses the fd after the close" aspects of safety. However, some system calls have additional unrelated safety aspects that must be considered when deciding whether an API wrapping them must be unsafe.

@the8472
Copy link
Member

the8472 commented Jun 7, 2022

I'd say this is a bit of a grey area. You can already do extremely dangerous (including memory safety) things just by writing to things like /proc/self/mem or (if you have root) /dev/sdx. We usually ignore those ambient footguns because otherwise even opening files could be unsafe (after all merely opening something on a FUSE filesystem could trigger anything).

It leaves the judgment call how airtight you want your wrappers to be to the developers. E.g. for a mmap wrapper you can either implement From<OwnedFd> on the grounds that anyone can dup the fd via /proc/self/fd/n anyway or you can not do that on the ground that - at least inside rust - you really want exclusive ownership (which means we can't trust safe code to not have duped the fd beforehand) of the file so you can guarantee mutable-exclusive access to the memory. I lean towards the latter case, constructing the mmap wrapper should be unsafe and the safety invariant should be that one asserts exclusive access to the file description.

Into<OwnedFd> is more straight-forward I think. If you got still things in flight relying on the underlying file that could experience safety violations from concurrent access then it's better not to hand it out.


I think the shared file description thing is a bit of a problem in general. E.g. you could write a thing that very very carefully pokes around on the block device underlying a mounted filesystem (e.g. only fiddling around with a boot sector which isn't used at runtime) and if everything goes right your system won't blow up. But if there's another thing accessing the same file description through a duped file descriptor and only does random seeks all the time (which in itself would appear to be perfectly harmless) then this will go horribly horribly wrong.
Well, maybe this isn't the best example. In this case it's the file descriptor (to the block device) itself which is hazardous and needs to be treated as if it were one giant unsafe block, not any of the other operations.
The mmap scenario probably is a better one.

@sunfishcode
Copy link
Member

I think we've addressed these gray areas at this point.

/proc/self/mem is considered to read and write to memory from the outside, as if by a debugger that is outside of Rust's control. This is the distinction effectively made by std itself and throughout the ecosystem. It is a popular question though, so I'll submit a PR to add documentation answering it.

The only time Rust code can assume it has exclusive access to a file description is if it created a file descriptor for itself and fully encapsulated it, including not implementing AsFd or using From<OwnedFd>. This has always been true of raw file descriptors, and continues to be true with OwnedFd/BorrowedFd. #97178 adds documentation about this.

@sunfishcode
Copy link
Member

I've now opened #97837 to add documentation about /proc/self/mem.

@the8472
Copy link
Member

the8472 commented Jun 7, 2022

This is specific to /proc/self/mem, it's just an easy example. There are many many accessible OS features that can subvert safety, doubly so if you're root.

Edit: I see the PR already tries to generalize beyond that


Regarding things that take AsFd, I don't think people will defend against shared file descriptions in practice because it means that seek positions can change under their nose and being defensive about that gets really tedious and taking AsFd is more of a tool to take either of StdinLock or File which on the surface do seem like you do get exclusive-under-sane-conditions access to the file.

@tanriol
Copy link
Contributor

tanriol commented Jun 7, 2022

I'd still suggest stating somewhere in the documentation that if safety of your type depends on some properties (filesystem, device driver, etc.) of the underlying file descriptor that can not hold for a reasonable FD (where "reasonable" means "not /proc/self/mem or something like it"), then implementing From<OwnedFd> is not appropriate and, if needed, you should make an unsafe associated function for that conversion instead.

@sunfishcode
Copy link
Member

On Unix-family platforms, std's types don't statically require specific types of file descriptors. Even specialized types like std::fs::File may safely hold non-file things like sockets or directories or timer fds or process fds or devices or many other things. Before OwnedFd, people have been known to deliberately use File in this way, as a makeshift OwnedFd. It's safe to do this, because maybe you'll get EISDIR, ESPIPE, or other errors when you try to use it, but you'll never get undefined behavior.

All open file descriptors are suitable for From<OwnedFd>. It's even valid to have a file descriptor for /proc/self/mem or even /dev/mem. If I call a library that accepts From<OwnedFd> and I pass it such a file descriptor, and Undefined Behavior ensues when the library tries to do I/O with it, the bug is in my code, even though it was the library that did the I/O which caused the Undefined Behavior. That's surprising, but arguably, it's surprising that a popular general-purpose OS would have a file in the default global namespace for all processes which behaves like this.

@the8472
Copy link
Member

the8472 commented Jun 7, 2022

I see several overlapping cases

  • The file descriptor is extremely hazardous. /dev/mem and the block devices underpinning mounted filesystems are such examples. Passing those around too liberally is indeed a fault of the caller
  • The library does some very unsafe things such as mmap or reading bytes and transmuting them into repr(C). It's probably the library's fault for exposing From<OwnedFd> in the first place or not checking all its preconditions properly
  • subtle interactions with shared file descriptions. Posix says it's library-UB to do many things on a file descriptor if its description has been used in an fdopendir. Some unsafe code operating on a file might be designed to be safe in the absence adversarial seek calls. Messing with fcntls on a file, e.g. unlocking it, could lead to undefined behavior due to IPC mechanisms ending up in an illegal state
  • A library that does very specific things that are either harmless or correct 99.9% of the time but could lead to corruption in some unfortunate combination of file descriptor and syscall. I think that's mostly ioctls. Libraries can probably defend against that by checking the device major/minor numbers, the filesystem type or something like that. The responsibility might be with both parties here. The caller fed arbitrary devices to a very specialized library and the library wasn't aware of the conflict potential because it does the right thing 99.9% of the time.

@sunfishcode
Copy link
Member

I'm having difficulty translating these into specific areas in the documentation that could be modified, and specific points to make. Would anyone be able to suggest wording here?

A library that does very specific things that are either harmless or correct 99.9% of the time but could lead to corruption in some unfortunate combination of file descriptor and syscall. I think that's mostly ioctls.

I'm not aware of any system calls which behave like this. Does anyone have specific examples of this?

@the8472
Copy link
Member

the8472 commented Jun 7, 2022

https://lore.kernel.org/lkml/[email protected]/

The ioctl 2 has different meanings for regular files and some character device drivers. That was about a kernel bug where the kernel misinterpreted it, but they fixed that which still means that it can cause different behavior (or error out because the ioctl is not supported) depending on what file descriptor you call it on.

@sunfishcode
Copy link
Member

APIs for ioctls that have multiple meanings, where it's possible to get Undefined Behavior when the wrong meaning is invoked, should either use unsafe or fstat to ensure that they only operate on the types of descriptors they expect.

From<OwnedFd> does not put any conditions on the types of file descriptors it can accept. To be sound, users of From<OwnedFd> must not permit undefined behavior for any possible file descriptor type.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jun 9, 2022
@sunfishcode
Copy link
Member

The 2 remaining items in the Steps / History at the top are completed:

The 2 Unresolved Questions are questions are questions for the future, and not questions that I expect need to be answered for this feature to proceed.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jun 15, 2022
…o-safety, r=joshtriplett

Implement stabilization of `#[feature(io_safety)]`.

Implement stabilization of [I/O safety], aka `#[feature(io_safety)]`.

Fixes rust-lang#87074.

[I/O safety]: https://github.com/rust-lang/rfcs/blob/master/text/3128-io-safety.md
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jun 15, 2022
…o-safety, r=joshtriplett

Implement stabilization of `#[feature(io_safety)]`.

Implement stabilization of [I/O safety], aka `#[feature(io_safety)]`.

Fixes rust-lang#87074.

[I/O safety]: https://github.com/rust-lang/rfcs/blob/master/text/3128-io-safety.md
@bors bors closed this as completed in 67ed99e Jun 15, 2022
@sunfishcode
Copy link
Member

Thanks to @RalfJung for helping me understand this space and @joshtriplett for mentoring and reviews, and many others for discussions and comments that helped shape this.

For the next chapter in the I/O safety story: once this feature reaches stable, the next step is to begin adding trait implementations for AsFd etc. for types in popular crates, to pave the way for a gradual migration. I've filed a planning issue here with the full plan:

sunfishcode/io-lifetimes#38

sunfishcode added a commit to sunfishcode/terminal-size that referenced this issue Jul 1, 2022
This migrates terminal_size from using libc directly to using rustix,
which eliminates a few unsafe blocks and manual error handling.

It does also add one new unsafe block, though this is because
`terminal_size_using_fd` is a public safe function that has a `RawFd`
argument. With [I/O safety], which is expected to be [stabilized in Rust 1.63],
there will be new guidance saying be that [such functions should be unsafe].
It's not urgent to make any changes for this right now though.

[I/O safety]: https://github.com/rust-lang/rfcs/blob/master/text/3128-io-safety.md
[expected to be stabilized in Rust 1.63]: rust-lang/rust#87074
[such functions should be unsafe]: https://doc.rust-lang.org/nightly/std/os/unix/io/index.html
sunfishcode added a commit to sunfishcode/terminal-size that referenced this issue Jul 1, 2022
This migrates terminal_size from using libc directly to using rustix,
which eliminates a few unsafe blocks and manual error handling.

It does also add one new unsafe block, though this is because
`terminal_size_using_fd` is a public safe function that has a `RawFd`
argument. With [I/O safety], which is expected to be [stabilized in Rust 1.63],
there will be new guidance saying be that [such functions should be unsafe].
It's not urgent to make any changes for this right now though.

[I/O safety]: https://github.com/rust-lang/rfcs/blob/master/text/3128-io-safety.md
[stabilized in Rust 1.63]: rust-lang/rust#87074
[such functions should be unsafe]: https://doc.rust-lang.org/nightly/std/os/unix/io/index.html
sunfishcode added a commit to sunfishcode/terminal-size that referenced this issue Jul 1, 2022
This migrates terminal_size from using libc directly to using rustix,
which eliminates a few unsafe blocks and manual error handling.

It does also add one new unsafe block, though this is because
`terminal_size_using_fd` is a public safe function that has a `RawFd`
argument. With [I/O safety], which is expected to be [stabilized in Rust 1.63],
there will be new guidance saying be that [such functions should be unsafe].
It's not urgent to make any changes for this right now though.

This also updates the minimum Rust version to 1.48, which is about as
old as the previous minimum Rust version was when it was last updated.

[I/O safety]: https://github.com/rust-lang/rfcs/blob/master/text/3128-io-safety.md
[stabilized in Rust 1.63]: rust-lang/rust#87074
[such functions should be unsafe]: https://doc.rust-lang.org/nightly/std/os/unix/io/index.html
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
…r=joshtriplett

 Define a dedicated error type for `HandleOrNull` and `HandleOrInvalid`.

Define `NullHandleError` and `InvalidHandleError` types, that implement std::error::Error, and use them as the error types in `HandleOrNull` and `HandleOrInvalid`,

This addresses [this concern](rust-lang/rust#87074 (comment)).

This is the same as #95387.

r? `@joshtriplett`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.