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 control message *decoding* and add support for ScmCredentials #923

Merged
merged 1 commit into from
Aug 6, 2018
Merged

Fix control message *decoding* and add support for ScmCredentials #923

merged 1 commit into from
Aug 6, 2018

Conversation

jonas-schievink
Copy link
Contributor

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.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good, especially the tests. Any idea why Travis didn't build this PR?

// (although it claims the kernel header checks this), but such
// a structure is clearly invalid, either way.
// Safe if: `self.buf` is `cmsghdr`-aligned.
let cmsg: &'a cmsghdr = unsafe { &*(self.buf[..mem::size_of::<cmsghdr>()].as_ptr() as *const cmsghdr) };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you guarantee that self.buf is correctly aligned?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The caller does. In this case its done by the CmsgSpace, which ensures alignment using a properly aligned member.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's not the caller but whoever creates the iterator. Here, the buffer comes from recvmsg and is a CmsgSpace which guarantees alignment.

cmsg_data.as_ptr() as *const _,
len))))
}
let cmsg_data = &self.buf[cmsg_align(mem::size_of::<cmsghdr>())..cmsg_len];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up above you requird self.buf to be aligned. Here, you seem to be handling the case where it isn't. Or am I misunderstanding?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cmsg_align just rounds up the size of cmsghdr to the right alignment. This is needed to include the right padding to reach the payload.

@@ -567,57 +569,95 @@ impl<'a> ControlMessage<'a> {
}
}

/// Returns the value to put into the `cmsg_type` field of the header.
fn type_(&self) -> libc::c_int {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like trailing underscores. How about naming this method cmsg_type instead?


match *self {
ControlMessage::ScmRights(fds) => {
let padlen = cmsg_align(mem::size_of_val(&cmsg)) -
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like padlen and buf are identical for all cases. Can you move their definitions outside of the match block?

@jonas-schievink
Copy link
Contributor Author

jonas-schievink commented Jul 6, 2018

Any idea why Travis didn't build this PR?

I added [ci skip] to the last commit as it was just updating the changelog. This causes Travis to ignore the commit which saves CI time.

(the previous commit was built by Travis and passed)

@jonas-schievink
Copy link
Contributor Author

jonas-schievink commented Jul 9, 2018

Uh, now the old test fails? That's odd.

EDIT: Uh okay the tests are really flaky - I suspect there's still some issues left

@jonas-schievink
Copy link
Contributor Author

jonas-schievink commented Jul 10, 2018

I think the issues are in qemu. I haven't managed to get any socket tests failing on my Raspberry Pi 1 (armv6) or 2 (armv7). Additionally, I wrote a test program in C (https://gist.github.com/jonas-schievink/cb6e6584a055539d2113f22d91068e2d) that works fine on bare metal but also shows this behavior under qemu. The control message encoding for nix and the test program is identical, and it's the kernel (or qemu) that strips out the second message, not our decoder doing weird stuff.

I'm going to build a smaller reproducer and report the bug to qemu, then #[ignore] the tests here.

@jonas-schievink
Copy link
Contributor Author

@asomers Alright, after a long journey this might be finally done (except the commit message for
386cd04 is wrong - but I'll fix that when squashing later). Care to take another look?

@@ -301,14 +301,36 @@ fn test_scm_credentials() {
/// Ensure that we can send `SCM_CREDENTIALS` and `SCM_RIGHTS` with a single
/// `sendmsg` call.
#[cfg(any(target_os = "android", target_os = "linux"))]
// qemu's handling of multiple cmsgs is bugged, ignore tests on non-x86-64
#[cfg_attr(not(target_arch = "x86_64"), ignore)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about i386? That doesn't use qemu on FreeBSD, and I don't think it does on Linux either.

@@ -301,14 +301,36 @@ fn test_scm_credentials() {
/// Ensure that we can send `SCM_CREDENTIALS` and `SCM_RIGHTS` with a single
/// `sendmsg` call.
#[cfg(any(target_os = "android", target_os = "linux"))]
// qemu's handling of multiple cmsgs is bugged, ignore tests on non-x86-64
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add an upstream bug link? Your C test case should make it easy to report.

// (although it claims the kernel header checks this), but such
// a structure is clearly invalid, either way.
// Safe if: `self.buf` is `cmsghdr`-aligned.
let cmsg: &'a cmsghdr = unsafe { &*(self.buf[..mem::size_of::<cmsghdr>()].as_ptr() as *const cmsghdr) };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap to 80 columns, please.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, now it LGTM. All it needs is a squash. @Susurrus any comments?

@asomers
Copy link
Member

asomers commented Jul 26, 2018

@jonas-schievink please squash this; then I'll merge it.

@Susurrus
Copy link
Contributor

Susurrus commented Aug 6, 2018

@asomers This all look good? You asked for it to be squashed, which is has been. We could split this up into two commits with the other one added the as_raw implementations for Pid and friends, but it's a minor nit, so I'm fine not worrying about it.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I'm cool.

@asomers
Copy link
Member

asomers commented Aug 6, 2018

bors r+

bors bot added a commit that referenced this pull request Aug 6, 2018
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
Copy link
Contributor

bors bot commented Aug 6, 2018

Timed out

@asomers
Copy link
Member

asomers commented Aug 6, 2018

bors retry

bors bot added a commit that referenced this pull request Aug 6, 2018
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
Copy link
Contributor

bors bot commented Aug 6, 2018

@bors bors bot merged commit 9f0af44 into nix-rust:master Aug 6, 2018
@jonas-schievink jonas-schievink deleted the scm-credentials branch October 4, 2018 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants