diff --git a/apps/app_chanspy.c b/apps/app_chanspy.c index 50d8d63210..4814f4ce5d 100644 --- a/apps/app_chanspy.c +++ b/apps/app_chanspy.c @@ -506,18 +506,24 @@ static struct ast_generator spygen = { static int start_spying(struct ast_autochan *autochan, const char *spychan_name, struct ast_audiohook *audiohook, struct ast_flags *flags) { + int res; + + ast_autochan_channel_lock(autochan); ast_log(LOG_NOTICE, "Attaching %s to %s\n", spychan_name, ast_channel_name(autochan->chan)); - if(ast_test_flag(flags, OPTION_READONLY)) { + + if (ast_test_flag(flags, OPTION_READONLY)) { ast_set_flag(audiohook, AST_AUDIOHOOK_MUTE_WRITE); } else { ast_set_flag(audiohook, AST_AUDIOHOOK_TRIGGER_SYNC); } - if(ast_test_flag(flags, OPTION_LONG_QUEUE)) { + if (ast_test_flag(flags, OPTION_LONG_QUEUE)) { ast_debug(9, "Using a long queue to store audio frames in spy audiohook\n"); } else { ast_set_flag(audiohook, AST_AUDIOHOOK_SMALL_QUEUE); } - return ast_audiohook_attach(autochan->chan, audiohook); + res = ast_audiohook_attach(autochan->chan, audiohook); + ast_autochan_channel_unlock(autochan); + return res; } static void change_spy_mode(const char digit, struct ast_flags *flags) @@ -601,8 +607,14 @@ static int attach_barge(struct ast_autochan *spyee_autochan, { int retval = 0; struct ast_autochan *internal_bridge_autochan; - RAII_VAR(struct ast_channel *, bridged, ast_channel_bridge_peer(spyee_autochan->chan), ast_channel_cleanup); + struct ast_channel *spyee_chan; + RAII_VAR(struct ast_channel *, bridged, NULL, ast_channel_cleanup); + ast_autochan_channel_lock(spyee_autochan); + spyee_chan = ast_channel_ref(spyee_autochan->chan); + ast_autochan_channel_unlock(spyee_autochan); + bridged = ast_channel_bridge_peer(spyee_chan); + ast_channel_unref(spyee_chan); if (!bridged) { return -1; } @@ -614,12 +626,10 @@ static int attach_barge(struct ast_autochan *spyee_autochan, return -1; } - ast_autochan_channel_lock(internal_bridge_autochan); if (start_spying(internal_bridge_autochan, spyer_name, bridge_whisper_audiohook, flags)) { ast_log(LOG_WARNING, "Unable to attach barge audiohook on spyee '%s'. Barge mode disabled.\n", name); retval = -1; } - ast_autochan_channel_unlock(internal_bridge_autochan); *spyee_bridge_autochan = internal_bridge_autochan; @@ -639,21 +649,25 @@ static int channel_spy(struct ast_channel *chan, struct ast_autochan *spyee_auto struct ast_autochan *spyee_bridge_autochan = NULL; const char *spyer_name; - if (ast_check_hangup(chan) || ast_check_hangup(spyee_autochan->chan) || - ast_test_flag(ast_channel_flags(spyee_autochan->chan), AST_FLAG_ZOMBIE)) { + ast_channel_lock(chan); + if (ast_check_hangup(chan)) { + ast_channel_unlock(chan); return 0; } - - ast_channel_lock(chan); spyer_name = ast_strdupa(ast_channel_name(chan)); ast_channel_unlock(chan); ast_autochan_channel_lock(spyee_autochan); + if (ast_check_hangup(spyee_autochan->chan) + || ast_test_flag(ast_channel_flags(spyee_autochan->chan), AST_FLAG_ZOMBIE)) { + ast_autochan_channel_unlock(spyee_autochan); + return 0; + } name = ast_strdupa(ast_channel_name(spyee_autochan->chan)); - ast_autochan_channel_unlock(spyee_autochan); ast_verb(2, "Spying on channel %s\n", name); publish_chanspy_message(chan, spyee_autochan->chan, 1); + ast_autochan_channel_unlock(spyee_autochan); memset(&csth, 0, sizeof(csth)); ast_copy_flags(&csth.flags, flags, AST_FLAGS_ALL); @@ -845,7 +859,7 @@ static int channel_spy(struct ast_channel *chan, struct ast_autochan *spyee_auto } static struct ast_autochan *next_channel(struct ast_channel_iterator *iter, - struct ast_autochan *autochan, struct ast_channel *chan) + struct ast_channel *chan) { struct ast_channel *next; struct ast_autochan *autochan_store; @@ -982,11 +996,12 @@ static int common_exec(struct ast_channel *chan, struct ast_flags *flags, waitms = 100; num_spyed_upon = 0; - for (autochan = next_channel(iter, autochan, chan); - autochan; - prev = autochan->chan, ast_autochan_destroy(autochan), - autochan = next_autochan ? next_autochan : - next_channel(iter, autochan, chan), next_autochan = NULL) { + for (autochan = next_channel(iter, chan); + autochan; + prev = autochan->chan, + ast_autochan_destroy(autochan), + autochan = next_autochan ?: next_channel(iter, chan), + next_autochan = NULL) { int igrp = !mygroup; int ienf = !myenforced; @@ -1000,13 +1015,19 @@ static int common_exec(struct ast_channel *chan, struct ast_flags *flags, break; } - if (ast_test_flag(flags, OPTION_BRIDGED) && !ast_channel_is_bridged(autochan->chan)) { + ast_autochan_channel_lock(autochan); + if (ast_test_flag(flags, OPTION_BRIDGED) + && !ast_channel_is_bridged(autochan->chan)) { + ast_autochan_channel_unlock(autochan); continue; } - if (ast_check_hangup(autochan->chan) || ast_test_flag(ast_channel_flags(autochan->chan), AST_FLAG_SPYING)) { + if (ast_check_hangup(autochan->chan) + || ast_test_flag(ast_channel_flags(autochan->chan), AST_FLAG_SPYING)) { + ast_autochan_channel_unlock(autochan); continue; } + ast_autochan_channel_unlock(autochan); if (mygroup) { int num_groups = 0; @@ -1024,11 +1045,13 @@ static int common_exec(struct ast_channel *chan, struct ast_flags *flags, /* Before dahdi scan was part of chanspy, it would use the "GROUP" variable * rather than "SPYGROUP", this check is done to preserve expected behavior */ + ast_autochan_channel_lock(autochan); if (ast_test_flag(flags, OPTION_DAHDI_SCAN)) { group = pbx_builtin_getvar_helper(autochan->chan, "GROUP"); } else { group = pbx_builtin_getvar_helper(autochan->chan, "SPYGROUP"); } + ast_autochan_channel_unlock(autochan); if (!ast_strlen_zero(group)) { ast_copy_string(dup_group, group, sizeof(dup_group)); @@ -1056,7 +1079,9 @@ static int common_exec(struct ast_channel *chan, struct ast_flags *flags, snprintf(buffer, sizeof(buffer) - 1, ":%s:", myenforced); + ast_autochan_channel_lock(autochan); ast_copy_string(ext + 1, ast_channel_name(autochan->chan), sizeof(ext) - 1); + ast_autochan_channel_unlock(autochan); if ((end = strchr(ext, '-'))) { *end++ = ':'; *end = '\0'; @@ -1078,7 +1103,9 @@ static int common_exec(struct ast_channel *chan, struct ast_flags *flags, char *ptr, *s; strcpy(peer_name, "spy-"); + ast_autochan_channel_lock(autochan); strncat(peer_name, ast_channel_name(autochan->chan), AST_NAME_STRLEN - 4 - 1); + ast_autochan_channel_unlock(autochan); if ((ptr = strchr(peer_name, '/'))) { *ptr++ = '\0'; for (s = peer_name; s < ptr; s++) { @@ -1143,12 +1170,14 @@ static int common_exec(struct ast_channel *chan, struct ast_flags *flags, next = ast_channel_unref(next); } else { /* stay on this channel, if it is still valid */ + ast_autochan_channel_lock(autochan); if (!ast_check_hangup(autochan->chan)) { next_autochan = ast_autochan_setup(autochan->chan); } else { /* the channel is gone */ next_autochan = NULL; } + ast_autochan_channel_unlock(autochan); } } else if (res == 0 && ast_test_flag(flags, OPTION_EXITONHANGUP)) { ast_autochan_destroy(autochan); diff --git a/apps/app_mixmonitor.c b/apps/app_mixmonitor.c index a5a00cc4ef..3258b301fa 100644 --- a/apps/app_mixmonitor.c +++ b/apps/app_mixmonitor.c @@ -617,6 +617,16 @@ static void mixmonitor_save_prep(struct mixmonitor *mixmonitor, char *filename, } } +static int mixmonitor_autochan_is_bridged(struct ast_autochan *autochan) +{ + int is_bridged; + + ast_autochan_channel_lock(autochan); + is_bridged = ast_channel_is_bridged(autochan->chan); + ast_autochan_channel_unlock(autochan); + return is_bridged; +} + static void *mixmonitor_thread(void *obj) { struct mixmonitor *mixmonitor = obj; @@ -674,8 +684,7 @@ static void *mixmonitor_thread(void *obj) ast_audiohook_unlock(&mixmonitor->audiohook); if (!ast_test_flag(mixmonitor, MUXFLAG_BRIDGED) - || (mixmonitor->autochan->chan - && ast_channel_is_bridged(mixmonitor->autochan->chan))) { + || mixmonitor_autochan_is_bridged(mixmonitor->autochan)) { ast_mutex_lock(&mixmonitor->mixmonitor_ds->lock); /* Write out the frame(s) */ @@ -724,11 +733,11 @@ static void *mixmonitor_thread(void *obj) ast_audiohook_unlock(&mixmonitor->audiohook); - ast_autochan_channel_lock(mixmonitor->autochan); if (ast_test_flag(mixmonitor, MUXFLAG_BEEP_STOP)) { + ast_autochan_channel_lock(mixmonitor->autochan); ast_stream_and_wait(mixmonitor->autochan->chan, "beep", ""); + ast_autochan_channel_unlock(mixmonitor->autochan); } - ast_autochan_channel_unlock(mixmonitor->autochan); ast_autochan_destroy(mixmonitor->autochan); @@ -800,11 +809,11 @@ static int setup_mixmonitor_ds(struct mixmonitor *mixmonitor, struct ast_channel return -1; } - ast_autochan_channel_lock(mixmonitor->autochan); if (ast_test_flag(mixmonitor, MUXFLAG_BEEP_START)) { + ast_autochan_channel_lock(mixmonitor->autochan); ast_stream_and_wait(mixmonitor->autochan->chan, "beep", ""); + ast_autochan_channel_unlock(mixmonitor->autochan); } - ast_autochan_channel_unlock(mixmonitor->autochan); mixmonitor_ds->samp_rate = 8000; mixmonitor_ds->audiohook = &mixmonitor->audiohook; diff --git a/include/asterisk/autochan.h b/include/asterisk/autochan.h index 319c203ab1..128377b57b 100644 --- a/include/asterisk/autochan.h +++ b/include/asterisk/autochan.h @@ -32,6 +32,7 @@ struct ast_autochan { struct ast_channel *chan; AST_LIST_ENTRY(ast_autochan) list; + ast_mutex_t lock; }; /*! @@ -61,19 +62,24 @@ struct ast_autochan { * ast_autochan_channel_lock and ast_autochan_channel_unlock. An attempt to lock * the autochan->chan directly may result in it being changed after you've * retrieved the value of chan, but before you've had a chance to lock it. - * First when chan is locked, the autochan structure is guaranteed to keep the + * While chan is locked, the autochan structure is guaranteed to keep the * same channel. */ +/*! + * \brief Lock the autochan's channel lock. + * + * \note We must do deadlock avoidance because the channel lock is + * superior to the autochan lock in locking order. + */ #define ast_autochan_channel_lock(autochan) \ do { \ - struct ast_channel *autochan_chan = autochan->chan; \ - ast_channel_lock(autochan_chan); \ - if (autochan->chan == autochan_chan) { \ - break; \ + ast_mutex_lock(&(autochan)->lock); \ + while (ast_channel_trylock((autochan)->chan)) { \ + DEADLOCK_AVOIDANCE(&(autochan)->lock); \ } \ - ast_channel_unlock(autochan_chan); \ - } while (1) + ast_mutex_unlock(&(autochan)->lock); \ + } while (0) #define ast_autochan_channel_unlock(autochan) \ ast_channel_unlock(autochan->chan) diff --git a/main/autochan.c b/main/autochan.c index c7e5c00425..68aeaf804a 100644 --- a/main/autochan.c +++ b/main/autochan.c @@ -46,15 +46,18 @@ struct ast_autochan *ast_autochan_setup(struct ast_channel *chan) if (!(autochan = ast_calloc(1, sizeof(*autochan)))) { return NULL; } + ast_mutex_init(&autochan->lock); autochan->chan = ast_channel_ref(chan); - ast_channel_lock(autochan->chan); /* autochan is still private, no need for ast_autochan_channel_lock() */ + ast_debug(1, "Created autochan %p to hold channel %s (%p)\n", + autochan, ast_channel_name(chan), chan); + + /* autochan is still private, no need for ast_autochan_channel_lock() */ + ast_channel_lock(autochan->chan); AST_LIST_INSERT_TAIL(ast_channel_autochans(autochan->chan), autochan, list); ast_channel_unlock(autochan->chan); - ast_debug(1, "Created autochan %p to hold channel %s (%p)\n", autochan, ast_channel_name(chan), chan); - return autochan; } @@ -75,6 +78,8 @@ void ast_autochan_destroy(struct ast_autochan *autochan) autochan->chan = ast_channel_unref(autochan->chan); + ast_mutex_destroy(&autochan->lock); + ast_free(autochan); } @@ -84,13 +89,16 @@ void ast_autochan_new_channel(struct ast_channel *old_chan, struct ast_channel * AST_LIST_APPEND_LIST(ast_channel_autochans(new_chan), ast_channel_autochans(old_chan), list); + /* Deadlock avoidance is not needed since the channels are already locked. */ AST_LIST_TRAVERSE(ast_channel_autochans(new_chan), autochan, list) { + ast_mutex_lock(&autochan->lock); if (autochan->chan == old_chan) { - autochan->chan = ast_channel_unref(old_chan); autochan->chan = ast_channel_ref(new_chan); + ast_channel_unref(old_chan); ast_debug(1, "Autochan %p used to hold channel %s (%p) but now holds channel %s (%p)\n", autochan, ast_channel_name(old_chan), old_chan, ast_channel_name(new_chan), new_chan); } + ast_mutex_unlock(&autochan->lock); } }