Fix session list deadlock

On session destroy, the session list lock is acquired but this call
could be made inside a section where this lock is already acquired. The
call has been changed to using the session pointer and not the name thus
removing a useless list walk.

Signed-off-by: David Goulet <david.goulet@polymtl.ca>

diff --git a/ltt-sessiond/main.c b/ltt-sessiond/main.c
index c66ecb95690df819ce95825b83f20ac147dd4137..edeb86d29d5b168b795af6869ef5dce86811971a 100644 (file)
--- a/ltt-sessiond/main.c
+++ b/ltt-sessiond/main.c
@@ -2312,7 +2312,7 @@ static int cmd_destroy_session(struct ltt_session *session, char *name)
                perror("write kernel poll pipe");
        }
 
                perror("write kernel poll pipe");
        }
 

-       ret = session_destroy(name);
+       ret = session_destroy(session);

 
        return ret;
 }
 
        return ret;
 }

diff --git a/ltt-sessiond/session.c b/ltt-sessiond/session.c
index b13f4a916c8963357cea305d4677649a7f955f05..ae83e45aec1b7610992772d48b35615a8adb1ba1 100644 (file)
--- a/ltt-sessiond/session.c
+++ b/ltt-sessiond/session.c
@@ -144,23 +144,20 @@ struct ltt_session *session_find_by_name(char *name)
  *
  * Return -1 if no session is found.  On success, return 1;
  */
  *
  * Return -1 if no session is found.  On success, return 1;
  */

-int session_destroy(char *name)
+int session_destroy(struct ltt_session *session)

 {
 {

-       struct ltt_session *iter, *tmp;
-
-       session_lock_list();
-       cds_list_for_each_entry_safe(iter, tmp, &ltt_session_list.head, list) {
-               if (strcmp(iter->name, name) == 0) {
-                       DBG("Destroying session %s", iter->name);
-                       del_session_list(iter);
-                       free(iter->name);
-                       free(iter->path);
-                       pthread_mutex_destroy(&iter->lock);
-                       free(iter);
-                       break;
-               }
+       /* Safety check */
+       if (session == NULL) {
+               ERR("Session pointer was null on session destroy");
+               return LTTCOMM_OK;

        }
        }

-       session_unlock_list();
+
+       DBG("Destroying session %s", session->name);
+       del_session_list(session);
+       free(session->name);
+       free(session->path);
+       pthread_mutex_destroy(&session->lock);
+       free(session);

 
        return LTTCOMM_OK;
 }
 
        return LTTCOMM_OK;
 }

diff --git a/ltt-sessiond/session.h b/ltt-sessiond/session.h
index bbbbffc4554d3aaa6e0dd5abf56da6d1aa6c738c..2f40f585d47f73b06c790577b98da7ed23e1677b 100644 (file)
--- a/ltt-sessiond/session.h
+++ b/ltt-sessiond/session.h
@@ -71,7 +71,7 @@ struct ltt_session {
 
 /* Prototypes */
 int session_create(char *name, char *path);
 
 /* Prototypes */
 int session_create(char *name, char *path);

-int session_destroy(char *name);
+int session_destroy(struct ltt_session *session);

 
 void session_lock(struct ltt_session *session);
 void session_lock_list(void);
 
 void session_lock(struct ltt_session *session);
 void session_lock_list(void);

diff --git a/tests/test_sessions.c b/tests/test_sessions.c
index 8ea365fed684f5d2ee09d08d79ab3400390ee8a2..3e9bbbbb5ae64307d1c020beee2e529593c48acf 100644 (file)
--- a/tests/test_sessions.c
+++ b/tests/test_sessions.c
@@ -142,15 +142,18 @@ static int create_one_session(char *name, char *path)
 /*
  * Test deletion of 1 session
  */
 /*
  * Test deletion of 1 session
  */

-static int destroy_one_session(char *name)
+static int destroy_one_session(struct ltt_session *session)

 {
        int ret;
 
 {
        int ret;
 

-       ret = session_destroy(name);
+       ret = session_destroy(session);

 
        if (ret == LTTCOMM_OK) {
                /* Validate */
 
        if (ret == LTTCOMM_OK) {
                /* Validate */

-               ret = find_session_name(name);
+               if (session == NULL) {
+                       return 0;
+               }
+               ret = find_session_name(session->name);

                if (ret < 0) {
                        /* Success, -1 means that the sesion is NOT found */
                        return 0;
                if (ret < 0) {
                        /* Success, -1 means that the sesion is NOT found */
                        return 0;


@@ -201,12 +204,6 @@ static int fuzzing_destroy_args(void)
                return -1;
        }
 
                return -1;
        }
 

-       ret = destroy_one_session(OVERFLOW_SESSION_NAME);
-       if (ret > 0) {
-               printf("Session destroyed with %s\n", OVERFLOW_SESSION_NAME);
-               return -1;
-       }
-

        /* Session list must be 0 */
        assert(!session_list->count);
 
        /* Session list must be 0 */
        assert(!session_list->count);
 


@@ -275,7 +272,7 @@ int main(int argc, char **argv)
        PRINT_OK();
 
        printf("Destroy 1 session %s: ", SESSION1);
        PRINT_OK();
 
        printf("Destroy 1 session %s: ", SESSION1);

-       ret = destroy_one_session(SESSION1);
+       ret = destroy_one_session(tmp);

        if (ret < 0) {
                return -1;
        }
        if (ret < 0) {
                return -1;
        }


@@ -319,7 +316,7 @@ int main(int argc, char **argv)
        printf("Destroying %d sessions: ", MAX_SESSIONS);
        for (i = 0; i < MAX_SESSIONS; i++) {
                cds_list_for_each_entry_safe(iter, tmp, &session_list->head, list) {
        printf("Destroying %d sessions: ", MAX_SESSIONS);
        for (i = 0; i < MAX_SESSIONS; i++) {
                cds_list_for_each_entry_safe(iter, tmp, &session_list->head, list) {

-                       ret = destroy_one_session(iter->name);
+                       ret = destroy_one_session(iter);

                        if (ret < 0) {
                                printf("session %d (name: %s) creation failed\n", i, iter->name);
                                return -1;
                        if (ret < 0) {
                                printf("session %d (name: %s) creation failed\n", i, iter->name);
                                return -1;