From: Matthew Jordan Date: Thu, 27 Feb 2014 12:47:29 +0000 (+0000) Subject: rtp_engine: fix crash during remote native bridging when calling get_codecs X-Git-Tag: 11.9.0-rc1~52 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6087765a48b1871a2f3ad1211e3f5806dc57081d;p=thirdparty%2Fasterisk.git rtp_engine: fix crash during remote native bridging when calling get_codecs When two RTP channels are in a remote bridge, the remote bridging loop in rtp_engine will periodically check to see if the two channels can still be bridged. One of the many things it checks is whether or not the codecs have changed on the channel. If the codec has changed, it will break out of the loop to re-determine which type of bridge is appropriate. In order to perform this check, the ast_rtp_glue virtual table's get_codec callback is called for each channel. The callback implementations assume that the channel tech private is valid when called; as such, there has always been some code in place to check whether or not the channel pvt is NULL before calling. However, this check is insufficient. The channels are unlocked during the remote bridging loop. It is possible for a channel to get masqueraded between the check for the pvt being NULL and the actual call to get_codec. When this occurs, the callback is called with a ZOMBIE channel, which now has a NULL pvt. Crash. While this has always been possible in Asterisk 1.8, it is much more likely to occur in Asterisk 11 and later versions due to the timing changes that occur when getting the codec from a channel. Note that this is much more likely to be reproduced on slow, boggy hardware running Asterisk 11 - but fairly rarely otherwise. Also Note: This crash was also caught by the various SIP blind transfer tests, in addition to the bug report Alec filed. Review: https://reviewboard.asterisk.org/r/3247/ (closes issue ASTERISK-21737) Reported by: Alec Davis Tested by: Alec Davis ........ Merged revisions 409001 from http://svn.asterisk.org/svn/asterisk/branches/1.8 git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/11@409002 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- diff --git a/include/asterisk/rtp_engine.h b/include/asterisk/rtp_engine.h index f6e536bd51..91e6335ded 100644 --- a/include/asterisk/rtp_engine.h +++ b/include/asterisk/rtp_engine.h @@ -539,7 +539,9 @@ struct ast_rtp_glue { enum ast_rtp_glue_result (*get_trtp_info)(struct ast_channel *chan, struct ast_rtp_instance **instance); /*! Callback for updating the destination that the remote side should send RTP to */ int (*update_peer)(struct ast_channel *chan, struct ast_rtp_instance *instance, struct ast_rtp_instance *vinstance, struct ast_rtp_instance *tinstance, const struct ast_format_cap *cap, int nat_active); - /*! Callback for retrieving codecs that the channel can do. Result returned in result_cap*/ + /*! Callback for retrieving codecs that the channel can do. Result returned in result_cap. + * \note The channel chan will be locked during this call. + */ void (*get_codec)(struct ast_channel *chan, struct ast_format_cap *result_cap); /*! Linked list information */ AST_RWLIST_ENTRY(ast_rtp_glue) entry; diff --git a/main/rtp_engine.c b/main/rtp_engine.c index a29d09e274..d88ff2f5b9 100644 --- a/main/rtp_engine.c +++ b/main/rtp_engine.c @@ -1224,10 +1224,12 @@ static enum ast_bridge_result remote_bridge_loop(struct ast_channel *c0, if (tinstance1) { ast_rtp_instance_get_remote_address(tinstance1, &tt1); } - if (glue1->get_codec) { + ast_channel_lock(c1); + if (glue1->get_codec && ast_channel_tech_pvt(c1)) { ast_format_cap_remove_all(cap1); glue1->get_codec(c1, cap1); } + ast_channel_unlock(c1); ast_rtp_instance_get_remote_address(instance0, &t0); if (vinstance0) { @@ -1236,10 +1238,12 @@ static enum ast_bridge_result remote_bridge_loop(struct ast_channel *c0, if (tinstance0) { ast_rtp_instance_get_remote_address(tinstance0, &tt0); } - if (glue0->get_codec) { + ast_channel_lock(c0); + if (glue0->get_codec && ast_channel_tech_pvt(c0)) { ast_format_cap_remove_all(cap0); glue0->get_codec(c0, cap0); } + ast_channel_unlock(c0); if ((ast_sockaddr_cmp(&t1, &ac1)) || (vinstance1 && ast_sockaddr_cmp(&vt1, &vac1)) || @@ -1353,6 +1357,7 @@ static enum ast_bridge_result remote_bridge_loop(struct ast_channel *c0, ast_rtp_instance_get_remote_address(instance1, &t1); ast_sockaddr_copy(&ac1, &t1); /* Update codec information */ + ast_channel_lock(c0); if (glue0->get_codec && ast_channel_tech_pvt(c0)) { ast_format_cap_remove_all(cap0); ast_format_cap_remove_all(oldcap0); @@ -1360,12 +1365,15 @@ static enum ast_bridge_result remote_bridge_loop(struct ast_channel *c0, ast_format_cap_append(oldcap0, cap0); } + ast_channel_unlock(c0); + ast_channel_lock(c1); if (glue1->get_codec && ast_channel_tech_pvt(c1)) { ast_format_cap_remove_all(cap1); ast_format_cap_remove_all(oldcap1); glue1->get_codec(c1, cap1); ast_format_cap_append(oldcap1, cap1); } + ast_channel_unlock(c1); /* Since UPDATE_BRIDGE_PEER is only used by the bridging code, don't forward it */ if (fr->subclass.integer != AST_CONTROL_UPDATE_RTP_PEER) { ast_indicate_data(other, fr->subclass.integer, fr->data.ptr, fr->datalen);