[Asterisk-code-review] res rtp asterisk.c: Fix DTLS negotiation delays. (asterisk[master])

[Matt Jordan]

Matt Jordan has submitted this change and it was merged.

Change subject: res_rtp_asterisk.c: Fix DTLS negotiation delays.
......................................................................


res_rtp_asterisk.c: Fix DTLS negotiation delays.

- Trigger pending DTLS packets to send out, once the RTP instance's remote
  address is set.
- Avoids locking the DTLS structure unnecessarily by only doing this if
  DTLS is passive.
- Add DTLS locks around the structurally sensitive calls in the SSL
  portion of __rtp_recvfrom, since dtls_srtp_check_pending does not lock
  inside of itself, and we're dealing with the SSL BIO in at least two
  threads.

WebRTC channels may receive a DTLS handshake before
ast_rtp_remote_address_set is called, which causes there to be a pending
response to send out.   Previous to 1ad827, this was handled by calling
dtls_srtp_check_pending on receipt of any RTP packet - a STUN or RTP
packet could trigger the pending handshake response.  Since that was
rightfully removed, whenever the DTLS handshake is received before the
remote address is set, we would have to wait until another SSL packet
arrives.

As of Chrome M47's optimizations to their handshake process, WebRTC
conversations between Chrome M47+ and Asterisk, where Asterisk is passive,
experience a 1 second delay without this patch, because the SSL handshake
is received before ICE negotation stores the remote_address, and the next
SSL packet isn't received until after a 1 second timeout in Chrome, which
causes a new handshake request.

ASTERISK-25614 #close

Change-Id: I547f1be7e302dbf71f6553dd8cbc0657b1d0b908
---
M res/res_rtp_asterisk.c
1 file changed, 30 insertions(+), 0 deletions(-)

Approvals:
  Anonymous Coward #1000019: Verified
  Matt Jordan: Looks good to me, approved
  Joshua Colp: Looks good to me, but someone else must approve



diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c
index ac83d73..6f09368 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -2118,6 +2118,8 @@
 			SSL_set_accept_state(dtls->ssl);
 		}
 
+		ast_mutex_lock(&dtls->lock);
+
 		dtls_srtp_check_pending(instance, rtp, rtcp);
 
 		BIO_write(dtls->read_bio, buf, len);
@@ -2128,6 +2130,7 @@
 			unsigned long error = ERR_get_error();
 			ast_log(LOG_ERROR, "DTLS failure occurred on RTP instance '%p' due to reason '%s', terminating\n",
 				instance, ERR_reason_error_string(error));
+			ast_mutex_unlock(&dtls->lock);
 			return -1;
 		}
 
@@ -2144,6 +2147,8 @@
 			/* Since we've sent additional traffic start the timeout timer for retransmission */
 			dtls_srtp_start_timeout_timer(instance, rtp, rtcp);
 		}
+
+		ast_mutex_unlock(&dtls->lock);
 
 		return res;
 	}
@@ -4816,6 +4821,9 @@
 static void ast_rtp_remote_address_set(struct ast_rtp_instance *instance, struct ast_sockaddr *addr)
 {
 	struct ast_rtp *rtp = ast_rtp_instance_get_data(instance);
+#ifdef HAVE_OPENSSL_SRTP
+	struct dtls_details *dtls;
+#endif
 
 	if (rtp->rtcp) {
 		ast_debug(1, "Setting RTCP address on RTP instance '%p'\n", instance);
@@ -4833,6 +4841,28 @@
 		rtp_learning_seq_init(&rtp->rtp_source_learn, rtp->seqno);
 	}
 
+#ifdef HAVE_OPENSSL_SRTP
+	/* Trigger pending outbound DTLS packets received before the address was set.  Avoid unnecessary locking
+	 * by checking if we're passive. Without this, we only send the pending packets once a new SSL packet is
+	 * received in __rtp_recvfrom.
+	 */
+	dtls = &rtp->dtls;
+	if (dtls->dtls_setup == AST_RTP_DTLS_SETUP_PASSIVE) {
+		ast_mutex_lock(&dtls->lock);
+		dtls_srtp_check_pending(instance, rtp, 0);
+		ast_mutex_unlock(&dtls->lock);
+	}
+
+	if (rtp->rtcp) {
+		dtls = &rtp->rtcp->dtls;
+		if (dtls->dtls_setup == AST_RTP_DTLS_SETUP_PASSIVE) {
+			ast_mutex_lock(&dtls->lock);
+			dtls_srtp_check_pending(instance, rtp, 1);
+			ast_mutex_unlock(&dtls->lock);
+		}
+	}
+#endif
+
 	return;
 }
 

-- 
To view, visit https://gerrit.asterisk.org/1819
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I547f1be7e302dbf71f6553dd8cbc0657b1d0b908
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>