Merge lp:~alan-griffiths/mir/fix-1526248 into lp:mir

[Alan Griffiths]

client: Fix racy access surfaces and streams in ConnectionSurfaceMap and to message objects in MirProtobufRpcChannel

client: Fix racy access surfaces and streams in ConnectionSurfaceMap and to message objects in MirProtobufRpcChannel

[Kevin DuBois (kdub)]

I have a more involved fix coming (probably today actually), where we switch to using shared ptrs.

Its a bit unintuitive, but since the resources in the map are raw ptrs, they can actually be deleted before the map gets the lock, and removes them. The branch I have switches to shared_ptrs, so the object can't get deleted while under SurfaceMap's lock.

[Kevin DuBois (kdub)]

https://code.launchpad.net/~kdub/mir/sort-out-surface-ownership/+merge/280481

[lp:~alan-griffiths/mir/fix-1526248]

3200. By Alan Griffiths on 2015-12-15

Fix race in mclr::MirProtobufRpcChannel::on_data_available()

[Alan Griffiths (alan-griffiths)]

> I have a more involved fix coming (probably today actually), where we switch
> to using shared ptrs.

Do you want to block this version?

Is there an additional reason to switch to shared_ptr<>?

[Kevin DuBois (kdub)]

The additional reason was that the shutdown sequence is somewhat confused, and its difficult to come up with a proper shutdown/disconnect message handling with NBS (which has more rapid events being sent)

No reason to block this one though, (esp if it has a second fix in it outside of SurfaceMap) lgtm.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3199
http://jenkins.qa.ubuntu.com/job/mir-ci/5849/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5316
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4222
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5265
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/159
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/177
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/177/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/177
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/177/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5265
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5265/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7785
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26067
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/157
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/157/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/16
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26078

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3200
http://jenkins.qa.ubuntu.com/job/mir-ci/5855/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5322
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4228
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5271
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/161
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/183
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/183/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/183
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/183/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5271
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5271/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7787
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26088
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/159
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/159/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/18
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26092

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

I recall a long time ago that locking during exec() caused deadlocks there. How sure are you that's now fixed?

- lk.unlock();
         exec(surface);

review: Needs Information

[Alan Griffiths (alan-griffiths)]

> I recall a long time ago that locking during exec() caused deadlocks there.
> How sure are you that's now fixed?
>
> - lk.unlock();
> exec(surface);

I've left the tests of this area looping over lunch and been running exploratory tests.

This isn't mathematical proof that there isn't a deadlock scenario, but deadlock is hypothetical and this fixes a demonstrable problem:

Unlocking is clearly wrong as it allows another thread to call erase() followed by deleting "surface" between unlock() and exec(). Holding a shared lock prevents that while still allowing non-mutating access.

[Alberto Aguirre (albaguirre)]

There's a lock order inversion report (with a TSan build)

but It looks benign since the only time the locks in question are acquired in different order is during surface creation.

During creation:

Callback MirSurface::created is invoked
-locks MirSurface::mutex
-calls buffer_stream_factory->make_producer_stream
--calls MirConnection::on_stream_created
---calls ConnectionSurfaceMap::insert
----write locks ConnectionSurfaceMap::guard

During normal operation:

MirProtobufRpcChannel::process_event_sequence
-calls ConnectionSurfaceMap::with_surface_do
--Read locks ConnectionSurfaceMap::guard
---Invokes callback which does MirSurface::handle_event
----locks MirSurface::mutex

http://paste.ubuntu.com/14054652/

I don't think the buffer stream creation needs to be done with a locked MirSurface::mutex; We should take a closer look at the uses of MirSurface::mutex, some don't seem necessary -but that's a separate issue outside the scope of this MP.

review: Approve

[Daniel van Vugt (vanvugt)]

review: Abstain

=== modified file 'src/client/connection_surface_map.h'
--- src/client/connection_surface_map.h	2015-07-16 07:03:19 +0000
+++ src/client/connection_surface_map.h	2015-12-15 15:42:58 +0000
@@ -21,8 +21,8 @@
 
 #include "surface_map.h"
 
+#include <shared_mutex>
 #include <unordered_map>
-#include <mutex>
 
 namespace mir
 {
@@ -49,7 +49,7 @@
     void erase(frontend::BufferStreamId surface_id);
 
 private:
-    std::mutex mutable guard;
+    std::shared_timed_mutex mutable guard;
     std::unordered_map<frontend::SurfaceId, MirSurface*> surfaces;
     std::unordered_map<frontend::BufferStreamId, StreamInfo> streams;
 };

=== modified file 'src/client/rpc/mir_basic_rpc_channel.cpp'
--- src/client/rpc/mir_basic_rpc_channel.cpp	2015-08-21 12:08:03 +0000
+++ src/client/rpc/mir_basic_rpc_channel.cpp	2015-12-15 15:42:58 +0000
@@ -66,10 +66,12 @@
     pending_calls[invoke.id()] = PendingCall(response, complete);
 }
 
-google::protobuf::MessageLite* mclrd::PendingCallCache::message_for_result(mir::protobuf::wire::Result& result)
+void mclrd::PendingCallCache::populate_message_for_result(
+    mir::protobuf::wire::Result& result,
+    std::function<void(google::protobuf::MessageLite*)> const& populator)
 {
     std::unique_lock<std::mutex> lock(mutex);
-    return pending_calls.at(result.id()).response;
+    populator(pending_calls.at(result.id()).response);
 }
 
 void mclrd::PendingCallCache::complete_response(mir::protobuf::wire::Result& result)

=== modified file 'src/client/rpc/mir_basic_rpc_channel.h'
--- src/client/rpc/mir_basic_rpc_channel.h	2015-08-21 12:08:03 +0000
+++ src/client/rpc/mir_basic_rpc_channel.h	2015-12-15 15:42:58 +0000
@@ -67,7 +67,9 @@
         google::protobuf::Closure* complete);
 
 
-    google::protobuf::MessageLite* message_for_result(mir::protobuf::wire::Result& result);
+    void populate_message_for_result(
+        mir::protobuf::wire::Result& result,
+        std::function<void(google::protobuf::MessageLite*)> const& populator);
 
     void complete_response(mir::protobuf::wire::Result& result);
 

=== modified file 'src/client/rpc/mir_protobuf_rpc_channel.cpp'
--- src/client/rpc/mir_protobuf_rpc_channel.cpp	2015-10-07 12:07:00 +0000
+++ src/client/rpc/mir_protobuf_rpc_channel.cpp	2015-12-15 15:42:58 +0000
@@ -382,9 +382,13 @@
 
         if (result->has_id())
         {
-            auto result_message = pending_calls.message_for_result(*result);
-            result_message->ParseFromString(result->response());
-            receive_file_descriptors(result_message);
+            pending_calls.populate_message_for_result(
+                *result,
+                [&](google::protobuf::MessageLite* result_message)
+                    {
+                        result_message->ParseFromString(result->response());
+                        receive_file_descriptors(result_message);
+                    });
 
             if (id_to_wait_for)
             {

=== modified file 'src/client/surface_map.cpp'
--- src/client/surface_map.cpp	2015-07-27 07:24:50 +0000
+++ src/client/surface_map.cpp	2015-12-15 15:42:58 +0000
@@ -37,7 +37,7 @@
         //Prevent TSAN from flagging lock ordering issues
         //as the surface/buffer_stream destructors acquire internal locks
         //The mutex lock is used mainly as a memory barrier here
-        std::lock_guard<std::mutex> lk(guard);
+        std::lock_guard<decltype(guard)> lk(guard);
         surface_map = std::move(surfaces);
         stream_map = std::move(streams);
     }
@@ -60,12 +60,11 @@
 void mcl::ConnectionSurfaceMap::with_surface_do(
     mf::SurfaceId surface_id, std::function<void(MirSurface*)> const& exec) const
 {
-    std::unique_lock<std::mutex> lk(guard);
+    std::shared_lock<decltype(guard)> lk(guard);
     auto const it = surfaces.find(surface_id);
     if (it != surfaces.end())
     {
         auto const surface = it->second;
-        lk.unlock();
         exec(surface);
     }
     else
@@ -81,14 +80,14 @@
     // get_buffer_stream has internal locks - call before locking mutex to
     // avoid locking ordering issues
     auto const stream = surface->get_buffer_stream();
-    std::lock_guard<std::mutex> lk(guard);
+    std::lock_guard<decltype(guard)> lk(guard);
     surfaces[surface_id] = surface;
     streams[mf::BufferStreamId(surface_id.as_value())] = {stream, false};
 }
 
 void mcl::ConnectionSurfaceMap::erase(mf::SurfaceId surface_id)
 {
-    std::lock_guard<std::mutex> lk(guard);
+    std::lock_guard<decltype(guard)> lk(guard);
     surfaces.erase(surface_id);
     streams.erase(mf::BufferStreamId(surface_id.as_value()));
 }
@@ -96,12 +95,11 @@
 void mcl::ConnectionSurfaceMap::with_stream_do(
     mf::BufferStreamId stream_id, std::function<void(ClientBufferStream*)> const& exec) const
 {
-    std::unique_lock<std::mutex> lk(guard);
+    std::shared_lock<decltype(guard)> lk(guard);
     auto const it = streams.find(stream_id);
     if (it != streams.end())
     {
         auto const stream = it->second.stream;
-        lk.unlock();
         exec(stream);
     }
     else
@@ -114,19 +112,19 @@
 
 void mcl::ConnectionSurfaceMap::with_all_streams_do(std::function<void(ClientBufferStream*)> const& fn) const
 {
-    std::unique_lock<std::mutex> lk(guard);
+    std::shared_lock<decltype(guard)> lk(guard);
     for(auto const& stream : streams)
         fn(stream.second.stream);
 }
 
 void mcl::ConnectionSurfaceMap::insert(mf::BufferStreamId stream_id, ClientBufferStream* stream)
 {
-    std::lock_guard<std::mutex> lk(guard);
+    std::lock_guard<decltype(guard)> lk(guard);
     streams[stream_id] = {stream, true};
 }
 
 void mcl::ConnectionSurfaceMap::erase(mf::BufferStreamId stream_id)
 {
-    std::lock_guard<std::mutex> lk(guard);
+    std::lock_guard<decltype(guard)> lk(guard);
     streams.erase(stream_id);
 }