Merge lp:~thomas-voss/mir/fix-fd-sharing into lp:~mir-team/mir/trunk

[Thomas Voß]

 * Use correct macros to calculate msg length and alignment when sharing fds.
 * Dispatch via io_service to ensure correct ordering.

 * Use correct macros to calculate msg length and alignment when sharing fds.
 * Dispatch via io_service to ensure correct ordering.

[Daniel van Vugt (vanvugt)]

1. Have we observed any problems with "incorrect" ordering? What's being fixed?

2. Please indent the lambda only 4 spaces in from the dispatch line. That should make it more readable.

review: Needs Information

[Thomas Voß (thomas-voss)]

> 1. Have we observed any problems with "incorrect" ordering? What's being
> fixed?
>

The issue reported in https://bugs.launchpad.net/mir/+bug/1204939 might be triggered by incorrect ordering of operations on the underlying socket.

> 2. Please indent the lambda only 4 spaces in from the dispatch line. That
> should make it more readable.

fixed.

[Alan Griffiths (alan-griffiths)]

Only the first really needs fixing, but while we're looking at this code...

There's corresponding code in MirSocketRpcChannel::receive_file_descriptors() that also uses "sizeof(struct cmsghdr) + sizeof(int) * n_fds" - the fix should be applied uniformly.

~~~~

77 + int i = 0;
78 + for (auto &fd: fds)
79 + data[i++] = fd;

I guess it is pre-existing, but what's wrong with using std::copy?

std::copy(fds.begin(), fds.end(), data);

~~~~

It is inefficiency rather than a bug, but we're copying a std::vector into the lambda simply to make it available as a data source for further copying.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

For future safety I would change:
    message->cmsg_len = CMSG_LEN(sizeof(int) * n_fds);
to:
    message->cmsg_len = CMSG_LEN(header.msg_controllen);

But it's not that important.

review: Approve

[Chris Halse Rogers (raof)]

Looks good to me.

Mostly; the lambda's capturing ‘fds’ by value which IIUC will imply another copy of the vector. Although this does mean that we don't need to care about synchronisation.

review: Approve

[Alan Griffiths (alan-griffiths)]

Enough for now. We can worry about the extra copy *if* we measure a performance problem.

review: Approve

[Daniel van Vugt (vanvugt)]

Is the lambda run in the same thread as the caller?
socket->get_io_service().dispatch(...

If not then it is using fds unsafely unlocked and/or potentially after the object is destroyed.

review: Needs Information

[Thomas Voß (thomas-voss)]

> Is the lambda run in the same thread as the caller?
> socket->get_io_service().dispatch(...
>

No, that's why the fds are copied into the lambda.

> If not then it is using fds unsafely unlocked and/or potentially after the
> object is destroyed.

It takes a copy of the fds to avoid this issue.

[Alan Griffiths (alan-griffiths)]

> > Is the lambda run in the same thread as the caller?
> > socket->get_io_service().dispatch(...
> >
>
> No, that's why the fds are copied into the lambda.
>
> > If not then it is using fds unsafely unlocked and/or potentially after the
> > object is destroyed.
>
> It takes a copy of the fds to avoid this issue.

Actually, there is an issue here.

That doesn't avoid the issue: the lifetime of the FDs is controlled by the resource_cache. E.g. ~GBMPlatformIPCPackage() closes the FD after the caller returns.

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:986
http://jenkins.qa.ubuntu.com/job/mir-ci/1325/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1770
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1655
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/563/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1325/rebuild

review: Needs Fixing (continuous-integration)

Unmerged revisions

986. By Thomas Voß on 2013-08-19

* Use correct macros in the client-side protobuf communicator, too.
* Replace manual for-loop with std::copy.

985. By Thomas Voß on 2013-08-19

Fix indent as requested in review.

984. By Thomas Voß on 2013-08-19

 * Use correct macros to calculate msg length and alignment when sharing fds.
 * Dispatch via io_service to ensure correct ordering.

983. By Alexandros Frantzis on 2013-08-16

shell: Add infrastructure for emitting and handling session related events.

Approved by PS Jenkins bot, Alan Griffiths.

982. By Alan Griffiths on 2013-08-16

config: use the DisplayServer to hold ownership of system components, not the DefaultServerConfiguration.

Approved by Alexandros Frantzis, Robert Carr, PS Jenkins bot.

981. By Chris Halse Rogers on 2013-08-16

Add missing mutex around display config call.

Approved by Alexandros Frantzis, PS Jenkins bot, Daniel van Vugt.

980. By Brandon Schaefer on 2013-08-16

Clean up config->cards when were are deleting the display config.

Approved by Alexandros Frantzis, Chris Halse Rogers, PS Jenkins bot.

979. By Alan Griffiths on 2013-08-16

tests: Workaround for test timeout under valgrind. Fixes: https://bugs.launchpad.net/bugs/1212518.

Approved by Daniel van Vugt, Alexandros Frantzis, PS Jenkins bot.

978. By Alan Griffiths on 2013-08-15

graphics::nested: sketch out some more of the nested mir implementation.

Approved by Alexandros Frantzis, PS Jenkins bot.

977. By Alexandros Frantzis on 2013-08-15

shell: Notify sessions when the display configuration changes

This patchset implements client notifications for display configuration changes. It also adds a mir_demo_client_display_config example which can be used to test and demo client initiated display configuration changes. Use of the example uncovered some issues that are also fixed by this patchset (see individual commits for more info).

Approved by PS Jenkins bot, Alan Griffiths.

=== modified file 'src/client/rpc/mir_socket_rpc_channel.cpp'
--- src/client/rpc/mir_socket_rpc_channel.cpp	2013-08-08 02:34:31 +0000
+++ src/client/rpc/mir_socket_rpc_channel.cpp	2013-08-19 09:19:06 +0000
@@ -176,7 +176,7 @@
 
     // Allocate space for control message
     auto n_fds = fds.size();
-    std::vector<char> control(sizeof(struct cmsghdr) + sizeof(int) * n_fds);
+    std::vector<char> control(CMSG_SPACE(sizeof(int) * n_fds));
 
     // Message to send
     struct msghdr header;
@@ -190,7 +190,7 @@
 
     // Control message contains file descriptors
     struct cmsghdr *message = CMSG_FIRSTHDR(&header);
-    message->cmsg_len = header.msg_controllen;
+    message->cmsg_len = CMSG_LEN(sizeof(int) * n_fds);
     message->cmsg_level = SOL_SOCKET;
     message->cmsg_type = SCM_RIGHTS;
 

=== modified file 'src/server/frontend/socket_messenger.cpp'
--- src/server/frontend/socket_messenger.cpp	2013-08-01 09:55:02 +0000
+++ src/server/frontend/socket_messenger.cpp	2013-08-19 09:19:06 +0000
@@ -33,9 +33,9 @@
 {
     struct ucred cr;
     socklen_t cl = sizeof(cr);
-    
+
     auto status = getsockopt(socket->native_handle(), SOL_SOCKET, SO_PEERCRED, &cr, &cl);
-    
+
     if (status)
         BOOST_THROW_EXCEPTION(std::runtime_error("Failed to query client socket credentials"));
     return cr.pid;
@@ -65,36 +65,39 @@
     auto n_fds = fds.size();
     if (n_fds > 0)
     {
-        // We send dummy data
-        struct iovec iov;
-        char dummy_iov_data = 'M';
-        iov.iov_base = &dummy_iov_data;
-        iov.iov_len = 1;
-
-        // Allocate space for control message
-        std::vector<char> control(sizeof(struct cmsghdr) + sizeof(int) * n_fds);
-
-        // Message to send
-        struct msghdr header;
-        header.msg_name = NULL;
-        header.msg_namelen = 0;
-        header.msg_iov = &iov;
-        header.msg_iovlen = 1;
-        header.msg_controllen = control.size();
-        header.msg_control = control.data();
-        header.msg_flags = 0;
-
-        // Control message contains file descriptors
-        struct cmsghdr *message = CMSG_FIRSTHDR(&header);
-        message->cmsg_len = header.msg_controllen;
-        message->cmsg_level = SOL_SOCKET;
-        message->cmsg_type = SCM_RIGHTS;
-        int *data = (int *)CMSG_DATA(message);
-        int i = 0;
-        for (auto &fd: fds)
-            data[i++] = fd;
-
-        sendmsg(socket->native_handle(), &header, 0);
+        socket->get_io_service().dispatch(
+            [this, fds, n_fds]()
+            {
+                // We send dummy data
+                struct iovec iov;
+                char dummy_iov_data = 'M';
+                iov.iov_base = &dummy_iov_data;
+                iov.iov_len = 1;
+
+                // Allocate space for control message
+                std::vector<char> control(CMSG_SPACE(sizeof(int) * n_fds));
+                // Message to send
+                struct msghdr header;
+                header.msg_name = NULL;
+                header.msg_namelen = 0;
+                header.msg_iov = &iov;
+                header.msg_iovlen = 1;
+                header.msg_controllen = control.size();
+                header.msg_control = control.data();
+                header.msg_flags = 0;
+
+                // Control message contains file descriptors
+                struct cmsghdr *message = CMSG_FIRSTHDR(&header);
+                message->cmsg_len = CMSG_LEN(sizeof(int) * n_fds);
+                message->cmsg_level = SOL_SOCKET;
+                message->cmsg_type = SCM_RIGHTS;
+                std::copy(
+                    fds.begin(),
+                    fds.end(),
+                    reinterpret_cast<int*>(CMSG_DATA(message)));
+
+                sendmsg(socket->native_handle(), &header, 0);
+            });
     }
 }
 
@@ -106,4 +109,4 @@
          buffer,
          boost::asio::transfer_exactly(size),
          handler);
-} 
+}