Merge lp:~afrantzis/mir/fix-1358191-connect-segv into lp:mir

[Alexandros Frantzis]

client: Ensure our platform library stays loaded for as long as it is needed by other objects. This avoids crashes observed on mir_connect failure
(LP: #1358191)

Both the DefaultClientConfiguration and the MirConnection need to keep the platform library loaded independently for their own reasons (see the bug addressed by this MP).

This MP drops the library cache since the dl library uses reference counting internally, so having a global cache to share libraries between connections complicates the code without offering any benefits.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1865
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2512/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1472
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1478
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1457
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1034
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1034/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/394
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/394/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2543
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/12138

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

LGTM

review: Approve

[Kevin DuBois (kdub)]

+1

review: Approve

[Alberto Aguirre (albaguirre)]

OK.

review: Approve

=== modified file 'src/client/connection_configuration.h'
--- src/client/connection_configuration.h	2014-07-30 15:25:54 +0000
+++ src/client/connection_configuration.h	2014-08-26 13:48:43 +0000
@@ -43,8 +43,6 @@
 
 namespace client
 {
-std::shared_ptr<SharedLibrary>& libraries_cache(std::string const& lib_name);
-
 class ConnectionSurfaceMap;
 class Logger;
 class ClientPlatformFactory;
@@ -67,6 +65,7 @@
     virtual std::shared_ptr<LifecycleControl> the_lifecycle_control() = 0;
     virtual std::shared_ptr<EventSink> the_event_sink() = 0;
     virtual std::shared_ptr<EventHandlerRegister> the_event_handler_register() = 0;
+    virtual std::shared_ptr<SharedLibrary> the_platform_library() = 0;
 
 protected:
     ConnectionConfiguration() = default;

=== modified file 'src/client/default_connection_configuration.cpp'
--- src/client/default_connection_configuration.cpp	2014-07-30 15:25:54 +0000
+++ src/client/default_connection_configuration.cpp	2014-08-26 13:48:43 +0000
@@ -42,20 +42,6 @@
 std::string const log_opt_val{"log"};
 std::string const lttng_opt_val{"lttng"};
 std::string const default_platform_lib{"libmirclientplatform.so"};
-
-mir::SharedLibrary const* load_library(std::string const& libname)
-{
-
-    if (auto& ptr = mcl::libraries_cache(libname))
-    {
-        return ptr.get();
-    }
-    else
-    {
-        ptr = std::make_shared<mir::SharedLibrary>(libname);
-        return ptr.get();
-    }
-}
 }
 
 mcl::DefaultConnectionConfiguration::DefaultConnectionConfiguration(
@@ -98,14 +84,10 @@
 mcl::DefaultConnectionConfiguration::the_client_platform_factory()
 {
     return client_platform_factory(
-        []
+        [this]
         {
-            auto const val_raw = getenv("MIR_CLIENT_PLATFORM_LIB");
-            std::string const val{val_raw ? val_raw : default_platform_lib};
-            auto const platform_lib = ::load_library(val);
-
             auto const create_client_platform_factory =
-                platform_lib->load_function<mcl::CreateClientPlatformFactory>(
+                the_platform_library()->load_function<mcl::CreateClientPlatformFactory>(
                     "create_client_platform_factory");
 
             return create_client_platform_factory();
@@ -199,3 +181,15 @@
             return std::make_shared<MirEventDistributor>();
         });
 }
+
+std::shared_ptr<mir::SharedLibrary> mcl::DefaultConnectionConfiguration::the_platform_library()
+{
+    if (!platform_library)
+    {
+        auto const val_raw = getenv("MIR_CLIENT_PLATFORM_LIB");
+        std::string const libname{val_raw ? val_raw : default_platform_lib};
+        platform_library = std::make_shared<mir::SharedLibrary>(libname);
+    }
+
+    return platform_library;
+}

=== modified file 'src/client/default_connection_configuration.h'
--- src/client/default_connection_configuration.h	2014-07-02 06:29:24 +0000
+++ src/client/default_connection_configuration.h	2014-08-26 13:48:43 +0000
@@ -57,12 +57,16 @@
     std::shared_ptr<LifecycleControl> the_lifecycle_control();
     std::shared_ptr<EventSink> the_event_sink();
     std::shared_ptr<EventHandlerRegister> the_event_handler_register();
+    std::shared_ptr<SharedLibrary> the_platform_library() override;
 
     virtual std::string the_socket_file();
     virtual std::shared_ptr<rpc::RpcReport> the_rpc_report();
     virtual std::shared_ptr<input::receiver::InputReceiverReport> the_input_receiver_report();
 
 protected:
+    // MUST be first data member so it is destroyed last.
+    std::shared_ptr<mir::SharedLibrary> platform_library;
+
     CachedPtr<google::protobuf::RpcChannel> rpc_channel;
     CachedPtr<mir::logging::Logger> logger;
     CachedPtr<ClientPlatformFactory> client_platform_factory;

=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2014-08-22 02:56:00 +0000
+++ src/client/mir_connection.cpp	2014-08-26 13:48:43 +0000
@@ -67,19 +67,6 @@
 
 std::mutex connection_guard;
 MirConnection* valid_connections{nullptr};
-// There's no point in loading twice, and it isn't safe to
-// unload while there are valid connections
-std::map<std::string, std::shared_ptr<mir::SharedLibrary>>* libraries_cache_ptr{nullptr};
-}
-
-std::shared_ptr<mir::SharedLibrary>& mcl::libraries_cache(std::string const& libname)
-{
-    std::lock_guard<std::mutex> lock(connection_guard);
-
-    if (!libraries_cache_ptr)
-        libraries_cache_ptr = new LibrariesCache;
-
-    return (*libraries_cache_ptr)[libname];
 }
 
 MirConnection::Deregisterer::~Deregisterer()
@@ -94,13 +81,6 @@
             break;
         }
     }
-
-    // When the last valid connection goes we can clear the libraries cache
-    if (!valid_connections)
-    {
-        delete libraries_cache_ptr;
-        libraries_cache_ptr = nullptr;
-    }
 }
 
 MirConnection::MirConnection(std::string const& error_message) :
@@ -114,6 +94,7 @@
 MirConnection::MirConnection(
     mir::client::ConnectionConfiguration& conf) :
         deregisterer{this},
+        platform_library{conf.the_platform_library()},
         channel(conf.the_rpc_channel()),
         server(channel.get(), ::google::protobuf::Service::STUB_DOESNT_OWN_CHANNEL),
         logger(conf.the_logger()),

=== modified file 'src/client/mir_connection.h'
--- src/client/mir_connection.h	2014-07-30 15:25:54 +0000
+++ src/client/mir_connection.h	2014-08-26 13:48:43 +0000
@@ -37,6 +37,8 @@
 
 namespace mir
 {
+class SharedLibrary;
+
 /// The client-side library implementation namespace
 namespace client
 {
@@ -138,6 +140,10 @@
     struct Deregisterer
     { MirConnection* const self; ~Deregisterer(); } deregisterer;
 
+    // MUST be placed before any variables for components that are loaded
+    // from a shared library, e.g., the ClientPlatform* objects.
+    std::shared_ptr<mir::SharedLibrary> const platform_library;
+
     std::mutex mutex; // Protects all members of *this (except release_wait_handles)
 
     std::shared_ptr<google::protobuf::RpcChannel> const channel;

=== modified file 'tests/acceptance-tests/test_client_authorization.cpp'
--- tests/acceptance-tests/test_client_authorization.cpp	2014-07-10 03:45:37 +0000
+++ tests/acceptance-tests/test_client_authorization.cpp	2014-08-26 13:48:43 +0000
@@ -23,6 +23,7 @@
 
 #include "mir_test/fake_shared.h"
 #include "mir_test_framework/display_server_test_fixture.h"
+#include "mir_test_doubles/stub_session_authorizer.h"
 
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
@@ -36,25 +37,11 @@
 namespace mf = mir::frontend;
 namespace mt = mir::test;
 namespace mtf = mir_test_framework;
-
-namespace
-{
-    char const* const mir_test_socket = mtf::test_socket_file().c_str();
-}
-
-namespace
-{
-struct ConnectingClient : TestingClientConfiguration
-{
-    MirConnection *connection;
-    void exec()
-    {
-        connection = mir_connect_sync(
-            mir_test_socket,
-            __PRETTY_FUNCTION__);
-        mir_connection_release(connection);
-    }
-};
+namespace mtd = mir::test::doubles;
+
+namespace
+{
+char const* const mir_test_socket = mtf::test_socket_file().c_str();
 
 struct ClientCredsTestFixture : BespokeDisplayServerTestFixture
 {
@@ -144,6 +131,10 @@
                 screencast_is_allowed(Truly(matches_creds)))
                 .Times(1)
                 .WillOnce(Return(false));
+            EXPECT_CALL(mock_authorizer,
+                prompt_session_is_allowed(Truly(matches_creds)))
+                .Times(1)
+                .WillOnce(Return(false));
             connect_sync.try_signal_ready_for();
         }
 
@@ -159,7 +150,7 @@
     launch_server_process(server_config);
 
 
-    struct ClientConfiguration : ConnectingClient
+    struct ClientConfiguration : TestingClientConfiguration
     {
         ClientConfiguration(ClientCredsTestFixture::SharedRegion *shared_region,
                             mtf::CrossProcessSync const& connect_sync)
@@ -172,7 +163,9 @@
         {
             shared_region->post_client_creds();
             connect_sync.wait_for_signal_ready_for();
-            ConnectingClient::exec();
+            auto const connection = mir_connect_sync(mir_test_socket, __PRETTY_FUNCTION__);
+            EXPECT_TRUE(mir_connection_is_valid(connection));
+            mir_connection_release(connection);
         }
 
         ClientCredsTestFixture::SharedRegion* shared_region;
@@ -181,83 +174,38 @@
     launch_client_process(client_config);
 }
 
-// TODO this test exposes a race condition when the client processes both an error
-// TODO from connect and the socket being dropped. RAOF is doing some rework that
-// TODO should fix this a side effect. It should be re-enabled when that is done.
-TEST_F(ClientCredsTestFixture, DISABLED_authorizer_may_prevent_connection_of_clients)
+// This test is also a regression test for https://bugs.launchpad.net/mir/+bug/1358191
+TEST_F(ClientCredsTestFixture, authorizer_may_prevent_connection_of_clients)
 {
     using namespace ::testing;
 
     struct ServerConfiguration : TestingServerConfiguration
     {
-        ServerConfiguration(ClientCredsTestFixture::SharedRegion *shared_region)
-            : shared_region(shared_region)
-        {
-        }
-
-        void exec() override
-        {
-            using namespace ::testing;
-
-            EXPECT_TRUE(shared_region->wait_for_client_creds());
-            auto matches_creds = [&](mf::SessionCredentials const& creds)
-            {
-                return shared_region->matches_client_process_creds(creds);
-            };
-
-            EXPECT_CALL(mock_authorizer,
-                connection_is_allowed(Truly(matches_creds)))
-                .Times(1)
-                .WillOnce(Return(false));
-        }
-
         std::shared_ptr<mf::SessionAuthorizer> the_session_authorizer() override
         {
-            return mt::fake_shared(mock_authorizer);
+            struct StubAuthorizer : mtd::StubSessionAuthorizer
+            {
+                bool connection_is_allowed(mir::frontend::SessionCredentials const&) override
+                {
+                    return false;
+                }
+            };
+
+            return std::make_shared<StubAuthorizer>();
         }
+    } server_config;
 
-        ClientCredsTestFixture::SharedRegion* shared_region;
-        MockSessionAuthorizer mock_authorizer;
-    } server_config(shared_region);
     launch_server_process(server_config);
 
-
-    struct ClientConfiguration : ConnectingClient
+    struct ClientConfiguration : TestingClientConfiguration
     {
-        ClientConfiguration(ClientCredsTestFixture::SharedRegion *shared_region)
-            : shared_region(shared_region)
-        {
-        }
-
         void exec() override
         {
-            shared_region->post_client_creds();
-
-            connection = mir_connect_sync(
-                mir_test_socket,
-                __PRETTY_FUNCTION__);
-
-            MirSurfaceParameters const parameters =
-            {
-                __PRETTY_FUNCTION__,
-                1, 1,
-                mir_pixel_format_abgr_8888,
-                mir_buffer_usage_hardware,
-                mir_display_output_id_invalid
-            };
-            mir_connection_create_surface_sync(connection, &parameters);
-            EXPECT_GT(strlen(mir_connection_get_error_message(connection)), static_cast<unsigned int>(0));
+            auto const connection = mir_connect_sync(mir_test_socket, __PRETTY_FUNCTION__);
+            EXPECT_FALSE(mir_connection_is_valid(connection));
             mir_connection_release(connection);
         }
-
-        //we are testing the connect function itself, without getting to the
-        // point where drivers are used, so force using production config
-        bool use_real_graphics(mir::options::Option const&) override
-        {
-            return true;
-        }
-
-        ClientCredsTestFixture::SharedRegion* shared_region;
-    } client_config(shared_region);
+    } client_config;
+
     launch_client_process(client_config);
 }