chan_sip.c: Fix channel staging assertion failure.

The failing assertion ensures that the final snapshot gets generated so
CDR records can get finalized.  The only place where a channel staging
snapshot flag could be left set is in chan_sip.c:handle_request_bye().
The function could return before clearing the flag because the channel
could dissappear while the function had to have the channel unlocked.

* Fixed handle_request_bye() channel snapshot staging coverage area to not
have a return in the middle of it and be unable to clear the staging flag.

* Pushed the channel snapshot staging coverage area into
ast_rtp_instance_set_stats_vars() to ensure that the staging is not
interrutped.

* Made callers of ast_rtp_instance_set_stats_vars() not call it with any
channels or channel driver private locks held to eliminate the deadlock
potential.  The callers must hold references to the passed in channel and
rtp objects.

* Eliminated sip_hangup() trying to get the bridge peer.  It is futile at
this point because the channel could never be in a bridge.

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

Merged revisions 412385 from http://svn.asterisk.org/svn/asterisk/branches/12

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

diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index ef57015..eb0a5f2 100644 (file)
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -7248,40 +7248,29 @@ static int sip_hangup(struct ast_channel *ast)
                        }
 
                        if (!p->pendinginvite) {
-                               RAII_VAR(struct ast_channel *, bridge, ast_channel_bridge_peer(oldowner), ast_channel_cleanup);
-                               char quality_buf[AST_MAX_USER_FIELD], *quality;
-
-                               /* We need to get the lock on bridge because ast_rtp_instance_set_stats_vars will attempt
-                                * to lock the bridge. This may get hairy...
-                                */
-                               while (bridge && ast_channel_trylock(bridge)) {
-                                       sip_pvt_unlock(p);
-                                       do {
-                                               CHANNEL_DEADLOCK_AVOIDANCE(oldowner);
-                                       } while (sip_pvt_trylock(p));
-                               }
-
-                               if (p->rtp || p->vrtp || p->trtp) {
-                                       ast_channel_stage_snapshot(oldowner);
-                               }
+                               char *quality;
+                               char quality_buf[AST_MAX_USER_FIELD];
 
                                if (p->rtp) {
-                                       ast_rtp_instance_set_stats_vars(oldowner, p->rtp);
-                               }
+                                       struct ast_rtp_instance *p_rtp;
 
-                               if (bridge) {
-                                       struct sip_pvt *q = ast_channel_tech_pvt(bridge);
-
-                                       if (IS_SIP_TECH(ast_channel_tech(bridge)) && q && q->rtp) {
-                                               ast_rtp_instance_set_stats_vars(bridge, q->rtp);
-                                       }
-                                       ast_channel_unlock(bridge);
+                                       p_rtp = p->rtp;
+                                       ao2_ref(p_rtp, +1);
+                                       ast_channel_unlock(oldowner);
+                                       sip_pvt_unlock(p);
+                                       ast_rtp_instance_set_stats_vars(oldowner, p_rtp);
+                                       ao2_ref(p_rtp, -1);
+                                       ast_channel_lock(oldowner);
+                                       sip_pvt_lock(p);
                                }
 
                                /*
                                 * The channel variables are set below just to get the AMI
                                 * VarSet event because the channel is being hungup.
                                 */
+                               if (p->rtp || p->vrtp || p->trtp) {
+                                       ast_channel_stage_snapshot(oldowner);
+                               }
                                if (p->rtp && (quality = ast_rtp_instance_get_quality(p->rtp, AST_RTP_INSTANCE_STAT_FIELD_QUALITY, quality_buf, sizeof(quality_buf)))) {
                                        if (p->do_history) {
                                                append_history(p, "RTCPaudio", "Quality:%s", quality);
@@ -26443,10 +26432,6 @@ static int handle_request_bye(struct sip_pvt *p, struct sip_request *req)
                }
        }
 
-       if ((p->rtp || p->vrtp || p->trtp) && p->owner) {
-               ast_channel_stage_snapshot(p->owner);
-       }
-
        /* Get RTCP quality before end of call */
        if (p->rtp) {
                if (p->do_history) {
@@ -26467,22 +26452,49 @@ static int handle_request_bye(struct sip_pvt *p, struct sip_request *req)
                if (p->owner) {
                        RAII_VAR(struct ast_channel *, owner_relock, NULL, ast_channel_cleanup);
                        RAII_VAR(struct ast_channel *, owner_ref, NULL, ast_channel_cleanup);
+                       struct ast_rtp_instance *p_rtp;
 
                        /* Grab a reference to p->owner to prevent it from going away */
                        owner_ref = ast_channel_ref(p->owner);
 
+                       p_rtp = p->rtp;
+                       ao2_ref(p_rtp, +1);
+
                        /* Established locking order here is bridge, channel, pvt
                         * and the bridge and channel will be locked during
                         * ast_rtp_instance_set_stats_vars */
                        ast_channel_unlock(owner_ref);
                        sip_pvt_unlock(p);
 
-                       ast_rtp_instance_set_stats_vars(owner_ref, p->rtp);
-                       if (peer_channel && IS_SIP_TECH(ast_channel_tech(peer_channel))) {
-                               struct sip_pvt *q = ast_channel_tech_pvt(peer_channel);
-                               if (q && q->rtp) {
-                                       ast_rtp_instance_set_stats_vars(peer_channel, q->rtp);
+                       ast_rtp_instance_set_stats_vars(owner_ref, p_rtp);
+                       ao2_ref(p_rtp, -1);
+
+                       if (peer_channel) {
+                               ast_channel_lock(peer_channel);
+                               if (IS_SIP_TECH(ast_channel_tech(peer_channel))) {
+                                       struct sip_pvt *peer_pvt;
+
+                                       peer_pvt = ast_channel_tech_pvt(peer_channel);
+                                       if (peer_pvt) {
+                                               ao2_ref(peer_pvt, +1);
+                                               sip_pvt_lock(peer_pvt);
+                                               if (peer_pvt->rtp) {
+                                                       struct ast_rtp_instance *peer_rtp;
+
+                                                       peer_rtp = peer_pvt->rtp;
+                                                       ao2_ref(peer_rtp, +1);
+                                                       ast_channel_unlock(peer_channel);
+                                                       sip_pvt_unlock(peer_pvt);
+                                                       ast_rtp_instance_set_stats_vars(peer_channel, peer_rtp);
+                                                       ao2_ref(peer_rtp, -1);
+                                                       ast_channel_lock(peer_channel);
+                                                       sip_pvt_lock(peer_pvt);
+                                               }
+                                               sip_pvt_unlock(peer_pvt);
+                                               ao2_ref(peer_pvt, -1);
+                                       }
                                }
+                               ast_channel_unlock(peer_channel);
                        }
 
                        owner_relock = sip_pvt_lock_full(p);
@@ -26511,10 +26523,6 @@ static int handle_request_bye(struct sip_pvt *p, struct sip_request *req)
                }
        }
 
-       if ((p->rtp || p->vrtp || p->trtp) && p->owner) {
-               ast_channel_stage_snapshot_done(p->owner);
-       }
-
        stop_media_flows(p); /* Immediately stop RTP, VRTP and UDPTL as applicable */
        stop_session_timer(p); /* Stop Session-Timer */
 

diff --git a/include/asterisk/rtp_engine.h b/include/asterisk/rtp_engine.h
index 677a59c..25a3a81 100644 (file)
--- a/include/asterisk/rtp_engine.h
+++ b/include/asterisk/rtp_engine.h
@@ -1745,6 +1745,8 @@ int ast_rtp_instance_get_stats(struct ast_rtp_instance *instance, struct ast_rtp
  * \param chan Channel to set the statistics on
  * \param instance The RTP instance that statistics will be retrieved from
  *
+ * \note Absolutely _NO_ channel locks should be held before calling this function.
+ *
  * Example usage:
  *
  * \code

diff --git a/main/rtp_engine.c b/main/rtp_engine.c
index fd058a4..4837230 100644 (file)
--- a/main/rtp_engine.c
+++ b/main/rtp_engine.c
@@ -1305,36 +1305,64 @@ char *ast_rtp_instance_get_quality(struct ast_rtp_instance *instance, enum ast_r
 
 void ast_rtp_instance_set_stats_vars(struct ast_channel *chan, struct ast_rtp_instance *instance)
 {
-       char quality_buf[AST_MAX_USER_FIELD], *quality;
-       RAII_VAR(struct ast_channel *, bridge, ast_channel_bridge_peer(chan), ast_channel_cleanup);
-
-       if ((quality = ast_rtp_instance_get_quality(instance, AST_RTP_INSTANCE_STAT_FIELD_QUALITY, quality_buf, sizeof(quality_buf)))) {
+       char quality_buf[AST_MAX_USER_FIELD];
+       char *quality;
+       struct ast_channel *bridge = ast_channel_bridge_peer(chan);
+
+       ast_channel_lock(chan);
+       ast_channel_stage_snapshot(chan);
+       ast_channel_unlock(chan);
+       if (bridge) {
+               ast_channel_lock(bridge);
+               ast_channel_stage_snapshot(bridge);
+               ast_channel_unlock(bridge);
+       }
+
+       quality = ast_rtp_instance_get_quality(instance, AST_RTP_INSTANCE_STAT_FIELD_QUALITY,
+               quality_buf, sizeof(quality_buf));
+       if (quality) {
                pbx_builtin_setvar_helper(chan, "RTPAUDIOQOS", quality);
                if (bridge) {
                        pbx_builtin_setvar_helper(bridge, "RTPAUDIOQOSBRIDGED", quality);
                }
        }
 
-       if ((quality = ast_rtp_instance_get_quality(instance, AST_RTP_INSTANCE_STAT_FIELD_QUALITY_JITTER, quality_buf, sizeof(quality_buf)))) {
+       quality = ast_rtp_instance_get_quality(instance,
+               AST_RTP_INSTANCE_STAT_FIELD_QUALITY_JITTER, quality_buf, sizeof(quality_buf));
+       if (quality) {
                pbx_builtin_setvar_helper(chan, "RTPAUDIOQOSJITTER", quality);
                if (bridge) {
                        pbx_builtin_setvar_helper(bridge, "RTPAUDIOQOSJITTERBRIDGED", quality);
                }
        }
 
-       if ((quality = ast_rtp_instance_get_quality(instance, AST_RTP_INSTANCE_STAT_FIELD_QUALITY_LOSS, quality_buf, sizeof(quality_buf)))) {
+       quality = ast_rtp_instance_get_quality(instance,
+               AST_RTP_INSTANCE_STAT_FIELD_QUALITY_LOSS, quality_buf, sizeof(quality_buf));
+       if (quality) {
                pbx_builtin_setvar_helper(chan, "RTPAUDIOQOSLOSS", quality);
                if (bridge) {
                        pbx_builtin_setvar_helper(bridge, "RTPAUDIOQOSLOSSBRIDGED", quality);
                }
        }
 
-       if ((quality = ast_rtp_instance_get_quality(instance, AST_RTP_INSTANCE_STAT_FIELD_QUALITY_RTT, quality_buf, sizeof(quality_buf)))) {
+       quality = ast_rtp_instance_get_quality(instance,
+               AST_RTP_INSTANCE_STAT_FIELD_QUALITY_RTT, quality_buf, sizeof(quality_buf));
+       if (quality) {
                pbx_builtin_setvar_helper(chan, "RTPAUDIOQOSRTT", quality);
                if (bridge) {
                        pbx_builtin_setvar_helper(bridge, "RTPAUDIOQOSRTTBRIDGED", quality);
                }
        }
+
+       ast_channel_lock(chan);
+       ast_channel_stage_snapshot_done(chan);
+       ast_channel_unlock(chan);
+       if (bridge) {
+               ast_channel_lock(bridge);
+               ast_channel_stage_snapshot_done(bridge);
+               ast_channel_unlock(bridge);
+               ast_channel_unref(bridge);
+       }
 }
 
 int ast_rtp_instance_set_read_format(struct ast_rtp_instance *instance, struct ast_format *format)