[Asterisk-code-review] pjsip distributor.c: Fix deadlock with TCP type transports. (asterisk[14])

[Richard Mudgett]

Richard Mudgett has uploaded this change for review. ( https://gerrit.asterisk.org/5920


Change subject: pjsip_distributor.c: Fix deadlock with TCP type transports.
......................................................................

pjsip_distributor.c: Fix deadlock with TCP type transports.

When a SIP message comes in on a transport, pjproject obtains the lock on
the transport and pulls the data out of the socket.  Unlike UDP, the TCP
transport does not allow concurrent access.  Without concurrency the
transport lock is not released when the transport's message complete
callback is called.  The processing continues and eventually Asterisk
starts processing the SIP message.  The first thing Asterisk tries to do
is determine the associated dialog of the message to determine the
associated serializer.  To get the associated serializer safely requires
us to get the dialog lock.

To send a request or response message for a dialog, pjproject obtains the
dialog lock and then obtains the transport lock.  Deadlock can result
because of the opposite order the locks are obtained.

* Fix the deadlock by passing all incoming SIP messages into a new master
distributor serializer.  The transport lock will never be held by the new
serializer so it is safe to get the dialog lock to determine which
serializer must process the message.

ASTERISK-27090 #close

Change-Id: I9ed63f4da9649e9db6ed4be29c360968917a89bd
---
M res/res_pjsip/pjsip_distributor.c
1 file changed, 55 insertions(+), 19 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/20/5920/1

diff --git a/res/res_pjsip/pjsip_distributor.c b/res/res_pjsip/pjsip_distributor.c
index cca26a8..c2b7429 100644
--- a/res/res_pjsip/pjsip_distributor.c
+++ b/res/res_pjsip/pjsip_distributor.c
@@ -58,6 +58,9 @@
 	char src_name[];
 };
 
+/*! Master distributor serializer to avoid deadlock */
+static struct ast_taskprocessor *distributor_master;
+
 /*! Number of serializers in pool if one not otherwise known.  (Best if prime number) */
 #define DISTRIBUTOR_POOL_SIZE		31
 
@@ -368,20 +371,12 @@
 	.on_rx_request = endpoint_lookup,
 };
 
-static pj_bool_t distributor(pjsip_rx_data *rdata)
+static int distribute_master(void *data)
 {
 	pjsip_dialog *dlg;
 	struct distributor_dialog_data *dist = NULL;
 	struct ast_taskprocessor *serializer = NULL;
-	pjsip_rx_data *clone;
-
-	if (!ast_test_flag(&ast_options, AST_OPT_FLAG_FULLY_BOOTED)) {
-		/*
-		 * Ignore everything until we are fully booted.  Let the
-		 * peer retransmit messages until we are ready.
-		 */
-		return PJ_TRUE;
-	}
+	pjsip_rx_data *rdata = data;
 
 	dlg = find_dialog(rdata);
 	if (dlg) {
@@ -417,7 +412,8 @@
 		/* We have a BYE or CANCEL request without a serializer. */
 		pjsip_endpt_respond_stateless(ast_sip_get_pjsip_endpoint(), rdata,
 			PJSIP_SC_CALL_TSX_DOES_NOT_EXIST, NULL, NULL, NULL);
-		return PJ_TRUE;
+		pjsip_rx_data_free_cloned(rdata);
+		return 0;
 	} else {
 		if (ast_taskprocessor_alert_get()) {
 			/*
@@ -431,26 +427,53 @@
 			 */
 			ast_debug(3, "Taskprocessor overload alert: Ignoring '%s'.\n",
 				pjsip_rx_data_get_info(rdata));
-			return PJ_TRUE;
+			pjsip_rx_data_free_cloned(rdata);
+			return 0;
 		}
 
 		/* Pick a serializer for the out-of-dialog request. */
 		serializer = ast_sip_get_distributor_serializer(rdata);
 	}
 
-	pjsip_rx_data_clone(rdata, 0, &clone);
-
 	if (dist) {
-		clone->endpt_info.mod_data[endpoint_mod.id] = ao2_bump(dist->endpoint);
+		rdata->endpt_info.mod_data[endpoint_mod.id] = ao2_bump(dist->endpoint);
 	}
 
-	if (ast_sip_push_task(serializer, distribute, clone)) {
-		ao2_cleanup(clone->endpt_info.mod_data[endpoint_mod.id]);
-		pjsip_rx_data_free_cloned(clone);
+	if (ast_sip_push_task(serializer, distribute, rdata)) {
+		ao2_cleanup(rdata->endpt_info.mod_data[endpoint_mod.id]);
+		pjsip_rx_data_free_cloned(rdata);
 	}
 
 	ast_taskprocessor_unreference(serializer);
 
+	return 0;
+}
+
+static pj_bool_t distributor(pjsip_rx_data *rdata)
+{
+	pjsip_rx_data *clone;
+
+	if (!ast_test_flag(&ast_options, AST_OPT_FLAG_FULLY_BOOTED)) {
+		/*
+		 * Ignore everything until we are fully booted.  Let the
+		 * peer retransmit messages until we are ready.
+		 */
+		return PJ_TRUE;
+	}
+
+	if (pjsip_rx_data_clone(rdata, 0, &clone) != PJ_SUCCESS) {
+		return PJ_TRUE;
+	}
+
+	/*
+	 * We have to push everything off into a serializer to avoid a
+	 * deadlock involving the transport lock and the dialog lock.
+	 * A TCP type transport has the transport lock still held at
+	 * this point while a UDP type transport does not.
+	 */
+	if (ast_sip_push_task(distributor_master, distribute_master, clone)) {
+		pjsip_rx_data_free_cloned(clone);
+	}
 	return PJ_TRUE;
 }
 
@@ -1037,11 +1060,17 @@
  */
 static void distributor_pool_shutdown(void)
 {
+	struct ast_taskprocessor *serializer;
 	int idx;
 
+	serializer = distributor_master;
+	distributor_master = NULL;
+	ast_taskprocessor_unreference(serializer);
+
 	for (idx = 0; idx < ARRAY_LEN(distributor_pool); ++idx) {
-		ast_taskprocessor_unreference(distributor_pool[idx]);
+		serializer = distributor_pool[idx];
 		distributor_pool[idx] = NULL;
+		ast_taskprocessor_unreference(serializer);
 	}
 }
 
@@ -1067,6 +1096,13 @@
 			return -1;
 		}
 	}
+
+	ast_taskprocessor_build_name(tps_name, sizeof(tps_name), "pjsip/distrib_master");
+	distributor_master = ast_sip_create_serializer(tps_name);
+	if (!distributor_master) {
+		return -1;
+	}
+
 	return 0;
 }
 

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

Gerrit-Project: asterisk
Gerrit-Branch: 14
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9ed63f4da9649e9db6ed4be29c360968917a89bd
Gerrit-Change-Number: 5920
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170628/0ba66548/attachment-0001.html>