[Asterisk-code-review] res pjsip pubsub: Prevent sending NOTIFY on destroyed dialog. (asterisk[13])

[Richard Mudgett]

Richard Mudgett has uploaded a new change for review.

  https://gerrit.asterisk.org/1499

Change subject: res_pjsip_pubsub: Prevent sending NOTIFY on destroyed dialog.
......................................................................

res_pjsip_pubsub: Prevent sending NOTIFY on destroyed dialog.

A certain situation can result in our attempting to send a NOTIFY on a
destroyed dialog. Say we attempt to send a NOTIFY to a subscriber, but
that subscriber has dropped off the network. We end up retransmitting
that NOTIFY until the appropriate SIP timer says to destroy the NOTIFY
transaction. When the pjsip evsub code is told that the transaction has
been terminated, it responds in kind by alerting us that the
subscription has been terminated, destroying the subscription, and then
removing its reference to the dialog, thus destroying the dialog.

The problem is that when we get told that the subscription is being
terminated, we detect that we have not sent a terminating NOTIFY
request, so we queue up such a NOTIFY to be sent out. By the time that
queued NOTIFY gets sent, the dialog has been destroyed, so attempting to
send that NOTIFY can result in a crash.

The fix being introduced here is actually a reintroduction of something
the pubsub code used to employ. We hold a reference to the dialog and
wait to decrement our reference to the dialog until our subscription
tree object is destroyed. This way, we can send messages on the dialog
even if the PJSIP evsub code wants to terminate earlier than we would
like.

In doing this, some NULL checks for subscription tree dialogs have been
removed since NULL dialogs are no longer actually possible.

Change-Id: I013f43cddd9408bb2a31b77f5db87a7972bfe1e5
---
M res/res_pjsip_pubsub.c
1 file changed, 20 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/99/1499/1

diff --git a/res/res_pjsip_pubsub.c b/res/res_pjsip_pubsub.c
index afcae34..cbedaf2 100644
--- a/res/res_pjsip_pubsub.c
+++ b/res/res_pjsip_pubsub.c
@@ -1177,6 +1177,24 @@
 		sub->handler->subscription_shutdown(sub);
 	}
 }
+static int subscription_unreference_dialog(void *obj)
+{
+	struct sip_subscription_tree *sub_tree = obj;
+
+	/* This is why we keep the dialog on the subscription. When the subscription
+	 * is destroyed, there is no guarantee that the underlying dialog is ready
+	 * to be destroyed. Furthermore, there's no guarantee in the opposite direction
+	 * either. The dialog could be destroyed before our subscription is. We fix
+	 * this problem by keeping a reference to the dialog until it is time to
+	 * destroy the subscription. We need to have the dialog available when the
+	 * subscription is destroyed so that we can guarantee that our attempt to
+	 * remove the serializer will be successful.
+	 */
+	pjsip_dlg_dec_session(sub_tree->dlg, &pubsub_module);
+	sub_tree->dlg = NULL;
+
+	return 0;
+}
 
 static void subscription_tree_destructor(void *obj)
 {
@@ -1190,6 +1208,7 @@
 
 	destroy_subscriptions(sub_tree->root);
 
+	ast_sip_push_task_synchronous(sub_tree->serializer, subscription_unreference_dialog, sub_tree);
 	ast_taskprocessor_unreference(sub_tree->serializer);
 	ast_module_unref(ast_module_info->self);
 }
@@ -1206,6 +1225,7 @@
 	ast_sip_dialog_set_serializer(dlg, sub_tree->serializer);
 	ast_sip_dialog_set_endpoint(dlg, sub_tree->endpoint);
 	pjsip_evsub_set_mod_data(sub_tree->evsub, pubsub_module.id, sub_tree);
+	pjsip_dlg_inc_session(dlg, &pubsub_module);
 }
 
 static struct sip_subscription_tree *allocate_subscription_tree(struct ast_sip_endpoint *endpoint)
@@ -1526,10 +1546,6 @@
 	pj_str_t name;
 
 	dlg = sub->tree->dlg;
-	if (!dlg) {
-		return NULL;
-	}
-
 	msg = ast_sip_mod_data_get(dlg->mod_data, pubsub_module.id, MOD_DATA_MSG);
 	pj_cstr(&name, header);
 
@@ -2136,10 +2152,6 @@
 	struct sip_subscription_tree *sub_tree = userdata;
 	pjsip_dialog *dlg = sub_tree->dlg;
 
-	if (!dlg) {
-		return 0;
-	}
-
 	pjsip_dlg_inc_lock(dlg);
 	/* It's possible that between when the notification was scheduled
 	 * and now, that a new SUBSCRIBE arrived, requiring full state to be
@@ -2193,10 +2205,6 @@
 	int res;
 	pjsip_dialog *dlg = sub->tree->dlg;
 
-	if (!dlg) {
-		return 0;
-	}
-
 	pjsip_dlg_inc_lock(dlg);
 
 	if (!sub->tree->evsub) {
@@ -2241,10 +2249,6 @@
 	pjsip_dialog *dlg;
 
 	dlg = sub->tree->dlg;
-	if (!dlg) {
-		*buf = '\0';
-		return;
-	}
 	ast_copy_pj_str(buf, &dlg->remote.info_str, size);
 }
 
@@ -3199,10 +3203,6 @@
 	struct sip_subscription_tree *sub_tree = userdata;
 	pjsip_dialog *dlg = sub_tree->dlg;
 
-	if (!dlg) {
-		return 0;
-	}
-
 	pjsip_dlg_inc_lock(dlg);
 	if (!sub_tree->evsub) {
 		pjsip_dlg_dec_lock(dlg);
@@ -3291,7 +3291,6 @@
 	sub_tree->evsub = NULL;
 	ast_sip_dialog_set_serializer(sub_tree->dlg, NULL);
 	ast_sip_dialog_set_endpoint(sub_tree->dlg, NULL);
-	sub_tree->dlg = NULL;
 	subscription_persistence_remove(sub_tree);
 	shutdown_subscriptions(sub_tree->root);
 
@@ -3303,10 +3302,6 @@
 {
 	struct sip_subscription_tree *sub_tree = userdata;
 	pjsip_dialog *dlg = sub_tree->dlg;
-
-	if (!dlg) {
-		return 0;
-	}
 
 	pjsip_dlg_inc_lock(dlg);
 	if (!sub_tree->evsub) {

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I013f43cddd9408bb2a31b77f5db87a7972bfe1e5
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>