<p>Joshua Colp <strong>merged</strong> this change.</p><p><a href="https://gerrit.asterisk.org/6561">View Change</a></p><div style="white-space:pre-wrap">Approvals:
  Joshua Colp: Looks good to me, but someone else must approve; Approved for Submit
  George Joseph: Looks good to me, approved

</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">res_rtp_asterisk.c: Fix bundled SSRC handling.<br><br>Assertions in the v15+ AST-2017-008 patches found that we were not<br>handling the case if the incoming SDP did not specify the required SSRC<br>attributes for bundled to work.<br><br>* Be strict on matching SSRC for bundled instances including the parent<br>instance.  If the SSRC doesn't match then discard the packet.  Bundled has<br>to tell us in the SDP signaling what SSRC to expect.  Otherwise, we will<br>not know how to find the bundled instance structure.<br><br>Change-Id: I152830bbff71c662408909042068fada39e617f9<br>---<br>M res/res_rtp_asterisk.c<br>1 file changed, 58 insertions(+), 42 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c<br>index 1440eb0..c8cc04f 100644<br>--- a/res/res_rtp_asterisk.c<br>+++ b/res/res_rtp_asterisk.c<br>@@ -257,6 +257,8 @@<br> struct rtp_ssrc_mapping {<br>   /*! \brief The received SSRC */<br>       unsigned int ssrc;<br>+   /*! True if the SSRC is available.  Otherwise, this is a placeholder mapping until the SSRC is set. */<br>+       unsigned int ssrc_valid;<br>      /*! \brief The RTP instance this SSRC belongs to*/<br>    struct ast_rtp_instance *instance;<br> };<br>@@ -3344,7 +3346,7 @@<br>  * \return 0 if element does not match.<br>  * \return Non-zero if element matches.<br>  */<br>-#define SSRC_MAPPING_ELEM_CMP(elem, value) (elem.instance == value)<br>+#define SSRC_MAPPING_ELEM_CMP(elem, value) ((elem).instance == (value))<br> <br> /*! \pre instance is locked */<br> static int ast_rtp_destroy(struct ast_rtp_instance *instance)<br>@@ -4788,18 +4790,26 @@<br>         struct ast_rtp *rtp, unsigned int ssrc)<br> {<br>   int index;<br>-   struct ast_rtp_instance *found = instance;<br> <br>+        if (!AST_VECTOR_SIZE(&rtp->ssrc_mapping)) {<br>+           /* This instance is not bundled */<br>+           return instance;<br>+     }<br>+<br>+ /* Find the bundled child instance */<br>         for (index = 0; index < AST_VECTOR_SIZE(&rtp->ssrc_mapping); ++index) {<br>             struct rtp_ssrc_mapping *mapping = AST_VECTOR_GET_ADDR(&rtp->ssrc_mapping, index);<br> <br>-         if (mapping->ssrc == ssrc) {<br>-                      found = mapping->instance;<br>-                        break;<br>+               if (mapping->ssrc_valid && mapping->ssrc == ssrc) {<br>+                    return mapping->instance;<br>          }<br>     }<br> <br>- return found;<br>+        /* Does the SSRC match the bundled parent? */<br>+        if (rtp->themssrc_valid && rtp->themssrc == ssrc) {<br>+            return instance;<br>+     }<br>+    return NULL;<br> }<br> <br> static const char *rtcp_payload_type2str(unsigned int pt)<br>@@ -5040,7 +5050,7 @@<br>                /* Determine the appropriate instance for this */<br>             if (ssrc_valid) {<br>                     child = rtp_find_instance_by_ssrc(transport, transport_rtp, ssrc);<br>-                   if (child != transport) {<br>+                    if (child && child != transport) {<br>                            /*<br>                             * It is safe to hold the child lock while holding the parent lock.<br>                            * We guarantee that the locking order is always parent->child or<br>@@ -5551,6 +5561,10 @@<br> <br>       /* Determine the appropriate instance for this */<br>     child = rtp_find_instance_by_ssrc(instance, rtp, ssrc);<br>+      if (!child) {<br>+                /* Neither the bundled parent nor any child has this SSRC */<br>+         return &ast_null_frame;<br>+  }<br>     if (child != instance) {<br>              /* It is safe to hold the child lock while holding the parent lock, we guarantee that the locking order<br>                * is always parent->child or that the child lock is not held when acquiring the parent lock.<br>@@ -5726,39 +5740,42 @@<br>     timestamp = ntohl(rtpheader[1]);<br> <br>   AST_LIST_HEAD_INIT_NOLOCK(&frames);<br>-      /* Force a marker bit and change SSRC if the SSRC changes */<br>- if (rtp->themssrc_valid && rtp->themssrc != ssrc) {<br>-            struct ast_frame *f, srcupdate = {<br>-                   AST_FRAME_CONTROL,<br>-                   .subclass.integer = AST_CONTROL_SRCCHANGE,<br>-           };<br> <br>-                if (!mark) {<br>-                 if (rtpdebug) {<br>-                              ast_debug(1, "Forcing Marker bit, because SSRC has changed\n");<br>+    /* Only non-bundled instances can change/learn the remote's SSRC implicitly. */<br>+  if (!child && !AST_VECTOR_SIZE(&rtp->ssrc_mapping)) {<br>+         /* Force a marker bit and change SSRC if the SSRC changes */<br>+         if (rtp->themssrc_valid && rtp->themssrc != ssrc) {<br>+                    struct ast_frame *f, srcupdate = {<br>+                           AST_FRAME_CONTROL,<br>+                           .subclass.integer = AST_CONTROL_SRCCHANGE,<br>+                   };<br>+<br>+                        if (!mark) {<br>+                         if (rtpdebug) {<br>+                                      ast_debug(1, "Forcing Marker bit, because SSRC has changed\n");<br>+                            }<br>+                            mark = 1;<br>                     }<br>-                    mark = 1;<br>+<br>+                 f = ast_frisolate(&srcupdate);<br>+                   AST_LIST_INSERT_TAIL(&frames, f, frame_list);<br>+<br>+                 rtp->seedrxseqno = 0;<br>+                     rtp->rxcount = 0;<br>+                 rtp->rxoctetcount = 0;<br>+                    rtp->cycles = 0;<br>+                  rtp->lastrxseqno = 0;<br>+                     rtp->last_seqno = 0;<br>+                      rtp->last_end_timestamp = 0;<br>+                      if (rtp->rtcp) {<br>+                          rtp->rtcp->expected_prior = 0;<br>+                         rtp->rtcp->received_prior = 0;<br>+                 }<br>             }<br> <br>-         f = ast_frisolate(&srcupdate);<br>-           AST_LIST_INSERT_TAIL(&frames, f, frame_list);<br>-<br>-         rtp->seedrxseqno = 0;<br>-             rtp->rxcount = 0;<br>-         rtp->rxoctetcount = 0;<br>-            rtp->cycles = 0;<br>-          rtp->lastrxseqno = 0;<br>-             rtp->last_seqno = 0;<br>-              rtp->last_end_timestamp = 0;<br>-              if (rtp->rtcp) {<br>-                  rtp->rtcp->expected_prior = 0;<br>-                 rtp->rtcp->received_prior = 0;<br>-         }<br>+            rtp->themssrc = ssrc; /* Record their SSRC to put in future RR */<br>+         rtp->themssrc_valid = 1;<br>   }<br>-    /* Bundled children cannot change/learn their SSRC implicitly. */<br>-    ast_assert(!child || (rtp->themssrc_valid && rtp->themssrc == ssrc));<br>-  rtp->themssrc = ssrc; /* Record their SSRC to put in future RR */<br>- rtp->themssrc_valid = 1;<br> <br>        /* Remove any padding bytes that may be present */<br>    if (padding) {<br>@@ -6520,12 +6537,13 @@<br>               return;<br>       }<br> <br>+ rtp->themssrc = ssrc;<br>+     rtp->themssrc_valid = 1;<br>+<br>        /* If this is bundled we need to update the SSRC mapping */<br>   if (rtp->bundled) {<br>                struct ast_rtp *bundled_rtp;<br>          int index;<br>-<br>-                ast_assert(rtp->themssrc_valid);<br> <br>                ao2_unlock(instance);<br> <br>@@ -6536,8 +6554,9 @@<br>               for (index = 0; index < AST_VECTOR_SIZE(&bundled_rtp->ssrc_mapping); ++index) {<br>                     struct rtp_ssrc_mapping *mapping = AST_VECTOR_GET_ADDR(&bundled_rtp->ssrc_mapping, index);<br> <br>-                 if (mapping->ssrc == rtp->themssrc) {<br>+                  if (mapping->instance == instance) {<br>                               mapping->ssrc = ssrc;<br>+                             mapping->ssrc_valid = 1;<br>                           break;<br>                        }<br>             }<br>@@ -6546,9 +6565,6 @@<br> <br>           ao2_lock(instance);<br>   }<br>-<br>- rtp->themssrc = ssrc;<br>-     rtp->themssrc_valid = 1;<br> }<br> <br> static void ast_rtp_set_stream_num(struct ast_rtp_instance *instance, int stream_num)<br>@@ -6601,8 +6617,8 @@<br>     /* Children maintain a reference to the parent to guarantee that the transport doesn't go away on them */<br>         child_rtp->bundled = ao2_bump(parent);<br> <br>- ast_assert(child_rtp->themssrc_valid);<br>     mapping.ssrc = child_rtp->themssrc;<br>+       mapping.ssrc_valid = child_rtp->themssrc_valid;<br>    mapping.instance = child;<br> <br>  ao2_unlock(child);<br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/6561">change 6561</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/6561"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: merged </div>
<div style="display:none"> Gerrit-Change-Id: I152830bbff71c662408909042068fada39e617f9 </div>
<div style="display:none"> Gerrit-Change-Number: 6561 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>