refactor: I/O safety for unistd.rs #2440
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SteveLauC
force-pushed
the
refactor/io_safety_unistd
branch
from
f7e7382 to
46a79c5
Compare
SteveLauC
force-pushed
the
refactor/io_safety_unistd
branch
from
46a79c5 to
edcb0dd
Compare
SteveLauC
commented
Jun 10, 2024
| ))] | ||
| pub(crate) fn at_rawfd(fd: Option<RawFd>) -> raw::c_int { | ||
| #[cfg(all(feature = "fanotify", target_os = "linux"))] | ||
| pub(crate) fn at_rawfd(fd: Option<RawFd>) -> RawFd { |
There was a problem hiding this comment.
This can be removed when we add I/O safety to module fanotify.
| @@ -73,7 +73,7 @@ impl IntoRawFd for PtyMaster { | |||
|
|
|||
| impl io::Read for PtyMaster { | |||
| fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> { | |||
| unistd::read(self.0.as_raw_fd(), buf).map_err(io::Error::from) | |||
There was a problem hiding this comment.
This Read implementation (and Write) feels a little bit weird to me, I kinda think we should not implement std Read/Write traits for Nix wrapper types, the I/O syscalls should be used instead.
But this implementation would definitely make our interface more rusty, so I am not quite against it, though I think we should be consistent, i.e., if one wrapper type has this trait implemented, we should do this to all the wrapper types.
src/unistd.rs
Outdated
| @@ -1295,19 +1504,21 @@ impl LinkatFlags { | |||
| /// # References | |||
| /// See also [linkat(2)](https://pubs.opengroup.org/onlinepubs/9699919799/functions/linkat.html) | |||
| #[cfg(not(target_os = "redox"))] // RedoxFS does not support symlinks yet | |||
There was a problem hiding this comment.
Not sure if hard link is supported by RedoxFS, but linkat() is for hard link rather than symlink
SteveLauC
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do
Add I/O safety to module unistd.
File descriptor duplication
The safe interface for
dup2()proposed in this PR looks like this:We take
newfdby mutable reference so that we can ensure we have exclusive access. This interface stops users from specifyingnewfdwith arbitrary fd values,unsafe fn dup2_raw()comes as a remedy:dup2_raw()is unsafe because when specifying a fd that is open, one has to ensure the returnedOwnedFdis the ONLY owner of this fd, or a double close will happen.Using the above
dup2()interface for stdin/stdout/stderr redirection is troublesome, one has to construct anOwnedFdfrom stdin/stdout/stderr, thenmem::forget()it. We provide 3 helper functions to make this easier:Since the only flag that can be used with
dup3()isO_CLOEXEC, which does not make much sense to stdin/stdout/stderr, I didn't provide the dup3 versions of these utilities.Checklist:
CONTRIBUTING.md