[Asterisk-code-review] res_pjsip_refer: Always serialize calls to refer_progress_notify (asterisk[16])

[George Joseph]

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


Change subject: res_pjsip_refer: Always serialize calls to refer_progress_notify
......................................................................

res_pjsip_refer: Always serialize calls to refer_progress_notify

refer_progress_notify wasn't always being called from the progress
serializer.  This could allow clearing notification->progress->sub
in one thread while another was trying to use it.

* refer_progress_notify is now called using the new function
  ast_sip_push_task_serializer() which runs the callback
  in-line if it's already called from the serializer and
  pushes it to the serializer (without a wait) if it isn't.

Change-Id: Idcf1934c4e873f2c82e2d106f8d9f040caf9fa1e
---
M include/asterisk/res_pjsip.h
M res/res_pjsip.c
M res/res_pjsip_refer.c
3 files changed, 63 insertions(+), 7 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/00/15400/1

diff --git a/include/asterisk/res_pjsip.h b/include/asterisk/res_pjsip.h
index 81161f3..8b17110 100644
--- a/include/asterisk/res_pjsip.h
+++ b/include/asterisk/res_pjsip.h
@@ -1674,6 +1674,25 @@
 int ast_sip_push_task_wait_serializer(struct ast_taskprocessor *serializer, int (*sip_task)(void *), void *task_data);
 
 /*!
+ * \brief Push a task to the serializer but don't wait for it to complete.
+ *
+ * Like \ref ast_sip_push_task_wait_serializer except that it doesn't block.
+ * If the specified serializer is the current thread then the task executes.
+ *
+ * \param serializer The SIP serializer to execute the task.  NULL if
+ * any of the default serializers can be used.
+ * \param sip_task The task to execute
+ * \param task_data The parameter to pass to the task when it executes
+ *
+ * \note The sip_task() return value may need to be distinguished from
+ * the failure to push the task.
+ *
+ * \return sip_task() return value on success.
+ * \retval -1 Failure to push the task.
+ */
+int ast_sip_push_task_serializer(struct ast_taskprocessor *serializer, int (*sip_task)(void *), void *task_data);
+
+/*!
  * \brief Determine if the current thread is a SIP servant thread
  *
  * \retval 0 This is not a SIP servant thread
diff --git a/res/res_pjsip.c b/res/res_pjsip.c
index 2ea07fd..33e74e9 100644
--- a/res/res_pjsip.c
+++ b/res/res_pjsip.c
@@ -4759,6 +4759,28 @@
 	return ast_sip_push_task_wait(serializer, sip_task, task_data);
 }
 
+int ast_sip_push_task_serializer(struct ast_taskprocessor *serializer, int (*sip_task)(void *), void *task_data)
+{
+	if (!serializer) {
+		/* Caller doesn't care which PJSIP serializer the task executes under. */
+		serializer = ast_serializer_pool_get(sip_serializer_pool);
+		if (!serializer) {
+			/* No serializer picked to execute the task */
+			return -1;
+		}
+	}
+	if (ast_taskprocessor_is_task(serializer)) {
+		/*
+		 * We are the requested serializer so we must execute
+		 * the task now or deadlock waiting on ourself to
+		 * execute it.
+		 */
+		return sip_task(task_data);
+	}
+
+	return ast_sip_push_task(serializer, sip_task, task_data);
+}
+
 void ast_copy_pj_str(char *dest, const pj_str_t *src, size_t size)
 {
 	size_t chars_to_copy = MIN(size - 1, pj_strlen(src));
diff --git a/res/res_pjsip_refer.c b/res/res_pjsip_refer.c
index 5d98f9e..f6dd2f3 100644
--- a/res/res_pjsip_refer.c
+++ b/res/res_pjsip_refer.c
@@ -127,8 +127,11 @@
 	 * but we are already running a task... thus it would deadlock */
 	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);
+			sub, notification->progress);
 		pjsip_dlg_inc_lock(notification->progress->dlg);
+		if (!notification->progress->sub) {
+			ast_log(LOG_WARNING, "Subscription %p was stepped on!", sub);
+		}
 		pjsip_evsub_set_mod_data(notification->progress->sub, refer_progress_module.id, NULL);
 		pjsip_dlg_dec_lock(notification->progress->dlg);
 
@@ -560,7 +563,9 @@
 		notification = refer_progress_notification_alloc(attended->progress, response,
 			PJSIP_EVSUB_STATE_TERMINATED);
 		if (notification) {
-			refer_progress_notify(notification);
+			if (ast_sip_push_task_serializer(attended->progress->serializer, refer_progress_notify, notification)) {
+				ao2_cleanup(notification);
+			}
 		}
 	}
 
@@ -616,7 +621,9 @@
 			PJSIP_EVSUB_STATE_TERMINATED);
 
 		if (notification) {
-			refer_progress_notify(notification);
+			if (ast_sip_push_task_serializer(refer->progress->serializer, refer_progress_notify, notification)) {
+				ao2_cleanup(notification);
+			}
 		}
 	} else if (refer->progress) {
 		/* If attended transfer and progress monitoring is being done attach a frame hook so we can monitor it */
@@ -637,7 +644,9 @@
 				ast_channel_name(chan));
 
 			if (notification) {
-				refer_progress_notify(notification);
+				if (ast_sip_push_task_serializer(refer->progress->serializer, refer_progress_notify, notification)) {
+					ao2_cleanup(notification);
+				}
 			}
 		}
 
@@ -660,7 +669,9 @@
 				ast_channel_name(chan));
 
 			if (notification) {
-				refer_progress_notify(notification);
+				if (ast_sip_push_task_serializer(refer->progress->serializer, refer_progress_notify, notification)) {
+					ao2_cleanup(notification);
+				}
 			}
 
 			ao2_cleanup(refer->progress);
@@ -680,7 +691,9 @@
 					ast_channel_name(chan));
 
 			if (notification) {
-				refer_progress_notify(notification);
+				if (ast_sip_push_task_serializer(refer->progress->serializer, refer_progress_notify, notification)) {
+					ao2_cleanup(notification);
+				}
 			}
 
 			ast_channel_lock(chan);
@@ -1155,7 +1168,9 @@
 
 		if (notification) {
 			/* The refer_progress_notify function will call ao2_cleanup on this for us */
-			refer_progress_notify(notification);
+			if (ast_sip_push_task_serializer(progress->serializer, refer_progress_notify, notification)) {
+				ao2_cleanup(notification);
+			}
 		}
 	}
 

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

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Idcf1934c4e873f2c82e2d106f8d9f040caf9fa1e
Gerrit-Change-Number: 15400
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/20210209/6e5c616e/attachment.html>