<p>Joshua Colp <strong>merged</strong> this change.</p><p><a href="https://gerrit.asterisk.org/6606">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 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;">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/6606">change 6606</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/6606"/><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: I6344ab7de21e26f84503c4d1fca1a41579364186 </div>
<div style="display:none"> Gerrit-Change-Number: 6606 </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>