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

Major cmsg cleanup #1020

Merged
merged 6 commits into from
Feb 14, 2019
Merged

Major cmsg cleanup #1020

merged 6 commits into from
Feb 14, 2019

Conversation

asomers
Copy link
Member

@asomers asomers commented Jan 30, 2019

This PR fixes multiple bugs in the cmsg code.
Fixes #994
Fixes #999
Fixes #1013

@asomers asomers requested a review from Susurrus January 30, 2019 20:55
@asomers
Copy link
Member Author

asomers commented Jan 30, 2019

cc @pusateri @Ralith @goffrie

/// [`unix(7)`](http://man7.org/linux/man-pages/man7/unix.7.html) man page.
// FIXME: When `#[repr(transparent)]` is stable, use it on `UnixCredentials`
// and put that in here instead of a raw ucred.
pub enum ControlMessageOwned {
Copy link

Choose a reason for hiding this comment

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

Per my previous comment, I think this is the wrong way to go, as it incurs avoidable heap allocation and copying.

Copy link
Member Author

Choose a reason for hiding this comment

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

I chose to go this path because it's closer to backwards-compatible. I tried the 100% backwards-compatible method first, but the code got very ugly very quick, and it involved even more copying.

Copy link

Choose a reason for hiding this comment

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

It seems to be that you've settled on the worst of both worlds: both a breaking change and inefficient.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you suggest something more efficient that causes no more pain for existing users? This solution, while not exactly backwards compatible, only requires a simple search and replace.

Copy link

@Ralith Ralith Jan 30, 2019

Choose a reason for hiding this comment

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

I don't think the current specific level of breakage makes sense as a line in the sand that cannot be crossed by a single step. That said, you could compromise by only using the flyweight pattern where heap allocation is otherwise necessary, i.e. in ScmRights, though this would make "Owned" no longer an accurate descriptor.

Copy link
Member Author

Choose a reason for hiding this comment

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

But what's to say that ScmRights's payload is aligned on all platforms that Nix will ever support? I don't think that's guaranteed, especially if a user ever passes an unaligned msg_control field to recvmsg.

Copy link

Choose a reason for hiding this comment

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

That's why I said "flyweight pattern" and not "a slice", yes. Refer to the comment I linked for an example implementation.

/// let _ = cmsg_space!(RawFd, TimeVal);
/// ```
// Unfortunately, CMSG_SPACE isn't a const_fn, or else we could return a
// stack-allocated array.
Copy link

Choose a reason for hiding this comment

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

Could this be corrected? This seems like exactly the special case that const fns can already handle gracefully, and is the intended use of the C macros.

Copy link
Member Author

@asomers asomers Jan 30, 2019

Choose a reason for hiding this comment

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

The libc people are resistant to that kind of change. rust-lang/libc#1213 (actually that's a separate issue). The cmsg functions can't be made const because that requires rustc 1.31.0. libc guarantees support for 1.13.0.

/// // Create a buffer big enough for a `ControlMessageOwned::ScmRights` message
/// // and a `ControlMessageOwned::ScmTimestamp` message
/// let _ = cmsg_space!(RawFd, TimeVal);
/// ```
Copy link

Choose a reason for hiding this comment

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

I love this interface!

// The number of bytes received.
pub bytes: usize,
cmsg_buffer: &'a [u8],
cmsghdr: *const cmsghdr,
Copy link

Choose a reason for hiding this comment

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

Why not a real reference?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because there are only three things we're going to do with that member. Pass it to CMSG_NXTHDR and CMSG_DATA, which expect pointere, and do some raw pointer arithmetic in decode_from. So no sense converting it to a reference when we'd just have to convert it back again.

Copy link

Choose a reason for hiding this comment

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

References can convert to pointers implicitly, and the PhantomData would be rendered unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

As a matter of fact, this turned out to be pretty easy. I'll do it.

@Ralith
Copy link

Ralith commented Jan 30, 2019

Thanks for your work on the libc macros, by the way!

@asomers
Copy link
Member Author

asomers commented Jan 31, 2019

The QEMU failures are due to a bug in QEMU itself. For once I'm sure of it. The emulated recvmsg is writing beyond the end of the supplied msg_control buffer. I think it's probably due to the linked qemu bug, which is already fixed. I'll disable the test. Once japaric/cross updates its image to use qemu 2.12.0, then we can reenable it.
https://bugs.launchpad.net/qemu/+bug/1701808
japaric/rust-cross#49

@asomers
Copy link
Member Author

asomers commented Jan 31, 2019

Sigh. Travis got confused somehow and thinks I'm missing a merge commit. I guess I may as well rebase.

@asomers asomers force-pushed the cmsg branch 2 times, most recently from 55b67b0 to 55e28c7 Compare January 31, 2019 23:28
@asomers
Copy link
Member Author

asomers commented Feb 12, 2019

I'm going to go ahead and merge this. @Ralith if you're still concerned, then please submit a separate PR to reduce data copies.

@asomers asomers force-pushed the cmsg branch 2 times, most recently from 2125d63 to f11fe7f Compare February 12, 2019 19:58
@asomers
Copy link
Member Author

asomers commented Feb 12, 2019

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 12, 2019

Merge conflict (retrying...)

@bors
Copy link
Contributor

bors bot commented Feb 12, 2019

Merge conflict

Our hand-rolled logic had subtle alignment bugs that caused
test_scm_rights to fail on OpenBSD (and probably could cause problems on
other platforms too).  Using cmsg(3) is much cleaner, shorter, and more
portable.  No user-visible changes.
There were two problems:
1) It would always return Ok, even on error
2) It could panic if there was an error, because
   sockaddr_storage_to_addr would be called on uninitialized memory.
CmsgSpace had three problems:
1) It would oversize buffers that expect multiple control messages
2) It didn't use the libc CMSG_SPACE(3) macro, so it might actually
   undersize a buffer for a single control message.
3) It could do bad things on drop, if you instantiate it with a type
   that implements Drop (which none of the currently supported
   ControlMessage types do).

Fixes nix-rust#994
On some platforms the alignment of cmsg_data could be less than the
alignment of the messages that it contains.  That led to unaligned
reads
on those platforms.  This change fixes the issue by always copying the
message contents into aligned objects.  The change is not 100%
backwards
compatible when using recvmsg.  Users may have to replace code like
this:

```rust
if let ControlMessage::ScmRights(&fds) = cmsg {
```

with this:
```rust
if let ControlMessageOwned::ScmRights(fds) = cmsg {
```

Fixes nix-rust#999
@asomers
Copy link
Member Author

asomers commented Feb 14, 2019

bors r+

bors bot added a commit that referenced this pull request Feb 14, 2019
1020: Major cmsg cleanup r=asomers a=asomers

This PR fixes multiple bugs in the cmsg code.
Fixes #994 
Fixes #999 
Fixes #1013 

Co-authored-by: Alan Somers <[email protected]>
@Ralith
Copy link

Ralith commented Feb 14, 2019

@Ralith if you're still concerned, then please submit a separate PR to reduce data copies

I don't actually use Nix at present, so I'm just going to stick with my current implementation that doesn't have these issues.

@bors
Copy link
Contributor

bors bot commented Feb 14, 2019

Build succeeded

@bors bors bot merged commit b5c4c7a into nix-rust:master Feb 14, 2019
@asomers asomers deleted the cmsg branch August 19, 2019 22:37
asomers added a commit to asomers/nix that referenced this pull request Aug 19, 2019
It was leftover from internal churn during PR nix-rust#1020.
asomers added a commit to asomers/nix that referenced this pull request Aug 19, 2019
It was leftover from internal churn during PR nix-rust#1020.
asomers added a commit to asomers/nix that referenced this pull request Aug 29, 2019
It was leftover from internal churn during PR nix-rust#1020.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants