Mir

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

Proposed by Alexandros Frantzis
Status: Merged
Approved by: Daniel van Vugt
Approved revision: no longer in the source branch.
Merged at revision: 1868
Proposed branch: lp:~afrantzis/mir/fix-1358191-connect-segv
Merge into: lp:mir
Diff against target: 344 lines (+57/-125)
6 files modified
src/client/connection_configuration.h (+1/-2)
src/client/default_connection_configuration.cpp (+14/-20)
src/client/default_connection_configuration.h (+4/-0)
src/client/mir_connection.cpp (+1/-20)
src/client/mir_connection.h (+6/-0)
tests/acceptance-tests/test_client_authorization.cpp (+31/-83)
To merge this branch: bzr merge lp:~afrantzis/mir/fix-1358191-connect-segv
Reviewer Review Type Date Requested Status
Alberto Aguirre (community) Approve
Kevin DuBois (community) Approve
Alan Griffiths Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+232218@code.launchpad.net

Commit message

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.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

LGTM

review: Approve
Revision history for this message
Kevin DuBois (kdub) wrote :

+1

review: Approve
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

OK.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
4343
44namespace client44namespace client
45{45{
46std::shared_ptr<SharedLibrary>& libraries_cache(std::string const& lib_name);
47
48class ConnectionSurfaceMap;46class ConnectionSurfaceMap;
49class Logger;47class Logger;
50class ClientPlatformFactory;48class ClientPlatformFactory;
@@ -67,6 +65,7 @@
67 virtual std::shared_ptr<LifecycleControl> the_lifecycle_control() = 0;65 virtual std::shared_ptr<LifecycleControl> the_lifecycle_control() = 0;
68 virtual std::shared_ptr<EventSink> the_event_sink() = 0;66 virtual std::shared_ptr<EventSink> the_event_sink() = 0;
69 virtual std::shared_ptr<EventHandlerRegister> the_event_handler_register() = 0;67 virtual std::shared_ptr<EventHandlerRegister> the_event_handler_register() = 0;
68 virtual std::shared_ptr<SharedLibrary> the_platform_library() = 0;
7069
71protected:70protected:
72 ConnectionConfiguration() = default;71 ConnectionConfiguration() = default;
7372
=== 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 @@
42std::string const log_opt_val{"log"};42std::string const log_opt_val{"log"};
43std::string const lttng_opt_val{"lttng"};43std::string const lttng_opt_val{"lttng"};
44std::string const default_platform_lib{"libmirclientplatform.so"};44std::string const default_platform_lib{"libmirclientplatform.so"};
45
46mir::SharedLibrary const* load_library(std::string const& libname)
47{
48
49 if (auto& ptr = mcl::libraries_cache(libname))
50 {
51 return ptr.get();
52 }
53 else
54 {
55 ptr = std::make_shared<mir::SharedLibrary>(libname);
56 return ptr.get();
57 }
58}
59}45}
6046
61mcl::DefaultConnectionConfiguration::DefaultConnectionConfiguration(47mcl::DefaultConnectionConfiguration::DefaultConnectionConfiguration(
@@ -98,14 +84,10 @@
98mcl::DefaultConnectionConfiguration::the_client_platform_factory()84mcl::DefaultConnectionConfiguration::the_client_platform_factory()
99{85{
100 return client_platform_factory(86 return client_platform_factory(
101 []87 [this]
102 {88 {
103 auto const val_raw = getenv("MIR_CLIENT_PLATFORM_LIB");
104 std::string const val{val_raw ? val_raw : default_platform_lib};
105 auto const platform_lib = ::load_library(val);
106
107 auto const create_client_platform_factory =89 auto const create_client_platform_factory =
108 platform_lib->load_function<mcl::CreateClientPlatformFactory>(90 the_platform_library()->load_function<mcl::CreateClientPlatformFactory>(
109 "create_client_platform_factory");91 "create_client_platform_factory");
11092
111 return create_client_platform_factory();93 return create_client_platform_factory();
@@ -199,3 +181,15 @@
199 return std::make_shared<MirEventDistributor>();181 return std::make_shared<MirEventDistributor>();
200 });182 });
201}183}
184
185std::shared_ptr<mir::SharedLibrary> mcl::DefaultConnectionConfiguration::the_platform_library()
186{
187 if (!platform_library)
188 {
189 auto const val_raw = getenv("MIR_CLIENT_PLATFORM_LIB");
190 std::string const libname{val_raw ? val_raw : default_platform_lib};
191 platform_library = std::make_shared<mir::SharedLibrary>(libname);
192 }
193
194 return platform_library;
195}
202196
=== 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 @@
57 std::shared_ptr<LifecycleControl> the_lifecycle_control();57 std::shared_ptr<LifecycleControl> the_lifecycle_control();
58 std::shared_ptr<EventSink> the_event_sink();58 std::shared_ptr<EventSink> the_event_sink();
59 std::shared_ptr<EventHandlerRegister> the_event_handler_register();59 std::shared_ptr<EventHandlerRegister> the_event_handler_register();
60 std::shared_ptr<SharedLibrary> the_platform_library() override;
6061
61 virtual std::string the_socket_file();62 virtual std::string the_socket_file();
62 virtual std::shared_ptr<rpc::RpcReport> the_rpc_report();63 virtual std::shared_ptr<rpc::RpcReport> the_rpc_report();
63 virtual std::shared_ptr<input::receiver::InputReceiverReport> the_input_receiver_report();64 virtual std::shared_ptr<input::receiver::InputReceiverReport> the_input_receiver_report();
6465
65protected:66protected:
67 // MUST be first data member so it is destroyed last.
68 std::shared_ptr<mir::SharedLibrary> platform_library;
69
66 CachedPtr<google::protobuf::RpcChannel> rpc_channel;70 CachedPtr<google::protobuf::RpcChannel> rpc_channel;
67 CachedPtr<mir::logging::Logger> logger;71 CachedPtr<mir::logging::Logger> logger;
68 CachedPtr<ClientPlatformFactory> client_platform_factory;72 CachedPtr<ClientPlatformFactory> client_platform_factory;
6973
=== 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 @@
6767
68std::mutex connection_guard;68std::mutex connection_guard;
69MirConnection* valid_connections{nullptr};69MirConnection* valid_connections{nullptr};
70// There's no point in loading twice, and it isn't safe to
71// unload while there are valid connections
72std::map<std::string, std::shared_ptr<mir::SharedLibrary>>* libraries_cache_ptr{nullptr};
73}
74
75std::shared_ptr<mir::SharedLibrary>& mcl::libraries_cache(std::string const& libname)
76{
77 std::lock_guard<std::mutex> lock(connection_guard);
78
79 if (!libraries_cache_ptr)
80 libraries_cache_ptr = new LibrariesCache;
81
82 return (*libraries_cache_ptr)[libname];
83}70}
8471
85MirConnection::Deregisterer::~Deregisterer()72MirConnection::Deregisterer::~Deregisterer()
@@ -94,13 +81,6 @@
94 break;81 break;
95 }82 }
96 }83 }
97
98 // When the last valid connection goes we can clear the libraries cache
99 if (!valid_connections)
100 {
101 delete libraries_cache_ptr;
102 libraries_cache_ptr = nullptr;
103 }
104}84}
10585
106MirConnection::MirConnection(std::string const& error_message) :86MirConnection::MirConnection(std::string const& error_message) :
@@ -114,6 +94,7 @@
114MirConnection::MirConnection(94MirConnection::MirConnection(
115 mir::client::ConnectionConfiguration& conf) :95 mir::client::ConnectionConfiguration& conf) :
116 deregisterer{this},96 deregisterer{this},
97 platform_library{conf.the_platform_library()},
117 channel(conf.the_rpc_channel()),98 channel(conf.the_rpc_channel()),
118 server(channel.get(), ::google::protobuf::Service::STUB_DOESNT_OWN_CHANNEL),99 server(channel.get(), ::google::protobuf::Service::STUB_DOESNT_OWN_CHANNEL),
119 logger(conf.the_logger()),100 logger(conf.the_logger()),
120101
=== 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 @@
3737
38namespace mir38namespace mir
39{39{
40class SharedLibrary;
41
40/// The client-side library implementation namespace42/// The client-side library implementation namespace
41namespace client43namespace client
42{44{
@@ -138,6 +140,10 @@
138 struct Deregisterer140 struct Deregisterer
139 { MirConnection* const self; ~Deregisterer(); } deregisterer;141 { MirConnection* const self; ~Deregisterer(); } deregisterer;
140142
143 // MUST be placed before any variables for components that are loaded
144 // from a shared library, e.g., the ClientPlatform* objects.
145 std::shared_ptr<mir::SharedLibrary> const platform_library;
146
141 std::mutex mutex; // Protects all members of *this (except release_wait_handles)147 std::mutex mutex; // Protects all members of *this (except release_wait_handles)
142148
143 std::shared_ptr<google::protobuf::RpcChannel> const channel;149 std::shared_ptr<google::protobuf::RpcChannel> const channel;
144150
=== 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 @@
2323
24#include "mir_test/fake_shared.h"24#include "mir_test/fake_shared.h"
25#include "mir_test_framework/display_server_test_fixture.h"25#include "mir_test_framework/display_server_test_fixture.h"
26#include "mir_test_doubles/stub_session_authorizer.h"
2627
27#include <gtest/gtest.h>28#include <gtest/gtest.h>
28#include <gmock/gmock.h>29#include <gmock/gmock.h>
@@ -36,25 +37,11 @@
36namespace mf = mir::frontend;37namespace mf = mir::frontend;
37namespace mt = mir::test;38namespace mt = mir::test;
38namespace mtf = mir_test_framework;39namespace mtf = mir_test_framework;
3940namespace mtd = mir::test::doubles;
40namespace41
41{42namespace
42 char const* const mir_test_socket = mtf::test_socket_file().c_str();43{
43}44char const* const mir_test_socket = mtf::test_socket_file().c_str();
44
45namespace
46{
47struct ConnectingClient : TestingClientConfiguration
48{
49 MirConnection *connection;
50 void exec()
51 {
52 connection = mir_connect_sync(
53 mir_test_socket,
54 __PRETTY_FUNCTION__);
55 mir_connection_release(connection);
56 }
57};
5845
59struct ClientCredsTestFixture : BespokeDisplayServerTestFixture46struct ClientCredsTestFixture : BespokeDisplayServerTestFixture
60{47{
@@ -144,6 +131,10 @@
144 screencast_is_allowed(Truly(matches_creds)))131 screencast_is_allowed(Truly(matches_creds)))
145 .Times(1)132 .Times(1)
146 .WillOnce(Return(false));133 .WillOnce(Return(false));
134 EXPECT_CALL(mock_authorizer,
135 prompt_session_is_allowed(Truly(matches_creds)))
136 .Times(1)
137 .WillOnce(Return(false));
147 connect_sync.try_signal_ready_for();138 connect_sync.try_signal_ready_for();
148 }139 }
149140
@@ -159,7 +150,7 @@
159 launch_server_process(server_config);150 launch_server_process(server_config);
160151
161152
162 struct ClientConfiguration : ConnectingClient153 struct ClientConfiguration : TestingClientConfiguration
163 {154 {
164 ClientConfiguration(ClientCredsTestFixture::SharedRegion *shared_region,155 ClientConfiguration(ClientCredsTestFixture::SharedRegion *shared_region,
165 mtf::CrossProcessSync const& connect_sync)156 mtf::CrossProcessSync const& connect_sync)
@@ -172,7 +163,9 @@
172 {163 {
173 shared_region->post_client_creds();164 shared_region->post_client_creds();
174 connect_sync.wait_for_signal_ready_for();165 connect_sync.wait_for_signal_ready_for();
175 ConnectingClient::exec();166 auto const connection = mir_connect_sync(mir_test_socket, __PRETTY_FUNCTION__);
167 EXPECT_TRUE(mir_connection_is_valid(connection));
168 mir_connection_release(connection);
176 }169 }
177170
178 ClientCredsTestFixture::SharedRegion* shared_region;171 ClientCredsTestFixture::SharedRegion* shared_region;
@@ -181,83 +174,38 @@
181 launch_client_process(client_config);174 launch_client_process(client_config);
182}175}
183176
184// TODO this test exposes a race condition when the client processes both an error177// This test is also a regression test for https://bugs.launchpad.net/mir/+bug/1358191
185// TODO from connect and the socket being dropped. RAOF is doing some rework that178TEST_F(ClientCredsTestFixture, authorizer_may_prevent_connection_of_clients)
186// TODO should fix this a side effect. It should be re-enabled when that is done.
187TEST_F(ClientCredsTestFixture, DISABLED_authorizer_may_prevent_connection_of_clients)
188{179{
189 using namespace ::testing;180 using namespace ::testing;
190181
191 struct ServerConfiguration : TestingServerConfiguration182 struct ServerConfiguration : TestingServerConfiguration
192 {183 {
193 ServerConfiguration(ClientCredsTestFixture::SharedRegion *shared_region)
194 : shared_region(shared_region)
195 {
196 }
197
198 void exec() override
199 {
200 using namespace ::testing;
201
202 EXPECT_TRUE(shared_region->wait_for_client_creds());
203 auto matches_creds = [&](mf::SessionCredentials const& creds)
204 {
205 return shared_region->matches_client_process_creds(creds);
206 };
207
208 EXPECT_CALL(mock_authorizer,
209 connection_is_allowed(Truly(matches_creds)))
210 .Times(1)
211 .WillOnce(Return(false));
212 }
213
214 std::shared_ptr<mf::SessionAuthorizer> the_session_authorizer() override184 std::shared_ptr<mf::SessionAuthorizer> the_session_authorizer() override
215 {185 {
216 return mt::fake_shared(mock_authorizer);186 struct StubAuthorizer : mtd::StubSessionAuthorizer
187 {
188 bool connection_is_allowed(mir::frontend::SessionCredentials const&) override
189 {
190 return false;
191 }
192 };
193
194 return std::make_shared<StubAuthorizer>();
217 }195 }
196 } server_config;
218197
219 ClientCredsTestFixture::SharedRegion* shared_region;
220 MockSessionAuthorizer mock_authorizer;
221 } server_config(shared_region);
222 launch_server_process(server_config);198 launch_server_process(server_config);
223199
224200 struct ClientConfiguration : TestingClientConfiguration
225 struct ClientConfiguration : ConnectingClient
226 {201 {
227 ClientConfiguration(ClientCredsTestFixture::SharedRegion *shared_region)
228 : shared_region(shared_region)
229 {
230 }
231
232 void exec() override202 void exec() override
233 {203 {
234 shared_region->post_client_creds();204 auto const connection = mir_connect_sync(mir_test_socket, __PRETTY_FUNCTION__);
235205 EXPECT_FALSE(mir_connection_is_valid(connection));
236 connection = mir_connect_sync(
237 mir_test_socket,
238 __PRETTY_FUNCTION__);
239
240 MirSurfaceParameters const parameters =
241 {
242 __PRETTY_FUNCTION__,
243 1, 1,
244 mir_pixel_format_abgr_8888,
245 mir_buffer_usage_hardware,
246 mir_display_output_id_invalid
247 };
248 mir_connection_create_surface_sync(connection, &parameters);
249 EXPECT_GT(strlen(mir_connection_get_error_message(connection)), static_cast<unsigned int>(0));
250 mir_connection_release(connection);206 mir_connection_release(connection);
251 }207 }
252208 } client_config;
253 //we are testing the connect function itself, without getting to the209
254 // point where drivers are used, so force using production config
255 bool use_real_graphics(mir::options::Option const&) override
256 {
257 return true;
258 }
259
260 ClientCredsTestFixture::SharedRegion* shared_region;
261 } client_config(shared_region);
262 launch_client_process(client_config);210 launch_client_process(client_config);
263}211}

Subscribers

People subscribed via source and target branches