Remove FdBrokerManager

When __run_exit_handlers() is called, singleton instances are destroyed.
If the rpc_port_proxy_h is member of singleton instance, FdBrokerManager
is destroyed before rpc_port_proxy_h is destroyed.
After this patch is applied, FdBrokerManager is removed.

+----------------------------------------------------------------------+
| Invalid read of size 4                                               |
|   at 0x5154B56: remove (list.tcc:334)                                |
|   by 0x5154B56: rpc_port::internal::FdBrokerManager::Remove          |
|        (rpc_port::internal::FdBroker) (fdbroker-internal.cc:943)     |
|   by 0x51556C5: rpc_port::internal::FdBroker::~FdBroker()            |
|        (fdbroker-internal.cc:206)                                    |
|   by 0x515761D: rpc_port::internal::Proxy::~Proxy()                  |
|        (new_allocator.h:110)                                         |
|   by 0x5158F4D: ~ProxyExt (list.tcc:70)                              |
|   by 0x5158F4D: ~ProxyExt (rpc-port.cc:50)                           |
|   by 0x5158F4D: rpc_port_proxy_destroy (rpc-port.cc:226)             |
| Address 0x92967f0 is 8 bytes inside a block of size 12 free'd        |
|   at 0x4847528: operator delete(void) (vg_replace_malloc.c:576)      |
|   by 0x5153C31: deallocate (new_allocator.h:110)                     |
|   by 0x5153C31: deallocate (alloc_traits.h:442)                      |
|   by 0x5153C31: Mput_node (stl_list.h:387)                           |
|   by 0x5153C31: Mclear (list.tcc:80)                                 |
|   by 0x5153C31: rpc_port::internal::FdBrokerManager::~FdBrokerManager|
|        () (stl_list.h:442)                                           |
|   by 0x4F1C773: run_exit_handlers (exit.c:106)                       |
|   by 0x4F1C89B: exit (exit.c:137)                                    |
|   by 0x4F052BF: (below main) (libc-start.c:323)                      |
| Block was alloc'd at                                                 |
|   at 0x4846088: operator new(unsigned int) (vg_replace_malloc.c:328) |
|   by 0x5154AE1: allocate (new_allocator.h:104)                       |
|   by 0x5154AE1: allocate (alloc_traits.h:416)                        |
|   by 0x5154AE1: Mget_node (stl_list.h:383)                           |
|   by 0x5154AE1: Mcreate_node<rpc_port::internal::FdBroker* const&>   |
|       (stl_list.h:568)                                               |
|   by 0x5154AE1: Minsert<rpc_port::internal::FdBroker* const&>        |
|       (stl_list.h:1770)                                              |
|   by 0x5154AE1: push_back (stl_list.h:1098)                          |
|   by 0x5154AE1: rpc_port::internal::FdBrokerManager::Add             |
|       (rpc_port::internal::FdBroker) (fdbroker-internal.cc:938)      |
|   by 0x5155437: rpc_port::internal::FdBroker::FdBroker(bool)         |
|       (fdbroker-internal.cc:202)                                     |
|   by 0x5158443: rpc_port::internal::Proxy::Proxy(bool)               |
|       (proxy-internal.cc:39)                                         |
|   by 0x5158DB3: rpc_port_proxy_create (rpc-port.cc:49)               |
+----------------------------------------------------------------------+

Change-Id: I5cf7902e093f66d262691b76707e5c26b69f827f
Signed-off-by: Hwankyu Jhun <h.jhun@samsung.com>

diff --git a/src/fdbroker-internal.cc b/src/fdbroker-internal.cc
index c280339..f09f7a6 100644 (file)
--- a/src/fdbroker-internal.cc
+++ b/src/fdbroker-internal.cc
@@ -213,16 +213,14 @@ GUnixFDList* FdBroker::FdList::GetRaw() {
 }
 
 FdBroker::FdBroker(bool mock) : mock_(mock) {
-  FdBrokerManager::GetInst().Add(this);
 }
 
 FdBroker::~FdBroker() {
-  FdBrokerManager::GetInst().Remove(this);
   Cancel();
 }
 
 void FdBroker::Cancel() {
-  std::lock_guard<std::recursive_mutex> lock(mutex_);
+  std::lock_guard<std::recursive_mutex> lock(GetMutex());
   LOGI("FdBroker::Cancel!");
 
   if (cancellable_) {
@@ -232,11 +230,13 @@ void FdBroker::Cancel() {
     cancellable_ = nullptr;
   }
 
-  if (conn_timer_ > 0) {
-    g_source_remove(conn_timer_);
-    conn_timer_ = 0;
+  if (res_data_) {
+    DestroyWeakPtr(res_data_);
+    res_data_ = nullptr;
   }
 
+  UnsetConnTimer();
+
   if (watcher_id_ > 0) {
     g_bus_unwatch_name(watcher_id_);
     watcher_id_ = 0;
@@ -318,6 +318,7 @@ int FdBroker::Send(const std::string& target_appid,
     return -1;  // LCOV_EXCL_LINE
   }
 
+  std::lock_guard<std::recursive_mutex> lock(GetMutex());
   if (cancellable_) {
     g_cancellable_cancel(cancellable_);  // LCOV_EXCL_LINE
     g_object_unref(cancellable_);  // LCOV_EXCL_LINE
@@ -330,11 +331,21 @@ int FdBroker::Send(const std::string& target_appid,
     return -1;  // LCOV_EXCL_LINE
   }
 
+  if (res_data_)
+    DestroyWeakPtr(res_data_);
+
+  res_data_ = CreateWeakPtr();
+  if (res_data_ == nullptr) {
+    LOGE("Failed to create weak ptr");
+    g_object_unref(msg);
+    return -1;
+  }
+
   g_dbus_message_set_unix_fd_list(msg, fd_list.GetRaw());
   g_dbus_connection_send_message_with_reply(
       DBusConnectionManager::GetInst().GetConnection(),
       msg, G_DBUS_SEND_MESSAGE_FLAGS_NONE,
-      5000, NULL, cancellable_, OnResultReceived, this);
+      5000, NULL, cancellable_, OnResultReceived, res_data_);
   g_object_unref(msg);
 
   (*fds)[0] = main_sock_pair.Detach(SocketPair::SENDER);
@@ -509,15 +520,16 @@ void FdBroker::OnReceiveDbusMethod(GDBusConnection *conn,
     const gchar *iface_name, const gchar *method_name,
     GVariant *parameters, GDBusMethodInvocation *invocation,
     gpointer user_data) {
-  FdBroker* broker = static_cast<FdBroker*>(user_data);
-  if (!FdBrokerManager::GetInst().Exist(broker)) {
-    LOGE("No such FdBroker(%p)", broker);
+  auto* ptr = static_cast<std::weak_ptr<FdBroker>*>(user_data);
+  auto broker = ptr->lock();
+  if (broker == nullptr) {
+    LOGE("broker is nullptr");
     return;
   }
 
-  std::lock_guard<std::recursive_mutex> lock(broker->mutex_);
+  std::lock_guard<std::recursive_mutex> lock(broker->GetMutex());
   if (broker->registration_id_ == 0) {
-    LOGE("Invalid context. FdBroker(%p)", broker);
+    LOGE("Invalid context. FdBroker(%p)", broker.get());
     return;
   }
 
@@ -657,10 +669,17 @@ int FdBroker::RegisterDbusInterface(const std::string& port_name) {
     return -1;
   }
 
+  auto broker = CreateWeakPtr();
+  if (broker == nullptr) {
+    LOGE("Failed to create weak ptr");
+    return -1;
+  }
+
+  std::lock_guard<std::recursive_mutex> lock(GetMutex());
   registration_id_ = g_dbus_connection_register_object(
       DBusConnectionManager::GetInst().GetConnection(),
       RPC_PORT_OBJECT_PATH, introspection_data->interfaces[0],
-      &interface_vtable, this, nullptr, nullptr);
+      &interface_vtable, broker, DestroyWeakPtr, nullptr);
 
   g_dbus_node_info_unref(introspection_data);
   if (registration_id_ == 0) {
@@ -719,23 +738,20 @@ void FdBroker::OnNameAppeared(GDBusConnection *connection,
                               const gchar *name,
                               const gchar *name_owner,
                               gpointer user_data) {
-  FdBroker* broker = static_cast<FdBroker*>(user_data);
-  if (!FdBrokerManager::GetInst().Exist(broker)) {
-    LOGE("No such FdBroker(%p)", broker);
+  auto* ptr = static_cast<std::weak_ptr<FdBroker>*>(user_data);
+  auto broker = ptr->lock();
+  if (broker == nullptr) {
+    LOGE("broker is nullptr");
     return;
   }
 
-  std::lock_guard<std::recursive_mutex> lock(broker->mutex_);
+  std::lock_guard<std::recursive_mutex> lock(broker->GetMutex());
   if (broker->watcher_id_ == 0) {
-    LOGE("Invalid context. FdBroker(%p)", broker);
+    LOGE("Invalid context. FdBroker(%p)", broker.get());
     return;
   }
 
-  if (broker->conn_timer_ > 0) {
-    g_source_remove(broker->conn_timer_);
-    broker->conn_timer_ = 0;
-  }
-
+  broker->UnsetConnTimer();
   int pid = broker->GetSenderPid(connection, name_owner);
   char owner_appid[255] = { 0, };
 
@@ -764,15 +780,16 @@ void FdBroker::OnNameAppeared(GDBusConnection *connection,
 void FdBroker::OnNameVanished(GDBusConnection *connection,
                               const gchar *name,
                               gpointer user_data) {
-  FdBroker* broker = static_cast<FdBroker*>(user_data);
-  if (!FdBrokerManager::GetInst().Exist(broker)) {
-    LOGE("No such FdBroker(%p)", broker);
+  auto* ptr = static_cast<std::weak_ptr<FdBroker>*>(user_data);
+  auto broker = ptr->lock();
+  if (broker == nullptr) {
+    LOGE("broker is nullptr");
     return;
   }
 
-  std::lock_guard<std::recursive_mutex> lock(broker->mutex_);
+  std::lock_guard<std::recursive_mutex> lock(broker->GetMutex());
   if (broker->watcher_id_ == 0) {
-    LOGE("Invalid context. FdBroker(%p)", broker);
+    LOGE("Invalid context. FdBroker(%p)", broker.get());
     return;
   }
 
@@ -782,7 +799,7 @@ void FdBroker::OnNameVanished(GDBusConnection *connection,
 
 int FdBroker::Watch(IEventWatcher* ev, const std::string& target_appid,
                     const std::string& port_name) {
-  std::lock_guard<std::recursive_mutex> lock(mutex_);
+  std::lock_guard<std::recursive_mutex> lock(GetMutex());
   if (ev == nullptr)
     return -1;
 
@@ -794,10 +811,8 @@ int FdBroker::Watch(IEventWatcher* ev, const std::string& target_appid,
   watch_appid_ = target_appid;
   watch_port_name_ = port_name;
 
-  if (conn_timer_ > 0)
-    g_source_remove(conn_timer_);
-
-  conn_timer_ = g_timeout_add(10 * 1000, OnDbusNameTimeout, this);
+  UnsetConnTimer();
+  SetConnTimer();
 
   if (mock_) {
     // LCOV_EXCL_START
@@ -819,14 +834,20 @@ int FdBroker::Watch(IEventWatcher* ev, const std::string& target_appid,
     // LCOV_EXCL_STOP
   }
 
+  auto broker = CreateWeakPtr();
+  if (broker == nullptr) {
+    LOGE("Failed to create weak ptr");
+    return -1;
+  }
+
   watcher_id_ = g_bus_watch_name_on_connection(
       DBusConnectionManager::GetInst().GetConnection(),
       interface_name.c_str(),
       G_BUS_NAME_WATCHER_FLAGS_NONE,
       OnNameAppeared,
       OnNameVanished,
-      this,
-      NULL);
+      broker,
+      DestroyWeakPtr);
   if (watcher_id_ == 0) {
     // LCOV_EXCL_START
     LOGE("Failed to watch connection(%s)", interface_name.c_str());
@@ -861,7 +882,7 @@ int FdBroker::Prepare(const std::string& target_appid,
 void FdBroker::OnResultReceived(GObject* source_object,
                                 GAsyncResult* res,
                                 gpointer user_data) {
-  LOGW("OnResultReceived() : FdBroker(%p)", user_data);
+  LOGW("OnResultReceived()");
   GDBusConnection* conn = reinterpret_cast<GDBusConnection*>(source_object);
   GError* err = nullptr;
   GDBusMessage* reply = g_dbus_connection_send_message_with_reply_finish(conn,
@@ -873,15 +894,16 @@ void FdBroker::OnResultReceived(GObject* source_object,
     return;
   }
 
-  FdBroker* broker = static_cast<FdBroker*>(user_data);
-  if (!FdBrokerManager::GetInst().Exist(broker)) {
-    LOGE("No such FdBroker(%p)", broker);
+  auto* ptr = static_cast<std::weak_ptr<FdBroker>*>(user_data);
+  auto broker = ptr->lock();
+  if (broker == nullptr) {
+    LOGE("broker is nullptr");
     return;
   }
 
-  std::lock_guard<std::recursive_mutex> lock(broker->mutex_);
+  std::lock_guard<std::recursive_mutex> lock(broker->GetMutex());
   if (broker->cancellable_ == nullptr) {
-    LOGE("Invalid context. Fdbroker(%p)", broker);
+    LOGE("Invalid context. Fdbroker(%p)", broker.get());
     return;
   }
 
@@ -932,19 +954,21 @@ void FdBroker::OnResultReceived(GObject* source_object,
 }
 
 gboolean FdBroker::OnDbusNameTimeout(gpointer user_data) {
-  FdBroker* broker = static_cast<FdBroker*>(user_data);
-  if (!FdBrokerManager::GetInst().Exist(broker)) {
-    LOGE("No such FdBroker(%p)", broker);
+  auto* ptr = static_cast<std::weak_ptr<FdBroker>*>(user_data);
+  auto broker = ptr->lock();
+  if (broker == nullptr) {
+    LOGE("broker is nullptr");
     return G_SOURCE_REMOVE;
   }
 
-  std::lock_guard<std::recursive_mutex> lock(broker->mutex_);
-  if (broker->conn_timer_ == 0) {
-    LOGE("Invalid context. FdBroker(%p)", broker);
+  std::lock_guard<std::recursive_mutex> lock(broker->GetMutex());
+  if (broker->conn_data_ == nullptr) {
+    LOGE("Invalid context. FdBroker(%p)", broker.get());
     return G_SOURCE_REMOVE;
   }
 
-  broker->conn_timer_ = 0;
+  DestroyWeakPtr(broker->conn_data_);
+  broker->conn_data_ = nullptr;
   broker->Cancel();
   auto* watcher = broker->watcher_;
   watcher->OnPortRejected(broker->watch_appid_, RPC_PORT_ERROR_IO_ERROR);
@@ -952,28 +976,48 @@ gboolean FdBroker::OnDbusNameTimeout(gpointer user_data) {
   return G_SOURCE_REMOVE;
 }
 
-FdBrokerManager& FdBrokerManager::GetInst() {
-  static FdBrokerManager inst;
-  return inst;
+std::shared_ptr<FdBroker> FdBroker::GetSharedPtr() {
+  return shared_from_this();
+}
+
+gpointer FdBroker::CreateWeakPtr() {
+  auto* ptr = new (std::nothrow) std::weak_ptr<FdBroker>(GetSharedPtr());
+  return static_cast<gpointer>(ptr);
 }
 
-void FdBrokerManager::Add(FdBroker* broker) {
-  std::lock_guard<std::recursive_mutex> lock(mutex_);
-  brokers_.push_back(broker);
+void FdBroker::DestroyWeakPtr(gpointer data) {
+  auto* ptr = static_cast<std::weak_ptr<FdBroker>*>(data);
+  delete ptr;
 }
 
-void FdBrokerManager::Remove(FdBroker* broker) {
-  std::lock_guard<std::recursive_mutex> lock(mutex_);
-  brokers_.remove(broker);
+void FdBroker::SetConnTimer() {
+  if (conn_data_) {
+    LOGW("Already exists");
+    return;
+  }
+
+  conn_data_ = CreateWeakPtr();
+  if (conn_data_ == nullptr)
+    LOGE("Out of memory");
+
+  g_timeout_add(10 * 1000, OnDbusNameTimeout, conn_data_);
 }
 
-bool FdBrokerManager::Exist(FdBroker* broker) {
-  std::lock_guard<std::recursive_mutex> lock(mutex_);
-  auto iter = std::find(brokers_.begin(), brokers_.end(), broker);
-  if (iter != brokers_.end())
-    return true;
+void FdBroker::UnsetConnTimer() {
+  if (conn_data_ == nullptr)
+    return;
+
+  GSource* source = g_main_context_find_source_by_user_data(nullptr,
+      conn_data_);
+  if (source && !g_source_is_destroyed(source))
+    g_source_destroy(source);
+
+  DestroyWeakPtr(conn_data_);
+  conn_data_ = nullptr;
+}
 
-  return false;
+std::recursive_mutex& FdBroker::GetMutex() const {
+  return mutex_;
 }
 
 }  // namespace internal

diff --git a/src/fdbroker-internal.h b/src/fdbroker-internal.h
index 881df3a..5086218 100644 (file)
--- a/src/fdbroker-internal.h
+++ b/src/fdbroker-internal.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017 Samsung Electronics Co., Ltd.
+ * Copyright (c) 2017 - 2020 Samsung Electronics Co., Ltd.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -35,7 +35,7 @@
 namespace rpc_port {
 namespace internal {
 
-class FdBroker {
+class FdBroker : public std::enable_shared_from_this<FdBroker> {
  public:
   class IEventListener {
    public:
@@ -177,6 +177,13 @@ class FdBroker {
                                gpointer user_data);
   static gboolean OnDbusNameTimeout(gpointer user_data);
 
+  std::shared_ptr<FdBroker> GetSharedPtr();
+  gpointer CreateWeakPtr();
+  static void DestroyWeakPtr(gpointer data);
+  void SetConnTimer();
+  void UnsetConnTimer();
+  std::recursive_mutex& GetMutex() const;
+
  private:
   IEventListener* listener_ = nullptr;
   int registration_id_ = 0;
@@ -187,23 +194,8 @@ class FdBroker {
   std::string watch_appid_;
   std::string watch_port_name_;
   GCancellable* cancellable_ = nullptr;
-  guint conn_timer_ = 0;
-  mutable std::recursive_mutex mutex_;
-};
-
-class FdBrokerManager {
- private:
-  FdBrokerManager() = default;
-  ~FdBrokerManager() = default;
-
- public:
-  static FdBrokerManager& GetInst();
-  void Add(FdBroker* broker);
-  void Remove(FdBroker* broker);
-  bool Exist(FdBroker* broker);
-
- private:
-  std::list<FdBroker*> brokers_;
+  gpointer res_data_ = nullptr;
+  gpointer conn_data_ = nullptr;
   mutable std::recursive_mutex mutex_;
 };
 

diff --git a/src/proxy-internal.cc b/src/proxy-internal.cc
index 040116d..f3e8b40 100644 (file)
--- a/src/proxy-internal.cc
+++ b/src/proxy-internal.cc
@@ -32,7 +32,7 @@ namespace rpc_port {
 namespace internal {
 
 Proxy::Proxy(bool mock)
-    : fd_broker_(mock) {}
+    : fd_broker_(new FdBroker(mock)) {}
 
 Proxy::~Proxy() {
   _D("Proxy::~Proxy");
@@ -114,7 +114,7 @@ void Proxy::OnPortAppeared(const std::string& appid,
 
   fds_[0] = 0;
   fds_[1] = 0;
-  int r = fd_broker_.Send(appid, port_name, &fds_);
+  int r = fd_broker_->Send(appid, port_name, &fds_);
   if (r <= 0) {
     // LCOV_EXCL_START
     IEventListener* listener = listener_;
@@ -188,7 +188,7 @@ int Proxy::Connect(std::string appid, std::string port_name,
   target_appid_ = std::move(appid);
   port_name_ = std::move(port_name);
 
-  int r = fd_broker_.Watch(this, target_appid_, port_name_);
+  int r = fd_broker_->Watch(this, target_appid_, port_name_);
   if (r < 0) {
     // LCOV_EXCL_START
     listener_ = nullptr;
@@ -217,7 +217,7 @@ int Proxy::ConnectSync(std::string appid, std::string port_name,
   target_appid_ = std::move(appid);
   port_name_ = std::move(port_name);
 
-  int ret = fd_broker_.Prepare(target_appid_, port_name_);
+  int ret = fd_broker_->Prepare(target_appid_, port_name_);
   if (ret < 0) {
     listener_ = nullptr;
     if (ret == -EILLEGALACCESS)
@@ -228,7 +228,7 @@ int Proxy::ConnectSync(std::string appid, std::string port_name,
 
   fds_[0] = 0;
   fds_[1] = 0;
-  ret = fd_broker_.SendSync(target_appid_, port_name_, &fds_);
+  ret = fd_broker_->SendSync(target_appid_, port_name_, &fds_);
   if (ret <= 0) {
     listener_ = nullptr;
     if (ret == -EILLEGALACCESS)

diff --git a/src/proxy-internal.h b/src/proxy-internal.h
index 485373e..dfb666f 100644 (file)
--- a/src/proxy-internal.h
+++ b/src/proxy-internal.h
@@ -99,7 +99,7 @@ class Proxy : public FdBroker::IEventWatcher {
   std::shared_ptr<ProxyPort> main_port_;
   std::shared_ptr<ProxyPort> delegate_port_;
   IEventListener* listener_ = nullptr;
-  FdBroker fd_broker_;
+  std::shared_ptr<FdBroker> fd_broker_;
   std::string target_appid_;
   int fds_[2];
   guint conn_timer_ = 0;

diff --git a/src/stub-internal.cc b/src/stub-internal.cc
index f73f3d2..35ecd09 100644 (file)
--- a/src/stub-internal.cc
+++ b/src/stub-internal.cc
@@ -28,7 +28,7 @@ namespace rpc_port {
 namespace internal {
 
 Stub::Stub(const std::string& port_name, bool mock)
-  : fd_broker_(mock),
+  : fd_broker_(new FdBroker(mock)),
     port_name_(port_name) {
 }
 
@@ -41,16 +41,16 @@ int Stub::Listen(IEventListener* ev) {
     return RPC_PORT_ERROR_INVALID_PARAMETER;
 
   listener_ = ev;
-  return fd_broker_.Listen(this, port_name_);
+  return fd_broker_->Listen(this, port_name_);
 }
 
 void Stub::AddPrivilege(const std::string& privilege) {
-  AccessController& ac = fd_broker_.GetAccessController();
+  AccessController& ac = fd_broker_->GetAccessController();
   ac.AddPrivilege(privilege);
 }
 
 void Stub::SetTrusted(const bool trusted) {
-  AccessController& ac = fd_broker_.GetAccessController();
+  AccessController& ac = fd_broker_->GetAccessController();
   ac.SetTrusted(trusted);
 }
 

diff --git a/src/stub-internal.h b/src/stub-internal.h
index 54022d2..9063acb 100644 (file)
--- a/src/stub-internal.h
+++ b/src/stub-internal.h
@@ -87,7 +87,7 @@ class Stub : private FdBroker::IEventListener {
  private:
   std::list<std::shared_ptr<AcceptedPort>> ports_;
   IEventListener* listener_ = nullptr;
-  FdBroker fd_broker_;
+  std::shared_ptr<FdBroker> fd_broker_;
   std::string port_name_;
 };