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

Make RawFd implement the RawFd traits #76969

Merged
merged 4 commits into from
Oct 1, 2020

Conversation

withoutboats
Copy link
Contributor

This PR makes RawFd implement AsRawFd, IntoRawFd and FromRawFd, so it can be passed to interfaces that use one of those traits as a bound.

@rust-highfive
Copy link
Collaborator

r? @dtolnay

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 20, 2020
@tmiasko
Copy link
Contributor

tmiasko commented Sep 20, 2020

I would generally assume that it is safe to take ownership of file descriptor returned from IntoRawFd. With changes here it would be unsafe, since RawFd is just an integer.

@withoutboats
Copy link
Contributor Author

@tmiasko That assumption would be wrong and any code which relies on it for safety is unsound, because IntoRawFd is not an unsafe trait and does not assert the implementation upholds any invariant.

@tmiasko
Copy link
Contributor

tmiasko commented Sep 20, 2020

Thanks for correction. I thought that trait was unsafe to implement. It is quite disappointing that it is otherwise. The search results from grep.app reveals that pretty much every single use of IntoRawFd as input bound is unsound, e.g., in wastime, nix, wayland-rs, duct.rs ...

@withoutboats
Copy link
Contributor Author

Glanced at some of those APIs in crates I already had a copy of on my system, and I expect that using these API with invalid RawFds would lead to program bugs, but not undefined behavior. It seems pretty uncommon to rely on on fd validity for soundness.

@cuviper
Copy link
Member

cuviper commented Sep 20, 2020

Note that RawFd is just a type alias, so these are ultimately implementations on any i32. That seems like a bad foot-gun, especially since i32 is the default {integer}.

@joshtriplett
Copy link
Member

It would be nice if it were an opaque type rather than an alias, but I don't think that could be changed backwards-compatibly.

@cuviper
Copy link
Member

cuviper commented Sep 21, 2020

Even a transparent newtype would have been fine, but alas.

@bugaevc
Copy link

bugaevc commented Sep 21, 2020

Previously implemented in #40842, then reverted in #41035

@withoutboats
Copy link
Contributor Author

I agree that it's unfortunate that we made RawFd an alias instead of a newtype, but the footgun stems from that choice and not whether we implement these traits. Anything that wants to use a rawfd (as I, for example, need to do for low-level usages of io-uring) is dealing with any i32 already.

The footgunniness is a consequence of that decision we have already made and stabilized; hobbling the API because of it is just leaving us in a worst-of-all-worlds situation where we have a footgunny API that is incomplete.

The previous PR and reversion in 2017 seems to have happened with basically no discussion. Going to FCP to get a discussion.

@rfcbot fcp merge

@withoutboats withoutboats added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 21, 2020
@withoutboats
Copy link
Contributor Author

@rfcbot fcp merge

(with team tags)

@rfcbot
Copy link

rfcbot commented Sep 21, 2020

Team member @withoutboats has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 21, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Sep 21, 2020

It looks like the contract of IntoRawFd is that ownership of the descriptor is transferred according to its docs:

This function transfers ownership of the underlying file descriptor to the caller. Callers are then the unique owners of the file descriptor and must close the descriptor once it's no longer needed.

It means you could start doing this:

fn take_fd(object: impl IntoRawFd) {
    let fd = object.into_raw_fd();

    // I'm now responsible for closing fd so I'd better do that
}
let fd = object.as_raw_fd();

take_fd(fd);

// Probably going to get an `io::Error` here
object.do_something_else();

But as we've pointed out that these traits are totally safe and RawFd is just an integer anyway I don't think we could really say this is any more dangerous than it was before.

@withoutboats
Copy link
Contributor Author

If people would rather I remove the impl of IntoRawFd I'm comfortable with that (though I prefer not to). I think we can all agree that FromRawFd is completely safe, and the one I really care about is AsRawFd.

The current API correctly understands that there is no guarantee that any method of getting a RawFd generically gives you a correct FD. That's why FromRawFd::from_raw_fd is marked unsafe; in the end it is on the user who has an i32 they're calling a RawFd to guarantee that its a valid file descriptor. Anyone trying to use IntoRawFd as generic bound that means the type converts to a valid, exclusive file descriptor has a soundness bug if they are relying on it for soundness.

But normally even doing something very wrong with an fd will "just" result in an program error, not unsoundness. A double close is not a double free, a read on a random FD is not a wild pointer dereference. They are very bad and can even in the worst case lead to terrible outcomes like irretrievable data loss, security vulnerabilities, and so on. But we do not guarantee that they will never occur in safe code the same way we do with memory vulnerabilities.

The best API makes this unlikely to occur. I think even with RawFd as a type alias, we've done a pretty good job because its very rare that users actually interact with RawFd, and they hopefully know to do so with care.

Mainly I have low-level APIs that need to take a RawFd, but I'd like to support passing other types to those APIs that provide stronger type safety guarantees as well. But I can't make the interface take &impl AsRawFd because then it can't accept RawFd.

@KodrAus
Copy link
Contributor

KodrAus commented Sep 21, 2020

Leaving the IntoRawFd impl also seems reasonable to me for the reasons you mention. I actually meant to check my box but must’ve missed... Should probably start using rfcbot commands instead 🙂

@cuviper
Copy link
Member

cuviper commented Sep 21, 2020

There was another attempt in #43254, again closed for the effect on i32.
There was also an RFC issue: rust-lang/rfcs#2074

@cuviper
Copy link
Member

cuviper commented Sep 21, 2020

I think we should seriously consider deprecating RawFd in favor of a better newtype, like:

#[repr(transparent)]
pub struct UnixFd(pub raw::c_int); // or even just `Fd`

@withoutboats
Copy link
Contributor Author

I think we should seriously consider deprecating RawFd in favor of a better newtype, like:

I also think this is a good idea to explore, but its gonna be a pretty big transition. Not sure if there's enough energy for it.

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Sep 22, 2020
@rfcbot
Copy link

rfcbot commented Sep 22, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Sep 22, 2020
@BurntSushi
Copy link
Member

Makes sense to me. The footgun is weird, but I buy @withoutboats' argument: adding these impls doesn't make the footgun worse.

A newtype might have been a better approach, but I don't know that it's enough of a value add to follow through on that at this point. At the very least, these impls improve the status quo and don't really make it any harder to pursue a newtype strategy later if we deemed it worth it.

@withoutboats
Copy link
Contributor Author

I would like to get feedback from @dtolnay before we merge this, since he participated in previous discussions and may have had concerns about these impls

@dtolnay
Copy link
Member

dtolnay commented Sep 25, 2020

I think this is fine. @rfcbot reviewed

I notice another thing we would want but can't do is impl<F> AsRawFd for &F where F: ?Sized + AsRawFd. :/

@dtolnay
Copy link
Member

dtolnay commented Oct 1, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 1, 2020

📌 Commit 35b30e2 has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 1, 2020
@bors
Copy link
Contributor

bors commented Oct 1, 2020

⌛ Testing commit 35b30e2 with merge 2ad6187...

@bors
Copy link
Contributor

bors commented Oct 1, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: dtolnay
Pushing 2ad6187 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 1, 2020
@bors bors merged commit 2ad6187 into rust-lang:master Oct 1, 2020
@rustbot rustbot added this to the 1.48.0 milestone Oct 1, 2020
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Oct 2, 2020
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Oct 8, 2020
sunfishcode added a commit to sunfishcode/io-extras that referenced this pull request Feb 20, 2021
This comes from the suggestion [here].

[here]: rust-lang/rust#76969 (comment)
@sunfishcode
Copy link
Member

If anyone is interested in a newtype for RawFd, I'm working on a crate called unsafe-io which calls this UnsafeHandle. On RawFd platforms it's a repr(transparent) wrapper around RawFd. AsUnsafeHandle is an unsafe trait which asserts that the returned handle is a copy of an owned handle, and IntoUnsafeHandle is an unsafe trait that asserts that the returned handle a transfer from an owned handle (as inspired by the discussion above).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. 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-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.