Merged revisions 336878 via svnmerge from
authorRussell Bryant <russell@russellbryant.com>
Tue, 20 Sep 2011 01:11:18 +0000 (01:11 +0000)
committerRussell Bryant <russell@russellbryant.com>
Tue, 20 Sep 2011 01:11:18 +0000 (01:11 +0000)
https://origsvn.digium.com/svn/asterisk/branches/10

................
  r336878 | russell | 2011-09-19 20:03:55 -0500 (Mon, 19 Sep 2011) | 43 lines

  Merged revisions 336877 via svnmerge from
  https://origsvn.digium.com/svn/asterisk/branches/1.8

  ........
    r336877 | russell | 2011-09-19 19:56:20 -0500 (Mon, 19 Sep 2011) | 36 lines

    Fix crashes in ast_rtcp_write().

    This patch addresses crashes related to RTCP handling.  The backtraces just
    show a crash in ast_rtcp_write() where it appears that the RTP instance is no
    longer valid.  There is a race condition with scheduled RTCP transmissions and
    the destruction of the RTP instance.  This patch utilizes the fact that
    ast_rtp_instance is a reference counted object and ensures that it will not get
    destroyed while a reference is still around due to scheduled RTCP
    transmissions.

    RTCP transmissions are scheduled and executed from the chan_sip scheduler
    context.  This scheduler context is processed in the SIP monitor thread.  The
    destruction of an RTP instance occurs when the associated sip_pvt gets
    destroyed (which happens when the sip_pvt reference count reaches 0).  However,
    the SIP monitor thread is not the only thread that can cause a sip_pvt to get
    destroyed.  The sip_hangup function, executed from a channel thread, also
    decrements the reference count on a sip_pvt and could cause it to get
    destroyed.

    While this is being changed anyway, the patch also removes calling
    ast_sched_del() from within the RTCP scheduler callback.  It's not helpful.
    Simply returning 0 prevents the callback from being rescheduled.

    (closes issue ASTERISK-18570)

    Related issues that look like they are the same problem:

    (issue ASTERISK-17560)
    (issue ASTERISK-15406)
    (issue ASTERISK-15257)
    (issue ASTERISK-13334)
    (issue ASTERISK-9977)
    (issue ASTERISK-9716)

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

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

res/res_rtp_asterisk.c

index 06af483..f4e8299 100644 (file)
@@ -522,7 +522,11 @@ static int ast_rtp_destroy(struct ast_rtp_instance *instance)
 
        /* Destroy RTCP if it was being used */
        if (rtp->rtcp) {
-               AST_SCHED_DEL(rtp->sched, rtp->rtcp->schedid);
+               /*
+                * It is not possible for there to be an active RTCP scheduler
+                * entry at this point since it holds a reference to the
+                * RTP instance while it's active.
+                */
                close(rtp->rtcp->s);
                ast_free(rtp->rtcp);
        }
@@ -830,8 +834,9 @@ static int ast_rtcp_write_rr(struct ast_rtp_instance *instance)
                return 0;
 
        if (ast_sockaddr_isnull(&rtp->rtcp->them)) {
-               ast_log(LOG_ERROR, "RTCP RR transmission error, rtcp halted\n");
-               AST_SCHED_DEL(rtp->sched, rtp->rtcp->schedid);
+               /*
+                * RTCP was stopped.
+                */
                return 0;
        }
 
@@ -886,8 +891,6 @@ static int ast_rtcp_write_rr(struct ast_rtp_instance *instance)
 
        if (res < 0) {
                ast_log(LOG_ERROR, "RTCP RR transmission error, rtcp halted: %s\n",strerror(errno));
-               /* Remove the scheduler */
-               AST_SCHED_DEL(rtp->sched, rtp->rtcp->schedid);
                return 0;
        }
 
@@ -932,8 +935,9 @@ static int ast_rtcp_write_sr(struct ast_rtp_instance *instance)
                return 0;
 
        if (ast_sockaddr_isnull(&rtp->rtcp->them)) {  /* This'll stop rtcp for this rtp session */
-               ast_verbose("RTCP SR transmission error, rtcp halted\n");
-               AST_SCHED_DEL(rtp->sched, rtp->rtcp->schedid);
+               /*
+                * RTCP was stopped.
+                */
                return 0;
        }
 
@@ -985,7 +989,6 @@ static int ast_rtcp_write_sr(struct ast_rtp_instance *instance)
                ast_log(LOG_ERROR, "RTCP SR transmission error to %s, rtcp halted %s\n",
                        ast_sockaddr_stringify(&rtp->rtcp->them),
                        strerror(errno));
-               AST_SCHED_DEL(rtp->sched, rtp->rtcp->schedid);
                return 0;
        }
 
@@ -1044,13 +1047,24 @@ static int ast_rtcp_write(const void *data)
        struct ast_rtp *rtp = ast_rtp_instance_get_data(instance);
        int res;
 
-       if (!rtp || !rtp->rtcp)
+       if (!rtp || !rtp->rtcp || rtp->rtcp->schedid == -1) {
+               ao2_ref(instance, -1);
                return 0;
+       }
 
-       if (rtp->txcount > rtp->rtcp->lastsrtxcount)
+       if (rtp->txcount > rtp->rtcp->lastsrtxcount) {
                res = ast_rtcp_write_sr(instance);
-       else
+       } else {
                res = ast_rtcp_write_rr(instance);
+       }
+
+       if (!res) {
+               /* 
+                * Not being rescheduled.
+                */
+               ao2_ref(instance, -1);
+               rtp->rtcp->schedid = -1;
+       }
 
        return res;
 }
@@ -1162,7 +1176,12 @@ static int ast_rtp_raw_write(struct ast_rtp_instance *instance, struct ast_frame
 
                        if (rtp->rtcp && rtp->rtcp->schedid < 1) {
                                ast_debug(1, "Starting RTCP transmission on RTP instance '%p'\n", instance);
+                               ao2_ref(instance, +1);
                                rtp->rtcp->schedid = ast_sched_add(rtp->sched, ast_rtcp_calc_interval(rtp), ast_rtcp_write, instance);
+                               if (rtp->rtcp->schedid < 0) {
+                                       ao2_ref(instance, -1);
+                                       ast_log(LOG_WARNING, "scheduling RTCP transmission failed.\n");
+                               }
                        }
                }
 
@@ -2180,7 +2199,12 @@ static struct ast_frame *ast_rtp_read(struct ast_rtp_instance *instance, int rtc
        /* Do not schedule RR if RTCP isn't run */
        if (rtp->rtcp && !ast_sockaddr_isnull(&rtp->rtcp->them) && rtp->rtcp->schedid < 1) {
                /* Schedule transmission of Receiver Report */
+               ao2_ref(instance, +1);
                rtp->rtcp->schedid = ast_sched_add(rtp->sched, ast_rtcp_calc_interval(rtp), ast_rtcp_write, instance);
+               if (rtp->rtcp->schedid < 0) {
+                       ao2_ref(instance, -1);
+                       ast_log(LOG_WARNING, "scheduling RTCP transmission failed.\n");
+               }
        }
        if ((int)rtp->lastrxseqno - (int)seqno  > 100) /* if so it would indicate that the sender cycled; allow for misordering */
                rtp->cycles += RTP_SEQ_MOD;
@@ -2592,9 +2616,14 @@ static void ast_rtp_stop(struct ast_rtp_instance *instance)
        struct ast_rtp *rtp = ast_rtp_instance_get_data(instance);
        struct ast_sockaddr addr = { {0,} };
 
-       if (rtp->rtcp) {
-               AST_SCHED_DEL(rtp->sched, rtp->rtcp->schedid);
+       if (rtp->rtcp && rtp->rtcp->schedid > 0) {
+               if (!ast_sched_del(rtp->sched, rtp->rtcp->schedid)) {
+                       /* successfully cancelled scheduler entry. */
+                       ao2_ref(instance, -1);
+               }
+               rtp->rtcp->schedid = -1;
        }
+
        if (rtp->red) {
                AST_SCHED_DEL(rtp->sched, rtp->red->schedid);
                free(rtp->red);