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

I/O safety. #87329

Merged
merged 28 commits into from
Aug 20, 2021
Merged

I/O safety. #87329

merged 28 commits into from
Aug 20, 2021

Conversation

sunfishcode
Copy link
Member

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: #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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 21, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@sunfishcode sunfishcode force-pushed the sunfishcode/io-safety branch from fbe1b82 to 712182c Compare July 21, 2021 02:35
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@joshtriplett
Copy link
Member

joshtriplett commented Jul 21, 2021

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.

I don't know how feasible this would be, but I wonder if we could eliminate sys::fs::FileDesc and have sys::fs::File directly use OwnedFd. That would give us what feels like the right amount of indirection: the portable File wraps the UNIX-specific sys::fs::File, which wraps an OwnedFd, which wraps the simple type alias RawFd.

(To clarify, though, that kind of cleanup belongs in a follow-up, not in the initial introduction of this.)

Copy link
Contributor

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good @sunfishcode, just some small comments.

library/std/src/os/unix/io/mod.rs Show resolved Hide resolved
library/std/src/os/unix/io/mod.rs Show resolved Hide resolved
library/std/src/os/unix/io/mod.rs Outdated Show resolved Hide resolved
library/std/src/os/wasi/io/raw.rs Outdated Show resolved Hide resolved
library/std/src/os/windows/io/handle.rs Outdated Show resolved Hide resolved
/// so it can be used in FFI in places where a handle is passed as a consumed
/// argument or returned as an owned value, and is never null.
///
/// Note that it *may* have the value `INVALID_HANDLE_VALUE`. See [here] for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as about with regard to INVALID_HANDLE_VALUE.

library/std/src/os/windows/io/handle.rs Outdated Show resolved Hide resolved
library/std/src/os/windows/io/mod.rs Show resolved Hide resolved
@ChrisDenton
Copy link
Member

In case it's relevant here, Windows essentially has two types of I/O HANDLE (it also reuses the type for other things but that's off topic):

  • Kernel handles. These are "real" handles (i.e. they are an offset into a table of kernel objects).
  • Pseudo handles. These are "fake" handles (i.e. managed by the Win32 subsystem). They have resources managed outside of the NT kernel so are often intentionally invalid kernel handles. The meaning of pseudo handles is very context dependent.

In both cases, zero is not a valid handle. It's also guaranteed that a kernel handle will not have the lowest two bits set. A pseudo handle has no such restriction and indeed may intentionally make use of the lowest two bits to distinguish themselves from "real" handles. Therefore INVALID_HANDLE_VALUE is not a valid kernel handle but can be reused as a pseudo handle.

(I'm also pretty sure a kernel handle will always be less than i32::MAX but I'm not sure to what extent that's guaranteed).

I'm uncertain if this information is helpful to anyone but I just wanted to document some of what I know.

@sunfishcode
Copy link
Member Author

Thanks @ChrisDenton! Do you by chance know of an authoritative reference stating that zero is not a valid handle? The code here assumes that, and for various reasons it seems like a safe assumption, but if there's official documentation about this, I'd like to cite it.

@ChrisDenton
Copy link
Member

Unfortunately I do not. It shakes out that way, as you note, for various practical reasons but I don't know that it's explicitly documented anywhere.

@ChrisDenton
Copy link
Member

@sunfishcode From GetStdHandle:

If an application does not have associated standard handles, such as a service running on an interactive desktop, and has not redirected them, the return value is NULL.

So here a NULL I/O handle means "no handle". You can redirect stdin etc to any I/O handle (where "I/O handle" means "can be passed to WriteFile, etc") therefore no valid I/O handle can be NULL because it would overlap with that meaning.

I admit this falls short of explicit documentation but it's the closest I can find. You could also cite functions such as OpenProcess that use NULL to indicate errors but obviously they're specific to the type of handle.

In short there's no explicit documentation that covers all types of handles but, as the blog you linked to notes, the very fact that it was a "toss up" whether to use NULL to signal an error is a good indication that it's not a valid handle, And unlike INVALID_HANDLE_VALUE there's no documentation that says "for this specific type of handle NULL is treated as a special, valid, handle value".

(more broadly, it would likely break a lot of things if NULL were ever to become valid but this is supposition on my part).

sunfishcode added a commit to sunfishcode/io-lifetimes that referenced this pull request Aug 21, 2021
I/O safety is now [in Rust Nightly]; add a mode to io-lifetimes to use
std's types and traits. See the blurb in README.md for more details.

[in Rust Nightly]: rust-lang/rust#87329
sunfishcode added a commit to sunfishcode/io-lifetimes that referenced this pull request Aug 21, 2021
I/O safety is now [in Rust Nightly]; add a mode to io-lifetimes to use
std's types and traits. See the blurb in README.md for more details.

[in Rust Nightly]: rust-lang/rust#87329
sunfishcode added a commit to sunfishcode/io-lifetimes that referenced this pull request Aug 21, 2021
I/O safety is now [in Rust Nightly]; add a mode to io-lifetimes to use
std's types and traits. See the blurb in README.md for more details.

[in Rust Nightly]: rust-lang/rust#87329
sunfishcode added a commit to sunfishcode/io-lifetimes that referenced this pull request Aug 21, 2021
I/O safety is now [in Rust Nightly]; add a mode to io-lifetimes to use
std's types and traits. See the blurb in README.md for more details.

[in Rust Nightly]: rust-lang/rust#87329
sunfishcode added a commit to sunfishcode/io-lifetimes that referenced this pull request Aug 21, 2021
I/O safety is now [in Rust Nightly]; add a mode to io-lifetimes to use
std's types and traits. See the blurb in README.md for more details.

[in Rust Nightly]: rust-lang/rust#87329
sunfishcode added a commit to sunfishcode/io-lifetimes that referenced this pull request Aug 21, 2021
I/O safety is now [in Rust Nightly]; add a mode to io-lifetimes to use
std's types and traits. See the blurb in README.md for more details.

[in Rust Nightly]: rust-lang/rust#87329
bors pushed a commit to rust-lang-ci/rust that referenced this pull request Aug 23, 2021
Implement `AsFd`, `From<OwnedFd>`, and `Into<OwnedFd>` for
`UnixListener`. This is a follow-up to rust-lang#87329.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 23, 2021
…-io-safety, r=joshtriplett

Implement `AsFd` etc. for `UnixListener`.

Implement `AsFd`, `From<OwnedFd>`, and `Into<OwnedFd>` for
`UnixListener`. This is a follow-up to rust-lang#87329.

r? `@joshtriplett`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 23, 2021
Fix WinUWP std compilation errors due to I/O safety

I/O safety for Windows has landed in rust-lang#87329. However, it does not cover UWP specific parts and prevents all UWP targets from building. See YtFlow/Maple#18. This PR fixes these compile errors when building std for UWP targets.
ehuss pushed a commit to ehuss/rust that referenced this pull request Oct 4, 2021
Fix WinUWP std compilation errors due to I/O safety

I/O safety for Windows has landed in rust-lang#87329. However, it does not cover UWP specific parts and prevents all UWP targets from building. See YtFlow/Maple#18. This PR fixes these compile errors when building std for UWP targets.
@Nashenas88
Copy link
Contributor

In case this is useful, we discovered that this change adds ambiguity for code that was relying on where T: From<F>, F: FromRawFd (roughly). This was a simple fix on our side, but we did see this breakage:

[115822/211194] RUST password_authenticator_integration_test exe.unstripped/password_authenticator_integration_test
FAILED: password_authenticator_integration_test exe.unstripped/password_authenticator_integration_test
mkdir -p ./exe.unstripped &&  RUST_BACKTRACE=1 ../../../recipe_cleanup/rustupp6g4sd/bin/rustc --color=always --crate-name password_authenticator_integration_test ../../src/identity/tests/password_a...
error[E0283]: type annotations needed
   --> ../../src/identity/tests/password_authenticator_integration/src/account_manager.rs:128:17
    |
128 |                 fs::File::from(fdio::create_fd(ramdisk_handle).expect("create fd of dev root"));
    |                 ^^^^^^^^^^^^^^ --------------------------------------------------------------- this method call resolves to `T`
    |                 |
    |                 cannot infer type for type parameter `T` declared on the trait `From`
    |
    = note: cannot satisfy `std::fs::File: From<_>`
note: required by `from`

error[E0283]: type annotations needed
   --> ../../src/identity/tests/password_authenticator_integration/src/account_manager.rs:250:9
    |
250 |           fs::File::from(
    |           ^^^^^^^^^^^^^^ cannot infer type for type parameter `T` declared on the trait `From`
251 | /             fdio::create_fd(
252 | |                 dev_root_proxy
253 | |                     .into_channel()
254 | |                     .expect("Could not convert dev root DirectoryProxy into channel")
...   |
257 | |             )
258 | |             .expect("create fd of dev root"),
    | |____________________________________________- this method call resolves to `T`
    |
    = note: cannot satisfy `std::fs::File: From<_>`
note: required by `from`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0283`.

Where fdio::create_fd has a signature of pub fn create_fd<F: FromRawFd>(handle: zx::Handle) -> Result<F, zx::Status>

@sunfishcode
Copy link
Member Author

Thanks for reporting this! I've now filed #89955 to track this.

Dylan-DPC pushed a commit to Dylan-DPC/rust that referenced this pull request Mar 19, 2022
Copy the relevant trait implementations from the Unix default.
Dylan-DPC pushed a commit to Dylan-DPC/rust that referenced this pull request Mar 19, 2022
Minimally comply with with rust-lang#87329 to avoid breaking tests on L4Re.
@adrianheine
Copy link
Contributor

I don't know if there's another way to handle this, but the since of 1.0.0 and 1.1.0 on items are correct for the re-exports under std::os::unix::io but obviously wrong for std::os::fd, which should be since = "1.56.0".

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 23, 2023
…r=workingjubilee

kmc-solid: I/O safety

Adds the I/O safety API (rust-lang#87329) for socket file descriptors in [`*-kmc-solid_*`](https://doc.rust-lang.org/nightly/rustc/platform-support/kmc-solid.html) Tier 3 targets. All new public items are gated by the `solid_ext` library feature.

This PR adds the following public types and traits:

    std::os::solid::io::AsFd
    std::os::solid::io::BorrowedFd
    std::os::solid::io::OwnedFd

    std::os::solid::prelude::AsFd (re-export)
    std::os::solid::prelude::BorrowedFd (re-export)
    std::os::solid::prelude::OwnedFd (re-export)

And trait implementations:

    From<std::net::TcpListener> for std::os::solid::io::OwnedFd
    From<std::net::TcpStream> for std::os::solid::io::OwnedFd
    From<std::net::UdpSocket> for std::os::solid::io::OwnedFd
    From<std::os::solid::io::OwnedFd> for std::net::TcpListener
    From<std::os::solid::io::OwnedFd> for std::net::TcpStream
    From<std::os::solid::io::OwnedFd> for std::net::UdpSocket
    std::fmt::Debug for std::os::solid::io::BorrowedFd<'_>
    std::fmt::Debug for std::os::solid::io::OwnedFd
    std::io::IsTerminal for std::os::solid::io::BorrowedFd<'_>
    std::io::IsTerminal for std::os::solid::io::OwnedFd
    std::os::fd::AsRawFd for std::os::solid::io::BorrowedFd<'_>
    std::os::fd::AsRawFd for std::os::solid::io::OwnedFd
    std::os::fd::FromRawFd for std::os::solid::io::OwnedFd
    std::os::fd::IntoRawFd for std::os::solid::io::OwnedFd
    std::os::solid::io::AsFd for &impl std::os::solid::io::AsFd
    std::os::solid::io::AsFd for &mut impl std::os::solid::io::AsFd
    std::os::solid::io::AsFd for Arc<impl std::os::solid::io::AsFd>
    std::os::solid::io::AsFd for Box<impl std::os::solid::io::AsFd>
    std::os::solid::io::AsFd for Rc<impl std::os::solid::io::AsFd>
    std::os::solid::io::AsFd for std::net::TcpListener
    std::os::solid::io::AsFd for std::net::TcpStream
    std::os::solid::io::AsFd for std::net::UdpSocket
    std::os::solid::io::AsFd for std::os::solid::io::BorrowedFd<'_>
    std::os::solid::io::AsFd for std::os::solid::io::OwnedFd

Taking advantage of the above change, this PR also refactors the internal details of `std::sys::solid::net` to match the design of other targets, e.g., by redefining `Socket` as a newtype of `OwnedFd`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 23, 2023
…r=workingjubilee

kmc-solid: I/O safety

Adds the I/O safety API (rust-lang#87329) for socket file descriptors in [`*-kmc-solid_*`](https://doc.rust-lang.org/nightly/rustc/platform-support/kmc-solid.html) Tier 3 targets. All new public items are gated by the `solid_ext` library feature.

This PR adds the following public types and traits:

    std::os::solid::io::AsFd
    std::os::solid::io::BorrowedFd
    std::os::solid::io::OwnedFd

    std::os::solid::prelude::AsFd (re-export)
    std::os::solid::prelude::BorrowedFd (re-export)
    std::os::solid::prelude::OwnedFd (re-export)

And trait implementations:

    From<std::net::TcpListener> for std::os::solid::io::OwnedFd
    From<std::net::TcpStream> for std::os::solid::io::OwnedFd
    From<std::net::UdpSocket> for std::os::solid::io::OwnedFd
    From<std::os::solid::io::OwnedFd> for std::net::TcpListener
    From<std::os::solid::io::OwnedFd> for std::net::TcpStream
    From<std::os::solid::io::OwnedFd> for std::net::UdpSocket
    std::fmt::Debug for std::os::solid::io::BorrowedFd<'_>
    std::fmt::Debug for std::os::solid::io::OwnedFd
    std::io::IsTerminal for std::os::solid::io::BorrowedFd<'_>
    std::io::IsTerminal for std::os::solid::io::OwnedFd
    std::os::fd::AsRawFd for std::os::solid::io::BorrowedFd<'_>
    std::os::fd::AsRawFd for std::os::solid::io::OwnedFd
    std::os::fd::FromRawFd for std::os::solid::io::OwnedFd
    std::os::fd::IntoRawFd for std::os::solid::io::OwnedFd
    std::os::solid::io::AsFd for &impl std::os::solid::io::AsFd
    std::os::solid::io::AsFd for &mut impl std::os::solid::io::AsFd
    std::os::solid::io::AsFd for Arc<impl std::os::solid::io::AsFd>
    std::os::solid::io::AsFd for Box<impl std::os::solid::io::AsFd>
    std::os::solid::io::AsFd for Rc<impl std::os::solid::io::AsFd>
    std::os::solid::io::AsFd for std::net::TcpListener
    std::os::solid::io::AsFd for std::net::TcpStream
    std::os::solid::io::AsFd for std::net::UdpSocket
    std::os::solid::io::AsFd for std::os::solid::io::BorrowedFd<'_>
    std::os::solid::io::AsFd for std::os::solid::io::OwnedFd

Taking advantage of the above change, this PR also refactors the internal details of `std::sys::solid::net` to match the design of other targets, e.g., by redefining `Socket` as a newtype of `OwnedFd`.
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` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.