From b48d1713b2309cb8c0afaed1aef3d5324dafb000 Mon Sep 17 00:00:00 2001 From: Rafal Walczyna Date: Tue, 25 Aug 2020 09:29:53 +0200 Subject: [PATCH] [MediaController] Add locks for variable used in different threads Handles for client and server can be used in different threads. It may cause segmentation fault when already freed handle will be used. Verification: Build successful Change-Id: Iba90d1a85be51710ccda169389ce393f5ce1d1a3 Signed-off-by: Rafal Walczyna --- src/mediacontroller/mediacontroller_client.cc | 24 ++++++++++++++++++++---- src/mediacontroller/mediacontroller_client.h | 6 ++++++ src/mediacontroller/mediacontroller_server.cc | 10 ++++++++-- src/mediacontroller/mediacontroller_server.h | 6 ++++++ 4 files changed, 40 insertions(+), 6 deletions(-) diff --git a/src/mediacontroller/mediacontroller_client.cc b/src/mediacontroller/mediacontroller_client.cc index 1cdcfef..44b6e0e 100644 --- a/src/mediacontroller/mediacontroller_client.cc +++ b/src/mediacontroller/mediacontroller_client.cc @@ -98,8 +98,12 @@ MediaControllerClient::~MediaControllerClient() { LoggerE("Failed to unset custom event listener"); } - if (nullptr != handle_ && MEDIA_CONTROLLER_ERROR_NONE != mc_client_destroy(handle_)) { - LoggerE("Unable to destroy media controller client"); + { + std::lock_guard lock(handle_mutex_); + if (nullptr != handle_ && MEDIA_CONTROLLER_ERROR_NONE != mc_client_destroy(handle_)) { + LoggerE("Unable to destroy media controller client"); + } + handle_ = nullptr; } } @@ -183,7 +187,10 @@ PlatformResult MediaControllerClient::FindServers(picojson::array* servers) { ScopeLogger(); int ret; - ret = mc_client_foreach_server(handle_, FindServersCallback, servers); + { + std::lock_guard lock(handle_mutex_); + ret = mc_client_foreach_server(handle_, FindServersCallback, servers); + } if (MEDIA_CONTROLLER_ERROR_NONE != ret) { return LogAndCreateResult( ErrorCode::UNKNOWN_ERR, "Unable to fetch active servers, error", @@ -241,7 +248,10 @@ PlatformResult MediaControllerClient::GetLatestServerInfo(picojson::value* serve free(name); }; mc_server_state_e state; - ret = mc_client_get_latest_server_info(handle_, &name, &state); + { + std::lock_guard lock(handle_mutex_); + ret = mc_client_get_latest_server_info(handle_, &name, &state); + } if (MEDIA_CONTROLLER_ERROR_NONE != ret) { return LogAndCreateResult( ErrorCode::UNKNOWN_ERR, "Error getting latest server info", @@ -857,6 +867,7 @@ PlatformResult MediaControllerClient::FindSubscribedServers(picojson::array* ser * If subscription is successful, then servers are subscribed to every subscription type, * so we can check only the one type of subscription to receive all subscribed servers. */ + std::lock_guard lock(handle_mutex_); int ret = mc_client_foreach_server_subscribed(handle_, MC_SUBSCRIPTION_TYPE_ABILITY_SUPPORT, FindServersCallback, servers); if (MEDIA_CONTROLLER_ERROR_NONE != ret) { @@ -1216,6 +1227,7 @@ PlatformResult MediaControllerClient::SendPlaybackState(const std::string& serve SCOPE_EXIT { free(request_id); };*/ + std::lock_guard lock(handle_mutex_); int ret = mc_client_send_playback_action_cmd(handle_, server_name.c_str(), action_e, nullptr); if (MEDIA_CONTROLLER_ERROR_NONE != ret) { return LogAndCreateResult(ErrorCode::UNKNOWN_ERR, "Error sending playback state", @@ -1234,6 +1246,7 @@ PlatformResult MediaControllerClient::SendPlaybackPosition(const std::string& se SCOPE_EXIT { free(request_id); };*/ + std::lock_guard lock(handle_mutex_); int ret = mc_client_send_playback_position_cmd(handle_, server_name.c_str(), static_cast(position), /*&request_id*/ nullptr); @@ -1253,6 +1266,7 @@ PlatformResult MediaControllerClient::SendShuffleMode(const std::string& server_ SCOPE_EXIT { free(request_id); };*/ + std::lock_guard lock(handle_mutex_); int ret = mc_client_send_shuffle_mode_cmd(handle_, server_name.c_str(), mode ? MC_SHUFFLE_MODE_ON : MC_SHUFFLE_MODE_OFF, /*&request_id*/ nullptr); @@ -1272,6 +1286,7 @@ PlatformResult MediaControllerClient::SendRepeatMode(const std::string& server_n SCOPE_EXIT { free(request_id); };*/ + std::lock_guard lock(handle_mutex_); int ret = mc_client_send_repeat_mode_cmd(handle_, server_name.c_str(), mode ? MC_REPEAT_MODE_ON : MC_REPEAT_MODE_OFF, /*&request_id*/ nullptr); @@ -1298,6 +1313,7 @@ PlatformResult MediaControllerClient::SendRepeatState(const std::string& server_ return result; } + std::lock_guard lock(handle_mutex_); int ret = mc_client_send_repeat_mode_cmd(handle_, server_name.c_str(), state_e, nullptr); if (MEDIA_CONTROLLER_ERROR_NONE != ret) { return LogAndCreateResult( diff --git a/src/mediacontroller/mediacontroller_client.h b/src/mediacontroller/mediacontroller_client.h index e4b255b..59e2370 100644 --- a/src/mediacontroller/mediacontroller_client.h +++ b/src/mediacontroller/mediacontroller_client.h @@ -19,6 +19,7 @@ #include #include +#include #include #include "common/optional.h" @@ -114,6 +115,11 @@ class MediaControllerClient { private: mc_client_h handle_; + // This mutex is used to guard handle_ usage in TaskQueue + // and its destruction, but there is no need to + // guard every call on main thread. + std::mutex handle_mutex_; + JsonCallback playback_info_listener_; JsonCallback server_status_listener_; JsonCallback command_reply_callback_; diff --git a/src/mediacontroller/mediacontroller_server.cc b/src/mediacontroller/mediacontroller_server.cc index 2e9b1c6..71d61f3 100644 --- a/src/mediacontroller/mediacontroller_server.cc +++ b/src/mediacontroller/mediacontroller_server.cc @@ -99,8 +99,12 @@ MediaControllerServer::~MediaControllerServer() { } } - if (nullptr != handle_ && MEDIA_CONTROLLER_ERROR_NONE != mc_server_destroy(handle_)) { - LoggerE("Unable to destroy media controller server"); + { + std::lock_guard lock(handle_mutex_); + if (nullptr != handle_ && MEDIA_CONTROLLER_ERROR_NONE != mc_server_destroy(handle_)) { + LoggerE("Unable to destroy media controller server"); + } + handle_ = nullptr; } } @@ -564,6 +568,7 @@ PlatformResult MediaControllerServer::SavePlaylist(const std::string& name) { "Playlist with given name doesn't exist"); } + std::lock_guard lock(handle_mutex_); int ret = mc_server_update_playlist_done(handle_, playlist_handle_map_[name]); if (MEDIA_CONTROLLER_ERROR_NONE != ret) { @@ -583,6 +588,7 @@ PlatformResult MediaControllerServer::DeletePlaylist(const std::string& name) { "Playlist with given name doesn't exist"); } + std::lock_guard lock(handle_mutex_); int ret = mc_server_delete_playlist(handle_, playlist_handle_map_[name]); if (MEDIA_CONTROLLER_ERROR_NONE != ret) { diff --git a/src/mediacontroller/mediacontroller_server.h b/src/mediacontroller/mediacontroller_server.h index 421847e..a9af304 100644 --- a/src/mediacontroller/mediacontroller_server.h +++ b/src/mediacontroller/mediacontroller_server.h @@ -18,6 +18,7 @@ #define MEDIACONTROLLER_MEDIACONTROLLER_SERVER_H_ #include +#include #include #include "common/platform_result.h" @@ -99,6 +100,11 @@ class MediaControllerServer { private: mc_server_h handle_; + // This mutex is used to guard handle_ usage in TaskQueue + // and its destruction, but there is no need to + // guard every call on main thread. + std::mutex handle_mutex_; + JsonCallback change_request_playback_info_listener_; JsonCallback event_reply_callback_; JsonCallback command_listener_; -- 2.7.4