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

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

diff --git a/include/asterisk/rtp_engine.h b/include/asterisk/rtp_engine.h
index 51935dbc52eeae438866e443392c0e43cfd3da66..1bd2eea813b45c0c2fd50df1c881c1143f3622cc 100644 (file)
--- a/include/asterisk/rtp_engine.h
+++ b/include/asterisk/rtp_engine.h
@@ -412,7 +412,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, format_t codecs, int nat_active);
-       /*! Callback for retrieving codecs that the channel can do */
+       /*! Callback for retrieving codecs that the channel can do
+        * \note chan will be locked when this function is called
+        */
        format_t (*get_codec)(struct ast_channel *chan);
        /*! Linked list information */
        AST_RWLIST_ENTRY(ast_rtp_glue) entry;

diff --git a/main/rtp_engine.c b/main/rtp_engine.c
index 6738f45b0f5f115f3c1a1cf094314670751bd51a..8a449e562348584b206566021144be104697696f 100644 (file)
--- a/main/rtp_engine.c
+++ b/main/rtp_engine.c
@@ -1051,9 +1051,11 @@ static enum ast_bridge_result remote_bridge_loop(struct ast_channel *c0, struct
                if (tinstance1) {
                        ast_rtp_instance_get_remote_address(tinstance1, &tt1);
                }
-               if (glue1->get_codec) {
+               ast_channel_lock(c1);
+               if (glue1->get_codec && c1->tech_pvt) {
                        codec1 = glue1->get_codec(c1);
                }
+               ast_channel_unlock(c1);
 
                ast_rtp_instance_get_remote_address(instance0, &t0);
                if (vinstance0) {
@@ -1062,9 +1064,11 @@ static enum ast_bridge_result remote_bridge_loop(struct ast_channel *c0, struct
                if (tinstance0) {
                        ast_rtp_instance_get_remote_address(tinstance0, &tt0);
                }
-               if (glue0->get_codec) {
+               ast_channel_lock(c0);
+               if (glue0->get_codec && c0->tech_pvt) {
                        codec0 = glue0->get_codec(c0);
                }
+               ast_channel_unlock(c0);
 
                if ((ast_sockaddr_cmp(&t1, &ac1)) ||
                    (vinstance1 && ast_sockaddr_cmp(&vt1, &vac1)) ||
@@ -1175,13 +1179,19 @@ static enum ast_bridge_result remote_bridge_loop(struct ast_channel *c0, struct
                                ast_sockaddr_copy(&ac0, &t0);
                                ast_rtp_instance_get_remote_address(instance1, &t1);
                                ast_sockaddr_copy(&ac1, &t1);
+
                                /* Update codec information */
+                               ast_channel_lock(c0);
                                if (glue0->get_codec && c0->tech_pvt) {
                                        oldcodec0 = codec0 = glue0->get_codec(c0);
                                }
+                               ast_channel_unlock(c0);
+                               ast_channel_lock(c1);
                                if (glue1->get_codec && c1->tech_pvt) {
                                        oldcodec1 = codec1 = glue1->get_codec(c1);
                                }
+                               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);