[Asterisk-code-review] res rtp asterisk: Resolve further timing issues with DTLS ne... (asterisk[13])

[Matt Jordan]

Matt Jordan has submitted this change and it was merged.

Change subject: res_rtp_asterisk: Resolve further timing issues with DTLS negotiation
......................................................................


res_rtp_asterisk: Resolve further timing issues with DTLS negotiation

Resolves an edge case dtls negotiation delay for certain networks which
somehow manage to drop the rtcp side's packet when these are both sent
ast_rtp_remote_address_set, causing it to have to time-out and restart
the handshake.

Move dtls pending bio flush in to it's own function, and call it from
ast_rtp_on_ice_complete, when we're rtp->ice, rather than when
ast_rtp_remote_address_set.

Keep the existing flush from the recent change to res_rtp_remote_address_set
if ice is not being used.

ASTERISK-25614 #close
Reported-by: XenCALL
Tested by: XenCALL

Change-Id: Ie2caedbdee1783159f375589b6fd3845c8577ba5
---
M res/res_rtp_asterisk.c
1 file changed, 31 insertions(+), 23 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 3dfaf87..db676f4 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -444,6 +444,7 @@
 #ifdef HAVE_OPENSSL_SRTP
 static int ast_rtp_activate(struct ast_rtp_instance *instance);
 static void dtls_srtp_check_pending(struct ast_rtp_instance *instance, struct ast_rtp *rtp, int rtcp);
+static void dtls_srtp_flush_pending(struct ast_rtp_instance *instance, struct ast_rtp *rtp);
 static void dtls_srtp_start_timeout_timer(struct ast_rtp_instance *instance, struct ast_rtp *rtp, int rtcp);
 static void dtls_srtp_stop_timeout_timer(struct ast_rtp_instance *instance, struct ast_rtp *rtp, int rtcp);
 #endif
@@ -1683,15 +1684,20 @@
 		if (rtp->rtcp) {
 			update_address_with_ice_candidate(rtp, AST_RTP_ICE_COMPONENT_RTCP, &rtp->rtcp->them);
 		}
-	}
  
 #ifdef HAVE_OPENSSL_SRTP
-	dtls_perform_handshake(instance, &rtp->dtls, 0);
+		if (rtp->dtls.dtls_setup != AST_RTP_DTLS_SETUP_PASSIVE) {
+			dtls_perform_handshake(instance, &rtp->dtls, 0);
+		}
+		else {
+			dtls_srtp_flush_pending(instance, rtp); /* this flushes pending BIO for both rtp & rtcp as needed. */
+		}
 
-	if (rtp->rtcp) {
-		dtls_perform_handshake(instance, &rtp->rtcp->dtls, 1);
-	}
+		if (rtp->rtcp && rtp->rtcp->dtls.dtls_setup != AST_RTP_DTLS_SETUP_PASSIVE) {
+			dtls_perform_handshake(instance, &rtp->rtcp->dtls, 1);
+		}
 #endif
+	}
 
 	if (!strictrtp) {
 		return;
@@ -1884,6 +1890,23 @@
 	struct dtls_details *dtls = !rtcp ? &rtp->dtls : &rtp->rtcp->dtls;
 
 	AST_SCHED_DEL_UNREF(rtp->sched, dtls->timeout_timer, ao2_ref(instance, -1));
+}
+
+static void dtls_srtp_flush_pending(struct ast_rtp_instance *instance, struct ast_rtp *rtp)
+{
+	struct dtls_details *dtls;
+
+	dtls = &rtp->dtls;
+	ast_mutex_lock(&dtls->lock);
+	dtls_srtp_check_pending(instance, rtp, 0);
+	ast_mutex_unlock(&dtls->lock);
+
+	if (rtp->rtcp) {
+		dtls = &rtp->rtcp->dtls;
+		ast_mutex_lock(&dtls->lock);
+		dtls_srtp_check_pending(instance, rtp, 1);
+		ast_mutex_unlock(&dtls->lock);
+	}
 }
 
 static void dtls_srtp_check_pending(struct ast_rtp_instance *instance, struct ast_rtp *rtp, int rtcp)
@@ -4822,9 +4845,6 @@
 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);
@@ -4845,22 +4865,10 @@
 #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.
+	 * received in __rtp_recvfrom.  If rtp->ice, this is instead done on_ice_complete
 	 */
-	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);
-		}
+	if (!rtp->ice && rtp->dtls.dtls_setup == AST_RTP_DTLS_SETUP_PASSIVE) {
+		dtls_srtp_flush_pending(instance, rtp);
 	}
 #endif
 

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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie2caedbdee1783159f375589b6fd3845c8577ba5
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Dade Brandon <dade at xencall.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>