[Asterisk-code-review] res pjsip session.c: Fix crash on call disconnect. (asterisk[13])

[Richard Mudgett]

Richard Mudgett has uploaded a new change for review.

  https://gerrit.asterisk.org/889

Change subject: res_pjsip_session.c: Fix crash on call disconnect.
......................................................................

res_pjsip_session.c: Fix crash on call disconnect.

The crash fix for ASTERISK-25183 backported some code from master to try
to make sure that a BYE response is processed by the same serializer used
by the BYE request.  The identified race condition causing that backport
was the BYE request code had not finished processing after sending the BYE
before the BYE response came in for processing under a different thread.
Unfortunately, there is still a race condition.  Now the race condition is
between destroying the call session's serializer in
ast_taskprocessor_unreference() and using ast_taskprocessor_get() to get a
reference to the serializer for a BYE response.  Even worse, the new race
condition is a design limitation of the taskprocessor implementation that
didn't matter in versions before v12.  Back then, taskprocessors were only
destroyed when a module unloaded.  Now res_pjsip can destroy them when a
call ends.

However, as noted on the ASTERISK-25183 commit,
session_inv_on_state_changed() is disassociating the dialog from the
session when the invite dialog state becomes PJSIP_INV_STATE_DISCONNECTED.
This is a tad too soon because our BYE request transaction has not
completed yet.

* Split session_end() that is called by session_inv_on_state_changed() to
hold off session destruction until the BYE transaction timeout occurs or a
failed initial INVITE transaction timeout occurs in
session_inv_on_tsx_state_changed().

ASTERISK-25201 #close
Reported by: Matt Jordan

Change-Id: Iaf8dc8485fd8392a2a3ee4ad3b7f7f04a0dcc961
---
M res/res_pjsip_session.c
1 file changed, 59 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/89/889/1

diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index 8c04a7c..2c2ea61 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -2247,7 +2247,7 @@
 	}
 }
 
-static int session_end(struct ast_sip_session *session)
+static void session_end(struct ast_sip_session *session)
 {
 	struct ast_sip_session_supplement *iter;
 
@@ -2256,14 +2256,28 @@
 		ao2_ref(session, -1);
 	}
 
-	/* Session is dead. Let's get rid of the reference to the session */
+	/* Session is dead.  Notify the supplements. */
 	AST_LIST_TRAVERSE(&session->supplements, iter, next) {
 		if (iter->session_end) {
 			iter->session_end(session);
 		}
 	}
+}
 
-	session->inv_session->mod_data[session_module.id] = NULL;
+/*!
+ * \internal
+ * \brief Complete ending session activities.
+ * \since 13.5.0
+ *
+ * \param vsession Which session to complete stopping.
+ *
+ * \retval 0 on success.
+ * \retval -1 on error.
+ */
+static int session_end_completion(void *vsession)
+{
+	struct ast_sip_session *session = vsession;
+
 	ast_sip_dialog_set_serializer(session->inv_session->dlg, NULL);
 	ast_sip_dialog_set_endpoint(session->inv_session->dlg, NULL);
 	ao2_cleanup(session);
@@ -2340,9 +2354,7 @@
 
 	print_debug_details(inv, tsx, e);
 	if (!session) {
-		/* Transaction likely timed out after the call was hung up. Just
-		 * ignore such transaction changes
-		 */
+		/* The session has ended.  Ignore the transaction change. */
 		return;
 	}
 	switch (e->body.tsx_state.type) {
@@ -2423,7 +2435,48 @@
 		}
 		break;
 	case PJSIP_EVENT_TRANSPORT_ERROR:
+		/*
+		 * Clear the module data now to block session_inv_on_state_changed()
+		 * from calling session_end() if it hasn't already done so.
+		 */
+		inv->mod_data[session_module.id] = NULL;
+
+		if (inv->state != PJSIP_INV_STATE_DISCONNECTED) {
+			session_end(session);
+		}
+
+		/*
+		 * Pass the session ref held by session->inv_session to
+		 * session_end_completion().
+		 */
+		session_end_completion(session);
+		return;
 	case PJSIP_EVENT_TIMER:
+		/*
+		 * The timer event is run by the pjsip monitor thread and not
+		 * by the session serializer.
+		 */
+		if (inv->state == PJSIP_INV_STATE_DISCONNECTED) {
+			/*
+			 * We are locking because ast_sip_dialog_get_session() needs
+			 * the dialog locked to get the session by other threads.
+			 */
+			pjsip_dlg_inc_lock(inv->dlg);
+			session = inv->mod_data[session_module.id];
+			inv->mod_data[session_module.id] = NULL;
+			pjsip_dlg_dec_lock(inv->dlg);
+
+			/*
+			 * Pass the session ref held by session->inv_session to
+			 * session_end_completion().
+			 */
+			if (ast_sip_push_task(session->serializer, session_end_completion, session)) {
+				/* Do it anyway even though this is not the right thread. */
+				session_end_completion(session);
+			}
+			return;
+		}
+		break;
 	case PJSIP_EVENT_USER:
 	case PJSIP_EVENT_UNKNOWN:
 	case PJSIP_EVENT_TSX_STATE:

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaf8dc8485fd8392a2a3ee4ad3b7f7f04a0dcc961
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>