[Asterisk-code-review] AST-2017-011 - res pjsip session: session leak when a call i... (asterisk[certified/13.13])

[George Joseph]

George Joseph has submitted this change and it was merged. ( https://gerrit.asterisk.org/7136 )

Change subject: AST-2017-011 - res_pjsip_session: session leak when a call is rejected
......................................................................

AST-2017-011 - res_pjsip_session: session leak when a call is rejected

A previous commit made it so when an invite session transitioned into a
disconnected state destruction of the Asterisk pjsip session object was
postponed until either a transport error occurred or the event timer
expired. However, if a call was rejected (for instance a 488) before the
session was fully established the event timer may not have been initiated,
or it was canceled without triggering either of the session finalizing states
mentioned above.

Really the only time destruction of the session should be delayed is when a
BYE is being transacted. This is because it's possible in some cases for the
session to be disconnected, but the BYE is still transacting.

This patch makes it so the session object always gets released (no more
memory leak) when the pjsip session is in a disconnected state. Except when
the method is a BYE. Then it waits until a transport error occurs or an event
timeout.

ASTERISK-27345 #close

Reported by: Corey Farrell

Change-Id: I1e724737b758c20ac76d19d3611e3d2876ae10ed
---
M res/res_pjsip_session.c
1 file changed, 42 insertions(+), 38 deletions(-)

Approvals:
  Jenkins2: Verified
  Joshua Colp: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved; Approved for Submit



diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index 0954a85..3de664c 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -2660,6 +2660,36 @@
 	/* XXX STUB */
 }
 
+static int session_end_if_disconnected(int id, pjsip_inv_session *inv)
+{
+	struct ast_sip_session *session;
+
+	if (inv->state != PJSIP_INV_STATE_DISCONNECTED) {
+		return 0;
+	}
+
+	/*
+	 * 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[id];
+	inv->mod_data[id] = NULL;
+	pjsip_dlg_dec_lock(inv->dlg);
+
+	/*
+	 * Pass the session ref held by session->inv_session to
+	 * session_end_completion().
+	 */
+	if (session
+		&& 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 1;
+}
+
 static void session_inv_on_tsx_state_changed(pjsip_inv_session *inv, pjsip_transaction *tsx, pjsip_event *e)
 {
 	ast_sip_session_response_cb cb;
@@ -2684,6 +2714,17 @@
 		/* The session has ended.  Ignore the transaction change. */
 		return;
 	}
+
+	/*
+	 * If the session is disconnected really nothing else to do unless currently transacting
+	 * a BYE. If a BYE then hold off destruction until the transaction timeout occurs. This
+	 * has to be done for BYEs because sometimes the dialog can be in a disconnected
+	 * state but the BYE request transaction has not yet completed.
+	 */
+	if (tsx->method.id != PJSIP_BYE_METHOD && session_end_if_disconnected(id, inv)) {
+		return;
+	}
+
 	switch (e->body.tsx_state.type) {
 	case PJSIP_EVENT_TX_MSG:
 		/* When we create an outgoing request, we do not have access to the transaction that
@@ -2806,49 +2847,12 @@
 		}
 		break;
 	case PJSIP_EVENT_TRANSPORT_ERROR:
-		if (inv->state == PJSIP_INV_STATE_DISCONNECTED) {
-			/*
-			 * 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[id] = NULL;
-
-			/*
-			 * Pass the session ref held by session->inv_session to
-			 * session_end_completion().
-			 */
-			if (session
-				&& 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_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[id];
-			inv->mod_data[id] = NULL;
-			pjsip_dlg_dec_lock(inv->dlg);
-
-			/*
-			 * Pass the session ref held by session->inv_session to
-			 * session_end_completion().
-			 */
-			if (session
-				&& 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);
-			}
+		if (session_end_if_disconnected(id, inv)) {
 			return;
 		}
 		break;

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

Gerrit-Project: asterisk
Gerrit-Branch: certified/13.13
Gerrit-MessageType: merged
Gerrit-Change-Id: I1e724737b758c20ac76d19d3611e3d2876ae10ed
Gerrit-Change-Number: 7136
Gerrit-PatchSet: 1
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171108/8f571312/attachment.html>