feat: I/O safety for 'sys/termios' & 'pty' #1921
Merged
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 this PR does:
sys/termios
andpty
Known Problems:
Double free issue on
PtyMaster
I have changed the
RawFd
inPtyMaster
toOwnedFd
in this PR, with thischange, the double-free issue still exists, see this test code snippet
(From this comment)
I have tested this code with
nix 0.26.1
, and I am still gettingEBADF
, which means the current impl does not prevent this problem either.If we still wanna the drop of
PtyMaster
panic when the internalfd
is invalidas we did in PtyMaster::drop should panic on EBADF #677, then we have to revert the changes to use
RawFd
and manually implDrop
.Some trait implementations for some types are removed
struct OpenptyResult
:struct ForkptyResult
:struct PtyMaster
:In the previous implementation, these trait impls are
#[derive()]
ed, due tothe type change to
OwnedFd
, we can no longer derive them. Should we manuallyimplement them?
I kinda think we should at least impl
PartialEq
andEq
forOpenptyResult
and
PtyMaster
.Some Clarifications that may help code review
For the basic
fd
-related syscall likeread(2)
,write(2)
andfcntl(2)
, I am still using the old
RawFd
interfaces, as they will be covered inother PRs.
Two helper functions
write_all()
intest/sys/test_termios.rs
:read_exact()
intest/test.rs
:I have added I/O safety for them, but it actually does not matter whether
they use
Fd: AsFd
orRawFd
. So feel free to ask me to discard these changesif you guys don't like it.