[Asterisk-code-review] res rtp asterisk.c: Fix TURN deadlock by using ICE session g... (asterisk[14])

[George Joseph]

George Joseph has submitted this change and it was merged. ( https://gerrit.asterisk.org/5972 )

Change subject: res_rtp_asterisk.c: Fix TURN deadlock by using ICE session group lock.
......................................................................

res_rtp_asterisk.c: Fix TURN deadlock by using ICE session group lock.

When a message is received on the TURN socket, the code processing the
message needs to call into the ICE/STUN session for further processing.
This code path locks the TURN group lock then the ICE/STUN group lock.  In
another thread an ICE/STUN timer can fire off to send a keep alive message
over the TURN socket.  In this code path, the ICE/STUN group lock is
obtained then the TURN group lock is obtained to send the packet.  A
classic deadlock case if the group locks are not the same.

* Made TURN get created using the ICE/STUN session's group lock.

NOTE: I was originally concerned that the ICE/STUN session can get
recreated by ice_reset_session() for an event like RTCP multiplexing
causing a change during SDP negotiation.  In this case the TURN group lock
would become different.  However, TURN is also recreated as part of the
ICE/STUN recreation in ice_create() when all known ICE candidates are
added to the new ICE session.  While the ICE/STUN and TURN sessions are
being recreated there is a period where the group locks could be
different.

ASTERISK-27023 #close
Patches:
    res_rtp_asterisk-turn-deadlock-fix.patch (license #6502)
        patch uploaded by Michael Walton (modified)

Change-Id: Ic870edb99ce4988a8c8eb6e678ca7f19da1432b9
---
M res/res_rtp_asterisk.c
1 file changed, 12 insertions(+), 1 deletion(-)

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



diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c
index 6714073..2a2fbf7 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -1291,6 +1291,8 @@
 	pj_turn_session_info info;
 	struct ast_sockaddr local, loop;
 	pj_status_t status;
+	pj_turn_sock_cfg turn_sock_cfg;
+	struct ice_wrap *ice;
 
 	ast_rtp_instance_get_local_address(instance, &local);
 	if (ast_sockaddr_is_ipv4(&local)) {
@@ -1353,11 +1355,20 @@
 
 	pj_stun_config_init(&stun_config, &cachingpool.factory, 0, rtp->ioqueue->ioqueue, rtp->ioqueue->timerheap);
 
+	/* Use ICE session group lock for TURN session to avoid deadlock */
+	pj_turn_sock_cfg_default(&turn_sock_cfg);
+	ice = rtp->ice;
+	if (ice) {
+		turn_sock_cfg.grp_lock = ice->real_ice->grp_lock;
+		ao2_ref(ice, +1);
+	}
+
 	/* Release the instance lock to avoid deadlock with PJPROJECT group lock */
 	ao2_unlock(instance);
 	status = pj_turn_sock_create(&stun_config,
 		ast_sockaddr_is_ipv4(&addr) ? pj_AF_INET() : pj_AF_INET6(), conn_type,
-		turn_cb, NULL, instance, turn_sock);
+		turn_cb, &turn_sock_cfg, instance, turn_sock);
+	ao2_cleanup(ice);
 	if (status != PJ_SUCCESS) {
 		ast_log(LOG_WARNING, "Could not create a TURN client socket\n");
 		ao2_lock(instance);

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

Gerrit-Project: asterisk
Gerrit-Branch: 14
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic870edb99ce4988a8c8eb6e678ca7f19da1432b9
Gerrit-Change-Number: 5972
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Michael Walton <mike at farsouthnet.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170710/bb79dd4e/attachment.html>