[Asterisk-code-review] res pjsip session: Handle race condition at shutdown with ti... (asterisk[master])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged.

Change subject: res_pjsip_session: Handle race condition at shutdown with timer.
......................................................................


res_pjsip_session: Handle race condition at shutdown with timer.

When shutting down res_pjsip_session will get unloaded before res_pjsip.
The act of unloading unregisters all the PJSIP services and sets
their module IDs to -1. In some cases it is possible for a timer to
occur after this happens which calls into res_pjsip_session. The
res_pjsip_session module can then try to get the session from the
INVITE session using the module ID. Since the module ID is now -1
this fails.

This change stores a copy of the module ID and uses it for the timer
callback scenario. If the module ID is -1 the callback immediately
returns but if the module ID is valid then it continues as normal.

This works as the original ID of the module is guaranteed to still
be valid when used with the INVITE session.

ASTERISK-26127 #close

Change-Id: I88df72525c4e9ef9f19c13aedddd3ac4a335c573
---
M res/res_pjsip_session.c
1 file changed, 24 insertions(+), 8 deletions(-)

Approvals:
  Mark Michelson: Looks good to me, approved
  Richard Mudgett: Looks good to me, but someone else must approve
  Joshua Colp: Verified



diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index fad0606..eb95510 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -2315,7 +2315,8 @@
 
 static void __print_debug_details(const char *function, pjsip_inv_session *inv, pjsip_transaction *tsx, pjsip_event *e)
 {
-	struct ast_sip_session *session;
+	int id = session_module.id;
+	struct ast_sip_session *session = NULL;
 
 	if (!DEBUG_ATLEAST(5)) {
 		/* Debug not spamy enough */
@@ -2330,7 +2331,9 @@
 			pjsip_tsx_state_str(tsx->state));
 		return;
 	}
-	session = inv->mod_data[session_module.id];
+	if (id > -1) {
+		session = inv->mod_data[session_module.id];
+	}
 	if (!session) {
 		ast_log(LOG_DEBUG, "inv_session %p has no ast session\n", inv);
 	} else {
@@ -2584,8 +2587,21 @@
 static void session_inv_on_tsx_state_changed(pjsip_inv_session *inv, pjsip_transaction *tsx, pjsip_event *e)
 {
 	ast_sip_session_response_cb cb;
-	struct ast_sip_session *session = inv->mod_data[session_module.id];
+	int id = session_module.id;
+	struct ast_sip_session *session;
 	pjsip_tx_data *tdata;
+
+	/*
+	 * A race condition exists at shutdown where the res_pjsip_session can be
+	 * unloaded but this callback may still get called afterwards. In this case
+	 * the id may end up being -1 which is useless to us. To work around this
+	 * we store the current value and check/use it.
+	 */
+	if (id < 0) {
+		return;
+	}
+
+	session = inv->mod_data[id];
 
 	print_debug_details(inv, tsx, e);
 	if (!session) {
@@ -2600,10 +2616,10 @@
 		 * we transfer the data into the transaction. This way, when we receive a response, we
 		 * can dig this data out again
 		 */
-		tsx->mod_data[session_module.id] = e->body.tsx_state.src.tdata->mod_data[session_module.id];
+		tsx->mod_data[id] = e->body.tsx_state.src.tdata->mod_data[id];
 		break;
 	case PJSIP_EVENT_RX_MSG:
-		cb = ast_sip_mod_data_get(tsx->mod_data, session_module.id, MOD_DATA_ON_RESPONSE);
+		cb = ast_sip_mod_data_get(tsx->mod_data, id, MOD_DATA_ON_RESPONSE);
 		/* As the PJSIP invite session implementation responds with a 200 OK before we have a
 		 * chance to be invoked session supplements for BYE requests actually end up executing
 		 * in the invite session state callback as well. To prevent session supplements from
@@ -2682,7 +2698,7 @@
 		 * 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;
+		inv->mod_data[id] = NULL;
 
 		if (inv->state != PJSIP_INV_STATE_DISCONNECTED) {
 			session_end(session);
@@ -2705,8 +2721,8 @@
 			 * 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;
+			session = inv->mod_data[id];
+			inv->mod_data[id] = NULL;
 			pjsip_dlg_dec_lock(inv->dlg);
 
 			/*

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I88df72525c4e9ef9f19c13aedddd3ac4a335c573
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Joshua Colp <jcolp 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: Richard Mudgett <rmudgett at digium.com>