Add defensive code to avoid crash

- The user callback can be called in _recorder_idle_event_callback()
  although handle is destroyed after g_rec_idle_event_lock is unlocked.
- Add new mutex and flag to avoid that situation.

[Version] 1.0.1
[Issue Type] Improvement

Change-Id: I6e19239b71c9b41d05c730f9ecd0d4c84c08c682
Signed-off-by: Jeongmo Yang <jm80.yang@samsung.com>

diff --git a/include/recorder_private.h b/include/recorder_private.h
index b5937c2c55a8cdd060d846d2a38db9b94148a2d4..365c19e9917606ff28eae091b023ef2ab4c26276 100644 (file)
--- a/include/recorder_private.h
+++ b/include/recorder_private.h
@@ -147,6 +147,7 @@ typedef struct _recorder_cb_info_s {
 
        /* idle event */
        GList *idle_event_list;
+       gboolean is_in_idle_cb;
 
        /* user callback */
        gpointer user_cb[MUSE_RECORDER_EVENT_TYPE_NUM];

diff --git a/packaging/capi-media-recorder.spec b/packaging/capi-media-recorder.spec
index fa683d997716f93a007949fc207f27bec2d7fb46..b5f743c90aaad0e25e2945660f91bb0e8a1513eb 100644 (file)
--- a/packaging/capi-media-recorder.spec
+++ b/packaging/capi-media-recorder.spec
@@ -1,7 +1,7 @@
 Name:       capi-media-recorder
 Summary:    A Recorder API
-Version:    1.0.0
-Release:    1
+Version:    1.0.1
+Release:    0
 Group:      Multimedia/API
 License:    Apache-2.0
 Source0:    %{name}-%{version}.tar.gz

diff --git a/src/recorder.c b/src/recorder.c
index 906621908619d97361947496674a752509201c56..967bdb895cf6800e052e132a9229723bdfeb6766 100644 (file)
--- a/src/recorder.c
+++ b/src/recorder.c
@@ -588,7 +588,6 @@ static gboolean _recorder_idle_event_callback(gpointer data)
        cb_info = rec_idle_event->cb_info;
        if (cb_info == NULL) {
                REC_LOG_WARNING("recorder cb_info is NULL. event[%p][%d]", rec_idle_event, rec_idle_event->event);
-               g_mutex_unlock(&g_rec_idle_event_lock);
                goto IDLE_EVENT_CALLBACK_DONE;
        }
 
@@ -596,15 +595,22 @@ static gboolean _recorder_idle_event_callback(gpointer data)
        if (cb_info->idle_event_list)
                cb_info->idle_event_list = g_list_remove(cb_info->idle_event_list, (gpointer)rec_idle_event);
 
+       cb_info->is_in_idle_cb = TRUE;
+
        g_mutex_unlock(&g_rec_idle_event_lock);
 
        /* user callback */
        _recorder_client_user_callback(cb_info, rec_idle_event->recv_msg, rec_idle_event->event, NULL);
 
+       g_mutex_lock(&g_rec_idle_event_lock);
+
+       cb_info->is_in_idle_cb = FALSE;
+
 IDLE_EVENT_CALLBACK_DONE:
+       g_mutex_unlock(&g_rec_idle_event_lock);
+
        /* release event */
        g_free(rec_idle_event);
-       rec_idle_event = NULL;
 
        return FALSE;
 }
@@ -620,44 +626,32 @@ static void _recorder_deactivate_idle_event_all(recorder_cb_info_s *cb_info)
                return;
        }
 
-       g_mutex_lock(&g_rec_idle_event_lock);
-
        if (cb_info->idle_event_list == NULL) {
-               REC_LOG_INFO("No event");
-       } else {
-               list = cb_info->idle_event_list;
-
-               while (list) {
-                       rec_idle_event = list->data;
-                       list = g_list_next(list);
-
-                       if (!rec_idle_event) {
-                               REC_LOG_WARNING("The event is NULL");
-                               continue;
-                       }
-
-                       if (g_idle_remove_by_data(rec_idle_event)) {
-                               REC_LOG_WARNING("remove idle event[%p] done", rec_idle_event);
+               REC_LOG_INFO("No remained idle event");
+               return;
+       }
 
-                               cb_info->idle_event_list = g_list_remove(cb_info->idle_event_list, (gpointer)rec_idle_event);
-                               g_free(rec_idle_event);
+       list = cb_info->idle_event_list;
 
-                               continue;
-                       }
+       while (list) {
+               rec_idle_event = list->data;
+               list = g_list_next(list);
 
-                       REC_LOG_WARNING("set NULL cb_info for event[%p][%d], it will be freed on idle callback",
-                               rec_idle_event, rec_idle_event->event);
+               if (!rec_idle_event) {
+                       REC_LOG_WARNING("The event is NULL");
+                       continue;
+               }
 
-                       rec_idle_event->cb_info = NULL;
+               REC_LOG_WARNING("set NULL cb_info for event[%d] %p, it will be freed on idle callback",
+                       rec_idle_event->event, rec_idle_event);
 
-                       cb_info->idle_event_list = g_list_remove(cb_info->idle_event_list, (gpointer)rec_idle_event);
-               }
+               rec_idle_event->cb_info = NULL;
 
-               g_list_free(cb_info->idle_event_list);
-               cb_info->idle_event_list = NULL;
+               cb_info->idle_event_list = g_list_remove(cb_info->idle_event_list, (gpointer)rec_idle_event);
        }
 
-       g_mutex_unlock(&g_rec_idle_event_lock);
+       g_list_free(cb_info->idle_event_list);
+       cb_info->idle_event_list = NULL;
 
        return;
 }
@@ -925,7 +919,7 @@ static void __recorder_process_msg(recorder_cb_info_s *cb_info, char *msg, int *
 
                        cb_info->api_activating[api]++;
 
-                       g_cond_signal(&cb_info->api_cond[api]);
+                       g_cond_broadcast(&cb_info->api_cond[api]);
                } else {
                        REC_LOG_ERROR("no waiting for api[%d]", api);
                }
@@ -1004,7 +998,7 @@ static void *_recorder_msg_handler_func(gpointer data)
 
                                        REC_LOG_DEBUG("recorder api[%d] - return[0x%x]", api, ret);
 
-                                       g_cond_signal(&cb_info->api_cond[api]);
+                                       g_cond_broadcast(&cb_info->api_cond[api]);
                                } else {
                                        REC_LOG_ERROR("no waiting for api [%d]", api);
                                }
@@ -1137,6 +1131,11 @@ static void *_recorder_msg_recv_func(gpointer data)
                muse_core_msg_free(error_msg);
                error_msg = NULL;
 
+               for (i = 0 ; i < MUSE_RECORDER_API_MAX ; i++) {
+                       if (cb_info->api_waiting[i])
+                               g_cond_broadcast(&cb_info->api_cond[i]);
+               }
+
                REC_LOG_ERROR("add error msg for service disconnection done");
 //LCOV_EXCL_STOP
        }
@@ -1336,6 +1335,11 @@ static int _recorder_client_wait_for_cb_return(muse_recorder_api_e api, recorder
                        goto _CB_RETURN_END;
                }
 
+               if (!cb_info->is_server_connected) {
+                       REC_LOG_ERROR("server is disconnected");
+                       return RECORDER_ERROR_SERVICE_DISCONNECTED;
+               }
+
                if (cb_info->api_activating[api] <= 0)
                        REC_LOG_WARNING("api[%d] invalid signal received, wait again...", api);
        }
@@ -1505,9 +1509,6 @@ static void _recorder_client_callback_destroy(recorder_cb_info_s *cb_info)
 
        g_return_if_fail(cb_info != NULL);
 
-       /* remove idle event */
-       _recorder_deactivate_idle_event_all(cb_info);
-
        REC_LOG_INFO("MSG receive thread[%p] destroy", cb_info->msg_recv_thread);
 
        g_thread_join(cb_info->msg_recv_thread);
@@ -1874,6 +1875,7 @@ int recorder_destroy(recorder_h recorder)
        int ret = RECORDER_ERROR_NONE;
        muse_recorder_api_e api = MUSE_RECORDER_API_DESTROY;
        recorder_cli_s *pc = (recorder_cli_s *)recorder;
+       g_autoptr(GMutexLocker) locker = g_mutex_locker_new(&g_rec_idle_event_lock);
 
        if (!pc || !pc->cb_info) {
                REC_LOG_ERROR("NULL handle");
@@ -1882,18 +1884,30 @@ int recorder_destroy(recorder_h recorder)
 
        REC_LOG_INFO("ENTER");
 
+       if (pc->cb_info->is_in_idle_cb) {
+               REC_LOG_ERROR("should not be called in idle callback");
+               return RECORDER_ERROR_INVALID_OPERATION;
+       }
+
        if (pc->cb_info->is_server_connected)
                _recorder_msg_send(api, pc->cb_info, &ret, RECORDER_CB_TIMEOUT);
        else
                REC_LOG_WARNING("server disconnected. release resource without send message.");
 
-       if (ret == RECORDER_ERROR_NONE) {
-               _recorder_client_callback_destroy(pc->cb_info);
-               g_free(pc);
-               pc = NULL;
+       if (ret != RECORDER_ERROR_NONE) {
+               REC_LOG_ERROR("handle destroy failed[0x%x]", ret);
+               return ret;
        }
 
-       REC_LOG_INFO("ret[0x%x]", ret);
+       _recorder_deactivate_idle_event_all(pc->cb_info);
+
+       g_clear_pointer(&locker, g_mutex_locker_free);
+
+       _recorder_client_callback_destroy(pc->cb_info);
+
+       g_free(pc);
+
+       REC_LOG_INFO("done");
 
        return ret;
 }