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

[Matt Jordan]

Matt Jordan has submitted this change and it was merged.

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(-)

Approvals:
  Anonymous Coward #1000019: Verified
  Matt Jordan: Looks good to me, approved
  Joshua Colp: Looks good to me, but someone else must approve



diff --git a/res/res_pjsip_pubsub.c b/res/res_pjsip_pubsub.c
index 94125b9..8d3ea6f 100644
--- a/res/res_pjsip_pubsub.c
+++ b/res/res_pjsip_pubsub.c
@@ -1172,6 +1172,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)
 {
@@ -1185,6 +1203,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);
 }
@@ -1201,6 +1220,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)
@@ -1521,10 +1541,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);
 
@@ -2124,10 +2140,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
@@ -2181,10 +2193,6 @@
 	int res;
 	pjsip_dialog *dlg = sub->tree->dlg;
 
-	if (!dlg) {
-		return 0;
-	}
-
 	pjsip_dlg_inc_lock(dlg);
 
 	if (!sub->tree->evsub) {
@@ -2229,10 +2237,6 @@
 	pjsip_dialog *dlg;
 
 	dlg = sub->tree->dlg;
-	if (!dlg) {
-		*buf = '\0';
-		return;
-	}
 	ast_copy_pj_str(buf, &dlg->remote.info_str, size);
 }
 
@@ -3189,10 +3193,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);
@@ -3281,7 +3281,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);
 
@@ -3293,10 +3292,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/1512
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I013f43cddd9408bb2a31b77f5db87a7972bfe1e5
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: certified/13.1
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>