[Asterisk-code-review] res_pjsip_refer: Refactor progress locking and serialization (asterisk[master])

[George Joseph]

George Joseph has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/15510 )


Change subject: res_pjsip_refer: Refactor progress locking and serialization
......................................................................

res_pjsip_refer: Refactor progress locking and serialization

Under certain conditions, the progress notify serializer can be
attempting to access the subscription while sending a notify
while another thread is cleaning the subscription up.

The serilizer usage and dialog locking have been refactored
to be both simpler and more consistent.

* refer_progress_notify(), which runs in the serializer,
  now holds the dialog lock for it's duration.

* refer_progress_on_evsub_state(), which doesn't run in
  the serializer but did already held the dialog lock
  for its duration, now directly sets progress->sub to NULL
  instead of queueing a task to the serializer.

ASTERISK-29313

Change-Id: I97a8bb01771a3c85345649b8124507f7622a8480
---
M res/res_pjsip_refer.c
1 file changed, 10 insertions(+), 28 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/10/15510/1

diff --git a/res/res_pjsip_refer.c b/res/res_pjsip_refer.c
index 030e2be..faffa92 100644
--- a/res/res_pjsip_refer.c
+++ b/res/res_pjsip_refer.c
@@ -115,10 +115,13 @@
 	pjsip_evsub *sub;
 	pjsip_tx_data *tdata;
 
+	pjsip_dlg_inc_lock(notification->progress->dlg);
+
 	/* If the subscription has already been terminated we can't send a notification */
 	if (!(sub = notification->progress->sub)) {
 		ast_debug(3, "Not sending NOTIFY of response '%d' and state '%u' on progress monitor '%p' as subscription has been terminated\n",
 			notification->response, notification->state, notification->progress);
+		pjsip_dlg_dec_lock(notification->progress->dlg);
 		return 0;
 	}
 
@@ -128,12 +131,9 @@
 	if (notification->state == PJSIP_EVSUB_STATE_TERMINATED) {
 		ast_debug(3, "Subscription '%p' is being terminated as a result of a NOTIFY, removing REFER progress structure early on progress monitor '%p'\n",
 			notification->progress->sub, notification->progress);
-		pjsip_dlg_inc_lock(notification->progress->dlg);
 		pjsip_evsub_set_mod_data(notification->progress->sub, refer_progress_module.id, NULL);
-		pjsip_dlg_dec_lock(notification->progress->dlg);
 
 		/* This is for dropping the reference on the subscription */
-		ao2_cleanup(notification->progress);
 
 		notification->progress->sub = NULL;
 	}
@@ -158,6 +158,12 @@
 		pjsip_xfer_send_request(sub, tdata);
 	}
 
+	if (notification->state == PJSIP_EVSUB_STATE_TERMINATED) {
+		ao2_cleanup(notification->progress);
+	}
+
+	pjsip_dlg_dec_lock(notification->progress->dlg);
+
 	return 0;
 }
 
@@ -293,17 +299,6 @@
 	ao2_cleanup(progress);
 }
 
-/*! \brief Serialized callback for subscription termination */
-static int refer_progress_terminate(void *data)
-{
-	struct refer_progress *progress = data;
-
-	/* The subscription is no longer valid */
-	progress->sub = NULL;
-
-	return 0;
-}
-
 /*! \brief Callback for REFER subscription state changes */
 static void refer_progress_on_evsub_state(pjsip_evsub *sub, pjsip_event *event)
 {
@@ -315,20 +310,7 @@
 		ast_debug(3, "Subscription '%p' has been remotely terminated, waiting for other tasks to complete on progress monitor '%p'\n",
 			sub, progress);
 
-		/* It's possible that a task is waiting to remove us already, so bump the refcount of progress so it doesn't get destroyed */
-		ao2_ref(progress, +1);
-		pjsip_dlg_dec_lock(progress->dlg);
-		/*
-		 * XXX We are always going to execute this inline rather than
-		 * in the serializer because this function is a PJPROJECT
-		 * callback and thus has to be a SIP servant thread.
-		 *
-		 * The likely remedy is to push most of this function into
-		 * refer_progress_terminate() with ast_sip_push_task().
-		 */
-		ast_sip_push_task_wait_servant(progress->serializer, refer_progress_terminate, progress);
-		pjsip_dlg_inc_lock(progress->dlg);
-		ao2_ref(progress, -1);
+		progress->sub = NULL;
 
 		ast_debug(3, "Subscription '%p' removed from progress monitor '%p'\n", sub, progress);
 

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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I97a8bb01771a3c85345649b8124507f7622a8480
Gerrit-Change-Number: 15510
Gerrit-PatchSet: 1
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210223/72154ec3/attachment.html>