[asterisk-commits] res pjsip pubsub: Ensure dialog lock balance. (asterisk[13])

[SVN commits to the Asterisk project]

Matt Jordan has submitted this change and it was merged.

Change subject: res_pjsip_pubsub: Ensure dialog lock balance.
......................................................................


res_pjsip_pubsub: Ensure dialog lock balance.

When sending a NOTIFY, we lock the dialog and then unlock the dialog
when finished. A recent change made it so that the subscription tree's
dialog pointer will be set NULL when sending the final NOTIFY request
out. This means that when we attempt to unlock the dialog, we pass a
NULL pointer to pjsip_dlg_dec_lock(). The result is that the dialog
remains locked after we think we have unlocked it. When a response to
the NOTIFY arrives, the monitor thread attempts to lock the dialog, but
it cannot because we never released the dialog lock. This results in
Asterisk being unable to process incoming SIP traffic any longer.

The fix in this patch is to use a local pointer to save off the pointer
value of the subscription tree's dialog when locking and unlocking the
dialog. This way, if the subscription tree's dialog pointer is NULLed
out, the local pointer will still have point to the proper place and the
dialog lock will be unlocked as we expect.

Change-Id: I7ddb3eaed7276cceb9a65daca701c3d5e728e63a
---
M res/res_pjsip_pubsub.c
1 file changed, 21 insertions(+), 17 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 2391c88..afcae34 100644
--- a/res/res_pjsip_pubsub.c
+++ b/res/res_pjsip_pubsub.c
@@ -2134,19 +2134,20 @@
 static int serialized_send_notify(void *userdata)
 {
 	struct sip_subscription_tree *sub_tree = userdata;
+	pjsip_dialog *dlg = sub_tree->dlg;
 
-	if (!sub_tree->dlg) {
+	if (!dlg) {
 		return 0;
 	}
 
-	pjsip_dlg_inc_lock(sub_tree->dlg);
+	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
 	 * sent out in an immediate NOTIFY. If that has happened, we need to
 	 * bail out here instead of sending the batched NOTIFY.
 	 */
 	if (!sub_tree->send_scheduled_notify) {
-		pjsip_dlg_dec_lock(sub_tree->dlg);
+		pjsip_dlg_dec_lock(dlg);
 		ao2_cleanup(sub_tree);
 		return 0;
 	}
@@ -2156,7 +2157,7 @@
 			"Resource: %s",
 			sub_tree->root->resource);
 	sub_tree->notify_sched_id = -1;
-	pjsip_dlg_dec_lock(sub_tree->dlg);
+	pjsip_dlg_dec_lock(dlg);
 	ao2_cleanup(sub_tree);
 	return 0;
 }
@@ -2190,21 +2191,22 @@
 		int terminate)
 {
 	int res;
+	pjsip_dialog *dlg = sub->tree->dlg;
 
-	if (!sub->tree->dlg) {
+	if (!dlg) {
 		return 0;
 	}
 
-	pjsip_dlg_inc_lock(sub->tree->dlg);
+	pjsip_dlg_inc_lock(dlg);
 
 	if (!sub->tree->evsub) {
-		pjsip_dlg_dec_lock(sub->tree->dlg);
+		pjsip_dlg_dec_lock(dlg);
 		return 0;
 	}
 
 	if (ast_sip_pubsub_generate_body_content(ast_sip_subscription_get_body_type(sub),
 				ast_sip_subscription_get_body_subtype(sub), notify_data, &sub->body_text)) {
-		pjsip_dlg_dec_lock(sub->tree->dlg);
+		pjsip_dlg_dec_lock(dlg);
 		return -1;
 	}
 
@@ -2225,7 +2227,7 @@
 		ao2_ref(sub->tree, -1);
 	}
 
-	pjsip_dlg_dec_lock(sub->tree->dlg);
+	pjsip_dlg_dec_lock(dlg);
 	return res;
 }
 
@@ -3195,14 +3197,15 @@
 static int serialized_pubsub_on_server_timeout(void *userdata)
 {
 	struct sip_subscription_tree *sub_tree = userdata;
+	pjsip_dialog *dlg = sub_tree->dlg;
 
-	if (!sub_tree->dlg) {
+	if (!dlg) {
 		return 0;
 	}
 
-	pjsip_dlg_inc_lock(sub_tree->dlg);
+	pjsip_dlg_inc_lock(dlg);
 	if (!sub_tree->evsub) {
-		pjsip_dlg_dec_lock(sub_tree->dlg);
+		pjsip_dlg_dec_lock(dlg);
 		return 0;
 	}
 	set_state_terminated(sub_tree->root);
@@ -3211,7 +3214,7 @@
 			"Resource: %s",
 			sub_tree->root->resource);
 
-	pjsip_dlg_dec_lock(sub_tree->dlg);
+	pjsip_dlg_dec_lock(dlg);
 	ao2_cleanup(sub_tree);
 	return 0;
 }
@@ -3299,14 +3302,15 @@
 static int serialized_pubsub_on_rx_refresh(void *userdata)
 {
 	struct sip_subscription_tree *sub_tree = userdata;
+	pjsip_dialog *dlg = sub_tree->dlg;
 
-	if (!sub_tree->dlg) {
+	if (!dlg) {
 		return 0;
 	}
 
-	pjsip_dlg_inc_lock(sub_tree->dlg);
+	pjsip_dlg_inc_lock(dlg);
 	if (!sub_tree->evsub) {
-		pjsip_dlg_dec_lock(sub_tree->dlg);
+		pjsip_dlg_dec_lock(dlg);
 		return 0;
 	}
 
@@ -3320,7 +3324,7 @@
 			"SUBSCRIPTION_TERMINATED" : "SUBSCRIPTION_REFRESHED",
 			"Resource: %s", sub_tree->root->resource);
 
-	pjsip_dlg_dec_lock(sub_tree->dlg);
+	pjsip_dlg_dec_lock(dlg);
 	ao2_cleanup(sub_tree);
 	return 0;
 }

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I7ddb3eaed7276cceb9a65daca701c3d5e728e63a
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
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>