[Asterisk-code-review] chan_pjsip.c: Check for channel and session to not be NULL in hangup (...asterisk[13])

[George Joseph]

Hello Abhay Gupta,

I'd like you to do a code review. Please visit

    https://gerrit.asterisk.org/c/asterisk/+/11448

to review the following change.


Change subject: chan_pjsip.c: Check for channel and session to not be NULL in hangup
......................................................................

chan_pjsip.c: Check for channel and session to not be NULL in hangup

We have seen some rare case of segmentation fault in hangup function
and we could notice that channel pointer was NULL.  Debug log shows
that there is a 200 OK answer and SIP timeout at the same time.  It
looks that while the SIP session was being destroyed due to timeout
call hangup due to answer event lead to race condition and channel
is being destroyed from two different places.  The check ensures we
check it not to be NULL before freeing it.

ASTERISK-25371

Change-Id: I19f6566830640625e08f7b87bfe15758ad33a778
---
M channels/chan_pjsip.c
1 file changed, 20 insertions(+), 10 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/48/11448/1

diff --git a/channels/chan_pjsip.c b/channels/chan_pjsip.c
index 0d565df..b0d5fda 100644
--- a/channels/chan_pjsip.c
+++ b/channels/chan_pjsip.c
@@ -2078,19 +2078,29 @@
 	struct hangup_data *h_data = data;
 	struct ast_channel *ast = h_data->chan;
 	struct ast_sip_channel_pvt *channel = ast_channel_tech_pvt(ast);
-	struct chan_pjsip_pvt *pvt = channel->pvt;
-	struct ast_sip_session *session = channel->session;
-	int cause = h_data->cause;
 
 	/*
-	 * It's possible that session_terminate might cause the session to be destroyed
-	 * immediately so we need to keep a reference to it so we can NULL session->channel
-	 * afterwards.
+	 * Before cleaning we have to ensure that channel or its session is not NULL
+	 * we have seen rare case when taskprocessor calls hangup but channel is NULL
+	 * due to SIP session timeout and answer happening at the same time
 	 */
-	ast_sip_session_terminate(ao2_bump(session), cause);
-	clear_session_and_channel(session, ast, pvt);
-	ao2_cleanup(session);
-	ao2_cleanup(channel);
+	if (channel) {
+		struct chan_pjsip_pvt *pvt = channel->pvt;
+		struct ast_sip_session *session = channel->session;
+		if (session) {
+			int cause = h_data->cause;
+
+			/*
+	 		* It's possible that session_terminate might cause the session to be destroyed
+	 		* immediately so we need to keep a reference to it so we can NULL session->channel
+	 		* afterwards.
+	 		*/
+			ast_sip_session_terminate(ao2_bump(session), cause);
+			clear_session_and_channel(session, ast, pvt);
+			ao2_cleanup(session);
+		}
+		ao2_cleanup(channel);
+	}
 	ao2_cleanup(h_data);
 	return 0;
 }

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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: I19f6566830640625e08f7b87bfe15758ad33a778
Gerrit-Change-Number: 11448
Gerrit-PatchSet: 1
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Abhay Gupta <abhay at avissol.com>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190610/6ce34df9/attachment.html>