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

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

diff --git a/CHANGES b/CHANGES
index 6c29773..84958a2 100644 (file)
--- 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 bb65df4..cc70bfd 100644 (file)
--- 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 e17c7df..1d9623a 100644 (file)
--- 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 8eefd64..68baa1e 100644 (file)
--- 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 cb282e5..1881106 100644 (file)
--- 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 abaf372..9d1528d 100644 (file)
--- 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)))) {