Merge lp:~robertcarr/mir/socket-messenger-race into lp:mir

[Robert Ancell]

socket_messenger: Protect whole_message with a lock.

It seems to me that socket messenger may be used from any thread (as a result of the shell generating various events, i.e. focus/unfocus).

This means that we need to protect whole_message with a lock.

[kevin gunn (kgunn72)]

should be ok from mirserver api, no break

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1078
http://jenkins.qa.ubuntu.com/job/mir-ci/1559/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2091
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1976
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/797/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-armhf-ci/59
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-armhf-ci/59/artifact/work/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

[Kevin DuBois (kdub)]

should retarget to development branch...

other than that though, the fix makes sense

review: Approve

[Alan Griffiths (alan-griffiths)]

I'm not sure this is adequate.

Unless I miss something, the lock needs to be held across the related send() and send_fds() calls originating in ProtobufMessageProcessor::send_response() - not just the send() call.

And that is best approached by changing interfaces so that the body and fds are passed to a single call.

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1078
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/32/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2143
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2028
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/32
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/32/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-team-mir-development-branch-ci/32/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I'm not sure this is adequate.

Unless I miss something, the lock needs to be held across the related send() and send_fds() calls originating in ProtobufMessageProcessor::send_response() - not just the send() call.

And that is best approached by changing interfaces so that the body and fds are passed to a single call.

review: Needs Fixing

[Robert Carr (robertcarr)]

Reworked the interfaces some to hold the lock across calls.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1081
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/39/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2152
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2037/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/39
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/39/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-team-mir-development-branch-ci/39/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1082
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/41/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2154/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2039/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/41/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-team-mir-development-branch-ci/41/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1083
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/43/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2156
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2041
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/43
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/43/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-team-mir-development-branch-ci/43/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

177 boost::system::error_code err;
178 ba::write(*socket, ba::buffer(whole_message), err);
179 +
180 + for (auto const& fds : fd_set)
181 + send_fds_locked(lg, fds);

I think that if the call to write fails we should skip the call(s) to send_fds_locked().

~~~~

91 + send_response(id, static_cast<google::protobuf::Message*>(response), {surface_fd, buffer_fd});;

Now that there is a third parameter to the "google::protobuf::Message*" overload of send_response() we don't need the cast to disambiguate.

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

+typedef std::initializer_list<std::vector<int32_t>> FdSets;

What's the use case for multiple sets of FDs?

review: Needs Information

[Robert Carr (robertcarr)]

Removed the error code usage so ba::write will throw, and catch the exceptions in processor/communicator (obsoleting socket-messenger-reporting).

Removed the casts.

Alf:

void mfd::ProtobufMessageProcessor::send_response(::google::protobuf::uint32 id, mir::protobuf::Surface* response)
{
    auto const& surface_fd = extract_fds_from(response);
    const auto& buffer_fd = response->has_buffer() ?
        extract_fds_from(response->mutable_buffer()) :
        std::vector<int32_t>();

    send_response(id, response, {surface_fd, buffer_fd});;
    resource_cache->free_resource(response);
}

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1087
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/57/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2182
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2067
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/57
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/57/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-team-mir-development-branch-ci/57/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1088
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/60/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2185
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2070
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/60
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/60/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-team-mir-development-branch-ci/60/rebuild

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Looks good.

> > What's the use case for multiple sets of FDs?
>
> send_response(id, response, {surface_fd, buffer_fd});

This is a pre-existing situation, made more obvious (to me) because of the interface change:

Since we don't process the different sets of FDs differently, this interface seems a bit strange (vs accepting a single vector containing all the FDs), and requires two sendmsg() calls on the server side, and two rcvmsg() calls on the client side. Unless I am missing something, we could send/receive the FDs in one message, although I am not sure if it is worth the trouble, since we cache FDs on the client side. Still, something to keep in mind...

review: Approve

[Alan Griffiths (alan-griffiths)]

void mfd::ProtobufMessageProcessor::send_response(
...
133 -
134 - sender->send(send_response_buffer);
135 + try
136 + {
137 + sender->send(send_response_buffer, fd_sets);
138 + }
139 + catch (std::exception const& error)
140 + {
141 + report->exception_handled(display_server.get(),
142 + id, error);
143 + }
144 }

Exceptions should not be eaten here. They should propagate to ProtobufMessageProcessor::dispatch() - which handles them correctly.

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> Exceptions should not be eaten here. They should propagate to ProtobufMessageProcessor::dispatch() - which handles them correctly.

Since we are moving towards reinstating previous send failure behavior, we should also change send_fds_locked() to throw in case of failure.

review: Needs Fixing

[Robert Carr (robertcarr)]

Like this? (r1090)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1090
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/116/
Executed test runs:
    SUCCESS: http://10.97.0.26:8080/job/mir-android-saucy-i386-build/2265
    SUCCESS: http://10.97.0.26:8080/job/mir-clang-saucy-amd64-build/2150
    FAILURE: http://10.97.0.26:8080/job/mir-team-mir-development-branch-saucy-amd64-ci/113/console

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/116/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1091
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/124/
Executed test runs:
    SUCCESS: http://10.97.0.26:8080/job/mir-android-saucy-i386-build/2274
    SUCCESS: http://10.97.0.26:8080/job/mir-clang-saucy-amd64-build/2159
    SUCCESS: http://10.97.0.26:8080/job/mir-team-mir-development-branch-saucy-amd64-ci/121
        deb: http://10.97.0.26:8080/job/mir-team-mir-development-branch-saucy-amd64-ci/121/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/124/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

134 - sender->send(send_response_buffer);
135 + try
136 + {
137 + sender->send(send_response_buffer, fd_sets);
138 + }
139 + catch (std::exception const& error)
140 + {
141 + report->exception_handled(display_server.get(),
142 + id, error);
143 + }

Exceptions should not be eaten here. They should propagate to ProtobufMessageProcessor::dispatch() - which handles them correctly.

review: Needs Fixing

[Robert Carr (robertcarr)]

Iterated.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1093
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/137/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2292
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2177
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/134
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/134/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/137/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

If there's a related bug please link it.

But regardless, I'm not familiar with this code and there seem to be enough opinions. So abstain for now.

review: Abstain

[Alan Griffiths (alan-griffiths)]

74 virtual void send(std::string const& body) = 0;
...
76 + virtual void send(std::string const& body, FdSets const& fds) = 0;

Do we really need both versions?

send(x) ought to be the same as send(x, {});

review: Approve

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

[Daniel van Vugt (vanvugt)]

That seems to be approval from everyone who had an opinion so good enough...

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'src/server/frontend/event_sender.cpp'
--- src/server/frontend/event_sender.cpp	2013-08-28 03:41:48 +0000
+++ src/server/frontend/event_sender.cpp	2013-10-08 18:30:35 +0000
@@ -80,5 +80,14 @@
     mir::protobuf::wire::Result result;
     result.add_events(send_buffer);
     result.SerializeToString(&send_buffer);
-    sender->send(send_buffer);
+
+    try
+    {
+        sender->send(send_buffer);
+    }
+    catch (std::exception const& error)
+    {
+        // TODO: We should report this state.
+        (void) error;
+    }
 }

=== added file 'src/server/frontend/fd_sets.h'
--- src/server/frontend/fd_sets.h	1970-01-01 00:00:00 +0000
+++ src/server/frontend/fd_sets.h	2013-10-08 18:30:35 +0000
@@ -0,0 +1,33 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Robert Carr <[e-mail]>
+ */
+#ifndef MIR_FRONTEND_FD_SETS_H_
+#define MIR_FRONTEND_FD_SETS_H_
+
+#include <vector>
+#include <initializer_list>
+#include <stdint.h>
+
+namespace mir
+{
+namespace frontend
+{
+typedef std::initializer_list<std::vector<int32_t>> FdSets;
+}
+} // namespace mir 
+
+#endif // MIR_FRONTEND_FD_SETS_H_

=== modified file 'src/server/frontend/message_sender.h'
--- src/server/frontend/message_sender.h	2013-08-28 03:41:48 +0000
+++ src/server/frontend/message_sender.h	2013-10-08 18:30:35 +0000
@@ -18,6 +18,8 @@
 #ifndef MIR_FRONTEND_MESSAGE_SENDER_H_
 #define MIR_FRONTEND_MESSAGE_SENDER_H_
 
+#include "fd_sets.h"
+
 #include <vector>
 #include <boost/asio.hpp>
 
@@ -31,7 +33,7 @@
 {
 public:
     virtual void send(std::string const& body) = 0;
-    virtual void send_fds(std::vector<int32_t> const& fd) = 0;
+    virtual void send(std::string const& body, FdSets const& fds) = 0;
 
 protected:
     MessageSender() = default;

=== modified file 'src/server/frontend/protobuf_message_processor.cpp'
--- src/server/frontend/protobuf_message_processor.cpp	2013-08-29 03:48:16 +0000
+++ src/server/frontend/protobuf_message_processor.cpp	2013-10-08 18:30:35 +0000
@@ -50,8 +50,7 @@
 void mfd::ProtobufMessageProcessor::send_response(::google::protobuf::uint32 id, mir::protobuf::Buffer* response)
 {
     const auto& fd = extract_fds_from(response);
-    send_response(id, static_cast<google::protobuf::Message*>(response));
-    sender->send_fds(fd);
+    send_response(id, response, {fd});
     resource_cache->free_resource(response);
 }
 
@@ -61,8 +60,7 @@
         extract_fds_from(response->mutable_platform()) :
         std::vector<int32_t>();
 
-    send_response(id, static_cast<google::protobuf::Message*>(response));
-    sender->send_fds(fd);
+    send_response(id, response, {fd});
     resource_cache->free_resource(response);
 }
 
@@ -73,9 +71,7 @@
         extract_fds_from(response->mutable_buffer()) :
         std::vector<int32_t>();
 
-    send_response(id, static_cast<google::protobuf::Message*>(response));
-    sender->send_fds(surface_fd);
-    sender->send_fds(buffer_fd);
+    send_response(id, response, {surface_fd, buffer_fd});;
     resource_cache->free_resource(response);
 }
 
@@ -126,6 +122,14 @@
     ::google::protobuf::uint32 id,
     google::protobuf::Message* response)
 {
+    send_response(id, response, FdSets());
+}
+
+void mfd::ProtobufMessageProcessor::send_response(
+    ::google::protobuf::uint32 id,
+    google::protobuf::Message* response,
+    FdSets const& fd_sets)
+{
     response->SerializeToString(&send_response_buffer);
 
     send_response_result.set_id(id);
@@ -133,7 +137,7 @@
 
     send_response_result.SerializeToString(&send_response_buffer);
 
-    sender->send(send_response_buffer);
+    sender->send(send_response_buffer, fd_sets);
 }
 
 bool mfd::ProtobufMessageProcessor::dispatch(mir::protobuf::wire::Invocation const& invocation)

=== modified file 'src/server/frontend/protobuf_message_processor.h'
--- src/server/frontend/protobuf_message_processor.h	2013-08-28 03:41:48 +0000
+++ src/server/frontend/protobuf_message_processor.h	2013-10-08 18:30:35 +0000
@@ -20,6 +20,8 @@
 #ifndef MIR_FRONTEND_PROTOBUF_MESSAGE_PROCESSOR_H_
 #define MIR_FRONTEND_PROTOBUF_MESSAGE_PROCESSOR_H_
 
+#include "fd_sets.h"
+
 #include "message_processor.h"
 #include "message_sender.h"
 
@@ -57,6 +59,8 @@
 
 private:
     void send_response(::google::protobuf::uint32 id, google::protobuf::Message* response);
+    void send_response(::google::protobuf::uint32 id, google::protobuf::Message* response,
+        FdSets const& fd_sets);
 
     template<class ResultMessage>
     void send_response(::google::protobuf::uint32 id, ResultMessage* response);

=== modified file 'src/server/frontend/socket_messenger.cpp'
--- src/server/frontend/socket_messenger.cpp	2013-09-25 14:18:18 +0000
+++ src/server/frontend/socket_messenger.cpp	2013-10-08 18:30:35 +0000
@@ -19,7 +19,13 @@
 #include "socket_messenger.h"
 #include "mir/frontend/client_constants.h"
 
-namespace mfd = mir::frontend::detail;
+#include <errno.h>
+#include <string.h>
+
+#include <stdexcept>
+
+namespace mf = mir::frontend;
+namespace mfd = mf::detail;
 namespace bs = boost::system;
 namespace ba = boost::asio;
 
@@ -43,12 +49,19 @@
 
 void mfd::SocketMessenger::send(std::string const& body)
 {
+    send(body, mf::FdSets());
+}
+
+void mfd::SocketMessenger::send(std::string const& body, FdSets const& fd_set)
+{
     const size_t size = body.size();
     const unsigned char header_bytes[2] =
     {
         static_cast<unsigned char>((size >> 8) & 0xff),
         static_cast<unsigned char>((size >> 0) & 0xff)
     };
+    
+    std::unique_lock<std::mutex> lg(message_lock);
 
     whole_message.resize(sizeof header_bytes + size);
     std::copy(header_bytes, header_bytes + sizeof header_bytes, whole_message.begin());
@@ -59,11 +72,13 @@
     // function has completed (if it would be executed asynchronously.
     // NOTE: we rely on this synchronous behavior as per the comment in
     // mf::SessionMediator::create_surface
-    boost::system::error_code err;
-    ba::write(*socket, ba::buffer(whole_message), err);
+    ba::write(*socket, ba::buffer(whole_message));
+
+    for (auto const& fds : fd_set)
+        send_fds_locked(lg, fds);
 }
 
-void mfd::SocketMessenger::send_fds(std::vector<int32_t> const& fds)
+void mfd::SocketMessenger::send_fds_locked(std::unique_lock<std::mutex> const&, std::vector<int32_t> const& fds)
 {
     auto n_fds = fds.size();
     if (n_fds > 0)
@@ -97,7 +112,9 @@
         for (auto &fd: fds)
             data[i++] = fd;
 
-        sendmsg(socket->native_handle(), &header, 0);
+        auto sent = sendmsg(socket->native_handle(), &header, 0);
+        if (sent < 0)
+            BOOST_THROW_EXCEPTION(std::runtime_error("Failed to send fds: " + std::string(strerror(errno))));
     }
 }
 

=== modified file 'src/server/frontend/socket_messenger.h'
--- src/server/frontend/socket_messenger.h	2013-08-28 03:41:48 +0000
+++ src/server/frontend/socket_messenger.h	2013-10-08 18:30:35 +0000
@@ -35,14 +35,18 @@
     SocketMessenger(std::shared_ptr<boost::asio::local::stream_protocol::socket> const& socket);
 
     void send(std::string const& body);
-    void send_fds(std::vector<int32_t> const& fds);
+    void send(std::string const& body, FdSets const& fd_set);
 
     void async_receive_msg(MirReadHandler const& handler, boost::asio::streambuf& buffer, size_t size); 
     pid_t client_pid();
 
 private:
     std::shared_ptr<boost::asio::local::stream_protocol::socket> socket;
+    
+    std::mutex message_lock;
     std::vector<char> whole_message;
+    
+    void send_fds_locked(std::unique_lock<std::mutex> const& lock, std::vector<int32_t> const& fds);
 };
 }
 }

=== modified file 'tests/unit-tests/frontend/test_event_sender.cpp'
--- tests/unit-tests/frontend/test_event_sender.cpp	2013-08-28 03:41:48 +0000
+++ tests/unit-tests/frontend/test_event_sender.cpp	2013-10-08 18:30:35 +0000
@@ -30,7 +30,8 @@
 
 namespace mt=mir::test;
 namespace mtd=mir::test::doubles;
-namespace mfd=mir::frontend::detail;
+namespace mf=mir::frontend;
+namespace mfd=mf::detail;
 namespace geom=mir::geometry;
 
 namespace 
@@ -38,7 +39,7 @@
 struct MockMsgSender : public mfd::MessageSender
 {
     MOCK_METHOD1(send, void(std::string const&));
-    MOCK_METHOD1(send_fds, void(std::vector<int32_t> const&));
+    MOCK_METHOD2(send, void(std::string const&, mf::FdSets const&));
 };
 }