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

Fix uds::pair_descriptors() to actually take an output argument as intended #1141

Merged
merged 1 commit into from Nov 11, 2019

Conversation

@mlang
Copy link
Contributor

mlang commented Nov 10, 2019

This took me a while to figure out. The original symptom I saw was that
registering UnixStream::pair() streams fails with "Bad file descriptor".
Digging deeper, I realized all operations on the streams result in EBADF.
Reading the code, it suddenly dawned on me:
mio::sys::unix::uds::pair_descriptors is used with the intention of fds
being an output argument. However, fds is declared as a mutable array, not a
reference to a mutable array. So what currently happens in the code path of
Unix{Datagram,Stream}::pair is that we initialize fds to [-1; 2], copy
that into pair_descriptors() and return it later as two streams or
datagrams. Always set to -1 because the modifications to fds by
pair_descriptors gets lost.

The fix is to change fds to a mutable reference, and update the call sites
to declare the array mutable.

This is tested and works, I can finally use UnixStream::pair in my mio poll
loop.

However, this patch uncovers another bug that should be fixed separately:

Unix{Datagram,Stream}::pair in src/sys/unix/uds/{datagram,stream}.rs both
have a special case for Darwin and Solaris. Quoting the code below:

   // Darwin and Solaris do not have SOCK_NONBLOCK or SOCK_CLOEXEC.
   //
   // In order to set those flags, additional `fcntl` sys calls must be
   // made in `pair_descriptors` that are fallible. If a `fnctl` fails
   // after the sockets have been created, the file descriptors will
   // leak. Creating `s1` and `s2` below ensure that if there is an
   // error, the file descriptors are closed.
   #[cfg(any(target_os = "ios", target_os = "macos", target_os = "solaris"))]
   let pair = {
       let s1 = unsafe { UnixStream::from_raw_fd(fds[0]) };
       let s2 = unsafe { UnixStream::from_raw_fd(fds[1]) };
       pair_descriptors(&mut fds, flags)?;
       (s1, s2)
   };

The comment is very careful to explain the situation, but the code below of it
is hopelessly wrong. from_raw_fd takes a copy of fd. so both streams again
get initialized with a -1 fd, and the result of pair_descriptors is
essentially discarded.

To sum up: with this path, Unix{Datagram,Stream}::pair() works correctly on
everything other then ios, macos and solaris.

I'd like to leave fixing of those other platforms to the person that wrong
this elaborate comment above.

…tended

This took me a while to figure out.  The original symptom I saw was
that registering `UnixStream::pair()` streams fails with "Bad file descriptor".
Digging deeper, I realized all operations on the streams result in EBADF.
Reading the code, it suddenly dawned on me: `mio::sys::unix::uds::pair_descriptors`
is used with the intention of `fds` being an output argument.
However, `fds` is declared as a mutable array, not a reference to a mutable array.
So what currently happens in the code path of `Unix{Datagram,Stream}::pair` is that
we initialize `fds` to [-1; 2], copy that into `pair_descriptors()` and return it
later as two streams or datagrams.  Always set to -1 because the modifications
to `fds` by `pair_descriptors` gets lost.

The fix is to change `fds` to a mutable reference, and update
the call sites to declare the array mutable.

This is tested and works, I can finally use `UnixStream::pair` in my mio poll loop.

However, this patch uncovers another bug that should be fixed separately:

`Unix{Datagram,Stream}::pair` in `src/sys/unix/uds/{datagram,stream}.rs`
both have a special case for Darwin and Solaris.  Quoting the code below:

```rust
    // Darwin and Solaris do not have SOCK_NONBLOCK or SOCK_CLOEXEC.
    //
    // In order to set those flags, additional `fcntl` sys calls must be
    // made in `pair_descriptors` that are fallible. If a `fnctl` fails
    // after the sockets have been created, the file descriptors will
    // leak. Creating `s1` and `s2` below ensure that if there is an
    // error, the file descriptors are closed.
    #[cfg(any(target_os = "ios", target_os = "macos", target_os = "solaris"))]
    let pair = {
        let s1 = unsafe { UnixStream::from_raw_fd(fds[0]) };
        let s2 = unsafe { UnixStream::from_raw_fd(fds[1]) };
        pair_descriptors(&mut fds, flags)?;
        (s1, s2)
    };
```

The comment is very careful to explain the situation, but
the code below of it is hopelessly wrong.  from_raw_fd takes a copy
of fd. so both streams again get initialized with a -1 fd, and the result
of pair_descriptors is essentially discarded.

To sum up: with this path, `Unix{Datagram,Stream}::pair()` works correctly
on everything other then ios, macos and solaris.

I'd like to leave fixing of those other platforms to the person that
wrong this elaborate comment above.
Copy link
Collaborator

Thomasdezeeuw left a comment

LGTM.

@Thomasdezeeuw Thomasdezeeuw requested a review from kleimkuhler Nov 10, 2019
@Thomasdezeeuw

This comment has been minimized.

Copy link
Collaborator

Thomasdezeeuw commented Nov 10, 2019

@kleimkuhler or @mlang can we add some tests for this? These thing should have really be caught by tests.

Copy link
Contributor

kleimkuhler left a comment

@mlang Thanks for sending this fix in! I'd like to see the pair_descriptors function go away in general. It's trying to reduce code duplication between UnixStream::pair and UnixDatagram::pair. Due to it having to return a Result<()> it is more complicated than it needs to be without actually reducing that much code.

I'd like to see pair methods fixed in a single PR, but @Thomasdezeeuw if you are okay merging this we can do a follow-up that does the above?

@kleimkuhler

This comment has been minimized.

Copy link
Contributor

kleimkuhler commented Nov 10, 2019

We can follow-up with this, or @mlang you can make these changes as part of this PR. The linked branch removes pair_descriptors and the pair methods handle cleanup if fcntl system calls fail. The tests similar, the differences being in the UnixStream and UnixDatagram APIs.

@kleimkuhler

This comment has been minimized.

Copy link
Contributor

kleimkuhler commented Nov 11, 2019

Merging and closing this PR so that the issue is at least resolved on Unix. I am following up with a PR that will resolve the issue on all platforms.

@kleimkuhler kleimkuhler merged commit 4ab3c65 into tokio-rs:master Nov 11, 2019
22 checks passed
22 checks passed
FreeBSD 11.2 amd64 Task Summary
Details
tokio-rs.mio Build #20191110.7 succeeded
Details
tokio-rs.mio (Check rustfmt) Check rustfmt succeeded
Details
tokio-rs.mio (Clippy) Clippy succeeded
Details
tokio-rs.mio (Cross Android) Cross Android succeeded
Details
tokio-rs.mio (Cross Android_32) Cross Android_32 succeeded
Details
tokio-rs.mio (Cross Android_ARM64) Cross Android_ARM64 succeeded
Details
tokio-rs.mio (Cross NetBSD) Cross NetBSD succeeded
Details
tokio-rs.mio (Cross Solaris) Cross Solaris succeeded
Details
tokio-rs.mio (Cross iOS_32) Cross iOS_32 succeeded
Details
tokio-rs.mio (Cross iOS_64) Cross iOS_64 succeeded
Details
tokio-rs.mio (Cross iOS_ARM) Cross iOS_ARM succeeded
Details
tokio-rs.mio (Min Rust Linux) Min Rust Linux succeeded
Details
tokio-rs.mio (Min Rust MacOS) Min Rust MacOS succeeded
Details
tokio-rs.mio (Min Rust Windows) Min Rust Windows succeeded
Details
tokio-rs.mio (Minimal versions Linux) Minimal versions Linux succeeded
Details
tokio-rs.mio (Minimal versions Windows) Minimal versions Windows succeeded
Details
tokio-rs.mio (Nightly Linux) Nightly Linux succeeded
Details
tokio-rs.mio (Test --release Linux) Test --release Linux succeeded
Details
tokio-rs.mio (Test Linux) Test Linux succeeded
Details
tokio-rs.mio (Test MacOS) Test MacOS succeeded
Details
tokio-rs.mio (Test Windows) Test Windows succeeded
Details
kleimkuhler added a commit that referenced this pull request Nov 12, 2019
#1141 Addressed an issue with `UnixStream::pair` and `UnixDatagram::pair`
where the file descriptor array passed in to `pair_descriptors` was copied in
instead of passed by reference. Even if `pair_descriptors` succeded, the file
descriptor array used to create the socket pairs was the original array
initialized to `[-1; 2]`.

The issue still remains for target platforms where additional `fcntl` sys
calls must be made. The sockets are created before the call to
`pair_descriptors`, but a successful return does not actually affect the
sockets.

Ultimately, both of the `pair` methods and `pair_descriptors` function were
incorrect. I feel `pair_descriptors` role in trying to reduce code duplication
while being generic over `UnixStream` and `UnixDatagram` adds some uncessary
complexity without actually resulting in less code.

### Solution

The `pair` methods are now separate and no longer dispatch to a generic
function. The sockets are created after the required sys call. The `fcntl`
calls are made after socket creation to allow for cleanup.

Tests have been added for these specific methods.

A final PR separate from this will expand all UDS tests so that it is more
heavily tested.

Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.