From: Terry Wilson <twilson@digium.com>
Date: Thu, 16 Jun 2011 22:35:41 +0000 (+0000)
Subject: Lock the channel before calling the setoption callback
X-Git-Tag: 1.8.5-rc1~11^2~20
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c84e7b911e81efa0ad1e3efbeeecfb05292a948a;p=thirdparty%2Fasterisk.git

Lock the channel before calling the setoption callback

The channel needs to be locked before calling these callback functions. Also,
sip_setoption needs to lock the pvt and a check p->rtp is non-null before using
it.

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


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

diff --git a/channels/chan_local.c b/channels/chan_local.c
index 61bb4f4805..eb96c0c8a4 100644
--- a/channels/chan_local.c
+++ b/channels/chan_local.c
@@ -218,6 +218,7 @@ static void awesome_locking(struct local_pvt *p, struct ast_channel **outchan, s
 	*outchan = p->chan;
 }
 
+/* Called with ast locked */
 static int local_setoption(struct ast_channel *ast, int option, void * data, int datalen)
 {
 	int res = 0;
@@ -226,27 +227,22 @@ static int local_setoption(struct ast_channel *ast, int option, void * data, int
 	ast_chan_write_info_t *write_info;
 
 	if (option != AST_OPTION_CHANNEL_WRITE) {
-		res = -1;
-		goto setoption_cleanup;
+		return -1;
 	}
 
 	write_info = data;
 
 	if (write_info->version != AST_CHAN_WRITE_INFO_T_VERSION) {
 		ast_log(LOG_ERROR, "The chan_write_info_t type has changed, and this channel hasn't been updated!\n");
-		res = -1;
-		goto setoption_cleanup;
+		return -1
 	}
 
 	/* get the tech pvt */
-	ast_channel_lock(ast);
 	if (!(p = ast->tech_pvt)) {
-		ast_channel_unlock(ast);
-		res = -1;
-		goto setoption_cleanup;
+		return -1;
 	}
 	ao2_ref(p, 1);
-	ast_channel_unlock(ast);
+	ast_channel_unlock(ast); /* Held when called, unlock before locking another channel */
 
 	/* get the channel we are supposed to write to */
 	ao2_lock(p);
@@ -273,6 +269,7 @@ setoption_cleanup:
 	if (otherchan) {
 		ast_channel_unref(otherchan);
 	}
+	ast_channel_lock(ast); /* Lock back before we leave */
 	return res;
 }
 
@@ -349,6 +346,7 @@ static struct ast_channel *local_bridgedchannel(struct ast_channel *chan, struct
 	return bridged;
 }
 
+/* Called with ast locked */
 static int local_queryoption(struct ast_channel *ast, int option, void *data, int *datalen)
 {
 	struct local_pvt *p;
@@ -362,21 +360,18 @@ static int local_queryoption(struct ast_channel *ast, int option, void *data, in
 	}
 
 	/* for some reason the channel is not locked in channel.c when this function is called */
-	ast_channel_lock(ast);
 	if (!(p = ast->tech_pvt)) {
-		ast_channel_unlock(ast);
 		return -1;
 	}
 
 	ao2_lock(p);
 	if (!(tmp = IS_OUTBOUND(ast, p) ? p->owner : p->chan)) {
 		ao2_unlock(p);
-		ast_channel_unlock(ast);
 		return -1;
 	}
 	ast_channel_ref(tmp);
 	ao2_unlock(p);
-	ast_channel_unlock(ast);
+	ast_channel_unlock(ast); /* Held when called, unlock before locking another channel */
 
 	ast_channel_lock(tmp);
 	if (!(bridged = ast_bridged_channel(tmp))) {
@@ -395,6 +390,7 @@ query_cleanup:
 	if (tmp) {
 		tmp = ast_channel_unref(tmp);
 	}
+	ast_channel_lock(ast); /* Lock back before we leave */
 
 	return res;
 }
diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index b6ee0b9b94..b756a1810e 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -4226,15 +4226,23 @@ static int sip_setoption(struct ast_channel *chan, int option, void *data, int d
 	int res = -1;
 	struct sip_pvt *p = chan->tech_pvt;
 
+	sip_pvt_lock(p);
+
 	switch (option) {
 	case AST_OPTION_FORMAT_READ:
-		res = ast_rtp_instance_set_read_format(p->rtp, *(int *) data);
+		if (p->rtp) {
+			res = ast_rtp_instance_set_read_format(p->rtp, *(int *) data);
+		}
 		break;
 	case AST_OPTION_FORMAT_WRITE:
-		res = ast_rtp_instance_set_write_format(p->rtp, *(int *) data);
+		if (p->rtp) {
+			res = ast_rtp_instance_set_write_format(p->rtp, *(int *) data);
+		}
 		break;
 	case AST_OPTION_MAKE_COMPATIBLE:
-		res = ast_rtp_instance_make_compatible(chan, p->rtp, (struct ast_channel *) data);
+		if (p->rtp) {
+			res = ast_rtp_instance_make_compatible(chan, p->rtp, (struct ast_channel *) data);
+		}
 		break;
 	case AST_OPTION_DIGIT_DETECT:
 		if ((ast_test_flag(&p->flags[0], SIP_DTMF) == SIP_DTMF_INBAND) ||
@@ -4262,6 +4270,8 @@ static int sip_setoption(struct ast_channel *chan, int option, void *data, int d
 		break;
 	}
 
+	sip_pvt_unlock(p);
+
 	return res;
 }
 
@@ -4273,16 +4283,16 @@ static int sip_queryoption(struct ast_channel *chan, int option, void *data, int
 	struct sip_pvt *p = (struct sip_pvt *) chan->tech_pvt;
 	char *cp;
 
+	sip_pvt_lock(p);
+
 	switch (option) {
 	case AST_OPTION_T38_STATE:
 		/* Make sure we got an ast_t38_state enum passed in */
 		if (*datalen != sizeof(enum ast_t38_state)) {
 			ast_log(LOG_ERROR, "Invalid datalen for AST_OPTION_T38_STATE option. Expected %d, got %d\n", (int)sizeof(enum ast_t38_state), *datalen);
-			return -1;
+			break;
 		}
 
-		sip_pvt_lock(p);
-
 		/* Now if T38 support is enabled we need to look and see what the current state is to get what we want to report back */
 		if (ast_test_flag(&p->flags[1], SIP_PAGE2_T38SUPPORT)) {
 			switch (p->t38.state) {
@@ -4298,8 +4308,6 @@ static int sip_queryoption(struct ast_channel *chan, int option, void *data, int
 			}
 		}
 
-		sip_pvt_unlock(p);
-
 		*((enum ast_t38_state *) data) = state;
 		res = 0;
 
@@ -4331,6 +4339,8 @@ static int sip_queryoption(struct ast_channel *chan, int option, void *data, int
 		break;
 	}
 
+	sip_pvt_unlock(p);
+
 	return res;
 }
 
diff --git a/include/asterisk/channel.h b/include/asterisk/channel.h
index f887ca913d..8fda15787e 100644
--- a/include/asterisk/channel.h
+++ b/include/asterisk/channel.h
@@ -572,10 +572,10 @@ struct ast_channel_tech {
 	/*! \brief Fix up a channel:  If a channel is consumed, this is called.  Basically update any ->owner links */
 	int (* const fixup)(struct ast_channel *oldchan, struct ast_channel *newchan);
 
-	/*! \brief Set a given option */
+	/*! \brief Set a given option. Called with chan locked */
 	int (* const setoption)(struct ast_channel *chan, int option, void *data, int datalen);
 
-	/*! \brief Query a given option */
+	/*! \brief Query a given option. Called with chan locked */
 	int (* const queryoption)(struct ast_channel *chan, int option, void *data, int *datalen);
 
 	/*! \brief Blind transfer other side (see app_transfer.c and ast_transfer() */
diff --git a/main/channel.c b/main/channel.c
index 2c61d86ce3..ffb0595d56 100644
--- a/main/channel.c
+++ b/main/channel.c
@@ -7387,28 +7387,42 @@ enum ast_bridge_result ast_channel_bridge(struct ast_channel *c0, struct ast_cha
 /*! \brief Sets an option on a channel */
 int ast_channel_setoption(struct ast_channel *chan, int option, void *data, int datalen, int block)
 {
+	int res;
+
+	ast_channel_lock(chan);
 	if (!chan->tech->setoption) {
 		errno = ENOSYS;
+		ast_channel_unlock(chan);
 		return -1;
 	}
 
 	if (block)
 		ast_log(LOG_ERROR, "XXX Blocking not implemented yet XXX\n");
 
-	return chan->tech->setoption(chan, option, data, datalen);
+	res = chan->tech->setoption(chan, option, data, datalen);
+	ast_channel_unlock(chan);
+
+	return res;
 }
 
 int ast_channel_queryoption(struct ast_channel *chan, int option, void *data, int *datalen, int block)
 {
+	int res;
+
+	ast_channel_lock(chan);
 	if (!chan->tech->queryoption) {
 		errno = ENOSYS;
+		ast_channel_unlock(chan);
 		return -1;
 	}
 
 	if (block)
 		ast_log(LOG_ERROR, "XXX Blocking not implemented yet XXX\n");
 
-	return chan->tech->queryoption(chan, option, data, datalen);
+	res = chan->tech->queryoption(chan, option, data, datalen);
+	ast_channel_unlock(chan);
+
+	return res;
 }
 
 struct tonepair_def {