Merged revisions 324048 via svnmerge from

https://origsvn.digium.com/svn/asterisk/branches/1.8

........
  r324048 | twilson | 2011-06-16 17:35:41 -0500 (Thu, 16 Jun 2011) | 8 lines

  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/trunk@324050 65c4cc65-6c06-0410-ace0-fbb531ad65f3

diff --git a/channels/chan_local.c b/channels/chan_local.c
index 0ceb432b032caf413e58e9c2b69ace4026f10651..3c16fc51a0747b989e6360296921e95fa27df3f6 100644 (file)
--- a/channels/chan_local.c
+++ b/channels/chan_local.c
@@ -217,6 +217,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;
@@ -225,27 +226,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);
@@ -272,6 +268,7 @@ setoption_cleanup:
        if (otherchan) {
                ast_channel_unref(otherchan);
        }
+       ast_channel_lock(ast); /* Lock back before we leave */
        return res;
 }
 
@@ -348,6 +345,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;
@@ -361,21 +359,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))) {
@@ -394,6 +389,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 7940b0157359c5878bec18fdaf46b309ec54c4fe..4c0e2a66ce121052ce3c5fbbe4c0e82ce4a1fcf7 100644 (file)
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -4262,15 +4262,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, (struct ast_format *) data);
+               if (p->rtp) {
+                       res = ast_rtp_instance_set_read_format(p->rtp, (struct ast_format *) data);
+               }
                break;
        case AST_OPTION_FORMAT_WRITE:
-               res = ast_rtp_instance_set_write_format(p->rtp, (struct ast_format *) data);
+               if (p->rtp) {
+                       res = ast_rtp_instance_set_write_format(p->rtp, (struct ast_format *) 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) ||
@@ -4298,6 +4306,8 @@ static int sip_setoption(struct ast_channel *chan, int option, void *data, int d
                break;
        }
 
+       sip_pvt_unlock(p);
+
        return res;
 }
 
@@ -4309,16 +4319,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) {
@@ -4337,8 +4347,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;
 
@@ -4370,6 +4378,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 5798e92bb2c22e50c9dd79b0df948c3c3159fe4b..031c6f7e93173c2a667924cae6792527b53082b5 100644 (file)
--- a/include/asterisk/channel.h
+++ b/include/asterisk/channel.h
@@ -576,10 +576,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 47ebe9b8078bdf89e1d93a23fdae0a05e2ff9ef7..57629dbc533fa1e131eff6f0132acf59c0b02017 100644 (file)
--- a/main/channel.c
+++ b/main/channel.c
@@ -7608,28 +7608,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 {