CHANNEL(peer), chan_iax2, res_fax, SNMP agent: Fix deadlock from reaching across a bridge.

Calling ast_channel_bridge_peer() cannot be done while holding any channel
locks.  The reported issue hit the deadlock in chan_iax2, but an audit of
the ast_channel_bridge_peer() calls found three more locations where the
same deadlock can occur.

* Made CHANNEL(peer), res_fax, and the SNMP agent not call
ast_channel_bridge_peer() with any channel locked.  For CHANNEL(peer) I
had to rework the logic to not hold the channel lock.

* Made chan_iax2 no longer call ast_channel_bridge_peer().  It was done
for legacy reasons that no longer apply.

* Removed the iax.conf forcejitterbuffer option.  It is now always enabled
when the jitterbuffer option is enabled.  If you put a jitter buffer on a
channel it will be on the channel.

ASTERISK-24600 #close
Reported by: Jeff Collell

Review: https://reviewboard.asterisk.org/r/4342/
........

Merged revisions 430817 from http://svn.asterisk.org/svn/asterisk/branches/13


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

6 files changed, 54 insertions, 81 deletions

diff --git a/CHANGES b/CHANGES
index 6c2977388..84958a2d4 100644
--- a/CHANGES
+++ b/CHANGES
@@ -35,6 +35,12 @@ chan_dahdi
  * The CALLERID(ani2) value for incoming calls is now populated in featdmf
    signaling mode.  The information was previously discarded.
 
+chan_iax2
+------------------
+ * The iax.conf forcejitterbuffer option has been removed.  It is now always
+   forced if you set iax.conf jitterbuffer=yes.  If you put a jitter buffer
+   on a channel it will be on the channel.
+
 chan_sip
 ------------------
  * New 'rtpbindaddr' global setting. This allows a user to define which
diff --git a/channels/chan_iax2.c b/channels/chan_iax2.c
index bb65df460..cc70bfd3a 100644
--- a/channels/chan_iax2.c
+++ b/channels/chan_iax2.c
@@ -455,7 +455,6 @@ struct iax2_context {
 #define IAX_RTCACHEFRIENDS      (uint64_t)(1 << 17)   /*!< let realtime stay till your reload */
 #define IAX_RTUPDATE            (uint64_t)(1 << 18)   /*!< Send a realtime update */
 #define IAX_RTAUTOCLEAR         (uint64_t)(1 << 19)   /*!< erase me on expire */
-#define IAX_FORCEJITTERBUF      (uint64_t)(1 << 20)   /*!< Force jitterbuffer, even when bridged to a channel that can take jitter */
 #define IAX_RTIGNOREREGEXPIRE   (uint64_t)(1 << 21)   /*!< When using realtime, ignore registration expiration */
 #define IAX_TRUNKTIMESTAMPS     (uint64_t)(1 << 22)   /*!< Send trunk timestamps */
 #define IAX_TRANSFERMEDIA       (uint64_t)(1 << 23)   /*!< When doing IAX2 transfers, transfer media only */
@@ -3180,7 +3179,7 @@ static int __find_callno(unsigned short callno, unsigned short dcallno, struct a
 			iaxs[x]->pingid = iax2_sched_add(sched, ping_time * 1000, send_ping, (void *)(long)x);
 			iaxs[x]->lagid = iax2_sched_add(sched, lagrq_time * 1000, send_lagrq, (void *)(long)x);
 			iaxs[x]->amaflags = amaflags;
-			ast_copy_flags64(iaxs[x], &globalflags, IAX_NOTRANSFER | IAX_TRANSFERMEDIA | IAX_USEJITTERBUF | IAX_FORCEJITTERBUF | IAX_SENDCONNECTEDLINE | IAX_RECVCONNECTEDLINE | IAX_FORCE_ENCRYPT);
+			ast_copy_flags64(iaxs[x], &globalflags, IAX_NOTRANSFER | IAX_TRANSFERMEDIA | IAX_USEJITTERBUF | IAX_SENDCONNECTEDLINE | IAX_RECVCONNECTEDLINE | IAX_FORCE_ENCRYPT);
 			ast_string_field_set(iaxs[x], accountcode, accountcode);
 			ast_string_field_set(iaxs[x], mohinterpret, mohinterpret);
 			ast_string_field_set(iaxs[x], mohsuggest, mohsuggest);
@@ -4191,8 +4190,6 @@ static int schedule_delivery(struct iax_frame *fr, int updatehistory, int fromtr
 	int type, len;
 	int ret;
 	int needfree = 0;
-	struct ast_channel *owner = NULL;
-	RAII_VAR(struct ast_channel *, bridge, NULL, ast_channel_cleanup);
 
 	/*
 	 * Clear fr->af.data if there is no data in the buffer.  Things
@@ -4233,45 +4230,6 @@ static int schedule_delivery(struct iax_frame *fr, int updatehistory, int fromtr
 		return -1;
 	}
 
-	iax2_lock_owner(fr->callno);
-	if (!iaxs[fr->callno]) {
-		/* The call dissappeared so discard this frame that we could not send. */
-		iax2_frame_free(fr);
-		return -1;
-	}
-	if ((owner = iaxs[fr->callno]->owner)) {
-		bridge = ast_channel_bridge_peer(owner);
-	}
-
-	/* if the user hasn't requested we force the use of the jitterbuffer, and we're bridged to
-	 * a channel that can accept jitter, then flush and suspend the jb, and send this frame straight through */
-	if ( (!ast_test_flag64(iaxs[fr->callno], IAX_FORCEJITTERBUF)) && owner && bridge && (ast_channel_tech(bridge)->properties & AST_CHAN_TP_WANTSJITTER) ) {
-		jb_frame frame;
-
-		ast_channel_unlock(owner);
-
-		/* deliver any frames in the jb */
-		while (jb_getall(iaxs[fr->callno]->jb, &frame) == JB_OK) {
-			__do_deliver(frame.data);
-			/* __do_deliver() can make the call disappear */
-			if (!iaxs[fr->callno])
-				return -1;
-		}
-
-		jb_reset(iaxs[fr->callno]->jb);
-
-		AST_SCHED_DEL(sched, iaxs[fr->callno]->jbid);
-
-		/* deliver this frame now */
-		if (tsout)
-			*tsout = fr->ts;
-		__do_deliver(fr);
-		return -1;
-	}
-	if (owner) {
-		ast_channel_unlock(owner);
-	}
-
 	/* insert into jitterbuffer */
 	/* TODO: Perhaps we could act immediately if it's not droppable and late */
 	ret = jb_put(iaxs[fr->callno]->jb, fr, type, len, fr->ts,
@@ -4670,7 +4628,7 @@ static int create_addr(const char *peername, struct ast_channel *c, struct ast_s
 	if (peer->maxms && ((peer->lastms > peer->maxms) || (peer->lastms < 0)))
 		goto return_unref;
 
-	ast_copy_flags64(cai, peer, IAX_SENDANI | IAX_TRUNK | IAX_NOTRANSFER | IAX_TRANSFERMEDIA | IAX_USEJITTERBUF | IAX_FORCEJITTERBUF | IAX_SENDCONNECTEDLINE | IAX_RECVCONNECTEDLINE | IAX_FORCE_ENCRYPT);
+	ast_copy_flags64(cai, peer, IAX_SENDANI | IAX_TRUNK | IAX_NOTRANSFER | IAX_TRANSFERMEDIA | IAX_USEJITTERBUF | IAX_SENDCONNECTEDLINE | IAX_RECVCONNECTEDLINE | IAX_FORCE_ENCRYPT);
 	cai->maxtime = peer->maxms;
 	cai->capability = peer->capability;
 	cai->encmethods = peer->encmethods;
@@ -7977,7 +7935,7 @@ static int check_access(int callno, struct ast_sockaddr *addr, struct iax_ies *i
 			iaxs[callno]->amaflags = user->amaflags;
 		if (!ast_strlen_zero(user->language))
 			ast_string_field_set(iaxs[callno], language, user->language);
-		ast_copy_flags64(iaxs[callno], user, IAX_NOTRANSFER | IAX_TRANSFERMEDIA | IAX_USEJITTERBUF | IAX_FORCEJITTERBUF | IAX_SENDCONNECTEDLINE | IAX_RECVCONNECTEDLINE);
+		ast_copy_flags64(iaxs[callno], user, IAX_NOTRANSFER | IAX_TRANSFERMEDIA | IAX_USEJITTERBUF | IAX_SENDCONNECTEDLINE | IAX_RECVCONNECTEDLINE);
 		/* Keep this check last */
 		if (!ast_strlen_zero(user->dbsecret)) {
 			char *family, *key=NULL;
@@ -12460,7 +12418,7 @@ static struct ast_channel *iax2_request(const char *type, struct ast_format_cap
 	memset(&cai, 0, sizeof(cai));
 	cai.capability = iax2_capability;
 
-	ast_copy_flags64(&cai, &globalflags, IAX_NOTRANSFER | IAX_TRANSFERMEDIA | IAX_USEJITTERBUF | IAX_FORCEJITTERBUF | IAX_SENDCONNECTEDLINE | IAX_RECVCONNECTEDLINE);
+	ast_copy_flags64(&cai, &globalflags, IAX_NOTRANSFER | IAX_TRANSFERMEDIA | IAX_USEJITTERBUF | IAX_SENDCONNECTEDLINE | IAX_RECVCONNECTEDLINE);
 
 	/* Populate our address from the given */
 	if (create_addr(pds.peer, NULL, &addr, &cai)) {
@@ -12482,7 +12440,7 @@ static struct ast_channel *iax2_request(const char *type, struct ast_format_cap
 	}
 
 	/* If this is a trunk, update it now */
-	ast_copy_flags64(iaxs[callno], &cai, IAX_TRUNK | IAX_SENDANI | IAX_NOTRANSFER | IAX_TRANSFERMEDIA | IAX_USEJITTERBUF | IAX_FORCEJITTERBUF | IAX_SENDCONNECTEDLINE | IAX_RECVCONNECTEDLINE);
+	ast_copy_flags64(iaxs[callno], &cai, IAX_TRUNK | IAX_SENDANI | IAX_NOTRANSFER | IAX_TRANSFERMEDIA | IAX_USEJITTERBUF | IAX_SENDCONNECTEDLINE | IAX_RECVCONNECTEDLINE);
 	if (ast_test_flag64(&cai, IAX_TRUNK)) {
 		int new_callno;
 		if ((new_callno = make_trunk(callno, 1)) != -1)
@@ -12800,7 +12758,7 @@ static struct iax2_peer *build_peer(const char *name, struct ast_variable *v, st
 
 	if (peer) {
 		if (firstpass) {
-			ast_copy_flags64(peer, &globalflags, IAX_USEJITTERBUF | IAX_FORCEJITTERBUF | IAX_SENDCONNECTEDLINE | IAX_RECVCONNECTEDLINE | IAX_FORCE_ENCRYPT);
+			ast_copy_flags64(peer, &globalflags, IAX_USEJITTERBUF | IAX_SENDCONNECTEDLINE | IAX_RECVCONNECTEDLINE | IAX_FORCE_ENCRYPT);
 			peer->encmethods = iax2_encryption;
 			peer->adsi = adsi;
 			ast_string_field_set(peer, secret, "");
@@ -12887,8 +12845,6 @@ static struct iax2_peer *build_peer(const char *name, struct ast_variable *v, st
 					ast_set_flags_to64(peer, IAX_NOTRANSFER|IAX_TRANSFERMEDIA, IAX_NOTRANSFER);
 			} else if (!strcasecmp(v->name, "jitterbuffer")) {
 				ast_set2_flag64(peer, ast_true(v->value), IAX_USEJITTERBUF);
-			} else if (!strcasecmp(v->name, "forcejitterbuffer")) {
-				ast_set2_flag64(peer, ast_true(v->value), IAX_FORCEJITTERBUF);
 			} else if (!strcasecmp(v->name, "host")) {
 				if (!strcasecmp(v->value, "dynamic")) {
 					/* They'll register with us */
@@ -13134,7 +13090,7 @@ static struct iax2_user *build_user(const char *name, struct ast_variable *v, st
 			user->calltoken_required = CALLTOKEN_DEFAULT;
 			ast_string_field_set(user, name, name);
 			ast_string_field_set(user, language, language);
-			ast_copy_flags64(user, &globalflags, IAX_USEJITTERBUF | IAX_FORCEJITTERBUF | IAX_CODEC_USER_FIRST | IAX_CODEC_NOPREFS | IAX_CODEC_NOCAP | IAX_SENDCONNECTEDLINE | IAX_RECVCONNECTEDLINE | IAX_FORCE_ENCRYPT);
+			ast_copy_flags64(user, &globalflags, IAX_USEJITTERBUF | IAX_CODEC_USER_FIRST | IAX_CODEC_NOPREFS | IAX_CODEC_NOCAP | IAX_SENDCONNECTEDLINE | IAX_RECVCONNECTEDLINE | IAX_FORCE_ENCRYPT);
 			ast_clear_flag64(user, IAX_HASCALLERID);
 			ast_string_field_set(user, cid_name, "");
 			ast_string_field_set(user, cid_num, "");
@@ -13216,8 +13172,6 @@ static struct iax2_user *build_user(const char *name, struct ast_variable *v, st
 				ast_set2_flag64(user, ast_true(v->value), IAX_IMMEDIATE);
 			} else if (!strcasecmp(v->name, "jitterbuffer")) {
 				ast_set2_flag64(user, ast_true(v->value), IAX_USEJITTERBUF);
-			} else if (!strcasecmp(v->name, "forcejitterbuffer")) {
-				ast_set2_flag64(user, ast_true(v->value), IAX_FORCEJITTERBUF);
 			} else if (!strcasecmp(v->name, "dbsecret")) {
 				ast_string_field_set(user, dbsecret, v->value);
 			} else if (!strcasecmp(v->name, "secret")) {
@@ -13430,7 +13384,7 @@ static void set_config_destroy(void)
 	amaflags = 0;
 	delayreject = 0;
 	ast_clear_flag64((&globalflags), IAX_NOTRANSFER | IAX_TRANSFERMEDIA | IAX_USEJITTERBUF |
-		IAX_FORCEJITTERBUF | IAX_SENDCONNECTEDLINE | IAX_RECVCONNECTEDLINE);
+		IAX_SENDCONNECTEDLINE | IAX_RECVCONNECTEDLINE);
 	delete_users();
 	ao2_callback(callno_limits, OBJ_NODATA, addr_range_delme_cb, NULL);
 	ao2_callback(calltoken_ignores, OBJ_NODATA, addr_range_delme_cb, NULL);
@@ -13661,8 +13615,6 @@ static int set_config(const char *config_file, int reload, int forced)
 			}
 		} else if (!strcasecmp(v->name, "jitterbuffer"))
 			ast_set2_flag64((&globalflags), ast_true(v->value), IAX_USEJITTERBUF);
-		else if (!strcasecmp(v->name, "forcejitterbuffer"))
-			ast_set2_flag64((&globalflags), ast_true(v->value), IAX_FORCEJITTERBUF);
 		else if (!strcasecmp(v->name, "delayreject"))
 			delayreject = ast_true(v->value);
 		else if (!strcasecmp(v->name, "allowfwdownload"))
diff --git a/configs/samples/iax.conf.sample b/configs/samples/iax.conf.sample
index e17c7dfeb..1d9623afb 100644
--- a/configs/samples/iax.conf.sample
+++ b/configs/samples/iax.conf.sample
@@ -170,12 +170,6 @@ disallow=lpc10
 ; jitterbuffer=yes|no: global default as to whether you want
 ; the jitter buffer at all.
 ;
-; forcejitterbuffer=yes|no: in the ideal world, when we bridge VoIP channels
-; we don't want to do jitterbuffering on the switch, since the endpoints
-; can each handle this.  However, some endpoints may have poor jitterbuffers
-; themselves, so this option will force * to always jitterbuffer, even in this
-; case.
-;
 ; maxjitterbuffer: a maximum size for the jitter buffer.
 ; Setting a reasonable maximum here will prevent the call delay
 ; from rising to silly values in extreme situations; you'll hear
@@ -202,7 +196,6 @@ disallow=lpc10
 ;
 
 jitterbuffer=no
-forcejitterbuffer=no
 ;maxjitterbuffer=1000
 ;maxjitterinterps=10
 ;resyncthreshold=1000
diff --git a/funcs/func_channel.c b/funcs/func_channel.c
index 8eefd6492..68baa1e5e 100644
--- a/funcs/func_channel.c
+++ b/funcs/func_channel.c
@@ -514,22 +514,34 @@ static int func_channel_read(struct ast_channel *chan, const char *function,
 		}
 		ast_channel_unlock(chan);
 	} else if (!strcasecmp(data, "peer")) {
-		RAII_VAR(struct ast_channel *, p, NULL, ast_channel_cleanup);
-
-		ast_channel_lock(chan);
-		p = ast_channel_bridge_peer(chan);
-		if (p || ast_channel_tech(chan)) /* dummy channel? if so, we hid the peer name in the language */
-			ast_copy_string(buf, (p ? ast_channel_name(p) : ""), len);
-		else {
-			/* a dummy channel can still pass along bridged peer info via
-                           the BRIDGEPEER variable */
-			const char *pname = pbx_builtin_getvar_helper(chan, "BRIDGEPEER");
-			if (!ast_strlen_zero(pname))
-				ast_copy_string(buf, pname, len); /* a horrible kludge, but... how else? */
-			else
-				buf[0] = 0;
+		struct ast_channel *peer;
+
+		peer = ast_channel_bridge_peer(chan);
+		if (peer) {
+			/* Only real channels could have a bridge peer this way. */
+			ast_channel_lock(peer);
+			ast_copy_string(buf, ast_channel_name(peer), len);
+			ast_channel_unlock(peer);
+			ast_channel_unref(peer);
+		} else {
+			buf[0] = '\0';
+			ast_channel_lock(chan);
+			if (!ast_channel_tech(chan)) {
+				const char *pname;
+
+				/*
+				 * A dummy channel can still pass along bridged peer info
+				 * via the BRIDGEPEER variable.
+				 *
+				 * A horrible kludge, but... how else?
+				 */
+				pname = pbx_builtin_getvar_helper(chan, "BRIDGEPEER");
+				if (!ast_strlen_zero(pname)) {
+					ast_copy_string(buf, pname, len);
+				}
+			}
+			ast_channel_unlock(chan);
 		}
-		ast_channel_unlock(chan);
 	} else if (!strcasecmp(data, "uniqueid")) {
 		locked_copy_string(chan, buf, ast_channel_uniqueid(chan), len);
 	} else if (!strcasecmp(data, "transfercapability")) {
diff --git a/res/res_fax.c b/res/res_fax.c
index cb282e5bc..18811063e 100644
--- a/res/res_fax.c
+++ b/res/res_fax.c
@@ -2888,6 +2888,7 @@ static struct fax_gateway *fax_gateway_new(struct ast_channel *chan, struct ast_
 static int fax_gateway_start(struct fax_gateway *gateway, struct ast_fax_session_details *details, struct ast_channel *chan)
 {
 	struct ast_fax_session *s;
+	int start_res;
 
 	/* create the FAX session */
 	if (!(s = fax_session_new(details, chan, gateway->s, gateway->token))) {
@@ -2908,7 +2909,10 @@ static int fax_gateway_start(struct fax_gateway *gateway, struct ast_fax_session
 	gateway->s = s;
 	gateway->token = NULL;
 
-	if (gateway->s->tech->start_session(gateway->s) < 0) {
+	ast_channel_unlock(chan);
+	start_res = gateway->s->tech->start_session(gateway->s);
+	ast_channel_lock(chan);
+	if (start_res < 0) {
 		ast_string_field_set(details, result, "FAILED");
 		ast_string_field_set(details, resultstr, "error starting gateway session");
 		ast_string_field_set(details, error, "INIT_ERROR");
diff --git a/res/snmp/agent.c b/res/snmp/agent.c
index abaf37224..9d1528dde 100644
--- a/res/snmp/agent.c
+++ b/res/snmp/agent.c
@@ -298,12 +298,18 @@ static u_char *ast_var_channels_table(struct variable *vp, oid *name, size_t *le
 		}
 		break;
 	case ASTCHANBRIDGE:
-		if ((bridge = ast_channel_bridge_peer(chan)) != NULL) {
+		ast_channel_unlock(chan);
+		bridge = ast_channel_bridge_peer(chan);
+		if (bridge) {
+			ast_channel_lock(bridge);
 			ast_copy_string(string_ret, ast_channel_name(bridge), sizeof(string_ret));
+			ast_channel_unlock(bridge);
+			ast_channel_unref(bridge);
+
 			*var_len = strlen(string_ret);
 			ret = (u_char *)string_ret;
-			ast_channel_unref(bridge);
 		}
+		ast_channel_lock(chan);
 		break;
 	case ASTCHANMASQ:
 		if (ast_channel_masq(chan) && !ast_strlen_zero(ast_channel_name(ast_channel_masq(chan)))) {