<p>Richard Mudgett has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.asterisk.org/6605">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">res_rtp_asterisk.c: Fix bridge_p2p_rtp_write() reentrancy potential.<br><br>The bridge_p2p_rtp_write() has potential reentrancy problems.<br><br>* Accessing the bridged RTP members must be done with the instance1 lock<br>held.  The DTMF and asymmetric codec checks must be split to be done with<br>the correct RTP instance struct locked.  i.e., They must be done when<br>working on the appropriate side of the point to point bridge.<br><br>* Forcing the RTP mark bit was referencing the wrong side of the point to<br>point bridge.  The set mark bit is used everywhere else to set the mark<br>bit when sending not receiving.<br><br>The patches for ASTERISK_26745 and ASTERISK_27158 did not take into<br>account that not everything carried by RTP uses a codec.  The telephony<br>DTMF events are not exchanged with a codec.  As a result when<br>RFC2833/RFC4733 sent digits you would crash if "core set debug 1" is<br>enabled, the DTMF digits would always get passed to the core even though<br>the local native RTP bridge is active, and the DTMF digits would go out<br>using the wrong SSRC id.<br><br>* Add protection for non-format payload types like DTMF when updating the<br>lastrxformat and lasttxformat.  Also protect against non-format payload<br>types when checking for asymmetric codecs.<br><br>ASTERISK-27292<br><br>Change-Id: I6344ab7de21e26f84503c4d1fca1a41579364186<br>---<br>M res/res_rtp_asterisk.c<br>1 file changed, 50 insertions(+), 35 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/05/6605/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 c8cc04f..b114c36 100644<br>--- a/res/res_rtp_asterisk.c<br>+++ b/res/res_rtp_asterisk.c<br>@@ -5316,7 +5316,7 @@<br>        struct ast_rtp_instance *instance1, unsigned int *rtpheader, int len, int hdrlen)<br> {<br>         struct ast_rtp *rtp = ast_rtp_instance_get_data(instance);<br>-   struct ast_rtp *bridged = ast_rtp_instance_get_data(instance1);<br>+      struct ast_rtp *bridged;<br>      int res = 0, payload = 0, bridged_payload = 0, mark;<br>  RAII_VAR(struct ast_rtp_payload_type *, payload_type, NULL, ao2_cleanup);<br>     int reconstruct = ntohl(rtpheader[0]);<br>@@ -5326,7 +5326,7 @@<br> <br>      /* Get fields from packet */<br>  payload = (reconstruct & 0x7f0000) >> 16;<br>-  mark = (((reconstruct & 0x800000) >> 23) != 0);<br>+    mark = (reconstruct & 0x800000) >> 23;<br> <br>   /* Check what the payload value should be */<br>  payload_type = ast_rtp_codecs_get_payload(ast_rtp_instance_get_codecs(instance), payload);<br>@@ -5349,12 +5349,6 @@<br>            return -1;<br>    }<br> <br>- /* If bridged peer is in dtmf, feed all packets to core until it finishes to avoid infinite dtmf */<br>-  if (bridged->sending_digit) {<br>-             ast_debug(1, "Feeding packets to core until DTMF finishes\n");<br>-             return -1;<br>-   }<br>-<br>  /*<br>     * Even if we are no longer in dtmf, we could still be receiving<br>       * re-transmissions of the last dtmf end still.  Feed those to the<br>@@ -5365,34 +5359,9 @@<br>            return -1;<br>    }<br> <br>-<br>-      ao2_replace(rtp->lastrxformat, payload_type->format);<br>-  ao2_replace(bridged->lasttxformat, payload_type->format);<br>-<br>-   /*<br>-    * If bridged peer has already received rtp, perform the asymmetric codec check<br>-       * if that feature has been activated<br>-         */<br>-  if (!bridged->asymmetric_codec && bridged->lastrxformat != ast_format_none) {<br>-          if (ast_format_cmp(bridged->lasttxformat, bridged->lastrxformat) == AST_FORMAT_CMP_NOT_EQUAL) {<br>-                        ast_debug(1, "Asymmetric RTP codecs detected (TX: %s, RX: %s) sending frame to core\n",<br>-                                    ast_format_get_name(bridged->lasttxformat),<br>-                                       ast_format_get_name(bridged->lastrxformat));<br>-                      return -1;<br>-           }<br>+    if (payload_type->asterisk_format) {<br>+              ao2_replace(rtp->lastrxformat, payload_type->format);<br>   }<br>-<br>- /* If the marker bit has been explicitly set turn it on */<br>-   if (ast_test_flag(rtp, FLAG_NEED_MARKER_BIT)) {<br>-              mark = 1;<br>-            ast_clear_flag(rtp, FLAG_NEED_MARKER_BIT);<br>-   }<br>-<br>- /* Reconstruct part of the packet */<br>- reconstruct &= 0xFF80FFFF;<br>-       reconstruct |= (bridged_payload << 16);<br>-        reconstruct |= (mark << 23);<br>-   rtpheader[0] = htonl(reconstruct);<br> <br>         /*<br>     * We have now determined that we need to send the RTP packet<br>@@ -5409,6 +5378,40 @@<br>         ao2_unlock(instance);<br>         ao2_lock(instance1);<br> <br>+      /*<br>+    * Get the peer rtp pointer now to emphasize that using it<br>+    * must happen while instance1 is locked.<br>+     */<br>+  bridged = ast_rtp_instance_get_data(instance1);<br>+<br>+<br>+        /* If bridged peer is in dtmf, feed all packets to core until it finishes to avoid infinite dtmf */<br>+  if (bridged->sending_digit) {<br>+             ast_debug(1, "Feeding packet to core until DTMF finishes\n");<br>+              ao2_unlock(instance1);<br>+               ao2_lock(instance);<br>+          return -1;<br>+   }<br>+<br>+ if (payload_type->asterisk_format) {<br>+              /*<br>+            * If bridged peer has already received rtp, perform the asymmetric codec check<br>+               * if that feature has been activated<br>+                 */<br>+          if (!bridged->asymmetric_codec<br>+                    && bridged->lastrxformat != ast_format_none<br>+                       && ast_format_cmp(payload_type->format, bridged->lastrxformat) == AST_FORMAT_CMP_NOT_EQUAL) {<br>+                  ast_debug(1, "Asymmetric RTP codecs detected (TX: %s, RX: %s) sending frame to core\n",<br>+                            ast_format_get_name(payload_type->format),<br>+                                ast_format_get_name(bridged->lastrxformat));<br>+                      ao2_unlock(instance1);<br>+                       ao2_lock(instance);<br>+                  return -1;<br>+           }<br>+<br>+         ao2_replace(bridged->lasttxformat, payload_type->format);<br>+      }<br>+<br>  ast_rtp_instance_get_remote_address(instance1, &remote_address);<br> <br>       if (ast_sockaddr_isnull(&remote_address)) {<br>@@ -5418,6 +5421,18 @@<br>               return 0;<br>     }<br> <br>+ /* If the marker bit has been explicitly set turn it on */<br>+   if (ast_test_flag(bridged, FLAG_NEED_MARKER_BIT)) {<br>+          mark = 1;<br>+            ast_clear_flag(bridged, FLAG_NEED_MARKER_BIT);<br>+       }<br>+<br>+ /* Reconstruct part of the packet */<br>+ reconstruct &= 0xFF80FFFF;<br>+       reconstruct |= (bridged_payload << 16);<br>+        reconstruct |= (mark << 23);<br>+   rtpheader[0] = htonl(reconstruct);<br>+<br>         /* Send the packet back out */<br>        res = rtp_sendto(instance1, (void *)rtpheader, len, 0, &remote_address, &ice);<br>        if (res < 0) {<br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/6605">change 6605</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/6605"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15 </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: I6344ab7de21e26f84503c4d1fca1a41579364186 </div>
<div style="display:none"> Gerrit-Change-Number: 6605 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Richard Mudgett <rmudgett@digium.com> </div>