Allow receiving multiple FD over ControlMessage in unix sockets #473

roblabla · 2016-11-19T01:13:51Z

Fixes #464

fiveop · 2016-11-19T07:24:59Z

Thanks, looks good to me. Could you add an entry to the CHANGELOG.md and write a test based on your failing example in #464?

roblabla · 2016-11-19T14:47:44Z

There's still a problem with the second test (the one that sends multiple cmsgs). I believe this is due to a mistake in the way space/len of CmsgSpace is implemented (although I'm uncertain).

roblabla · 2016-11-19T15:11:29Z

Nevermind my previous comment. I think the problem comes from

nix/src/sys/socket/mod.rs

Line 241 in 80a34a5

let buf2 = &mut &mut buf[padlen..];

. This creates a new pointer, writes the FD in it, but the "old" buf pointer isn't incremented by the size of the FD.

This means that when writing the second control message, it will overwrite the data of the first control message. I am unsure of how to fix this though, the code is pretty hard to follow and I'm unsure of how it interacts with lifetimes and all.

fiveop

I commented on the easy problems. I'll read through socket/mod.rs on thursday and hope to be able to help with the problem afterwards.

fiveop · 2016-11-22T20:12:21Z

test/sys/test_socket.rs

+
+#[test]
+fn test_scm_rights_multiple_fd() {
+    use std::os::unix::net::UnixDatagram;


This import breaks ARM and 1.7.0 builds.

fiveop · 2016-12-03T19:43:50Z

I now have an idea what the problem is. copy_bytes used by encode_into called in sendmsg updates dst so that it points after the bytes just copied into. This works for the cmsghdr. For the second copy_bytes, however, another slice buf2 is used (which probably gets updated), thus buf (in encode_into) or cmsg_buffer (in sendmsg) is not updated. When two ControlMessages are encoded, this is a problem.

homu · 2016-12-16T16:48:57Z

☔ The latest upstream changes (presumably #478) made this pull request unmergeable. Please resolve the merge conflicts.

Fix ControlMessage::encode_into when encoding multiple messages copy_bytes updates dst so that it points after the bytes that were just copied into it. encode_into did not advance the buffer in the same way when encoding the data. See #473

kamalmarhubi · 2017-02-14T06:10:59Z

@fiveop did your #489 fix this issue? If so, should we close this PR?

fiveop · 2017-02-15T10:33:32Z

No. It was a step toward fixing it.

Susurrus · 2017-06-05T00:19:19Z

What's necessary to wrap this PR up and get everything merged?

roblabla · 2017-06-05T15:54:55Z

Well, I rebased the commits on the latest master, which makes this mergeable again. Worth noting that I ran the "Add some tests" commit locally (independently of the fix) to make sure the bugs were still present. Indeed they are.

After my fix, test_scm_rights_multiple_cmsg still fails though, and test_scm_rights_multiple_fd segfaults on armv7 and aarch64

Susurrus · 2017-06-06T16:59:06Z

@roblabla Thanks for updating this!

Could you add some more details to the "Add some tests" commit and also some comments to the tests summarizing basically the discussion here for some context.

The segfaults are likely QEMU bugs as those are run on that. You can just ignore those tests for those targets.

But why is the "fixed" test still failing? Is the test not correct or is your "fix" not actually the correct fix? You didn't offer any commentary on that here, would you like a second pair of eyes on this or do you know how to fix it?

roblabla · 2017-06-07T08:58:33Z

My main use when I made the PR was to fix my use-case, which is multiple FD. But while testing, I discovered there really were two bugs (depending on how you achieve multiple FD, with one cmsg containing multiple FD, or multiple cmsg containing each one FD).

I fixed one bug (multiple FD, one CMSG), but never quite managed to figure out the source of the other (multiple CMSG).

So the test is right (at least as far as I can see), but there is still a bug in nix somewhere.

I'll add some details to the commit as soon as I'm on a work machine.

lucab · 2017-08-25T09:34:37Z

Bump. Is there any way to make progress and eventually land this?

Susurrus · 2017-08-25T16:50:55Z

@lucab I think we're basically waiting to resolve the second bug, which has not been resolved if I'm following this correctly.. If you're willing to dive into this, your help would be greatly appreciated and we could get this merged. @roblabla Can you confirm this is the current state of things and maybe provide any more context that's missing if you aren't able to continue to push this forward?

roblabla · 2017-08-25T17:01:32Z

Well, this PR basically fixes what I needed fixed (I needed the ability to send multiple FD over one message, which is fixed), and I don't have the time to figure out what the other bug is.

IIRC, there's a problem in the way we encode multiple Control Messages which causes the kernel to only consider the first Control Message sent. The specifics of where the bug is, I have no idea. I mean, what needs to be done is take the failing test and figure out what's wrong with it.

In the meantime, maybe I could set the second test as ignored so we can land the first fix, and make an issue about the second bug so we can track further progress there ?

Susurrus · 2017-08-25T17:02:44Z

@roblabla Yes, can you cut out the second test into it's own issue (someone can then take it and make a PR to fix it). And then we can merge your single fix and test case for it.

Susurrus · 2017-08-25T17:03:16Z

This could also use a rebase and the CHANGELOG entry needs to be moved up into the current Unreleased section.

Susurrus · 2017-09-01T02:42:49Z

@roblabla Just wanted to ping you on this as I'd love to get it merged!

We currently don't make sure sending multiple FD over a single Control Message or sending multiple Control Message works. We add two tests, and mark them as ignored since the behavior is currently broken.

Fixes nix-rust#464

roblabla · 2017-09-01T10:59:15Z

Thanks for reminding me ! It had slipped from my mind ^^'.

Should I still remove the UnixDatagram stuff from the test ? Those types were added in 1.10, AFAIK we support minimum of 1.13 ?

Susurrus · 2017-09-01T13:26:37Z

I think the UnixDatagram stuff is the bug we haven't figure out yet, yes? I assume at this point it isn't a wrong value in a constant. So can you create a separate issue for that and copy/paste the test code there and give as much context as you can towards the error?

roblabla · 2017-09-01T15:10:14Z

I'm not sure we understood eachother ? I meant that I'm using std::net::unix::UnixDatagram in the tests, which fiveop told me would break ARM 1.7 builds way back when, so I was wondering if the comment was still valid - that is, if I should be using the nix functions to create a UnixDatagram (which I find less convenient).

#756 contains the details of what I know about the remaining bug.

I'm waiting on the travis results to ignore out the test on the platforms that use the broken qemu implementation :)

Susurrus · 2017-09-05T00:56:40Z

@roblabla We aim to support Rust 1.13, so if the tests pass on all supported platforms (which test the minimum supported Rust version), then we should be fine!

I saw #756, which does a good job detailing the other bug you found, so thanks for that.

And Travis finished but found a few failures, so those'll need to be addressed (I haven't looked at them).

roblabla · 2017-09-05T08:02:40Z

So, it looks like it segfaults basically every single qemu instance. Besides, I also have failures on IOS and MacOS, but the output is empty 😕.

Should I just #cfg[] the tests out ?

No output :

List of qemu-related failures (qemu segfaults) :

aarch64-unknown-linux-gnu
arm-unknown-linux-gnueabi
armv7-unknown-linux-gnueabihf
mips-unknown-linux-gnu
mips64-unknown-linux-gnuabi64
mips64el-unknown-linux-gnuabi64
mipsel-unknown-linux-gnu
powerpc64-unknown-linux-gnu
powerpc64le-unknown-linux-gnu

Susurrus · 2017-09-06T04:31:20Z

I guess so. Add a clear comment and use the travis feature with #cfg to ignore them.

Susurrus · 2017-09-06T04:32:01Z

We would ideally file a bug with upstreaam and include that in the comment about why we exclude it.

Susurrus · 2017-12-04T04:31:35Z

It's been about 3 months since there's been any movement on this, so I'm going to go ahead and close this.

roblabla mentioned this pull request Nov 19, 2016

Development log roblabla/i3-tmux-integration#1

Open

roblabla force-pushed the bugfix-multipleFdInCmsg branch from 93b6cfc to f2ffc05 Compare November 19, 2016 14:46

fiveop reviewed Nov 22, 2016

View reviewed changes

fiveop mentioned this pull request Dec 10, 2016

Fix ControlMessage::encode_into when encoding multiple messages #489

Merged

roblabla force-pushed the bugfix-multipleFdInCmsg branch 2 times, most recently from 63b9f00 to 18f705d Compare June 5, 2017 14:40

albel727 mentioned this pull request Jun 10, 2017

Split out low level interface to a separate dbus-sys crate diwic/dbus-rs#85

Closed

roblabla force-pushed the bugfix-multipleFdInCmsg branch from 18f705d to 664e4cb Compare August 11, 2017 23:24

roblabla mentioned this pull request Sep 1, 2017

Sending multiple ControlMessage in a single Unix Socket packet fails #756

Closed

roblabla and others added 2 commits September 1, 2017 12:53

Test sending multiple fd and multiple cmsg

11244ba

We currently don't make sure sending multiple FD over a single Control Message or sending multiple Control Message works. We add two tests, and mark them as ignored since the behavior is currently broken.

Allow receiving multiple FD over a ControlMessage

4debb45

Fixes nix-rust#464

roblabla force-pushed the bugfix-multipleFdInCmsg branch from 664e4cb to 4debb45 Compare September 1, 2017 10:56

Susurrus closed this Dec 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow receiving multiple FD over ControlMessage in unix sockets #473

Allow receiving multiple FD over ControlMessage in unix sockets #473

roblabla commented Nov 19, 2016

fiveop commented Nov 19, 2016

roblabla commented Nov 19, 2016

roblabla commented Nov 19, 2016

fiveop left a comment

fiveop Nov 22, 2016

fiveop commented Dec 3, 2016

homu commented Dec 16, 2016

kamalmarhubi commented Feb 14, 2017

fiveop commented Feb 15, 2017

Susurrus commented Jun 5, 2017

roblabla commented Jun 5, 2017

Susurrus commented Jun 6, 2017

roblabla commented Jun 7, 2017

lucab commented Aug 25, 2017

Susurrus commented Aug 25, 2017

roblabla commented Aug 25, 2017

Susurrus commented Aug 25, 2017

Susurrus commented Aug 25, 2017

Susurrus commented Sep 1, 2017

roblabla commented Sep 1, 2017

Susurrus commented Sep 1, 2017

roblabla commented Sep 1, 2017 •

edited

Loading

Susurrus commented Sep 5, 2017

roblabla commented Sep 5, 2017 •

edited

Loading

Susurrus commented Sep 6, 2017

Susurrus commented Sep 6, 2017

Susurrus commented Dec 4, 2017

Allow receiving multiple FD over ControlMessage in unix sockets #473

Allow receiving multiple FD over ControlMessage in unix sockets #473

Conversation

roblabla commented Nov 19, 2016

fiveop commented Nov 19, 2016

roblabla commented Nov 19, 2016

roblabla commented Nov 19, 2016

fiveop left a comment

Choose a reason for hiding this comment

fiveop Nov 22, 2016

Choose a reason for hiding this comment

fiveop commented Dec 3, 2016

homu commented Dec 16, 2016

kamalmarhubi commented Feb 14, 2017

fiveop commented Feb 15, 2017

Susurrus commented Jun 5, 2017

roblabla commented Jun 5, 2017

Susurrus commented Jun 6, 2017

roblabla commented Jun 7, 2017

lucab commented Aug 25, 2017

Susurrus commented Aug 25, 2017

roblabla commented Aug 25, 2017

Susurrus commented Aug 25, 2017

Susurrus commented Aug 25, 2017

Susurrus commented Sep 1, 2017

roblabla commented Sep 1, 2017

Susurrus commented Sep 1, 2017

roblabla commented Sep 1, 2017 • edited Loading

Susurrus commented Sep 5, 2017

roblabla commented Sep 5, 2017 • edited Loading

Susurrus commented Sep 6, 2017

Susurrus commented Sep 6, 2017

Susurrus commented Dec 4, 2017

roblabla commented Sep 1, 2017 •

edited

Loading

roblabla commented Sep 5, 2017 •

edited

Loading