Merge lp:~kdub/mir/fix-1394362 into lp:mir

[Kevin DuBois]

fix lp: #1394362 by making the fd reception code exception-safe.

lp: #1394362

fix lp: #1394362 by making the fd reception code exception-safe.

lp: #1394362

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2073
http://jenkins.qa.ubuntu.com/job/mir-ci/2173/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/219
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/219
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/196/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/162
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/162/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/196
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/196/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3464/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16022

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

review: Needs Fixing (continuous-integration)

[Alexandros Frantzis (afrantzis)]

39 + if (fd > 0)

The fd could also be 0 (not very likely, but possible).

41 BOOST_THROW_EXCEPTION(std::runtime_error("Receieved fewer fds than expected"));

Pre-existing typo: "Receieved"

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

40 + ::close(fd);

It would be good to clear the fds vector (not very likely that it will be accessed after an exception, but still...).

[Alexandros Frantzis (afrantzis)]

Looks good. Once we are able to use owning mir::Fds the code will become much simpler (i.e. no need for manual cleanup).

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2075
http://jenkins.qa.ubuntu.com/job/mir-ci/2183/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/236
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/236
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/212
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/172
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/172/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/212
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/212/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3477
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16055

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

OK

review: Approve

[Alberto Aguirre (albaguirre)]

Nit:

39+ for(auto fd : fds)

space needed between for and (

review: Approve

=== modified file 'src/common/fd/fd_socket_transmission.cpp'
--- src/common/fd/fd_socket_transmission.cpp	2014-10-22 15:43:57 +0000
+++ src/common/fd/fd_socket_transmission.cpp	2014-11-20 18:49:22 +0000
@@ -148,18 +148,24 @@
         struct cmsghdr const* const cmsg = CMSG_FIRSTHDR(&header);
         if (cmsg)
         {
-            // NOTE: This relies on the file descriptor cmsg being read
-            // (and written) atomically.
-            if (cmsg->cmsg_len > CMSG_LEN(fds_bytes) || (header.msg_flags & MSG_CTRUNC))
-                BOOST_THROW_EXCEPTION(std::runtime_error("Received more fds than expected"));
             if ((cmsg->cmsg_level == SOL_SOCKET) && (cmsg->cmsg_type == SCM_CREDENTIALS))
                 BOOST_THROW_EXCEPTION(fd_reception_error("received SCM_CREDENTIALS when expecting fd"));
             if (cmsg->cmsg_level != SOL_SOCKET || cmsg->cmsg_type != SCM_RIGHTS)
                 BOOST_THROW_EXCEPTION(fd_reception_error("Invalid control message for receiving file descriptors"));
+
             int const* const data = reinterpret_cast<int const*>CMSG_DATA(cmsg);
             ptrdiff_t const header_size = reinterpret_cast<char const*>(data) - reinterpret_cast<char const*>(cmsg);
             int const nfds = (cmsg->cmsg_len - header_size) / sizeof(int);
 
+            // NOTE: This relies on the file descriptor cmsg being read
+            // (and written) atomically.
+            if (cmsg->cmsg_len > CMSG_LEN(fds_bytes) || (header.msg_flags & MSG_CTRUNC))
+            {
+                for (int i = 0; i < nfds; i++)
+                    ::close(data[i]);
+                BOOST_THROW_EXCEPTION(std::runtime_error("Received more fds than expected"));
+            }
+
             // We can't properly pass mir::Fds through google::protobuf::Message,
             // which is where these get shoved.
             //
@@ -173,5 +179,11 @@
     }
 
     if (fds_read < fds.size())
-        BOOST_THROW_EXCEPTION(std::runtime_error("Receieved fewer fds than expected"));
+    {
+        for (auto fd : fds)
+            if (fd >= 0)
+                ::close(fd);
+        fds.clear();
+        BOOST_THROW_EXCEPTION(std::runtime_error("Received fewer fds than expected"));
+    }
 }