<p>Richard Mudgett has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.asterisk.org/6562">View Change</a></p><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;">git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/62/6562/1</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 923ec84..8f66a0a 100644<br>--- a/res/res_rtp_asterisk.c<br>+++ b/res/res_rtp_asterisk.c<br>@@ -250,6 +250,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>@@ -3336,7 +3338,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>@@ -4780,18 +4782,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>@@ -5032,7 +5042,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>@@ -5529,6 +5539,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>@@ -5711,39 +5725,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>@@ -6481,12 +6498,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>@@ -6497,8 +6515,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>@@ -6507,9 +6526,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>@@ -6562,8 +6578,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/6562">change 6562</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/6562"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15.0 </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: I152830bbff71c662408909042068fada39e617f9 </div>
<div style="display:none"> Gerrit-Change-Number: 6562 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Richard Mudgett <rmudgett@digium.com> </div>