From 16fdb11bc3bb23a2fd645a24a7ba56d9708960ff Mon Sep 17 00:00:00 2001 From: Richard Mudgett Date: Wed, 8 Feb 2017 14:27:18 -0600 Subject: [PATCH] core: Cleanup some channel snapshot staging anomalies. We shouldn't unlock the channel after starting a snapshot staging because another thread may interfere and do its own snapshot staging. * app_dial.c:dial_exec_full() made hold the channel lock while setting up the outgoing channel staging. Made hold the channel lock after the called party answers while updating the caller channel staging. * chan_sip.c:sip_new() completed the channel staging on off-nominal exit. Also we need to use ast_hangup() instead of ast_channel_unref() at that location. * channel.c:__ast_channel_alloc_ap() added a comment about not needing to complete the channel snapshot staging on off-nominal exit paths. * rtp_engine.c:ast_rtp_instance_set_stats_vars() made hold the channel locks while staging the channels for the stats channel variables. Change-Id: Iefb6336893163f6447bad65568722ad5d5d8212a --- apps/app_dial.c | 26 +++++++++++++++----------- channels/chan_sip.c | 4 +++- main/channel.c | 21 ++++++++++++--------- main/rtp_engine.c | 14 ++++++-------- 4 files changed, 36 insertions(+), 29 deletions(-) diff --git a/apps/app_dial.c b/apps/app_dial.c index 2f5935d19c..1cb91811fe 100644 --- a/apps/app_dial.c +++ b/apps/app_dial.c @@ -2541,16 +2541,14 @@ static int dial_exec_full(struct ast_channel *chan, const char *data, struct ast continue; } - ast_channel_lock(tc); - ast_channel_stage_snapshot(tc); - ast_channel_unlock(tc); - ast_channel_get_device_name(tc, device_name, sizeof(device_name)); if (!ignore_cc) { ast_cc_extension_monitor_add_dialstring(chan, tmp->interface, device_name); } ast_channel_lock_both(tc, chan); + ast_channel_stage_snapshot(tc); + pbx_builtin_setvar_helper(tc, "DIALEDPEERNUMBER", tmp->number); /* Setup outgoing SDP to match incoming one */ @@ -2566,7 +2564,6 @@ static int dial_exec_full(struct ast_channel *chan, const char *data, struct ast ast_channel_appl_set(tc, "AppDial"); ast_channel_data_set(tc, "(Outgoing Line)"); - ast_channel_publish_snapshot(tc); memset(ast_channel_whentohangup(tc), 0, sizeof(*ast_channel_whentohangup(tc))); @@ -2791,15 +2788,14 @@ static int dial_exec_full(struct ast_channel *chan, const char *data, struct ast } } else { const char *number; + const char *name; int dial_end_raised = 0; int cause = -1; - if (ast_test_flag64(&opts, OPT_CALLER_ANSWER)) + if (ast_test_flag64(&opts, OPT_CALLER_ANSWER)) { ast_answer(chan); + } - strcpy(pa.status, "ANSWER"); - ast_channel_stage_snapshot(chan); - pbx_builtin_setvar_helper(chan, "DIALSTATUS", pa.status); /* Ah ha! Someone answered within the desired timeframe. Of course after this we will always return with -1 so that it is hung up properly after the conversation. */ @@ -2821,10 +2817,10 @@ static int dial_exec_full(struct ast_channel *chan, const char *data, struct ast hanguptree(&out_chans, peer, cause >= 0 ? cause : AST_CAUSE_ANSWERED_ELSEWHERE); /* If appropriate, log that we have a destination channel and set the answer time */ - if (ast_channel_name(peer)) - pbx_builtin_setvar_helper(chan, "DIALEDPEERNAME", ast_channel_name(peer)); ast_channel_lock(peer); + name = ast_strdupa(ast_channel_name(peer)); + number = pbx_builtin_getvar_helper(peer, "DIALEDPEERNUMBER"); if (ast_strlen_zero(number)) { number = NULL; @@ -2832,8 +2828,16 @@ static int dial_exec_full(struct ast_channel *chan, const char *data, struct ast number = ast_strdupa(number); } ast_channel_unlock(peer); + ast_channel_lock(chan); + ast_channel_stage_snapshot(chan); + + strcpy(pa.status, "ANSWER"); + pbx_builtin_setvar_helper(chan, "DIALSTATUS", pa.status); + + pbx_builtin_setvar_helper(chan, "DIALEDPEERNAME", name); pbx_builtin_setvar_helper(chan, "DIALEDPEERNUMBER", number); + ast_channel_stage_snapshot_done(chan); ast_channel_unlock(chan); diff --git a/channels/chan_sip.c b/channels/chan_sip.c index 3bb6e74b6d..5da134086e 100644 --- a/channels/chan_sip.c +++ b/channels/chan_sip.c @@ -8141,7 +8141,9 @@ static struct ast_channel *sip_new(struct sip_pvt *i, int state, const char *tit if (!fmt) { ast_log(LOG_WARNING, "No compatible formats could be found for %s\n", ast_channel_name(tmp)); ao2_ref(caps, -1); - tmp = ast_channel_unref(tmp); + ast_channel_stage_snapshot_done(tmp); + ast_channel_unlock(tmp); + ast_hangup(tmp); return NULL; } } diff --git a/main/channel.c b/main/channel.c index 2349193365..992c940861 100644 --- a/main/channel.c +++ b/main/channel.c @@ -823,7 +823,12 @@ __ast_channel_alloc_ap(int needqueue, int state, const char *cid_num, const char ast_channel_stage_snapshot(tmp); if (!(nativeformats = ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT))) { - /* format capabilities structure allocation failure */ + /* + * Aborting the channel creation. We do not need to complete staging + * the channel snapshot because the channel has not been finalized or + * linked into the channels container yet. Nobody else knows about + * this channel nor will anybody ever know about it. + */ return ast_channel_unref(tmp); } ast_format_cap_append(nativeformats, ast_format_none, 0); @@ -849,6 +854,7 @@ __ast_channel_alloc_ap(int needqueue, int state, const char *cid_num, const char if (!(schedctx = ast_sched_context_create())) { ast_log(LOG_WARNING, "Channel allocation failed: Unable to create schedule context\n"); + /* See earlier channel creation abort comment above. */ return ast_channel_unref(tmp); } ast_channel_sched_set(tmp, schedctx); @@ -863,6 +869,7 @@ __ast_channel_alloc_ap(int needqueue, int state, const char *cid_num, const char ast_channel_caller(tmp)->id.name.valid = 1; ast_channel_caller(tmp)->id.name.str = ast_strdup(cid_name); if (!ast_channel_caller(tmp)->id.name.str) { + /* See earlier channel creation abort comment above. */ return ast_channel_unref(tmp); } } @@ -870,6 +877,7 @@ __ast_channel_alloc_ap(int needqueue, int state, const char *cid_num, const char ast_channel_caller(tmp)->id.number.valid = 1; ast_channel_caller(tmp)->id.number.str = ast_strdup(cid_num); if (!ast_channel_caller(tmp)->id.number.str) { + /* See earlier channel creation abort comment above. */ return ast_channel_unref(tmp); } } @@ -883,6 +891,7 @@ __ast_channel_alloc_ap(int needqueue, int state, const char *cid_num, const char } if (needqueue && ast_channel_internal_alertpipe_init(tmp)) { + /* See earlier channel creation abort comment above. */ return ast_channel_unref(tmp); } @@ -974,20 +983,14 @@ __ast_channel_alloc_ap(int needqueue, int state, const char *cid_num, const char if (assignedids && (does_id_conflict(assignedids->uniqueid) || does_id_conflict(assignedids->uniqueid2))) { ast_channel_internal_errno_set(AST_CHANNEL_ERROR_ID_EXISTS); ao2_unlock(channels); - /* This is a bit unorthodox, but we can't just call ast_channel_stage_snapshot_done() - * because that will result in attempting to publish the channel snapshot. That causes - * badness in some places, such as CDRs. So we need to manually clear the flag on the - * channel that says that a snapshot is being cleared. - */ - ast_clear_flag(ast_channel_flags(tmp), AST_FLAG_SNAPSHOT_STAGE); ast_channel_unlock(tmp); + /* See earlier channel creation abort comment above. */ return ast_channel_unref(tmp); } + /* Finalize and link into the channels container. */ ast_channel_internal_finalize(tmp); - ast_atomic_fetchadd_int(&chancount, +1); - ao2_link_flags(channels, tmp, OBJ_NOLOCK); ao2_unlock(channels); diff --git a/main/rtp_engine.c b/main/rtp_engine.c index 3ef2d876bf..a465463186 100644 --- a/main/rtp_engine.c +++ b/main/rtp_engine.c @@ -1899,16 +1899,16 @@ void ast_rtp_instance_set_stats_vars(struct ast_channel *chan, struct ast_rtp_in { char quality_buf[AST_MAX_USER_FIELD]; char *quality; - struct ast_channel *bridge = ast_channel_bridge_peer(chan); + struct ast_channel *bridge; - ast_channel_lock(chan); - ast_channel_stage_snapshot(chan); - ast_channel_unlock(chan); + bridge = ast_channel_bridge_peer(chan); if (bridge) { - ast_channel_lock(bridge); + ast_channel_lock_both(chan, bridge); ast_channel_stage_snapshot(bridge); - ast_channel_unlock(bridge); + } else { + ast_channel_lock(chan); } + ast_channel_stage_snapshot(chan); quality = ast_rtp_instance_get_quality(instance, AST_RTP_INSTANCE_STAT_FIELD_QUALITY, quality_buf, sizeof(quality_buf)); @@ -1946,11 +1946,9 @@ void ast_rtp_instance_set_stats_vars(struct ast_channel *chan, struct ast_rtp_in } } - ast_channel_lock(chan); ast_channel_stage_snapshot_done(chan); ast_channel_unlock(chan); if (bridge) { - ast_channel_lock(bridge); ast_channel_stage_snapshot_done(bridge); ast_channel_unlock(bridge); ast_channel_unref(bridge);