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

[Matt Jordan]

Matt Jordan has submitted this change and it was merged.

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

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



diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index 84c343d..950fc2d 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);
@@ -2372,9 +2386,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) {
@@ -2455,7 +2467,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/890
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Iaf8dc8485fd8392a2a3ee4ad3b7f7f04a0dcc961
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>