Fix thread safe issue

 -Thread-safe issue occurs when a sub-thread deregisters a port while receiving message.

Change-Id: I6e6c72fb7b0915f7d7c21e3345eebcf1e7136d90
Signed-off-by: jusung son <jusung07.son@samsung.com>

diff --git a/src/message_port_common.c b/src/message_port_common.c
index 77519bc..af01ea9 100644 (file)
--- a/src/message_port_common.c
+++ b/src/message_port_common.c
@@ -43,7 +43,10 @@ GDBusConnection *gdbus_conn;
 char *app_id;
 
 static const int MAX_MESSAGE_SIZE = 16 * 1024;
-static pthread_mutex_t _message_port_mutex = PTHREAD_MUTEX_INITIALIZER;
+static GRecMutex __rec_mutex;
+
+#define MESSAGE_PORT_CTOR __attribute__((constructor))
+#define MESSAGE_PORT_DTOR __attribute__((destructor))
 
 int write_socket(int fd,
                const char *buffer,
@@ -345,12 +348,27 @@ char *get_encoded_name(const char *remote_app_id, const char *port_name, bool is
        return md5_digest;
 }
 
+MESSAGE_PORT_CTOR static void __message_port_init(void)
+{
+       g_rec_mutex_init(&__rec_mutex);
+}
+
+MESSAGE_PORT_DTOR static void __message_port_fini(void)
+{
+       if (g_rec_mutex_trylock(&__rec_mutex))
+               g_rec_mutex_unlock(&__rec_mutex);
+       else
+               _LOGE("rec_mutex is locked by another thread.");
+
+       g_rec_mutex_clear(&__rec_mutex);
+}
+
 void message_port_lock_mutex()
 {
-       pthread_mutex_lock(&_message_port_mutex);
+       g_rec_mutex_lock(&__rec_mutex);
 }
 
 void message_port_unlock_mutex()
 {
-       pthread_mutex_unlock(&_message_port_mutex);
+       g_rec_mutex_unlock(&__rec_mutex);
 }

diff --git a/src/message_port_remote.c b/src/message_port_remote.c
index c18dc48..31463fc 100644 (file)
--- a/src/message_port_remote.c
+++ b/src/message_port_remote.c
@@ -47,7 +47,7 @@
 static bool _initialized = false;
 static GHashTable *__local_port_info;
 static GHashTable *__trusted_app_list_hash;
-static GHashTable *__callback_info_hash;
+static GList *__callback_info_list;
 static GHashTable *__sender_appid_hash;
 
 typedef struct message_port_pkt {
@@ -67,11 +67,6 @@ typedef struct message_port_callback_info {
        int g_src_id;
 } message_port_callback_info_s;
 
-typedef struct callback_key_info {
-       int local_id;
-       message_port_callback_info_s *callback_info;
-} callback_key_info_s;
-
 static void __callback_info_free(gpointer data)
 {
        message_port_callback_info_s *callback_info = (message_port_callback_info_s *)data;
@@ -110,28 +105,34 @@ static void __callback_info_free(gpointer data)
 /* LCOV_EXCL_START */
 static void __callback_info_free_by_info(message_port_callback_info_s *callback_info)
 {
-       GList *callback_info_list = g_hash_table_lookup(__callback_info_hash, GUINT_TO_POINTER(callback_info->local_id));
        GList *find_list;
 
-       if (callback_info_list == NULL)
-               return;
-
-       find_list = g_list_find(callback_info_list, callback_info);
+       find_list = g_list_find(__callback_info_list, callback_info);
        if (find_list == NULL)
                return;
 
-       callback_info_list = g_list_remove_link(callback_info_list, find_list);
+       __callback_info_list = g_list_remove_link(__callback_info_list, find_list);
        __callback_info_free(callback_info);
        g_list_free(find_list);
 }
-/* LCOV_EXCL_STOP */
 
-static void __hash_destroy_callback_info(gpointer data)
+static void __callback_info_free_by_local_id(int local_id)
 {
-       GList *callback_list = (GList *)data;
-
-       if (callback_list != NULL)
-               g_list_free_full(callback_list, __callback_info_free);
+       GList *iter;
+       GList *callback_info;
+
+       for (iter = __callback_info_list; iter != NULL; ) {
+               if (((message_port_callback_info_s *)iter->data)->local_id == local_id) {
+                       callback_info = iter;
+                       iter = iter->next;
+                       __callback_info_list =
+                                       g_list_remove_link(__callback_info_list, callback_info);
+                       __callback_info_free(callback_info->data);
+                       g_list_free(callback_info);
+                       continue;
+               }
+               iter = iter->next;
+       }
 }
 
 /* LCOV_EXCL_START */
@@ -168,11 +169,6 @@ static bool __initialize(void)
                retvm_if(!__trusted_app_list_hash, false, "fail to create __trusted_app_list_hash");
        }
 
-       if (__callback_info_hash == NULL) {
-               __callback_info_hash = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, __hash_destroy_callback_info);
-               retvm_if(!__callback_info_hash, false, "fail to create __callback_info_hash");
-       }
-
        _initialized = true;
 
        return true;
@@ -297,35 +293,26 @@ static message_port_pkt_s *__message_port_recv_raw(int fd)
        return pkt;
 }
 
-static bool __validate_callback_info(callback_key_info_s *key_info)
+static bool __validate_callback_info(message_port_callback_info_s *callback_info)
 {
        GList *cb_list;
 
-       cb_list = g_hash_table_lookup(__callback_info_hash,
-                       GUINT_TO_POINTER(key_info->local_id));
+       cb_list = g_list_find(__callback_info_list, callback_info);
        if (cb_list == NULL) {
-               _LOGI("local_info : %d is already released", key_info->local_id);
-               return false;
-       }
-
-       cb_list = g_list_find(cb_list, key_info->callback_info);
-       if (cb_list == NULL) {
-               _LOGI("local_info : %d is already released from list",
-                               key_info->local_id);
+               _LOGI("local_info : %p is already released from list", callback_info);
                return false;
        }
 
        return true;
 }
 
-static bool __validate_local_info(callback_key_info_s *key_info)
+static bool __validate_local_info(int local_id)
 {
        GList *cb_list;
 
-       cb_list = g_hash_table_lookup(__local_port_info,
-                       GUINT_TO_POINTER(key_info->local_id));
+       cb_list = g_hash_table_lookup(__local_port_info, GUINT_TO_POINTER(local_id));
        if (cb_list == NULL) {
-               _LOGI("local_info : %d is already released", key_info->local_id);
+               _LOGI("local_info : %d is already released", local_id);
                return false;
        }
 
@@ -337,36 +324,30 @@ static gboolean __socket_request_handler(GIOChannel *gio,
                gpointer data)
 {
        int fd = 0;
-       message_port_callback_info_s *mi = NULL;
-       callback_key_info_s *key_info;
+       message_port_callback_info_s *callback_info;
        message_port_pkt_s *pkt = NULL;
        message_port_local_port_info_s *local_port_info;
        bundle *kb = NULL;
        bool ret = true;
        bool existed = true;
 
-       key_info = (callback_key_info_s *)data;
-       if (key_info == NULL)
+       callback_info = (message_port_callback_info_s *)data;
+       if (callback_info == NULL)
                return FALSE;
 
        message_port_lock_mutex();
-       if (__validate_callback_info(key_info) == false) {
+       if (__validate_callback_info(callback_info) == false) {
                ret = FALSE;
                existed = FALSE;
-               message_port_unlock_mutex();
                goto out;
        }
 
-       if (__validate_local_info(key_info) == false) {
+       if (__validate_local_info(callback_info->local_id) == false) {
                ret = FALSE;
-               message_port_unlock_mutex();
                goto out;
        }
-       message_port_unlock_mutex();
 
-       mi = key_info->callback_info;
-
-       local_port_info = mi->local_info;
+       local_port_info = callback_info->local_info;
        if (local_port_info == NULL || local_port_info->callback == NULL) {
                _LOGE("Failed to get callback info");
                ret = FALSE;
@@ -401,11 +382,13 @@ static gboolean __socket_request_handler(GIOChannel *gio,
        }
 
        if (pkt->is_bidirection)
-               local_port_info->callback(mi->local_id, mi->remote_app_id,
-                       pkt->remote_port_name, pkt->is_trusted, kb, local_port_info->user_data);
+               local_port_info->callback(callback_info->local_id,
+                               callback_info->remote_app_id, pkt->remote_port_name,
+                               pkt->is_trusted, kb, local_port_info->user_data);
        else
-               local_port_info->callback(mi->local_id, mi->remote_app_id,
-                       NULL, pkt->is_trusted, kb, local_port_info->user_data);
+               local_port_info->callback(callback_info->local_id,
+                               callback_info->remote_app_id, NULL,
+                               pkt->is_trusted, kb, local_port_info->user_data);
 
        bundle_free(kb);
 
@@ -418,39 +401,12 @@ out:
                free(pkt);
        }
 
-       if (mi && ret == FALSE && existed == TRUE) {
-               message_port_lock_mutex();
-               __callback_info_free_by_info(mi);
-               message_port_unlock_mutex();
-       }
-
-       return ret;
-}
-
+       if (ret == FALSE && existed == TRUE)
+               __callback_info_free_by_info(callback_info);
 
-static void __socket_destroy_handler(gpointer data)
-{
-       _LOGI("__socket_destroy_handler");
-       callback_key_info_s *key_info = (callback_key_info_s *)data;
-       free(key_info);
-}
-
-static callback_key_info_s *__create_callback_key_info(message_port_callback_info_s *callback_info)
-{
-       callback_key_info_s *_key_info = (callback_key_info_s *)
-               calloc(1, sizeof(callback_key_info_s));
-
-       if (_key_info == NULL) {
-/* LCOV_EXCL_START */
-               _LOGE("out of memory");
-               return NULL;
-/* LCOV_EXCL_STOP */
-       }
-
-       _key_info->local_id = callback_info->local_id;
-       _key_info->callback_info = callback_info;
+       message_port_unlock_mutex();
 
-       return _key_info;
+       return ret;
 }
 
 static message_port_callback_info_s *__create_callback_info(message_port_local_port_info_s *mi, char *local_appid)
@@ -510,55 +466,26 @@ out:
        return callback_info;
 }
 
-static bool __callback_info_append(message_port_callback_info_s *callback_info)
+static void __callback_info_append(message_port_callback_info_s *callback_info)
 {
-       GList *callback_info_list = NULL;
-       message_port_callback_info_s *head_callback_info;
-
-       callback_info_list = g_hash_table_lookup(__callback_info_hash, GUINT_TO_POINTER(callback_info->local_id));
-       if (callback_info_list == NULL) {
-               head_callback_info = (message_port_callback_info_s *)calloc(1, sizeof(message_port_callback_info_s));
-               if (head_callback_info == NULL) {
-/* LCOV_EXCL_START */
-                       _LOGE("fail to alloc head_callback_info");
-                       return false;
-/* LCOV_EXCL_STOP */
-               }
-               head_callback_info->local_id = 0;
-               head_callback_info->remote_app_id = NULL;
-               head_callback_info->local_info = NULL;
-               head_callback_info->gio_read = NULL;
-               head_callback_info->g_src_id = 0;
-               callback_info_list = g_list_append(callback_info_list, head_callback_info);
-               callback_info_list = g_list_append(callback_info_list, callback_info);
-               g_hash_table_insert(__callback_info_hash, GUINT_TO_POINTER(callback_info->local_id), callback_info_list);
-       } else {
-               callback_info_list = g_list_append(callback_info_list, callback_info);
-       }
-
-       return true;
+       __callback_info_list = g_list_append(__callback_info_list, callback_info);
 }
 
 static void __callback_info_update_user_data(int local_id, message_port_message_cb callback, void *user_data)
 {
-       GList *callback_info_list;
        GList *iter;
        message_port_callback_info_s *callback_info;
 
-       callback_info_list = g_hash_table_lookup(__callback_info_hash, GUINT_TO_POINTER(local_id));
-       if (callback_info_list != NULL) {
 /* LCOV_EXCL_START */
-               for (iter = callback_info_list; iter != NULL; iter = iter->next) {
-                       callback_info = (message_port_callback_info_s *)iter->data;
-                       if (callback_info->local_info != NULL) {
-                               callback_info->local_info->callback = callback;
-                               callback_info->local_info->user_data = user_data;
-                       }
+       for (iter = __callback_info_list; iter != NULL; iter = iter->next) {
+               callback_info = (message_port_callback_info_s *)iter->data;
+               if (callback_info->local_id == local_id
+                               && callback_info->local_info != NULL) {
+                       callback_info->local_info->callback = callback;
+                       callback_info->local_info->user_data = user_data;
                }
-/* LCOV_EXCL_STOP */
-       } else {
-               _LOGE("fail to find local_id %d ", local_id);
        }
+/* LCOV_EXCL_STOP */
 }
 
 static bool __receive_message(GVariant *parameters, GDBusMethodInvocation *invocation)
@@ -577,7 +504,6 @@ static bool __receive_message(GVariant *parameters, GDBusMethodInvocation *invoc
        message_port_local_port_info_s *mi;
        int local_reg_id = 0;
        message_port_callback_info_s *callback_info = NULL;
-       callback_key_info_s *key_info;
 
        char buf[1024];
        GDBusMessage *msg;
@@ -588,6 +514,7 @@ static bool __receive_message(GVariant *parameters, GDBusMethodInvocation *invoc
        bool ret = false;
        char *key_appid;
 
+       message_port_lock_mutex();
        g_variant_get(parameters, "(&s&sbb&s&sbu&s)", &local_appid, &local_port, &local_trusted, &bi_dir,
                        &remote_appid, &remote_port, &remote_trusted, &len, &raw);
 
@@ -605,19 +532,15 @@ static bool __receive_message(GVariant *parameters, GDBusMethodInvocation *invoc
                goto out;
        }
 
-       message_port_lock_mutex();
        if (!is_local_port_registed(remote_port, remote_trusted, &local_reg_id, &mi)) {
                _LOGE("Invalid argument : remote_port:(%s) trusted(%d)", remote_port, remote_trusted);
-               message_port_unlock_mutex();
                goto out;
        }
 
        callback_info = __create_callback_info(mi, local_appid);
        if (callback_info == NULL) {
-               message_port_unlock_mutex();
                goto out;
        }
-       message_port_unlock_mutex();
 
        if (!local_port) {
                _LOGE("Invalid argument : local_port");
@@ -693,37 +616,23 @@ static bool __receive_message(GVariant *parameters, GDBusMethodInvocation *invoc
                                goto out;
                        }
 
-                       key_info = __create_callback_key_info(callback_info);
-                       if (key_info == NULL) {
-                               _LOGE("out of memory");
-                               ret = false;
-                               goto out;
-                       }
-
                        callback_info->g_src_id = g_io_add_watch_full(
                                        callback_info->gio_read,
                                        G_PRIORITY_DEFAULT,
                                        G_IO_IN | G_IO_HUP,
-                                       __socket_request_handler,
-                                       (gpointer)key_info,
-                                       __socket_destroy_handler);
+                                       __socket_request_handler, callback_info,
+                                       NULL);
                        if (callback_info->g_src_id == 0) {
                                _LOGE("fail to add watch on socket");
-                               free(key_info);
                                ret = false;
                                goto out;
                        }
-
-                       message_port_lock_mutex();
-                       if (__callback_info_append(callback_info) == false) {
-                               _LOGE("fail to append callback_info");
-                               ret = false;
-                       }
-                       message_port_unlock_mutex();
+                       __callback_info_append(callback_info);
                }
        }
 
 out:
+       message_port_unlock_mutex();
        if (ret == false)
                __callback_info_free(callback_info);
 
@@ -873,11 +782,34 @@ static const GDBusInterfaceVTable interface_vtable = {
        NULL
 };
 
-static int __register_dbus_interface(const char *port_name, bool is_trusted)
+static bool __message_port_register_port(const int local_id, const char *local_port, bool is_trusted, message_port_message_cb callback, void *user_data)
 {
+       message_port_local_port_info_s *mi = (message_port_local_port_info_s *)calloc(1, sizeof(message_port_local_port_info_s));
+       retvm_if(!mi, false, "Malloc failed");
+
+       mi->callback = callback;
+       mi->is_trusted = is_trusted;
+       mi->port_name = strdup(local_port);
+       if (mi->port_name == NULL) {
+/* LCOV_EXCL_START */
+               _LOGE("Malloc failed (%s)", local_port);
+               free(mi);
+               return false;
+/* LCOV_EXCL_STOP */
+       }
+       mi->local_id = local_id;
+       mi->user_data = user_data;
+
+       g_hash_table_insert(__local_port_info, GINT_TO_POINTER(mi->local_id), mi);
+       return true;
+}
 
+static int __register_dbus_interface(const char *port_name,
+               bool is_trusted, message_port_message_cb callback, void *user_data)
+{
        GDBusNodeInfo *introspection_data = NULL;
        int registration_id = 0;
+       int ret = MESSAGE_PORT_ERROR_NONE;
 
        static gchar introspection_prefix[] =
                "<node>"
@@ -902,7 +834,6 @@ static int __register_dbus_interface(const char *port_name, bool is_trusted)
        char *introspection_xml = NULL;
        int introspection_xml_len = 0;
 
-
        int owner_id = 0;
        GError *error = NULL;
        char *bus_name = NULL;
@@ -927,6 +858,32 @@ static int __register_dbus_interface(const char *port_name, bool is_trusted)
 /* LCOV_EXCL_STOP */
        }
 
+       snprintf(introspection_xml, introspection_xml_len, "%s%s%s", introspection_prefix, interface_name, introspection_postfix);
+
+       introspection_data = g_dbus_node_info_new_for_xml(introspection_xml, NULL);
+       if (!introspection_data) {
+               ret = MESSAGE_PORT_ERROR_IO_ERROR;
+               _LOGE("g_dbus_node_info_new_for_xml() is failed.");
+               goto out;
+       }
+
+       registration_id = g_dbus_connection_register_object(gdbus_conn,
+                                               MESSAGEPORT_OBJECT_PATH, introspection_data->interfaces[0],
+                                               &interface_vtable, NULL, NULL, NULL);
+
+       _LOGD("registration_id %d", registration_id);
+
+       if (registration_id == 0) {
+               ret = MESSAGE_PORT_ERROR_IO_ERROR;
+               _LOGE("Failed to g_dbus_connection_register_object");
+               goto out;
+       }
+
+       if (!__message_port_register_port(registration_id, port_name, is_trusted,
+                               callback, user_data)) {
+               ret = MESSAGE_PORT_ERROR_OUT_OF_MEMORY;
+               goto out;
+       }
 
        result = g_dbus_connection_call_sync(
                        gdbus_conn,
@@ -942,39 +899,23 @@ static int __register_dbus_interface(const char *port_name, bool is_trusted)
                        &error);
        if (error) {
                _LOGE("RequestName fail : %s", error->message);
+               ret = MESSAGE_PORT_ERROR_IO_ERROR;
                g_error_free(error);
                goto out;
        }
        if (result == NULL) {
+               ret = MESSAGE_PORT_ERROR_IO_ERROR;
                _LOGE("fail to get name NULL");
                goto out;
        }
        g_variant_get(result, "(u)", &owner_id);
        if (owner_id == 0) {
+               ret = MESSAGE_PORT_ERROR_IO_ERROR;
                _LOGE("Acquiring the own name is failed");
                goto out;
        }
 
-       _LOGD("Acquiring the own name : %d", owner_id);
-
-       snprintf(introspection_xml, introspection_xml_len, "%s%s%s", introspection_prefix, interface_name, introspection_postfix);
-
-       introspection_data = g_dbus_node_info_new_for_xml(introspection_xml, NULL);
-       if (!introspection_data) {
-               _LOGE("g_dbus_node_info_new_for_xml() is failed.");
-               goto out;
-       }
-
-       registration_id = g_dbus_connection_register_object(gdbus_conn,
-                                               MESSAGEPORT_OBJECT_PATH, introspection_data->interfaces[0],
-                                               &interface_vtable, NULL, NULL, NULL);
-
-       _LOGD("registration_id %d", registration_id);
-
-       if (registration_id == 0) {
-               _LOGE("Failed to g_dbus_connection_register_object");
-               goto out;
-       }
+       _LOGI("Acquiring the own name : %d", owner_id);
 
 out:
        if (introspection_data)
@@ -986,30 +927,13 @@ out:
        if (result)
                g_variant_unref(result);
 
-
-       return registration_id;
-}
-
-static bool __message_port_register_port(const int local_id, const char *local_port, bool is_trusted, message_port_message_cb callback, void *user_data)
-{
-       message_port_local_port_info_s *mi = (message_port_local_port_info_s *)calloc(1, sizeof(message_port_local_port_info_s));
-       retvm_if(!mi, false, "Malloc failed");
-
-       mi->callback = callback;
-       mi->is_trusted = is_trusted;
-       mi->port_name = strdup(local_port);
-       if (mi->port_name == NULL) {
-/* LCOV_EXCL_START */
-               _LOGE("Malloc failed (%s)", local_port);
-               free(mi);
-               return false;
-/* LCOV_EXCL_STOP */
+       if (ret != MESSAGE_PORT_ERROR_NONE) {
+               if (registration_id != 0)
+                       g_hash_table_remove(__local_port_info, GINT_TO_POINTER(registration_id));
+               registration_id = ret;
        }
-       mi->local_id = local_id;
-       mi->user_data = user_data;
 
-       g_hash_table_insert(__local_port_info, GINT_TO_POINTER(mi->local_id), mi);
-       return true;
+       return registration_id;
 }
 
 int get_local_port_info(int id, message_port_local_port_info_s **info)
@@ -1042,15 +966,10 @@ int register_message_port(const char *local_port, bool is_trusted, message_port_
                return local_id;
        }
 
-       local_id = __register_dbus_interface(local_port, is_trusted);
+       local_id = __register_dbus_interface(local_port, is_trusted, callback, user_data);
        if (local_id < 1) {
                _LOGE("register_dbus_interface fail !!");
-               return MESSAGE_PORT_ERROR_OUT_OF_MEMORY;
        }
-
-       if (!__message_port_register_port(local_id, local_port, is_trusted, callback, user_data))
-               return MESSAGE_PORT_ERROR_OUT_OF_MEMORY;
-
        return local_id;
 }
 
@@ -1078,7 +997,7 @@ int unregister_local_port(int local_port_id, bool trusted_port)
        if (mi->is_trusted != trusted_port)
                return MESSAGE_PORT_ERROR_INVALID_PARAMETER;
 
-       g_hash_table_remove(__callback_info_hash, GUINT_TO_POINTER(local_port_id));
+       __callback_info_free_by_local_id(local_port_id);
 
        bus_name = get_encoded_name(app_id, mi->port_name, mi->is_trusted);
        if (bus_name == NULL)

diff --git a/test/unit_tests/test_message_port.cc b/test/unit_tests/test_message_port.cc
index d347fa5..2e76c71 100644 (file)
--- a/test/unit_tests/test_message_port.cc
+++ b/test/unit_tests/test_message_port.cc
@@ -311,7 +311,8 @@ TEST_F(MessagePortTest, message_port_register_local_port3) {
   __port_id = 20;
   int port = message_port_register_local_port("PORT3", __message_cb, nullptr);
   EXPECT_EQ(port, __port_id);
-  __io_notify(__io_data);
+  if (__io_notify)
+    __io_notify(__io_data);
 }
 
 TEST_F(MessagePortTest, message_port_register_local_port_n) {
@@ -391,7 +392,8 @@ TEST_F(MessagePortTest, message_port_send_message_with_local_port3) {
       "remote_app", "PORT", message.GetHandle(), 1);
   __call_sync_reply = nullptr;
   EXPECT_EQ(ret, MESSAGE_PORT_ERROR_NONE);
-  __io_notify(__io_data);
+  if (__io_notify)
+    __io_notify(__io_data);
 }
 
 TEST_F(MessagePortTest, message_port_register_trusted_local_port) {