{PipeRead,PipeWrite}::from_raw_fd_checked is unsound
#31
Comments
|
That's quite reasonable. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The documentation is enough to see it:
(sic)
The method is allegedly safe, but the argument, however, is a
RawFd. It's not possible forPipeWriteto take ownership of aRawFd, and remain a safe method: taking ownership of aRawFdis an inherently unsafe operation, as can be seen instd's documentation for converting aRawFdinto anOwnedFd. This is because you, the caller, must maintain the invariant that you yourself have the ownership to give, as it isn't (yet) tracked by the type system. Were it safe, you could simply convert oneRawFdinto twoOwnedFds, each believing itself to be the owner of the same FD. The same problem exists here.Under the hood,
PipeWritedoes not convert the FD to anOwnedFd; rather, it just keeps theRawFd, but the "take the ownership" does happen; that is visible in, e.g.,PipeFd::drop:(
PipeFdis a bit of common-code; bothPipeWrite&PipeReadend up holding aPipeFd.)Given this, it's easy to whip up an example that triggers UB:
Cargo.toml
Cargo.lock
This program should fail at
let p2 =, as it isn't logically possible for both to own the same FD. However, neither fail, so we end up with both "owning" the same FD. Thedrop(p1)closes the pipe, setting upp2to drop a completely random FD. Thedrop(p2)call here ends up closing the FD owned by theFile, and the write fails with "Bad file descriptor".In a larger program this would be rather hard to debug; writes could easily end up in unintended places as FDs are recycled, or reads could occur unexpectedly on FDs owned by other objects. UB.
I wonder if a better design here would be for
PipeWrite&PipeReadto be, e.g.,PipeWrite<T: AsRawFd>; one could then pass anOwnedFd(in which casePipeWriteet al. would own it) or aRawFd(and it would not); the concern of safely managing the ownership of the raw FD is essentially put onto the caller. This is, of course, a breaking change.The text was updated successfully, but these errors were encountered: