From e770b1fe9826a4392b6c8f48338af23ad03c2433 Mon Sep 17 00:00:00 2001 From: Hwankyu Jhun Date: Tue, 5 Jan 2021 08:49:29 +0900 Subject: [PATCH] Fix invalid read If a stub handle is defined to a global variable, a process accesses to the released DBusConnectionManager instance while calling the __run_exit_handlers(). The GDBusConnection instance is moved to the FdBroker object. Change-Id: I6040a9737f5b23c644f91eccb330b6aba4104ac0 Signed-off-by: Hwankyu Jhun --- src/fdbroker-internal.cc | 75 ++++++++++++------------------------------------ src/fdbroker-internal.hh | 27 ++--------------- 2 files changed, 20 insertions(+), 82 deletions(-) diff --git a/src/fdbroker-internal.cc b/src/fdbroker-internal.cc index 6cc085d..285b335 100644 --- a/src/fdbroker-internal.cc +++ b/src/fdbroker-internal.cc @@ -41,57 +41,19 @@ namespace rpc_port { namespace internal { -FdBroker::DBusConnectionManager& FdBroker::DBusConnectionManager::GetInst() { - static DBusConnectionManager dm; - - if (dm.disposed_) { - dm.Init(); - } - - return dm; -} - -// LCOV_EXCL_START -void FdBroker::DBusConnectionManager::Dispose() { - if (!disposed_) - Fini(); -} -// LCOV_EXCL_STOP - -GDBusConnection* FdBroker::DBusConnectionManager::GetConnection() { - return gdbus_conn_; -} - -FdBroker::DBusConnectionManager::~DBusConnectionManager() { - if (!disposed_) - Fini(); -} - -void FdBroker::DBusConnectionManager::Init() { - GError* error = nullptr; - - gdbus_conn_ = g_bus_get_sync(G_BUS_TYPE_SESSION, NULL, &error); +GDBusConnection* FdBroker::GetConnection() { if (gdbus_conn_ == nullptr) { - // LCOV_EXCL_START - if (error != nullptr) { - _E("Failed to get dbus [%s]", error->message); - g_error_free(error); + GError* error = nullptr; + gdbus_conn_ = g_bus_get_sync(G_BUS_TYPE_SESSION, NULL, &error); + if (gdbus_conn_ == nullptr) { + _E("g_bus_get_sync() is failed. error(%s)", + error ? error->message : "Unknown"); + g_clear_error(&error); + return nullptr; } - - return; - // LCOV_EXCL_STOP } - disposed_ = false; -} - -void FdBroker::DBusConnectionManager::Fini() { - if (gdbus_conn_ != nullptr) { - g_object_unref(gdbus_conn_); - gdbus_conn_ = nullptr; - } - - disposed_ = true; + return gdbus_conn_; } // LCOV_EXCL_START @@ -212,6 +174,9 @@ FdBroker::FdBroker(bool mock) : mock_(mock) { FdBroker::~FdBroker() { Cancel(); + + if (gdbus_conn_) + g_object_unref(gdbus_conn_); } void FdBroker::Cancel() { @@ -238,9 +203,7 @@ void FdBroker::Cancel() { } if (registration_id_ > 0) { - g_dbus_connection_unregister_object( - DBusConnectionManager::GetInst().GetConnection(), - registration_id_); + g_dbus_connection_unregister_object(GetConnection(), registration_id_); registration_id_ = 0; } @@ -337,8 +300,7 @@ int FdBroker::Send(const std::string& target_appid, } g_dbus_message_set_unix_fd_list(msg, fd_list.GetRaw()); - g_dbus_connection_send_message_with_reply( - DBusConnectionManager::GetInst().GetConnection(), + g_dbus_connection_send_message_with_reply(GetConnection(), msg, G_DBUS_SEND_MESSAGE_FLAGS_NONE, 5000, NULL, cancellable_, OnResultReceived, res_data_); g_object_unref(msg); @@ -420,8 +382,7 @@ int FdBroker::SendSync(const std::string& target_appid, g_dbus_message_set_unix_fd_list(msg, fd_list.GetRaw()); - reply = g_dbus_connection_send_message_with_reply_sync( - DBusConnectionManager::GetInst().GetConnection(), + reply = g_dbus_connection_send_message_with_reply_sync(GetConnection(), msg, G_DBUS_SEND_MESSAGE_FLAGS_NONE, 500, nullptr, nullptr, &err); clock_gettime(CLOCK_MONOTONIC, &end_time); g_object_unref(msg); @@ -553,7 +514,7 @@ int FdBroker::GetOwnerId(const std::string& interface_name) { GError* error = NULL; GVariant* result = g_dbus_connection_call_sync( - DBusConnectionManager::GetInst().GetConnection(), + GetConnection(), DBUS_SERVICE_DBUS, DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, @@ -672,7 +633,7 @@ int FdBroker::RegisterDbusInterface(const std::string& port_name) { std::lock_guard lock(GetMutex()); registration_id_ = g_dbus_connection_register_object( - DBusConnectionManager::GetInst().GetConnection(), + GetConnection(), RPC_PORT_OBJECT_PATH, introspection_data->interfaces[0], &interface_vtable, broker, DestroyWeakPtr, nullptr); @@ -831,7 +792,7 @@ int FdBroker::Watch(IEventWatcher* ev, const std::string& target_appid, } watcher_id_ = g_bus_watch_name_on_connection( - DBusConnectionManager::GetInst().GetConnection(), + GetConnection(), interface_name.c_str(), G_BUS_NAME_WATCHER_FLAGS_NONE, OnNameAppeared, diff --git a/src/fdbroker-internal.hh b/src/fdbroker-internal.hh index bc02d81..01dc6d0 100644 --- a/src/fdbroker-internal.hh +++ b/src/fdbroker-internal.hh @@ -54,10 +54,6 @@ class FdBroker : public std::enable_shared_from_this { explicit FdBroker(bool mock = false); ~FdBroker(); - static void Dispose() { - DBusConnectionManager::GetInst().Dispose(); - } - void Cancel(); int Send(const std::string& target_appid, const std::string& port_name, int (*fds)[2]); @@ -70,27 +66,6 @@ class FdBroker : public std::enable_shared_from_this { int (*fds)[2]); private: - class DBusConnectionManager { - public: - DBusConnectionManager(const DBusConnectionManager&) = delete; - DBusConnectionManager& operator = (const DBusConnectionManager&) = delete; - - static DBusConnectionManager& GetInst(); - void Dispose(); - GDBusConnection* GetConnection(); - - private: - DBusConnectionManager() = default; - ~DBusConnectionManager(); - - void Init(); - void Fini(); - - private: - bool disposed_ = true; - GDBusConnection* gdbus_conn_ = nullptr; - }; - class DBusMock { public: DBusMock(const DBusMock&) = delete; @@ -177,6 +152,7 @@ class FdBroker : public std::enable_shared_from_this { void SetConnTimer(); void UnsetConnTimer(); std::recursive_mutex& GetMutex() const; + GDBusConnection* GetConnection(); private: IEventListener* listener_ = nullptr; @@ -191,6 +167,7 @@ class FdBroker : public std::enable_shared_from_this { gpointer res_data_ = nullptr; gpointer conn_data_ = nullptr; mutable std::recursive_mutex mutex_; + GDBusConnection* gdbus_conn_ = nullptr; }; } // namespace internal -- 2.7.4