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 passing multiple file descriptors / control messages via sendmsg #918

Merged
merged 1 commit into from Jul 5, 2018

Conversation

@jonas-schievink
Copy link
Contributor

@jonas-schievink jonas-schievink commented Jul 1, 2018

Fixes #464
Closes #874 because it's incorporated here
Closes #756 because it adds the test from that issue (with fixes)

@jonas-schievink
Copy link
Contributor Author

@jonas-schievink jonas-schievink commented Jul 1, 2018

Current CI failures:

  • Only 1 fd was successfully received, the other got lost
    • FreeBSD x86_64
    • powerpc64-unknown-linux-gnu
    • powerpc64le-unknown-linux-gnu
    • mips64-unknown-linux-gnuabi64
    • mips64el-unknown-linux-gnuabi64
    • aarch64-unknown-linux-gnu
  • EINVAL
    • i686-apple-darwin
    • x86_64-apple-darwin
@jonas-schievink
Copy link
Contributor Author

@jonas-schievink jonas-schievink commented Jul 2, 2018

Every single CI job that failed passes another test that also passes 2 fds, but only using a single control message. Seems like you're not supposed to use more than 1 SCM_RIGHTS message.

I've deleted the broken test and added a note.

Copy link
Member

@asomers asomers left a comment

Looks good so far! Does Nix also have a bug with sending two dissimilar control messages in a single sendmsg call, and if so does this change fix it?

Cargo.toml Outdated
@@ -50,3 +50,8 @@ harness = false
[[test]]
name = "test-ptymaster-drop"
path = "test/test_ptymaster_drop.rs"

[[test]]

This comment has been minimized.

@asomers

asomers Jul 2, 2018
Member

Why did you need to use a separate test program? If this program is meant to be interactive, and provides no additional test coverage over the other unit test that you wrote, then it should go in the examples directory, not the tests directory.

This comment has been minimized.

@jonas-schievink

jonas-schievink Jul 2, 2018
Author Contributor

Turns out the test I added in a recent commit essentially does the same thing (pass 2 fds using 1 cmsg), so I'll remove this one.

Adding examples to nix is a very good idea, but I guess it should be done as a more coordinated approach rather than in this PR.

@@ -301,6 +301,18 @@ unsafe fn copy_bytes<'a, 'b, T: ?Sized>(src: &T, dst: &'a mut &'b mut [u8]) {
mem::swap(dst, &mut remainder);
}

/// Pad byte slice dst with `len` zeroes, and update the slice to point to
/// the remainder.
fn pad_bytes(len: usize, dst: &mut &mut [u8]) {

This comment has been minimized.

@asomers

asomers Jul 2, 2018
Member

Return arguments is very C-ish. Could you return a reference to the remainder instead? Like this?

fn pad_bytes(len: usize, sl: &'a mut [u8]) -> &'a mut [u8];
fn pad_bytes(len: usize, dst: &mut &mut [u8]) {
let mut tmpbuf = &mut [][..];
mem::swap(&mut tmpbuf, dst);
let (padding, mut remainder) = tmpbuf.split_at_mut(len);

This comment has been minimized.

@asomers

asomers Jul 2, 2018
Member

Seems like you're padding the left half of the slice, which is not what I though from the function description. Could you please make that more clear?

@@ -434,6 +446,10 @@ pub enum ControlMessage<'a> {
///
/// See the description in the "Ancillary messages" section of the
/// [unix(7) man page](http://man7.org/linux/man-pages/man7/unix.7.html).
///
/// Do not use more than one `ScmRights` message for a single `sendmsg` call - Depending on the

This comment has been minimized.

@asomers

asomers Jul 2, 2018
Member

This is a pretty strong statement. If we're going to lecture our users about portability best-practices, then we should at least describe it as a recommendation, not a requirement.

mem::swap(&mut tmpbuf, buf);
let (_padding, mut remainder) = tmpbuf.split_at_mut(padlen);
mem::swap(buf, &mut remainder);
pad_bytes(padlen, buf);

This comment has been minimized.

@asomers

asomers Jul 2, 2018
Member

It looks like the caller doesn't care that encode_into returns the new buffer reference by argument. So we can simplify this function by making buf a &mut [u8], then simplifying pad_bytes into something more like:

for i in 0..padlen {
  buf[i] = 0;
}
buf = buf[padlen..]
}
},
Err(_) => {
panic!();

This comment has been minimized.

@asomers

asomers Jul 2, 2018
Member

These two panic messages are indistinguishable since you don't provide descriptions for them. And if the error case is a simple panic, you may as well just use unwrap instead of if let.

let mut buf = [0u8; 8];
let iovec = [IoVec::from_mut_slice(&mut buf)];
let mut space = CmsgSpace::<[RawFd; 2]>::new();
let (paneid, fds) = match recvmsg(receive.as_raw_fd(), &iovec, Some(&mut space), MsgFlags::empty()) {

This comment has been minimized.

@asomers

asomers Jul 2, 2018
Member

What is a "paneid" ?


let slice = [1u8, 2u8, 3u8, 4u8, 5u8, 6u8, 7u8, 8u8];
let iov = [IoVec::from_slice(&slice)];
let arr = [0, 1]; // Pass stdin and stdout

This comment has been minimized.

@asomers

asomers Jul 2, 2018
Member

This should be libc::STDIN_FILENO and libc::STDOUT_FILENO.


let slice = [1u8, 2u8, 3u8, 4u8, 5u8, 6u8, 7u8, 8u8];
let iov = [IoVec::from_slice(&slice)];
let fds = [0, 1]; // pass stdin and stdout

This comment has been minimized.

@asomers

asomers Jul 2, 2018
Member

should be libc::STDIN_FILENO and libc::STDOUT_FILENO.

@jonas-schievink jonas-schievink force-pushed the jonas-schievink:sendmsg-fix branch from f6ebe30 to 3ab0bf7 Jul 2, 2018
@jonas-schievink
Copy link
Contributor Author

@jonas-schievink jonas-schievink commented Jul 2, 2018

Does Nix also have a bug with sending two dissimilar control messages in a single sendmsg call, and if so does this change fix it?

This should fix that, but I haven't added a test since there's no convenient ControlMessage that can be used to test it. I might add support for SCM_CREDENTIALS in a later PR and use that along with SCM_RIGHTS to test this.

@asomers
Copy link
Member

@asomers asomers commented Jul 2, 2018

Why not use ScmRights with ScmTimestamp?

@jonas-schievink
Copy link
Contributor Author

@jonas-schievink jonas-schievink commented Jul 2, 2018

Attempting to send a SCM_TIMESTAMP results in EINVAL on my system (x64 Linux). Even when it's just a single SCM_TIMESTAMP control message, and even from C++, so it's not a bug in nix, you're just not supposed to send those by yourself (the kernel does).

@jonas-schievink jonas-schievink force-pushed the jonas-schievink:sendmsg-fix branch from 3ab0bf7 to 6625c74 Jul 4, 2018
@jonas-schievink
Copy link
Contributor Author

@jonas-schievink jonas-schievink commented Jul 4, 2018

@asomers Anything left to do here?

Copy link
Member

@asomers asomers left a comment

Getting close now. I only have cosmetic comments at this point. Also, we'll need a squash before we're done.

///
/// Panics when `dst` is too small for `src` (panics if `mem::size_of_val(src) >= dst.len()`).
///
/// Unsafe because it exposes all bytes in `src`, which may be UB if some of them are uninitialized

This comment has been minimized.

@asomers

asomers Jul 4, 2018
Member

It's not actually unsafe to read an uninitialized slice, because slices are always supposed to be initialized. The unsafe part is creating the uninitialized slice in the first place. However, it is unsafe to call copy_nonoverlapping without verifying that the slices don't overlap, without verifying that T implements Copy, and without droping the contents of dst. Please update the comment.

This comment has been minimized.

@jonas-schievink

jonas-schievink Jul 4, 2018
Author Contributor

The problem is that this is reading the bytes of an arbitrary T, including its padding. If reading the bytes from a T is safe, then so is this function. It doesn't create any uninitialized memory or slices.

AFAIK, LLVM considers reads of padding bytes undef, which would be unsafe, but I don't know if rustc makes it safe in some way (eg. by providing its own padding members that read as junk instead of undef).

This comment has been minimized.

@asomers

asomers Jul 4, 2018
Member

So the unsafe part is that it's effectively transmuting the T. Transmutation is unsafe regardless of padding issues.

This comment has been minimized.

@jonas-schievink

jonas-schievink Jul 4, 2018
Author Contributor

I don't think transmutation to just bytes is unsafe except when padding is involved (as there's no way to read data that isn't valid as a u8), but I've clarified what's going on here

/// Fills `dst` with `len` zero bytes and returns the remainder of the slice.
///
/// Panics when `len >= dst.len()`.
fn pad_bytes(len: usize, dst: &mut [u8]) -> &mut [u8] {

This comment has been minimized.

@asomers

asomers Jul 4, 2018
Member

Much nicer than the previous version. I like.


let mut cmsgs = msg.cmsgs();
match cmsgs.next() {
Some(ControlMessage::ScmRights(fds)) => assert_eq!(fds.len(), 2, "unexpected fd count (expected 2 fds)"),

This comment has been minimized.

@asomers

asomers Jul 4, 2018
Member

80 chars per line, please.

@jonas-schievink jonas-schievink force-pushed the jonas-schievink:sendmsg-fix branch from 6625c74 to 02f463d Jul 4, 2018
@jonas-schievink jonas-schievink force-pushed the jonas-schievink:sendmsg-fix branch from dbb2632 to 5d6dc26 Jul 4, 2018
@asomers
asomers approved these changes Jul 5, 2018
@asomers
Copy link
Member

@asomers asomers commented Jul 5, 2018

bors r+

bors bot added a commit that referenced this pull request Jul 5, 2018
Merge #918
918: Fix passing multiple file descriptors / control messages via sendmsg r=asomers a=jonas-schievink

Fixes #464
Closes #874 because it's incorporated here
Closes #756 because it adds the test from that issue (with fixes)

Co-authored-by: alecmocatta <alec@mocatta.net>
@bors
Copy link
Contributor

@bors bors bot commented Jul 5, 2018

@jonas-schievink
Copy link
Contributor Author

@jonas-schievink jonas-schievink commented Jul 5, 2018

     Running /target/i686-unknown-linux-gnu/release/deps/test_aio_drop-88766c7ced1811bd
running 1 test
test test_drop ... error: An unknown error occurred

That doesn't look good - but also spurious

@asomers
Copy link
Member

@asomers asomers commented Jul 5, 2018

bors retry

bors bot added a commit that referenced this pull request Jul 5, 2018
Merge #918
918: Fix passing multiple file descriptors / control messages via sendmsg r=asomers a=jonas-schievink

Fixes #464
Closes #874 because it's incorporated here
Closes #756 because it adds the test from that issue (with fixes)

Co-authored-by: alecmocatta <alec@mocatta.net>
@bors bors bot merged commit 5d6dc26 into nix-rust:master Jul 5, 2018
4 checks passed
4 checks passed
bors Build succeeded
Details
buildbot/nix-rust/nix amd64_fbsd11 Build done.
Details
buildbot/nix-rust/nix i386_fbsd11 Build done.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jonas-schievink jonas-schievink deleted the jonas-schievink:sendmsg-fix branch Jul 5, 2018
bors bot added a commit that referenced this pull request Aug 6, 2018
Merge #923
923: Fix control message *decoding* and add support for `ScmCredentials` r=asomers a=jonas-schievink

While #918 fixed the *encoding*, the *decoding* done by the `CmsgIterator` still remained broken when multiple messages are received. However, since nix didn't support any control message that could reliably be sent twice in one `sendmsg` call, this couldn't be tested properly.

This PR addresses this by adding `SCM_CREDENTIALS` support and testing all of this by passing both an `SCM_CREDENTIALS` and a `SCM_RIGHTS` message at the same time.

I've also verified that the resulting encoding is the same as for roughly equivalent C code.

Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>
bors bot added a commit that referenced this pull request Aug 6, 2018
Merge #923
923: Fix control message *decoding* and add support for `ScmCredentials` r=asomers a=jonas-schievink

While #918 fixed the *encoding*, the *decoding* done by the `CmsgIterator` still remained broken when multiple messages are received. However, since nix didn't support any control message that could reliably be sent twice in one `sendmsg` call, this couldn't be tested properly.

This PR addresses this by adding `SCM_CREDENTIALS` support and testing all of this by passing both an `SCM_CREDENTIALS` and a `SCM_RIGHTS` message at the same time.

I've also verified that the resulting encoding is the same as for roughly equivalent C code.

Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants