[Asterisk-code-review] res rtp asterisk: Fix issues with ICE renegotiation. (asterisk[master])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/5918 )

Change subject: res_rtp_asterisk: Fix issues with ICE renegotiation.
......................................................................

res_rtp_asterisk: Fix issues with ICE renegotiation.

When re-inviting to add more streams it is possible for
the role of existing ICE sessions to be changed to the
incorrect value. This results in subsequent refreshes
within the sessions getting a role conflict and the ICE
session breaking down. This change only sets the role to
be the new value if an ICE renegotiation is actually
going to happen, otherwise the existing role is preserved.

As well if we encounter a situation where a unidirectional
ICE negotiation happens and the other side does not send us
candidates we will not store any information for sending
traffic, even though we know where they are reachable. This
change fixes this by using the source of the ICE traffic
itself as the target if no candidates are known and we
receive some ICE traffic.

ASTERISK-27088

Change-Id: I71228181e358917fcefc3100fad21b2fc02a59a9
---
M res/res_rtp_asterisk.c
1 file changed, 18 insertions(+), 7 deletions(-)

Approvals:
  Kevin Harwell: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved
  Joshua Colp: Approved for Submit



diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c
index 600846c..01dfe76 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -317,6 +317,7 @@
 	ast_cond_t cond;            /*!< ICE/TURN condition for signaling */
 
 	struct ice_wrap *ice;       /*!< ao2 wrapped ICE session */
+	enum ast_rtp_ice_role role; /*!< Our role in ICE negotiation */
 	pj_turn_sock *turn_rtp;     /*!< RTP TURN relay */
 	pj_turn_sock *turn_rtcp;    /*!< RTCP TURN relay */
 	pj_turn_state_t turn_state; /*!< Current state of the TURN relay session */
@@ -683,7 +684,6 @@
 static int ice_reset_session(struct ast_rtp_instance *instance)
 {
 	struct ast_rtp *rtp = ast_rtp_instance_get_data(instance);
-	pj_ice_sess_role role = rtp->ice->real_ice->role;
 	int res;
 
 	ast_debug(3, "Resetting ICE for RTP instance '%p'\n", instance);
@@ -695,8 +695,9 @@
 	ast_debug(3, "Recreating ICE session %s (%d) for RTP instance '%p'\n", ast_sockaddr_stringify(&rtp->ice_original_rtp_addr), rtp->ice_port, instance);
 	res = ice_create(instance, &rtp->ice_original_rtp_addr, rtp->ice_port, 1);
 	if (!res) {
-		/* Preserve the role that the old ICE session used */
-		pj_ice_sess_change_role(rtp->ice->real_ice, role);
+		/* Use the current expected role for the ICE session */
+		pj_ice_sess_change_role(rtp->ice->real_ice, rtp->role == AST_RTP_ICE_ROLE_CONTROLLED ?
+			PJ_ICE_SESS_ROLE_CONTROLLED : PJ_ICE_SESS_ROLE_CONTROLLING);
 	}
 
 	/* If we only have one component now, and we previously set up TURN for RTCP,
@@ -767,6 +768,8 @@
 		ast_debug(3, "Proposed == active candidates for RTP instance '%p'\n", instance);
 		ao2_cleanup(rtp->ice_proposed_remote_candidates);
 		rtp->ice_proposed_remote_candidates = NULL;
+		/* If this ICE session is being preserved then go back to the role it currently is */
+		rtp->role = rtp->ice->real_ice->role;
 		return;
 	}
 
@@ -940,10 +943,7 @@
 		return;
 	}
 
-	pj_thread_register_check();
-
-	pj_ice_sess_change_role(rtp->ice->real_ice, role == AST_RTP_ICE_ROLE_CONTROLLED ?
-		PJ_ICE_SESS_ROLE_CONTROLLED : PJ_ICE_SESS_ROLE_CONTROLLING);
+	rtp->role = role;
 }
 
 /*! \pre instance is locked */
@@ -2526,6 +2526,17 @@
 			return -1;
 		}
 		if (!rtp->passthrough) {
+			/* If a unidirectional ICE negotiation occurs then lock on to the source of the
+			 * ICE traffic and use it as the target. This will occur if the remote side only
+			 * wants to receive media but never send to us.
+			 */
+			if (!rtp->ice_active_remote_candidates && !rtp->ice_proposed_remote_candidates) {
+				if (rtcp) {
+					ast_sockaddr_copy(&rtp->rtcp->them, sa);
+				} else {
+					ast_rtp_instance_set_remote_address(instance, sa);
+				}
+			}
 			return 0;
 		}
 		rtp->passthrough = 0;

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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I71228181e358917fcefc3100fad21b2fc02a59a9
Gerrit-Change-Number: 5918
Gerrit-PatchSet: 2
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170630/9270b709/attachment.html>