[Asterisk-code-review] res pjsip/pjsip transport management.c: Fix deadlock with tr... (asterisk[certified/13.21])

[Kevin Harwell]

Kevin Harwell has submitted this change and it was merged. ( https://gerrit.asterisk.org/10003 )

Change subject: res_pjsip/pjsip_transport_management.c: Fix deadlock with transport keep alive.
......................................................................

res_pjsip/pjsip_transport_management.c: Fix deadlock with transport keep alive.

Using the keep_alive_interval option can result in a deadlock between the
pjproject transport manager group lock and the monitored transports ao2
container lock.  The pjproject transport manager group lock has to be
superior in the locking order to the monitored transports ao2 container
lock because of pjproject callbacks called when already holding the group
lock.  The lock inversion happens when Asterisk attempts to send a keep
alive packet over the reliable transports.

* Made keepalive_transport_thread() iterate over the monitored transports
container rather than use the ao2_callback() method.  This avoids holding
the container lock when sending the keep alive packet.

ASTERISK-26686

Change-Id: I5d5392a52e698bbe41a93f7d8e92bf0e61fe3951
---
M res/res_pjsip/pjsip_transport_management.c
1 file changed, 25 insertions(+), 10 deletions(-)

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



diff --git a/res/res_pjsip/pjsip_transport_management.c b/res/res_pjsip/pjsip_transport_management.c
index 1404b6e..d9866f1 100644
--- a/res/res_pjsip/pjsip_transport_management.c
+++ b/res/res_pjsip/pjsip_transport_management.c
@@ -56,21 +56,22 @@
 	int sip_received;
 };
 
-/*! \brief Callback function to send keepalive */
-static int keepalive_transport_cb(void *obj, void *arg, int flags)
+static void keepalive_transport_send_keepalive(struct monitored_transport *monitored)
 {
-	struct monitored_transport *monitored = obj;
 	pjsip_tpselector selector = {
 		.type = PJSIP_TPSELECTOR_TRANSPORT,
 		.u.transport = monitored->transport,
 	};
 
 	pjsip_tpmgr_send_raw(pjsip_endpt_get_tpmgr(ast_sip_get_pjsip_endpoint()),
-		monitored->transport->key.type, &selector, NULL, keepalive_packet.ptr, keepalive_packet.slen,
-		&monitored->transport->key.rem_addr, pj_sockaddr_get_len(&monitored->transport->key.rem_addr),
+		monitored->transport->key.type,
+		&selector,
+		NULL,
+		keepalive_packet.ptr,
+		keepalive_packet.slen,
+		&monitored->transport->key.rem_addr,
+		pj_sockaddr_get_len(&monitored->transport->key.rem_addr),
 		NULL, NULL);
-
-	return 0;
 }
 
 /*! \brief Thread which sends keepalives to all active connection-oriented transports */
@@ -90,12 +91,26 @@
 		return NULL;
 	}
 
-	/* Once loaded this module just keeps on going as it is unsafe to stop and change the underlying
-	 * callback for the transport manager.
+	/*
+	 * Once loaded this module just keeps on going as it is unsafe to stop
+	 * and change the underlying callback for the transport manager.
 	 */
 	while (keepalive_interval) {
+		struct ao2_iterator iter;
+		struct monitored_transport *monitored;
+
 		sleep(keepalive_interval);
-		ao2_callback(transports, OBJ_NODATA, keepalive_transport_cb, NULL);
+
+		/*
+		 * We must use the iterator to avoid deadlock between the container lock
+		 * and the pjproject transport manager group lock when sending
+		 * the keepalive packet.
+		 */
+		iter = ao2_iterator_init(transports, 0);
+		for (; (monitored = ao2_iterator_next(&iter)); ao2_ref(monitored, -1)) {
+			keepalive_transport_send_keepalive(monitored);
+		}
+		ao2_iterator_destroy(&iter);
 	}
 
 	ao2_ref(transports, -1);

-- 
To view, visit https://gerrit.asterisk.org/10003
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: certified/13.21
Gerrit-MessageType: merged
Gerrit-Change-Id: I5d5392a52e698bbe41a93f7d8e92bf0e61fe3951
Gerrit-Change-Number: 10003
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180828/2476ac0b/attachment.html>