From c13a2fb3806abdd1721a6681529e926cab10f00d 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 | 124 ++++++++++++++++++++++-------------- 2 files changed, 76 insertions(+), 50 deletions(-) diff --git a/src/include/switch_core.h b/src/include/switch_core.h index fbe73404c5..6f5e20eeae 100644 --- a/src/include/switch_core.h +++ b/src/include/switch_core.h @@ -396,7 +396,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 7299a431dd..6a672c6af3 100644 --- a/src/switch_core_media_bug.c +++ b/src/switch_core_media_bug.c @@ -35,22 +35,51 @@ #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; + + 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 (bug->raw_write_buffer) { - switch_buffer_destroy(&bug->raw_write_buffer); + 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); } } @@ -887,7 +916,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; @@ -1031,7 +1060,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; @@ -1181,6 +1210,7 @@ SWITCH_DECLARE(switch_status_t) switch_core_media_bug_remove_all_function(switch { switch_media_bug_t *bp, *last = NULL; switch_status_t status = SWITCH_STATUS_FALSE; + switch_media_bug_t *closed = NULL; if (session->bugs) { switch_thread_rwlock_wrlock(session->bug_rwlock); @@ -1203,12 +1233,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); } @@ -1216,7 +1255,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; @@ -1236,44 +1275,21 @@ SWITCH_DECLARE(switch_status_t) switch_core_media_bug_close(switch_media_bug_t * 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->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->write_video_queue) { + switch_queue_push(bp->write_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); - } - - 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; } @@ -1327,7 +1343,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; @@ -1367,7 +1383,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; } @@ -1378,7 +1394,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); @@ -1394,15 +1410,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); }