[prev in list] [next in list] [prev in thread] [next in thread]
List: asterisk-commits
Subject: [asterisk-commits] =?utf-8?q?res_rtp_asterisk=2Ec=3A_Fix_bridge_p?= =?utf-8?q?2p_rtp_write=28=29_ree
From: SVN commits to the Asterisk project <asterisk-commits () lists ! digium ! com>
Date: 2017-09-28 11:49:30
Message-ID: mailman.59808.1506624568.27840.asterisk-commits () lists ! digium ! com
[Download RAW message or body]
Jenkins2 has submitted this change and it was merged. ( https://gerrit.asterisk.org/6605 )
Change subject: res_rtp_asterisk.c: Fix bridge_p2p_rtp_write() reentrancy potential.
......................................................................
res_rtp_asterisk.c: Fix bridge_p2p_rtp_write() reentrancy potential.
The bridge_p2p_rtp_write() has potential reentrancy problems.
* Accessing the bridged RTP members must be done with the instance1 lock
held. The DTMF and asymmetric codec checks must be split to be done with
the correct RTP instance struct locked. i.e., They must be done when
working on the appropriate side of the point to point bridge.
* Forcing the RTP mark bit was referencing the wrong side of the point to
point bridge. The set mark bit is used everywhere else to set the mark
bit when sending not receiving.
The patches for ASTERISK_26745 and ASTERISK_27158 did not take into
account that not everything carried by RTP uses a codec. The telephony
DTMF events are not exchanged with a codec. As a result when
RFC2833/RFC4733 sent digits you would crash if "core set debug 1" is
enabled, the DTMF digits would always get passed to the core even though
the local native RTP bridge is active, and the DTMF digits would go out
using the wrong SSRC id.
* Add protection for non-format payload types like DTMF when updating the
lastrxformat and lasttxformat. Also protect against non-format payload
types when checking for asymmetric codecs.
ASTERISK-27292
Change-Id: I6344ab7de21e26f84503c4d1fca1a41579364186
---
M res/res_rtp_asterisk.c
1 file changed, 50 insertions(+), 35 deletions(-)
Approvals:
Joshua Colp: Looks good to me, but someone else must approve
George Joseph: Looks good to me, approved
Jenkins2: Approved for Submit
diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c
index c8cc04f..b114c36 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -5316,7 +5316,7 @@
struct ast_rtp_instance *instance1, unsigned int *rtpheader, int len, int hdrlen)
{
struct ast_rtp *rtp = ast_rtp_instance_get_data(instance);
- struct ast_rtp *bridged = ast_rtp_instance_get_data(instance1);
+ struct ast_rtp *bridged;
int res = 0, payload = 0, bridged_payload = 0, mark;
RAII_VAR(struct ast_rtp_payload_type *, payload_type, NULL, ao2_cleanup);
int reconstruct = ntohl(rtpheader[0]);
@@ -5326,7 +5326,7 @@
/* Get fields from packet */
payload = (reconstruct & 0x7f0000) >> 16;
- mark = (((reconstruct & 0x800000) >> 23) != 0);
+ mark = (reconstruct & 0x800000) >> 23;
/* Check what the payload value should be */
payload_type = ast_rtp_codecs_get_payload(ast_rtp_instance_get_codecs(instance), payload);
@@ -5349,12 +5349,6 @@
return -1;
}
- /* If bridged peer is in dtmf, feed all packets to core until it finishes to avoid infinite dtmf */
- if (bridged->sending_digit) {
- ast_debug(1, "Feeding packets to core until DTMF finishes\n");
- return -1;
- }
-
/*
* Even if we are no longer in dtmf, we could still be receiving
* re-transmissions of the last dtmf end still. Feed those to the
@@ -5365,34 +5359,9 @@
return -1;
}
-
- ao2_replace(rtp->lastrxformat, payload_type->format);
- ao2_replace(bridged->lasttxformat, payload_type->format);
-
- /*
- * If bridged peer has already received rtp, perform the asymmetric codec check
- * if that feature has been activated
- */
- if (!bridged->asymmetric_codec && bridged->lastrxformat != ast_format_none) {
- if (ast_format_cmp(bridged->lasttxformat, bridged->lastrxformat) == AST_FORMAT_CMP_NOT_EQUAL) {
- ast_debug(1, "Asymmetric RTP codecs detected (TX: %s, RX: %s) sending frame to core\n",
- ast_format_get_name(bridged->lasttxformat),
- ast_format_get_name(bridged->lastrxformat));
- return -1;
- }
+ if (payload_type->asterisk_format) {
+ ao2_replace(rtp->lastrxformat, payload_type->format);
}
-
- /* If the marker bit has been explicitly set turn it on */
- if (ast_test_flag(rtp, FLAG_NEED_MARKER_BIT)) {
- mark = 1;
- ast_clear_flag(rtp, FLAG_NEED_MARKER_BIT);
- }
-
- /* Reconstruct part of the packet */
- reconstruct &= 0xFF80FFFF;
- reconstruct |= (bridged_payload << 16);
- reconstruct |= (mark << 23);
- rtpheader[0] = htonl(reconstruct);
/*
* We have now determined that we need to send the RTP packet
@@ -5409,6 +5378,40 @@
ao2_unlock(instance);
ao2_lock(instance1);
+ /*
+ * Get the peer rtp pointer now to emphasize that using it
+ * must happen while instance1 is locked.
+ */
+ bridged = ast_rtp_instance_get_data(instance1);
+
+
+ /* If bridged peer is in dtmf, feed all packets to core until it finishes to avoid infinite dtmf */
+ if (bridged->sending_digit) {
+ ast_debug(1, "Feeding packet to core until DTMF finishes\n");
+ ao2_unlock(instance1);
+ ao2_lock(instance);
+ return -1;
+ }
+
+ if (payload_type->asterisk_format) {
+ /*
+ * If bridged peer has already received rtp, perform the asymmetric codec check
+ * if that feature has been activated
+ */
+ if (!bridged->asymmetric_codec
+ && bridged->lastrxformat != ast_format_none
+ && ast_format_cmp(payload_type->format, bridged->lastrxformat) == AST_FORMAT_CMP_NOT_EQUAL) {
+ ast_debug(1, "Asymmetric RTP codecs detected (TX: %s, RX: %s) sending frame to core\n",
+ ast_format_get_name(payload_type->format),
+ ast_format_get_name(bridged->lastrxformat));
+ ao2_unlock(instance1);
+ ao2_lock(instance);
+ return -1;
+ }
+
+ ao2_replace(bridged->lasttxformat, payload_type->format);
+ }
+
ast_rtp_instance_get_remote_address(instance1, &remote_address);
if (ast_sockaddr_isnull(&remote_address)) {
@@ -5418,6 +5421,18 @@
return 0;
}
+ /* If the marker bit has been explicitly set turn it on */
+ if (ast_test_flag(bridged, FLAG_NEED_MARKER_BIT)) {
+ mark = 1;
+ ast_clear_flag(bridged, FLAG_NEED_MARKER_BIT);
+ }
+
+ /* Reconstruct part of the packet */
+ reconstruct &= 0xFF80FFFF;
+ reconstruct |= (bridged_payload << 16);
+ reconstruct |= (mark << 23);
+ rtpheader[0] = htonl(reconstruct);
+
/* Send the packet back out */
res = rtp_sendto(instance1, (void *)rtpheader, len, 0, &remote_address, &ice);
if (res < 0) {
--
To view, visit https://gerrit.asterisk.org/6605
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: merged
Gerrit-Change-Id: I6344ab7de21e26f84503c4d1fca1a41579364186
Gerrit-Change-Number: 6605
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Mudgett <rmudgett@digium.com>
Gerrit-Reviewer: George Joseph <gjoseph@digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp@digium.com>
[Attachment #3 (text/html)]
<p>Jenkins2 <strong>merged</strong> this change.</p><p><a \
href="https://gerrit.asterisk.org/6605">View Change</a></p><div \
style="white-space:pre-wrap">Approvals: Joshua Colp: Looks good to me, but someone \
else must approve George Joseph: Looks good to me, approved
Jenkins2: Approved for Submit
</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/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: merged </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> <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>
--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-commits mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-commits
--===============5688792302377303555==--
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic