From a14651110bc9222a8a419f90c4d14707521d68e9 Mon Sep 17 00:00:00 2001 From: Anthony Minessale Date: Tue, 23 May 2017 12:30:50 -0500 Subject: [PATCH] FS-10231: [freeswitch-core] Some media bugs not fully cleaned up when session is destroyed #comment Regression causing deadlock when holding write lock and close/destroying all the bugs but the video recording thread tries to read lock. Separating destroy out so it can be called outside the lock after the bugs are detached. --- src/include/switch_core.h | 2 +- src/switch_core_media_bug.c | 132 +++++++++++++++++++++--------------- 2 files changed, 80 insertions(+), 54 deletions(-) diff --git a/src/include/switch_core.h b/src/include/switch_core.h index 318562ca88..eddf0f5654 100644 --- a/src/include/switch_core.h +++ b/src/include/switch_core.h @@ -398,7 +398,7 @@ SWITCH_DECLARE(switch_status_t) switch_core_media_bug_remove_callback(switch_cor \param bug bug to remove \return SWITCH_STATUS_SUCCESS if the operation was a success */ -SWITCH_DECLARE(switch_status_t) switch_core_media_bug_close(_Inout_ switch_media_bug_t **bug); +SWITCH_DECLARE(switch_status_t) switch_core_media_bug_close(_Inout_ switch_media_bug_t **bug, switch_bool_t destroy); /*! \brief Remove all media bugs from the session \param session the session to remove the bugs from diff --git a/src/switch_core_media_bug.c b/src/switch_core_media_bug.c index fa2121ee9a..83892017ab 100644 --- a/src/switch_core_media_bug.c +++ b/src/switch_core_media_bug.c @@ -35,22 +35,56 @@ #include "switch.h" #include "private/switch_core_pvt.h" -static void switch_core_media_bug_destroy(switch_media_bug_t *bug) +static void switch_core_media_bug_destroy(switch_media_bug_t **bug) { switch_event_t *event = NULL; + switch_media_bug_t *bp = *bug; - if (bug->raw_read_buffer) { - switch_buffer_destroy(&bug->raw_read_buffer); + *bug = NULL; + + if (bp->text_buffer) { + switch_buffer_destroy(&bp->text_buffer); + switch_safe_free(bp->text_framedata); } - if (bug->raw_write_buffer) { - switch_buffer_destroy(&bug->raw_write_buffer); + switch_img_free(&bp->spy_img[0]); + switch_img_free(&bp->spy_img[1]); + + if (bp->video_bug_thread) { + switch_status_t st; + int i; + + for (i = 0; i < 2; i++) { + void *pop; + switch_image_t *img; + + if (bp->spy_video_queue[i]) { + while (switch_queue_trypop(bp->spy_video_queue[i], &pop) == SWITCH_STATUS_SUCCESS && pop) { + img = (switch_image_t *) pop; + switch_img_free(&img); + } + } + } + + switch_thread_join(&st, bp->video_bug_thread); + } + + if (switch_test_flag(bp, SMBF_READ_VIDEO_PATCH) && bp->session->video_read_codec) { + switch_clear_flag(bp->session->video_read_codec, SWITCH_CODEC_FLAG_VIDEO_PATCHING); + } + + if (bp->raw_read_buffer) { + switch_buffer_destroy(&bp->raw_read_buffer); + } + + if (bp->raw_write_buffer) { + switch_buffer_destroy(&bp->raw_write_buffer); } if (switch_event_create(&event, SWITCH_EVENT_MEDIA_BUG_STOP) == SWITCH_STATUS_SUCCESS) { - switch_event_add_header(event, SWITCH_STACK_BOTTOM, "Media-Bug-Function", "%s", bug->function); - switch_event_add_header(event, SWITCH_STACK_BOTTOM, "Media-Bug-Target", "%s", bug->target); - if (bug->session) switch_channel_event_set_data(bug->session->channel, event); + switch_event_add_header(event, SWITCH_STACK_BOTTOM, "Media-Bug-Function", "%s", bp->function); + switch_event_add_header(event, SWITCH_STACK_BOTTOM, "Media-Bug-Target", "%s", bp->target); + if (bp->session) switch_channel_event_set_data(bp->session->channel, event); switch_event_fire(&event); } } @@ -899,7 +933,7 @@ SWITCH_DECLARE(switch_status_t) switch_core_media_bug_add(switch_core_session_t if (bug->callback) { switch_bool_t result = bug->callback(bug, bug->user_data, SWITCH_ABC_TYPE_INIT); if (result == SWITCH_FALSE) { - switch_core_media_bug_destroy(bug); + switch_core_media_bug_destroy(&bug); switch_log_printf(SWITCH_CHANNEL_SESSION_LOG(session), SWITCH_LOG_ERROR, "Error attaching BUG to %s\n", switch_channel_get_name(session->channel)); return SWITCH_STATUS_GENERR; @@ -1043,7 +1077,7 @@ SWITCH_DECLARE(switch_status_t) switch_core_media_bug_transfer_callback(switch_c switch_core_media_bug_add(new_session, cur->function, cur->target, cur->callback, user_data_dup_func(new_session, cur->user_data), cur->stop_time, cur->flags, &new_bug); - switch_core_media_bug_destroy(cur); + switch_core_media_bug_destroy(&cur); total++; } else { last = cur; @@ -1193,6 +1227,7 @@ SWITCH_DECLARE(switch_status_t) switch_core_media_bug_remove_all_function(switch { switch_media_bug_t *bp, *last = NULL, *next = NULL; switch_status_t status = SWITCH_STATUS_FALSE; + switch_media_bug_t *closed = NULL; if (session->bugs) { switch_thread_rwlock_wrlock(session->bug_rwlock); @@ -1217,12 +1252,21 @@ SWITCH_DECLARE(switch_status_t) switch_core_media_bug_remove_all_function(switch session->bugs = bp->next; } - switch_core_media_bug_close(&bp); + bp->next = closed; + closed = bp; + + switch_core_media_bug_close(&bp, SWITCH_FALSE); } switch_thread_rwlock_unlock(session->bug_rwlock); status = SWITCH_STATUS_SUCCESS; } + if (closed) { + for (bp = session->bugs; bp; bp = bp->next) { + switch_core_media_bug_destroy(&bp); + } + } + if (switch_core_codec_ready(&session->bug_codec)) { switch_core_codec_destroy(&session->bug_codec); } @@ -1230,7 +1274,7 @@ SWITCH_DECLARE(switch_status_t) switch_core_media_bug_remove_all_function(switch return status; } -SWITCH_DECLARE(switch_status_t) switch_core_media_bug_close(switch_media_bug_t **bug) +SWITCH_DECLARE(switch_status_t) switch_core_media_bug_close(switch_media_bug_t **bug, switch_bool_t destroy) { switch_media_bug_t *bp = *bug; @@ -1244,55 +1288,27 @@ SWITCH_DECLARE(switch_status_t) switch_core_media_bug_close(switch_media_bug_t * bp->callback(bp, bp->user_data, SWITCH_ABC_TYPE_CLOSE); } - if (bp->text_buffer) { - switch_buffer_destroy(&bp->text_buffer); - switch_safe_free(bp->text_framedata); - } - if (switch_test_flag(bp, SMBF_READ_VIDEO_STREAM) || switch_test_flag(bp, SMBF_WRITE_VIDEO_STREAM) || switch_test_flag(bp, SMBF_READ_VIDEO_PING) || switch_test_flag(bp, SMBF_WRITE_VIDEO_PING)) { switch_channel_clear_flag_recursive(bp->session->channel, CF_VIDEO_DECODED_READ); } bp->ready = 0; - switch_img_free(&bp->spy_img[0]); - switch_img_free(&bp->spy_img[1]); - - if (bp->video_bug_thread) { - switch_status_t st; - int i; - - for (i = 0; i < 2; i++) { - void *pop; - switch_image_t *img; - - if (bp->spy_video_queue[i]) { - while (switch_queue_trypop(bp->spy_video_queue[i], &pop) == SWITCH_STATUS_SUCCESS && pop) { - img = (switch_image_t *) pop; - switch_img_free(&img); - } - } - } - - if (bp->read_video_queue) { - switch_queue_push(bp->read_video_queue, NULL); - } - - if (bp->write_video_queue) { - switch_queue_push(bp->write_video_queue, NULL); - } - - switch_thread_join(&st, bp->video_bug_thread); + if (bp->read_video_queue) { + switch_queue_push(bp->read_video_queue, NULL); } - if (switch_test_flag(bp, SMBF_READ_VIDEO_PATCH) && bp->session->video_read_codec) { - switch_clear_flag(bp->session->video_read_codec, SWITCH_CODEC_FLAG_VIDEO_PATCHING); + if (bp->write_video_queue) { + switch_queue_push(bp->write_video_queue, NULL); } - switch_core_media_bug_destroy(bp); switch_log_printf(SWITCH_CHANNEL_SESSION_LOG(switch_core_media_bug_get_session(*bug)), SWITCH_LOG_DEBUG, "Removing BUG from %s\n", switch_channel_get_name(bp->session->channel)); - *bug = NULL; + + if (destroy) { + switch_core_media_bug_destroy(bug); + } + return SWITCH_STATUS_SUCCESS; } @@ -1346,7 +1362,7 @@ SWITCH_DECLARE(switch_status_t) switch_core_media_bug_remove(switch_core_session switch_thread_rwlock_unlock(session->bug_rwlock); if (bp) { - status = switch_core_media_bug_close(&bp); + status = switch_core_media_bug_close(&bp, SWITCH_TRUE); } return status; @@ -1386,7 +1402,7 @@ SWITCH_DECLARE(uint32_t) switch_core_media_bug_prune(switch_core_session_t *sess if (bp) { switch_clear_flag(bp, SMBF_LOCK); bp->thread_id = 0; - switch_core_media_bug_close(&bp); + switch_core_media_bug_close(&bp, SWITCH_TRUE); ttl++; goto top; } @@ -1397,7 +1413,7 @@ SWITCH_DECLARE(uint32_t) switch_core_media_bug_prune(switch_core_session_t *sess SWITCH_DECLARE(switch_status_t) switch_core_media_bug_remove_callback(switch_core_session_t *session, switch_media_bug_callback_t callback) { - switch_media_bug_t *cur = NULL, *bp = NULL, *last = NULL; + switch_media_bug_t *cur = NULL, *bp = NULL, *last = NULL, *closed = NULL; int total = 0; switch_thread_rwlock_wrlock(session->bug_rwlock); @@ -1413,15 +1429,25 @@ SWITCH_DECLARE(switch_status_t) switch_core_media_bug_remove_callback(switch_cor } else { session->bugs = cur->next; } - if (switch_core_media_bug_close(&cur) == SWITCH_STATUS_SUCCESS) { + if (switch_core_media_bug_close(&cur, SWITCH_FALSE) == SWITCH_STATUS_SUCCESS) { total++; } + + cur->next = closed; + closed = cur; + } else { last = cur; } } } + if (closed) { + for (bp = session->bugs; bp; bp = bp->next) { + switch_core_media_bug_destroy(&bp); + } + } + if (!session->bugs && switch_core_codec_ready(&session->bug_codec)) { switch_core_codec_destroy(&session->bug_codec); }