From c6189d7457f4e953de9293e0a2971a1268f6a50f Mon Sep 17 00:00:00 2001 From: Tomasz Marciniak Date: Mon, 28 Sep 2015 13:38:02 +0200 Subject: [PATCH] [NetworkBearerSelection] Fix for potential memory leaks. [Verification] Code compiles. TCT pass rate 100%. Change-Id: Idba09da6bcf7a69e959881bb35ea963e9f2b2bdb Signed-off-by: Tomasz Marciniak --- .../networkbearerselection_manager.cc | 261 ++++++++++-------- .../networkbearerselection_manager.h | 32 ++- 2 files changed, 174 insertions(+), 119 deletions(-) diff --git a/src/networkbearerselection/networkbearerselection_manager.cc b/src/networkbearerselection/networkbearerselection_manager.cc index b6675ff4..1abbbf3a 100644 --- a/src/networkbearerselection/networkbearerselection_manager.cc +++ b/src/networkbearerselection/networkbearerselection_manager.cc @@ -16,6 +16,7 @@ #include "networkbearerselection_manager.h" #include "common/logger.h" +#include "common/scope_exit.h" #include #include @@ -45,15 +46,15 @@ struct NetworkBearerSelectionReleaseEvent { void NetworkBearerSelectionManager::AddListener( NetworkBearerSelectionListener* listener) { LoggerD("Enter"); - std::lock_guard lock(m_mutex); - m_listeners.push_back(listener); + std::lock_guard lock(m_mutex_); + m_listeners_.push_back(listener); } void NetworkBearerSelectionManager::RemoveListener( NetworkBearerSelectionListener* listener) { LoggerD("Enter"); - std::lock_guard lock(m_mutex); - m_listeners.remove(listener); + std::lock_guard lock(m_mutex_); + m_listeners_.remove(listener); } NetworkBearerSelectionManager* NetworkBearerSelectionManager::GetInstance() { @@ -63,33 +64,42 @@ NetworkBearerSelectionManager* NetworkBearerSelectionManager::GetInstance() { } NetworkBearerSelectionManager::NetworkBearerSelectionManager() - : m_connectionHandle(nullptr), - m_profileHandle(nullptr), - m_connectionState(ConnectionState::Unknown), - m_isConnectionOpen(false) { + : m_connection_handle_(nullptr), + m_profile_handle_(nullptr), + m_connection_state_(ConnectionState::Unknown), + m_is_connection_open_(false) { LoggerD("Enter"); - int ret = connection_create(&m_connectionHandle); + int ret = connection_create(&m_connection_handle_); if (CONNECTION_ERROR_NONE == ret) { LoggerD("Client registration success"); } else { LoggerE("Client registration failed"); - m_connectionHandle = nullptr; + m_connection_handle_ = nullptr; } } NetworkBearerSelectionManager::~NetworkBearerSelectionManager() { LoggerD("Enter"); - if (m_connectionHandle != nullptr) { + if (m_connection_handle_ != nullptr) { LoggerD("Client deregistration success"); - if (m_profileHandle) { - connection_profile_destroy(m_profileHandle); - m_profileHandle = nullptr; - } - connection_destroy(m_connectionHandle); + destroyProfileHandle(); + connection_destroy(m_connection_handle_); } else { LoggerE("Client deregistration failed"); } + + { + std::lock_guard lock(m_request_mutex_); + LoggerD("Delete %d request object(s)", m_request_events_.size()); + m_request_events_.clear(); + } + + { + std::lock_guard lock(m_release_mutex_); + LoggerD("Delete %d release object(s)", m_release_events_.size()); + m_release_events_.clear(); + } } void NetworkBearerSelectionManager::connection_state_changed_callback( @@ -102,13 +112,17 @@ void NetworkBearerSelectionManager::connection_state_changed_callback( LoggerD("association state"); return; } - NetworkBearerSelectionRequestEvent* event = - static_cast(user_data); + + RequestEventPtr event = NetworkBearerSelectionManager::GetInstance()->getRequestEvent( + static_cast(user_data)); + if (!event) { + LoggerD("Event is not found."); + return; + } if (state == CONNECTION_PROFILE_STATE_DISCONNECTED) { std::string domain_name = event->domain_name; NetworkBearerSelectionManager::GetInstance()->deregistStateChangeListener(domain_name); - delete event; NetworkBearerSelectionManager::GetInstance()->makeDisconnectCallback(domain_name); } @@ -124,10 +138,14 @@ void NetworkBearerSelectionManager::connection_profile_opened_callback( return; } - NetworkBearerSelectionRequestEvent* event = - static_cast(user_data); + RequestEventPtr event = NetworkBearerSelectionManager::GetInstance()->getRequestEvent( + static_cast(user_data)); + if (!event) { + LoggerD("Event is not found."); + return; + } + std::string domain_name = event->domain_name; - delete event; if (result == CONNECTION_ERROR_NONE) { LoggerD("Connection open Succeeded"); @@ -151,11 +169,15 @@ void NetworkBearerSelectionManager::connection_closed_callback( return; } - NetworkBearerSelectionReleaseEvent* event = - static_cast(user_data); + ReleaseEventPtr event = NetworkBearerSelectionManager::GetInstance()->getReleaseEvent( + static_cast(user_data)); + if (!event) { + LoggerD("Event is not found."); + return; + } + std::string domain_name = event->domain_name; ReleaseReplyCallback callback = event->callback; - delete event; if (result == CONNECTION_ERROR_NONE) { LoggerD("Connection close Succeeded"); @@ -169,24 +191,15 @@ void NetworkBearerSelectionManager::connection_closed_callback( } } -void NetworkBearerSelectionManager::connection_closed_callback2( - connection_error_e result, - void* user_data) { - LoggerD("enter"); - if (result == CONNECTION_ERROR_NONE) { - LoggerD("Connection close Succeeded"); - } -} - void NetworkBearerSelectionManager::requestRouteToHost( const std::string& domain_name) { LoggerD("NetworkBearerSelectionManager::requestRouteToHost"); connection_profile_h profileHandle; - if (m_connectionState == ConnectionState::Connected) { + if (m_connection_state_ == ConnectionState::Connected) { LoggerD("connection is already opened."); - for (std::list::iterator it = m_domainNames.begin(); - it != m_domainNames.end(); + for (std::list::iterator it = m_domain_names_.begin(); + it != m_domain_names_.end(); it++) { if (*it == domain_name) { LoggerD("Same domain name is exist in list."); @@ -196,15 +209,12 @@ void NetworkBearerSelectionManager::requestRouteToHost( } } - if (m_profileHandle) { - connection_profile_destroy(m_profileHandle); - m_profileHandle = nullptr; - } + destroyProfileHandle(); if (connection_get_default_cellular_service_profile( - m_connectionHandle, + m_connection_handle_, CONNECTION_CELLULAR_SERVICE_TYPE_INTERNET, - &m_profileHandle) != CONNECTION_ERROR_NONE) { + &m_profile_handle_) != CONNECTION_ERROR_NONE) { LoggerE("Fail to get profile handle"); makeErrorCallback(domain_name, kPlatformError); return; @@ -213,7 +223,7 @@ void NetworkBearerSelectionManager::requestRouteToHost( char* defaultProfileName_c = nullptr; std::string defaultProfileName; - connection_profile_get_name(m_profileHandle, &defaultProfileName_c); + connection_profile_get_name(m_profile_handle_, &defaultProfileName_c); if (defaultProfileName_c == nullptr) { LoggerE("default profile is not exist."); makeErrorCallback(domain_name, kPlatformError); @@ -223,7 +233,7 @@ void NetworkBearerSelectionManager::requestRouteToHost( free(defaultProfileName_c); defaultProfileName_c = nullptr; - if (connection_get_current_profile(m_connectionHandle, &profileHandle) != + if (connection_get_current_profile(m_connection_handle_, &profileHandle) != CONNECTION_ERROR_NONE) { LoggerE("Fail to get current profile handle"); makeErrorCallback(domain_name, kPlatformError); @@ -249,17 +259,19 @@ void NetworkBearerSelectionManager::requestRouteToHost( currentProfileName_c = nullptr; if (defaultProfileName != currentProfileName) { - NetworkBearerSelectionRequestEvent* event = - new NetworkBearerSelectionRequestEvent(domain_name); - if (connection_open_profile(m_connectionHandle, - m_profileHandle, + RequestEventPtr event(new NetworkBearerSelectionRequestEvent(domain_name)); + + if (connection_open_profile(m_connection_handle_, + m_profile_handle_, connection_profile_opened_callback, - event) != CONNECTION_ERROR_NONE) { + event.get()) != CONNECTION_ERROR_NONE) { LoggerE("Connection open Failed"); - delete event; makeErrorCallback(domain_name, kPlatformError); } else { - m_isConnectionOpen = true; + m_is_connection_open_ = true; + + std::lock_guard lock(m_request_mutex_); + m_request_events_.push_back(event); } } else { registStateChangeListener(domain_name); @@ -270,39 +282,38 @@ common::PlatformResult NetworkBearerSelectionManager::releaseRouteToHost( const std::string& domain_name, const ReleaseReplyCallback& reply_cb) { LoggerD("enter"); - for (const auto& name : m_domainNames) { + for (const auto& name : m_domain_names_) { if (name == domain_name) { LoggerD("Same domain name is exist in list."); - m_domainNames.remove(domain_name); - LoggerD("list size : %i", m_domainNames.size()); - if (m_domainNames.size() == 0) { - if (!m_profileHandle) { + m_domain_names_.remove(domain_name); + LoggerD("list size : %i", m_domain_names_.size()); + if (m_domain_names_.size() == 0) { + if (!m_profile_handle_) { return common::PlatformResult(common::ErrorCode::UNKNOWN_ERR, "Already in use"); } - if (connection_profile_unset_state_changed_cb(m_profileHandle) != + if (connection_profile_unset_state_changed_cb(m_profile_handle_) != CONNECTION_ERROR_NONE) { LoggerE("unset callback is failed"); - if (m_profileHandle) { - connection_profile_destroy(m_profileHandle); - m_profileHandle = NULL; - } + destroyProfileHandle(); return common::PlatformResult(common::ErrorCode::NO_ERROR); } - if (m_isConnectionOpen) { - NetworkBearerSelectionReleaseEvent* event = - new NetworkBearerSelectionReleaseEvent(domain_name, reply_cb); - if (connection_close_profile(m_connectionHandle, - m_profileHandle, + if (m_is_connection_open_) { + ReleaseEventPtr event(new NetworkBearerSelectionReleaseEvent(domain_name, reply_cb)); + + if (connection_close_profile(m_connection_handle_, + m_profile_handle_, connection_closed_callback, - event) != CONNECTION_ERROR_NONE) { + event.get()) != CONNECTION_ERROR_NONE) { LoggerE("connection close failed"); - delete event; reply_cb(false); } else { - m_isConnectionOpen = false; + m_is_connection_open_ = false; deregistStateChangeListener(domain_name); + + std::lock_guard lock(m_release_mutex_); + m_release_events_.push_back(event); } } else { reply_cb(true); @@ -323,13 +334,17 @@ void NetworkBearerSelectionManager::registStateChangeListener( std::unique_ptr host_addr_ptr(nullptr, &std::free); struct addrinfo* servinfo = nullptr; + SCOPE_EXIT { + if (interfaceName) { + free(interfaceName); + } + freeaddrinfo(servinfo); + }; + if (connection_profile_get_network_interface_name( - m_profileHandle, &interfaceName) != CONNECTION_ERROR_NONE) { + m_profile_handle_, &interfaceName) != CONNECTION_ERROR_NONE) { LoggerE("Fail to get interface name!"); - if (m_profileHandle) { - connection_profile_destroy(m_profileHandle); - m_profileHandle = nullptr; - } + destroyProfileHandle(); makeErrorCallback(domain_name, kPlatformError); } else { LoggerD("Interface name : %s", interfaceName); @@ -340,10 +355,7 @@ void NetworkBearerSelectionManager::registStateChangeListener( int ret_val = getaddrinfo(domain_name.c_str() , nullptr , nullptr , &servinfo); if (0 != ret_val) { LoggerE("Error while calling getaddrinfo(): %s", gai_strerror(ret_val)); - if (m_profileHandle) { - connection_profile_destroy(m_profileHandle); - m_profileHandle = nullptr; - } + destroyProfileHandle(); makeErrorCallback(domain_name, kPlatformError); return; } else { @@ -360,61 +372,54 @@ void NetworkBearerSelectionManager::registStateChangeListener( } if (nullptr == inet_ntop(servinfo->ai_family, addr, hostAddr, servinfo->ai_addrlen)) { LoggerE("Error while calling inet_ntop()"); - if (m_profileHandle) { - connection_profile_destroy(m_profileHandle); - m_profileHandle = nullptr; - } + destroyProfileHandle(); makeErrorCallback(domain_name, kPlatformError); - freeaddrinfo(servinfo); return; } LoggerD("hostAddr : %s", hostAddr); - - freeaddrinfo(servinfo); } - NetworkBearerSelectionRequestEvent* event = - new NetworkBearerSelectionRequestEvent(domain_name); - if (connection_profile_set_state_changed_cb(m_profileHandle, + RequestEventPtr event(new NetworkBearerSelectionRequestEvent(domain_name)); + + if (connection_profile_set_state_changed_cb(m_profile_handle_, connection_state_changed_callback, - event) != CONNECTION_ERROR_NONE) { + event.get()) != CONNECTION_ERROR_NONE) { LoggerE("Callback register is failed."); - if (m_profileHandle) { - connection_profile_destroy(m_profileHandle); - m_profileHandle = nullptr; - } - delete event; + destroyProfileHandle(); } else { - if (connection_add_route(m_connectionHandle, interfaceName, hostAddr) != + if (connection_add_route(m_connection_handle_, interfaceName, hostAddr) != CONNECTION_ERROR_NONE) { LoggerE("add route is failed."); - connection_profile_unset_state_changed_cb(m_profileHandle); + connection_profile_unset_state_changed_cb(m_profile_handle_); makeErrorCallback(domain_name, kPlatformError); } else { LoggerD("add route is successed."); - m_domainNames.push_back(domain_name); + m_domain_names_.push_back(domain_name); makeSuccessCallback(domain_name); } + + std::lock_guard lock(m_request_mutex_); + m_request_events_.push_back(event); } } void NetworkBearerSelectionManager::deregistStateChangeListener( const std::string& domain_name) { LoggerD("enter"); - if (m_profileHandle) { - connection_profile_unset_state_changed_cb(m_profileHandle); - connection_profile_destroy(m_profileHandle); - m_profileHandle = NULL; + if (m_profile_handle_) { + connection_profile_unset_state_changed_cb(m_profile_handle_); + connection_profile_destroy(m_profile_handle_); + m_profile_handle_ = nullptr; } - m_domainNames.remove(domain_name); - m_connectionState = ConnectionState::Disconnected; + m_domain_names_.remove(domain_name); + m_connection_state_ = ConnectionState::Disconnected; } void NetworkBearerSelectionManager::makeSuccessCallback( const std::string& domain_name) { LoggerD("enter"); - std::lock_guard lock(m_mutex); - for (NetworkBearerSelectionListener* listener : m_listeners) + std::lock_guard lock(m_mutex_); + for (NetworkBearerSelectionListener* listener : m_listeners_) listener->onNBSSuccess(domain_name); } @@ -430,18 +435,54 @@ void NetworkBearerSelectionManager::makeErrorCallback( const std::string& domain_name, const std::string& info) { LoggerD("enter"); - std::lock_guard lock(m_mutex); - for (NetworkBearerSelectionListener* listener : m_listeners) + std::lock_guard lock(m_mutex_); + for (NetworkBearerSelectionListener* listener : m_listeners_) listener->onNBSError(domain_name, info); } void NetworkBearerSelectionManager::makeDisconnectCallback( const std::string& domain_name) { LoggerD("enter"); - std::lock_guard lock(m_mutex); - for (NetworkBearerSelectionListener* listener : m_listeners) + std::lock_guard lock(m_mutex_); + for (NetworkBearerSelectionListener* listener : m_listeners_) listener->onNBSDisconnect(domain_name); } +void NetworkBearerSelectionManager::destroyProfileHandle() { + LoggerD("enter"); + if (m_profile_handle_) { + connection_profile_destroy(m_profile_handle_); + m_profile_handle_ = nullptr; + } +} + +RequestEventPtr NetworkBearerSelectionManager::getRequestEvent(NetworkBearerSelectionRequestEvent* event) { + LoggerD("enter"); + std::lock_guard lock(m_request_mutex_); + for (auto it = m_request_events_.begin(); it != m_request_events_.end(); it++) { + if (it->get() == event) { + LoggerD("Found object [%p]", it->get()); + RequestEventPtr ev = *it; + m_request_events_.erase(it); + return ev; + } + } + return nullptr; +} + +ReleaseEventPtr NetworkBearerSelectionManager::getReleaseEvent(NetworkBearerSelectionReleaseEvent* event) { + LoggerD("enter"); + std::lock_guard lock(m_release_mutex_); + for (auto it = m_release_events_.begin(); it != m_release_events_.end(); it++) { + if (it->get() == event) { + LoggerD("Found object [%p]", it->get()); + ReleaseEventPtr ev = *it; + m_release_events_.erase(it); + return ev; + } + } + return nullptr; +} + } // namespace networkbearerselection } // namespace extension diff --git a/src/networkbearerselection/networkbearerselection_manager.h b/src/networkbearerselection/networkbearerselection_manager.h index b5e1c1eb..baddec50 100644 --- a/src/networkbearerselection/networkbearerselection_manager.h +++ b/src/networkbearerselection/networkbearerselection_manager.h @@ -19,7 +19,9 @@ #include #include +#include #include +#include #include #include #include @@ -41,6 +43,12 @@ enum class NetworkType { Unknown }; +struct NetworkBearerSelectionRequestEvent; +struct NetworkBearerSelectionReleaseEvent; + +typedef std::shared_ptr RequestEventPtr; +typedef std::shared_ptr ReleaseEventPtr; + class NetworkBearerSelectionListener { public: virtual void onNBSSuccess(const std::string& domain_name) = 0; @@ -73,8 +81,6 @@ class NetworkBearerSelectionManager { void* user_data); static void connection_closed_callback(connection_error_e result, void* user_data); - static void connection_closed_callback2(connection_error_e result, - void* user_data); void registStateChangeListener(const std::string& domain_name); void deregistStateChangeListener(const std::string& domain_name); @@ -84,18 +90,26 @@ class NetworkBearerSelectionManager { void makeErrorCallback(const std::string& domain_name, const std::string& info); void makeDisconnectCallback(const std::string& domain_name); + void destroyProfileHandle(); + RequestEventPtr getRequestEvent(NetworkBearerSelectionRequestEvent* event); + ReleaseEventPtr getReleaseEvent(NetworkBearerSelectionReleaseEvent* event); NetworkBearerSelectionManager(); ~NetworkBearerSelectionManager(); - std::list m_listeners; + std::list m_listeners_; + + connection_h m_connection_handle_; + connection_profile_h m_profile_handle_; + std::list m_domain_names_; + std::vector m_request_events_; + std::vector m_release_events_; + ConnectionState m_connection_state_; + bool m_is_connection_open_; - connection_h m_connectionHandle; - connection_profile_h m_profileHandle; - std::list m_domainNames; - ConnectionState m_connectionState; - bool m_isConnectionOpen; - std::mutex m_mutex; + std::mutex m_mutex_; + std::mutex m_request_mutex_; + std::mutex m_release_mutex_; }; } // namespace networkbearerselection -- 2.34.1