Fix sendmsg on macOS when passing a zero entry cmsgs array. #623

kinetiknz · 2017-06-20T03:33:29Z

On macOS, trying to call sendmsg with a zero entry cmsgs buffer, e.g.:

sendmsg(fd, [IoVec::from_slice(buf)], &[], MsgFlags::empty(), None)

...fails with EINVAL. This occurs due to the pointer value for zero capacity Vecs being 0x1 rather than 0x0, to distinguish allocated-but-zero-size from nullptr. The kernel validates both the msghdr.msg_control and msghdr.msg_controllen fields and rejects msghdr.msg_control == 0x1 as invalid.

This doesn't show up on Linux because the kernel validates msghdr.msg_controllen first and ignores the value of msghdr.msg_control if the length was 0.

Susurrus · 2017-06-20T04:01:05Z

src/sys/socket/mod.rs

@@ -279,12 +277,18 @@ pub fn sendmsg<'a>(fd: RawFd, iov: &[IoVec<&'a [u8]>], cmsgs: &[ControlMessage<'
        None => (0 as *const _, 0),
    };

+    let cmsg_ptr = if capacity > 0 {


Is this not supposed to be len?

I think, if anything, cmsg_buffer should be allocated with capacity rather than len.

I'm not certain on this API, so I just wanted to confirm that len was correct.

Susurrus · 2017-06-21T04:50:10Z

Since you have a simple test case, can you add it as a test to the test suite? That way this will never regress after this fix.

Susurrus · 2017-06-21T04:50:37Z

Also we need an entry in CHANGELOG.md under the Fixed category for this.

kinetiknz · 2017-06-29T21:50:49Z

Thanks for the feedback. I've pushed an updated version.

Since you have a simple test case, can you add it as a test to the test suite? That way this will never regress after this fix.

Done.

Also we need an entry in CHANGELOG.md under the Fixed category for this.

Done.

Susurrus · 2017-06-30T00:15:47Z

This looks pretty good to me, but right now we're blocking on a fix for #634.

Susurrus · 2017-06-30T03:48:48Z

@kinetiknz I actually wanted to request that you add a summary of what's going on in your test, either inline or at the top. I don't understand this subsystem, so being able to roughly understand it without having to dig through the low-level code would be appreciated.

kinetiknz · 2017-07-02T04:12:14Z

@kinetiknz I actually wanted to request that you add a summary of what's going on in your test, either inline or at the top. I don't understand this subsystem, so being able to roughly understand it without having to dig through the low-level code would be appreciated.

I added a bit of a description to the top of the test. Let me know if that makes sense.

I've found another couple of issues in this area of the code while updating this PR with your feedback. I'll send another PR for those soon.

Susurrus · 2017-07-04T18:53:55Z

Can you rebase this? We've fixed the mount tests and enabled some more platforms, but I think this is RTM otherwise.

kinetiknz · 2017-07-05T00:36:13Z

Thanks for the update. Rebased patch pushed.

Susurrus · 2017-07-05T15:09:10Z

test/sys/test_socket.rs

+        for _ in msg.cmsgs() {
+            panic!("unexpected cmsg");
+        }
+        assert_eq!(msg.flags & (MSG_TRUNC | MSG_CTRUNC), MsgFlags::empty());


This would be more clear if it was written as:

assert!(!msg.flags.intersects(MSG_TRUNC | MSG_CTRUNC));

Fixed there and in test_scm_rights, where it was copied from.

Susurrus · 2017-07-06T02:57:53Z

This LGTM, @asomers wanna take a look over this as well and merge it if so?

Susurrus · 2017-07-09T16:52:58Z

bors r+

bors · 2017-07-09T16:53:56Z

Merge conflict

asomers · 2017-07-09T17:44:05Z

@kinetiknz please rebase your PR.

…cks.

kinetiknz · 2017-07-10T20:22:38Z

@kinetiknz please rebase your PR.

Done. I assume I don't have access to get bors to retry the merge.

Susurrus · 2017-07-10T20:31:07Z

I don't think you could even if you specified me as a reviewer. Don't worry, though, I pay close attention to our massive backlog of PRs!

bors r+

623: Fix sendmsg on macOS when passing a zero entry cmsgs array. r=Susurrus On macOS, trying to call `sendmsg` with a zero entry cmsgs buffer, e.g.: sendmsg(fd, [IoVec::from_slice(buf)], &[], MsgFlags::empty(), None) ...fails with `EINVAL`. This occurs due to the pointer value for zero capacity `Vec`s being 0x1 rather than 0x0, to distinguish allocated-but-zero-size from nullptr. The [kernel validates](https://github.com/opensource-apple/xnu/blob/dc0628e187c3148723505cf1f1d35bb948d3195b/bsd/kern/uipc_syscalls.c#L1304) both the `msghdr.msg_control` and `msghdr.msg_controllen` fields and rejects `msghdr.msg_control == 0x1` as invalid. This doesn't show up on Linux because the [kernel validates](https://github.com/torvalds/linux/blob/9705596d08ac87c18aee32cc97f2783b7d14624e/net/core/scm.c#L139) `msghdr.msg_controllen` first and ignores the value of `msghdr.msg_control` if the length was 0.

bors · 2017-07-11T00:58:52Z

Timed out

Susurrus · 2017-07-11T01:51:44Z

There's just a big backlog of PRs right now which is why bors timed out. Given that builds currently take almost two hours, I'm tempted to suggest we should double our timeout for bors again. @asomers any opinion on this?

bors r+

623: Fix sendmsg on macOS when passing a zero entry cmsgs array. r=Susurrus On macOS, trying to call `sendmsg` with a zero entry cmsgs buffer, e.g.: sendmsg(fd, [IoVec::from_slice(buf)], &[], MsgFlags::empty(), None) ...fails with `EINVAL`. This occurs due to the pointer value for zero capacity `Vec`s being 0x1 rather than 0x0, to distinguish allocated-but-zero-size from nullptr. The [kernel validates](https://github.com/opensource-apple/xnu/blob/dc0628e187c3148723505cf1f1d35bb948d3195b/bsd/kern/uipc_syscalls.c#L1304) both the `msghdr.msg_control` and `msghdr.msg_controllen` fields and rejects `msghdr.msg_control == 0x1` as invalid. This doesn't show up on Linux because the [kernel validates](https://github.com/torvalds/linux/blob/9705596d08ac87c18aee32cc97f2783b7d14624e/net/core/scm.c#L139) `msghdr.msg_controllen` first and ignores the value of `msghdr.msg_control` if the length was 0.

asomers · 2017-07-11T02:50:33Z

@Susurrus I think that's reasonable. I'll open a new issue for it.

bors · 2017-07-11T03:08:55Z

Build succeeded

Susurrus reviewed Jun 20, 2017

View reviewed changes

kinetiknz force-pushed the sendmsg_cmsg branch from 400a77a to 242a6a0 Compare June 29, 2017 21:17

kinetiknz force-pushed the sendmsg_cmsg branch from 242a6a0 to 981bbdb Compare July 2, 2017 04:08

kinetiknz force-pushed the sendmsg_cmsg branch from 981bbdb to a71e6c8 Compare July 5, 2017 00:35

Susurrus reviewed Jul 5, 2017

View reviewed changes

kinetiknz force-pushed the sendmsg_cmsg branch from a71e6c8 to 8ddc693 Compare July 5, 2017 23:24

kinetiknz mentioned this pull request Jul 5, 2017

Fix struct msghdr types on non-Linux platforms. #648

Merged

kinetiknz added 2 commits July 10, 2017 13:03

Simplify Vec allocation in sendmsg after 077d979 removed alignment ha…

2e00adf

…cks.

Fix sendmsg on macOS when passing a zero entry cmsgs array.

64815c6

kinetiknz force-pushed the sendmsg_cmsg branch from 8ddc693 to 64815c6 Compare July 10, 2017 01:04

bors bot merged commit 64815c6 into nix-rust:master Jul 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sendmsg on macOS when passing a zero entry cmsgs array. #623

Fix sendmsg on macOS when passing a zero entry cmsgs array. #623

kinetiknz commented Jun 20, 2017 •

edited

Loading

Susurrus Jun 20, 2017

kinetiknz Jun 20, 2017 •

edited

Loading

Susurrus Jun 21, 2017

Susurrus commented Jun 21, 2017

Susurrus commented Jun 21, 2017

kinetiknz commented Jun 29, 2017

Susurrus commented Jun 30, 2017

Susurrus commented Jun 30, 2017

kinetiknz commented Jul 2, 2017

Susurrus commented Jul 4, 2017

kinetiknz commented Jul 5, 2017

Susurrus Jul 5, 2017

kinetiknz Jul 5, 2017

Susurrus commented Jul 6, 2017

Susurrus commented Jul 9, 2017

bors bot commented Jul 9, 2017

asomers commented Jul 9, 2017

kinetiknz commented Jul 10, 2017

Susurrus commented Jul 10, 2017

bors bot commented Jul 11, 2017

Susurrus commented Jul 11, 2017

asomers commented Jul 11, 2017

bors bot commented Jul 11, 2017

Fix sendmsg on macOS when passing a zero entry cmsgs array. #623

Fix sendmsg on macOS when passing a zero entry cmsgs array. #623

Conversation

kinetiknz commented Jun 20, 2017 • edited Loading

Susurrus Jun 20, 2017

Choose a reason for hiding this comment

kinetiknz Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

Susurrus Jun 21, 2017

Choose a reason for hiding this comment

Susurrus commented Jun 21, 2017

Susurrus commented Jun 21, 2017

kinetiknz commented Jun 29, 2017

Susurrus commented Jun 30, 2017

Susurrus commented Jun 30, 2017

kinetiknz commented Jul 2, 2017

Susurrus commented Jul 4, 2017

kinetiknz commented Jul 5, 2017

Susurrus Jul 5, 2017

Choose a reason for hiding this comment

kinetiknz Jul 5, 2017

Choose a reason for hiding this comment

Susurrus commented Jul 6, 2017

Susurrus commented Jul 9, 2017

bors bot commented Jul 9, 2017

Merge conflict

asomers commented Jul 9, 2017

kinetiknz commented Jul 10, 2017

Susurrus commented Jul 10, 2017

bors bot commented Jul 11, 2017

Timed out

Susurrus commented Jul 11, 2017

asomers commented Jul 11, 2017

bors bot commented Jul 11, 2017

Build succeeded

kinetiknz commented Jun 20, 2017 •

edited

Loading

kinetiknz Jun 20, 2017 •

edited

Loading