channel locking: Add locking for channel snapshot creation

Original commit message by mmichelson (asterisk 12 r403311):

"This adds channel locks around calls to create channel snapshots as well
as other functions which operate on a channel and then end up
creating a channel snapshot. Functions that expect the channel to be
locked prior to being called have had their documentation updated to
indicate such."

The above was initially committed and then reverted at r403398.  The problem
was found to be in core_local.c in the publish_local_bridge_message function.
The ast_unreal_lock_all function locks and adds a reference to the returned
channels and while they were being unlocked they were not being unreffed when
no longer needed.  Fixed by unreffing the channels.

Also in bridge.c a lock was obtained on "other->chan", but then an attempt was
made to unlock "other" and not the previously locked channel.  Fixed by
unlocking "other->chan"

(closes issue ASTERISK-22709)
Reported by: John Bigelow
........

Merged revisions 404237 from http://svn.asterisk.org/svn/asterisk/branches/12


git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@404260 65c4cc65-6c06-0410-ace0-fbb531ad65f3

11 files changed, 92 insertions, 13 deletions

diff --git a/main/bridge.c b/main/bridge.c
index 8fd7847ec..8acf260d1 100644
--- a/main/bridge.c
+++ b/main/bridge.c
@@ -2524,7 +2524,13 @@ static int try_swap_optimize_out(struct ast_bridge *chan_bridge,
 
 	other = ast_bridge_channel_peer(src_bridge_channel);
 	if (other && other->state == BRIDGE_CHANNEL_STATE_WAIT) {
-		unsigned int id = ast_atomic_fetchadd_int((int *) &optimization_id, +1);
+		unsigned int id;
+
+		if (ast_channel_trylock(other->chan)) {
+			return 1;
+		}
+
+		id = ast_atomic_fetchadd_int((int *) &optimization_id, +1);
 
 		ast_verb(3, "Move-swap optimizing %s <-- %s.\n",
 			ast_channel_name(dst_bridge_channel->chan),
@@ -2546,6 +2552,7 @@ static int try_swap_optimize_out(struct ast_bridge *chan_bridge,
 		if (pvt && pvt->callbacks && pvt->callbacks->optimization_finished) {
 			pvt->callbacks->optimization_finished(pvt, res == 1, id);
 		}
+		ast_channel_unlock(other->chan);
 	}
 	return res;
 }
diff --git a/main/bridge_channel.c b/main/bridge_channel.c
index 96bfb209a..ae33cd056 100644
--- a/main/bridge_channel.c
+++ b/main/bridge_channel.c
@@ -235,12 +235,16 @@ void ast_bridge_channel_update_linkedids(struct ast_bridge_channel *bridge_chann
 		return;
 	}
 
+	ast_channel_lock(bridge_channel->chan);
 	ast_channel_linkedid_set(bridge_channel->chan, oldest_linkedid);
+	ast_channel_unlock(bridge_channel->chan);
 	AST_LIST_TRAVERSE(&bridge->channels, other, entry) {
 		if (other == swap) {
 			continue;
 		}
+		ast_channel_lock(other->chan);
 		ast_channel_linkedid_set(other->chan, oldest_linkedid);
+		ast_channel_unlock(other->chan);
 	}
 }
 
@@ -253,6 +257,7 @@ void ast_bridge_channel_update_accountcodes(struct ast_bridge_channel *bridge_ch
 		if (other == swap) {
 			continue;
 		}
+		ast_channel_lock_both(bridge_channel->chan, other->chan);
 
 		if (!ast_strlen_zero(ast_channel_accountcode(bridge_channel->chan)) && ast_strlen_zero(ast_channel_peeraccount(other->chan))) {
 			ast_debug(1, "Setting peeraccount to %s for %s from data on channel %s\n",
@@ -286,6 +291,8 @@ void ast_bridge_channel_update_accountcodes(struct ast_bridge_channel *bridge_ch
 				ast_channel_peeraccount_set(bridge_channel->chan, ast_channel_accountcode(other->chan));
 			}
 		}
+		ast_channel_unlock(bridge_channel->chan);
+		ast_channel_unlock(other->chan);
 	}
 }
 
@@ -624,14 +631,18 @@ int ast_bridge_channel_write_hold(struct ast_bridge_channel *bridge_channel, con
 		datalen = 0;
 	}
 
+	ast_channel_lock(bridge_channel->chan);
 	ast_channel_publish_blob(bridge_channel->chan, ast_channel_hold_type(), blob);
+	ast_channel_unlock(bridge_channel->chan);
 	return ast_bridge_channel_write_control_data(bridge_channel, AST_CONTROL_HOLD,
 		moh_class, datalen);
 }
 
 int ast_bridge_channel_write_unhold(struct ast_bridge_channel *bridge_channel)
 {
+	ast_channel_lock(bridge_channel->chan);
 	ast_channel_publish_blob(bridge_channel->chan, ast_channel_unhold_type(), NULL);
+	ast_channel_unlock(bridge_channel->chan);
 	return ast_bridge_channel_write_control_data(bridge_channel, AST_CONTROL_UNHOLD, NULL, 0);
 }
 
diff --git a/main/cel.c b/main/cel.c
index db51361e9..b9f992f6f 100644
--- a/main/cel.c
+++ b/main/cel.c
@@ -1614,7 +1614,9 @@ void ast_cel_publish_event(struct ast_channel *chan,
 		"event_type", event_type,
 		"event_details", blob);
 
+	ast_channel_lock(chan);
 	message = ast_channel_blob_create(chan, cel_generic_type(), cel_blob);
+	ast_channel_unlock(chan);
 	if (message) {
 		stasis_publish(ast_cel_topic(), message);
 	}
diff --git a/main/channel.c b/main/channel.c
index 418e6a772..669b81529 100644
--- a/main/channel.c
+++ b/main/channel.c
@@ -678,7 +678,9 @@ int ast_str2cause(const char *name)
 static struct stasis_message *create_channel_snapshot_message(struct ast_channel *channel)
 {
 	RAII_VAR(struct ast_channel_snapshot *, snapshot, NULL, ao2_cleanup);
+	ast_channel_lock(channel);
 	snapshot = ast_channel_snapshot_create(channel);
+	ast_channel_unlock(channel);
 	if (!snapshot) {
 		return NULL;
 	}
@@ -1267,6 +1269,7 @@ int ast_queue_hold(struct ast_channel *chan, const char *musicclass)
 	struct ast_frame f = { AST_FRAME_CONTROL, .subclass.integer = AST_CONTROL_HOLD };
 	int res;
 
+	ast_channel_lock(chan);
 	if (!ast_strlen_zero(musicclass)) {
 		f.data.ptr = (void *) musicclass;
 		f.datalen = strlen(musicclass) + 1;
@@ -1278,6 +1281,7 @@ int ast_queue_hold(struct ast_channel *chan, const char *musicclass)
 	ast_channel_publish_blob(chan, ast_channel_hold_type(), blob);
 
 	res = ast_queue_frame(chan, &f);
+	ast_channel_unlock(chan);
 	return res;
 }
 
@@ -1286,9 +1290,11 @@ int ast_queue_unhold(struct ast_channel *chan)
 	struct ast_frame f = { AST_FRAME_CONTROL, .subclass.integer = AST_CONTROL_UNHOLD };
 	int res;
 
+	ast_channel_lock(chan);
 	ast_channel_publish_blob(chan, ast_channel_unhold_type(), NULL);
 
 	res = ast_queue_frame(chan, &f);
+	ast_channel_unlock(chan);
 	return res;
 }
 
@@ -2254,7 +2260,9 @@ static void ast_channel_destructor(void *obj)
 
 	/* Things that may possibly raise Stasis messages shouldn't occur after this point */
 	ast_set_flag(ast_channel_flags(chan), AST_FLAG_DEAD);
+	ast_channel_lock(chan);
 	ast_channel_publish_snapshot(chan);
+	ast_channel_unlock(chan);
 	publish_cache_clear(chan);
 
 	ast_channel_lock(chan);
@@ -5324,7 +5332,9 @@ static int set_format(struct ast_channel *chan,
 			generator_write_format_change(chan);
 		}
 
+		ast_channel_lock(chan);
 		ast_channel_publish_snapshot(chan);
+		ast_channel_unlock(chan);
 
 		return 0;
 	}
@@ -5405,7 +5415,9 @@ static int set_format(struct ast_channel *chan,
 		generator_write_format_change(chan);
 	}
 
+	ast_channel_lock(chan);
 	ast_channel_publish_snapshot(chan);
+	ast_channel_unlock(chan);
 
 	return res;
 }
@@ -5622,7 +5634,9 @@ struct ast_channel *ast_call_forward(struct ast_channel *caller, struct ast_chan
 	/* Copy/inherit important information into new channel */
 	if (oh) {
 		if (oh->vars) {
+			ast_channel_lock(new_chan);
 			ast_set_variables(new_chan, oh->vars);
+			ast_channel_unlock(new_chan);
 		}
 		if (oh->parent_channel) {
 			call_forward_inherit(new_chan, oh->parent_channel, orig);
@@ -5683,7 +5697,9 @@ struct ast_channel *__ast_request_and_dial(const char *type, struct ast_format_c
 
 	if (oh) {
 		if (oh->vars) {
+			ast_channel_lock(chan);
 			ast_set_variables(chan, oh->vars);
+			ast_channel_unlock(chan);
 		}
 		if (!ast_strlen_zero(oh->cid_num) && !ast_strlen_zero(oh->cid_name)) {
 			/*
@@ -5955,7 +5971,9 @@ struct ast_channel *ast_request(const char *type, struct ast_format_cap *request
 		if (requestor) {
 			struct ast_callid *callid = ast_channel_callid(requestor);
 			if (callid) {
+				ast_channel_lock(c);
 				ast_channel_callid_set(c, callid);
+				ast_channel_unlock(c);
 				callid = ast_callid_unref(callid);
 			}
 		}
diff --git a/main/core_local.c b/main/core_local.c
index 4a047032c..edf28399b 100644
--- a/main/core_local.c
+++ b/main/core_local.c
@@ -498,29 +498,32 @@ static void publish_local_bridge_message(struct local_pvt *p)
 	RAII_VAR(struct stasis_message *, msg, NULL, ao2_cleanup);
 	RAII_VAR(struct ast_channel_snapshot *, one_snapshot, NULL, ao2_cleanup);
 	RAII_VAR(struct ast_channel_snapshot *, two_snapshot, NULL, ao2_cleanup);
-	SCOPED_AO2LOCK(lock, p);
+	struct ast_channel *owner;
+	struct ast_channel *chan;
+
+	ast_unreal_lock_all(&p->base, &chan, &owner);
 
 	blob = ast_json_pack("{s: s, s: s, s: b}",
 		"context", p->context,
 		"exten", p->exten,
 		"can_optimize", !ast_test_flag(&p->base, AST_UNREAL_NO_OPTIMIZATION));
 	if (!blob) {
-		return;
+		goto end;
 	}
 
 	multi_blob = ast_multi_channel_blob_create(blob);
 	if (!multi_blob) {
-		return;
+		goto end;
 	}
 
-	one_snapshot = ast_channel_snapshot_create(p->base.owner);
+	one_snapshot = ast_channel_snapshot_create(owner);
 	if (!one_snapshot) {
-		return;
+		goto end;
 	}
 
-	two_snapshot = ast_channel_snapshot_create(p->base.chan);
+	two_snapshot = ast_channel_snapshot_create(chan);
 	if (!two_snapshot) {
-		return;
+		goto end;
 	}
 
 	ast_multi_channel_blob_add_channel(multi_blob, "1", one_snapshot);
@@ -528,10 +531,19 @@ static void publish_local_bridge_message(struct local_pvt *p)
 
 	msg = stasis_message_create(ast_local_bridge_type(), multi_blob);
 	if (!msg) {
-		return;
+		goto end;
 	}
 
-	stasis_publish(ast_channel_topic(p->base.owner), msg);
+	stasis_publish(ast_channel_topic(owner), msg);
+
+end:
+	ast_channel_unlock(owner);
+	ast_channel_unref(owner);
+
+	ast_channel_unlock(chan);
+	ast_channel_unref(chan);
+
+	ao2_unlock(&p->base);
 }
 
 int ast_local_setup_bridge(struct ast_channel *ast, struct ast_bridge *bridge, struct ast_channel *swap, struct ast_bridge_features *features)
diff --git a/main/dial.c b/main/dial.c
index ca0b9c8d1..6ff0e7f7d 100644
--- a/main/dial.c
+++ b/main/dial.c
@@ -303,6 +303,7 @@ static int begin_dial_prerun(struct ast_dial_channel *channel, struct ast_channe
 	cap_request = NULL;
 	cap_all_audio = ast_format_cap_destroy(cap_all_audio);
 
+	ast_channel_lock(channel->owner);
 	ast_channel_stage_snapshot(channel->owner);
 
 	ast_channel_appl_set(channel->owner, "AppDial2");
@@ -332,6 +333,7 @@ static int begin_dial_prerun(struct ast_dial_channel *channel, struct ast_channe
 	}
 
 	ast_channel_stage_snapshot_done(channel->owner);
+	ast_channel_unlock(channel->owner);
 
 	if (!ast_strlen_zero(predial_string)) {
 		const char *predial_callee = ast_app_expand_sub_args(chan, predial_string);
diff --git a/main/endpoints.c b/main/endpoints.c
index 4be4eb31b..9cc0178e0 100644
--- a/main/endpoints.c
+++ b/main/endpoints.c
@@ -203,7 +203,9 @@ int ast_endpoint_add_channel(struct ast_endpoint *endpoint,
 	ast_str_container_add(endpoint->channel_ids, ast_channel_uniqueid(chan));
 	ao2_unlock(endpoint);
 
+	ast_channel_lock(chan);
 	ast_publish_channel_state(chan);
+	ast_channel_unlock(chan);
 	endpoint_publish_snapshot(endpoint);
 
 	return 0;
diff --git a/main/pbx.c b/main/pbx.c
index 726677f4a..1e6a24625 100644
--- a/main/pbx.c
+++ b/main/pbx.c
@@ -1612,9 +1612,11 @@ int pbx_exec(struct ast_channel *c,	/*!< Channel */
 	saved_c_appl= ast_channel_appl(c);
 	saved_c_data= ast_channel_data(c);
 
+	ast_channel_lock(c);
 	ast_channel_appl_set(c, app->name);
 	ast_channel_data_set(c, data);
 	ast_channel_publish_snapshot(c);
+	ast_channel_unlock(c);
 
 	if (app->module)
 		u = __ast_module_user_add(app->module, c);
@@ -6281,7 +6283,9 @@ static enum ast_pbx_result __ast_pbx_run(struct ast_channel *c,
 		if (!callid) {
 			callid = ast_create_callid();
 			if (callid) {
+				ast_channel_lock(c);
 				ast_channel_callid_set(c, callid);
+				ast_channel_unlock(c);
 			}
 		}
 		ast_callid_threadassoc_add(callid);
@@ -10249,6 +10253,7 @@ static int pbx_outgoing_attempt(const char *type, struct ast_format_cap *cap, co
 		return -1;
 	}
 
+	ast_channel_lock(dialed);
 	if (vars) {
 		ast_set_variables(dialed, vars);
 	}
@@ -10257,6 +10262,7 @@ static int pbx_outgoing_attempt(const char *type, struct ast_format_cap *cap, co
 		ast_channel_accountcode_set(dialed, account);
 	}
 	ast_set_flag(ast_channel_flags(dialed), AST_FLAG_ORIGINATED);
+	ast_channel_unlock(dialed);
 
 	if (!ast_strlen_zero(cid_num) || !ast_strlen_zero(cid_name)) {
 		struct ast_party_connected_line connected;
@@ -10364,7 +10370,6 @@ static int pbx_outgoing_attempt(const char *type, struct ast_format_cap *cap, co
 			ast_set_variables(failed, vars);
 			snprintf(failed_reason, sizeof(failed_reason), "%d", ast_dial_reason(outgoing->dial, 0));
 			pbx_builtin_setvar_helper(failed, "REASON", failed_reason);
-
 			ast_channel_unlock(failed);
 
 			if (ast_pbx_run(failed)) {
@@ -10666,10 +10671,12 @@ static int pbx_builtin_busy(struct ast_channel *chan, const char *data)
 	ast_indicate(chan, AST_CONTROL_BUSY);
 	/* Don't change state of an UP channel, just indicate
 	   busy in audio */
+	ast_channel_lock(chan);
 	if (ast_channel_state(chan) != AST_STATE_UP) {
 		ast_channel_hangupcause_set(chan, AST_CAUSE_BUSY);
 		ast_setstate(chan, AST_STATE_BUSY);
 	}
+	ast_channel_unlock(chan);
 	wait_for_hangup(chan, data);
 	return -1;
 }
@@ -10682,10 +10689,12 @@ static int pbx_builtin_congestion(struct ast_channel *chan, const char *data)
 	ast_indicate(chan, AST_CONTROL_CONGESTION);
 	/* Don't change state of an UP channel, just indicate
 	   congestion in audio */
+	ast_channel_lock(chan);
 	if (ast_channel_state(chan) != AST_STATE_UP) {
 		ast_channel_hangupcause_set(chan, AST_CAUSE_CONGESTION);
 		ast_setstate(chan, AST_STATE_BUSY);
 	}
+	ast_channel_unlock(chan);
 	wait_for_hangup(chan, data);
 	return -1;
 }
diff --git a/main/pickup.c b/main/pickup.c
index 7ae6927fa..a415f1672 100644
--- a/main/pickup.c
+++ b/main/pickup.c
@@ -354,11 +354,17 @@ int ast_do_pickup(struct ast_channel *chan, struct ast_channel *target)
 	/* setting the HANGUPCAUSE so the ringing channel knows this call was not a missed call */
 	ast_channel_hangupcause_set(chan, AST_CAUSE_ANSWERED_ELSEWHERE);
 
-	if (!(chan_snapshot = ast_channel_snapshot_create(chan))) {
+	ast_channel_lock(chan);
+	chan_snapshot = ast_channel_snapshot_create(chan);
+	ast_channel_unlock(chan);
+	if (!chan_snapshot) {
 		goto pickup_failed;
 	}
 
-	if (!(target_snapshot = ast_channel_snapshot_create(target))) {
+	ast_channel_lock(target);
+	target_snapshot = ast_channel_snapshot_create(target);
+	ast_channel_unlock(target);
+	if (!target_snapshot) {
 		goto pickup_failed;
 	}
 
diff --git a/main/stasis_bridges.c b/main/stasis_bridges.c
index 7f6679b00..b8d4ae8e1 100644
--- a/main/stasis_bridges.c
+++ b/main/stasis_bridges.c
@@ -399,7 +399,9 @@ struct stasis_message *ast_bridge_blob_create(
 	}
 
 	if (chan) {
+		ast_channel_lock(chan);
 		obj->channel = ast_channel_snapshot_create(chan);
+		ast_channel_unlock(chan);
 		if (obj->channel == NULL) {
 			return NULL;
 		}
@@ -583,7 +585,9 @@ static int bridge_channel_snapshot_pair_init(struct ast_bridge_channel_pair *pai
 		}
 	}
 
+	ast_channel_lock(pair->channel);
 	snapshot_pair->channel_snapshot = ast_channel_snapshot_create(pair->channel);
+	ast_channel_unlock(pair->channel);
 	if (!snapshot_pair->channel_snapshot) {
 		return -1;
 	}
@@ -919,7 +923,9 @@ void ast_bridge_publish_attended_transfer_link(int is_external, enum ast_transfe
 
 	transfer_msg->dest_type = AST_ATTENDED_TRANSFER_DEST_LINK;
 	for (i = 0; i < 2; ++i) {
+		ast_channel_lock(locals[i]);
 		transfer_msg->dest.links[i] = ast_channel_snapshot_create(locals[i]);
+		ast_channel_unlock(locals[i]);
 		if (!transfer_msg->dest.links[i]) {
 			return;
 		}
diff --git a/main/stasis_channels.c b/main/stasis_channels.c
index 8a39bdfcb..677527ba3 100644
--- a/main/stasis_channels.c
+++ b/main/stasis_channels.c
@@ -317,14 +317,18 @@ void ast_channel_publish_dial_forward(struct ast_channel *caller, struct ast_cha
 	}
 
 	if (caller) {
+		ast_channel_lock(caller);
 		caller_snapshot = ast_channel_snapshot_create(caller);
+		ast_channel_unlock(caller);
 		if (!caller_snapshot) {
 			return;
 		}
 		ast_multi_channel_blob_add_channel(payload, "caller", caller_snapshot);
 	}
 
+	ast_channel_lock(peer);
 	peer_snapshot = ast_channel_snapshot_create(peer);
+	ast_channel_unlock(peer);
 	if (!peer_snapshot) {
 		return;
 	}