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

2 files changed, 18 insertions, 17 deletions

diff --git a/main/channel.c b/main/channel.c
index 28cfa79df..3d51651fd 100644
--- a/main/channel.c
+++ b/main/channel.c
@@ -820,7 +820,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);
@@ -846,6 +851,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);
@@ -860,6 +866,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);
 		}
 	}
@@ -867,6 +874,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);
 		}
 	}
@@ -880,6 +888,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);
 	}
 
@@ -971,20 +980,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 5c7b7a07b..0f09e2f8a 100644
--- a/main/rtp_engine.c
+++ b/main/rtp_engine.c
@@ -1394,16 +1394,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));
@@ -1441,11 +1441,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);