[Asterisk-code-review] res/res pjsip: Fix crash due to misuse of session->media bet... (asterisk[13])

[Paulo Vicentini]

Paulo Vicentini has uploaded this change for review. ( https://gerrit.asterisk.org/10601


Change subject: res/res_pjsip: Fix crash due to misuse of session->media between threads.
......................................................................

res/res_pjsip: Fix crash due to misuse of session->media between threads.

This patch makes sure that thread running ast_taskprocessor_execute
cannot suddenly dispose the session->media object making the other
threads (running pbx_thread / bridge_channel_ind_thread) crash when they
try to access the pointer to invalid memory. We were experiencing a crash due
to a misuse of session->media container between threads running
(bridge_channel_ind_thread/pbx_thread) and the thread running
ast_taskprocessor_execute. Depending on the SIP flow (during a disconnection)
and the threads' code path, the session->media container was being destroyed
(and set to NULL) by the thread running ast_taskprocessor_execute while the
thread running t38_framehook_read was still referring to it.
Now the "parent" thread bumps the session->media's refcount on behalf of the new
thread before the object is passed.The new thread decrements refcount before
terminating.

ASTERISK-28156

Change-Id: Ia92e2389b8d804bf205473e92ec06217e87ce237
---
M include/asterisk/res_pjsip_session.h
M res/res_pjsip_session.c
M res/res_pjsip_t38.c
3 files changed, 25 insertions(+), 4 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/01/10601/1

diff --git a/include/asterisk/res_pjsip_session.h b/include/asterisk/res_pjsip_session.h
index 0c2b1e3..79fda6c 100644
--- a/include/asterisk/res_pjsip_session.h
+++ b/include/asterisk/res_pjsip_session.h
@@ -159,6 +159,8 @@
 	unsigned int defer_end:1;
 	/*! Session end (remote hangup) requested while termination deferred */
 	unsigned int ended_while_deferred:1;
+	/*! Session has ended (i.e. session_end has been called) */
+	unsigned int ended:1;
 	/*! DTMF mode to use with this session, from endpoint but can change */
 	enum ast_sip_dtmf_mode dtmf;
 	/*! Initial incoming INVITE Request-URI.  NULL otherwise. */
diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index 669315f..eedc0f1 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -1319,7 +1319,9 @@
 
 	ast_taskprocessor_unreference(session->serializer);
 	ao2_cleanup(session->datastores);
-	ao2_cleanup(session->media);
+	if (!session->ended) {
+		ao2_cleanup(session->media);
+	}
 
 	AST_LIST_HEAD_DESTROY(&session->supplements);
 	while ((delay = AST_LIST_REMOVE_HEAD(&session->delayed_requests, next))) {
@@ -2564,7 +2566,11 @@
 
 	/* Release any media resources. */
 	ao2_cleanup(session->media);
-	session->media = NULL;
+	/*
+	 * We are sharing ownership with other threads also
+	 * using session->media directly
+	 */
+	session->ended = 1;
 
 	return 0;
 }
diff --git a/res/res_pjsip_t38.c b/res/res_pjsip_t38.c
index cff625f..3eccc6e 100644
--- a/res/res_pjsip_t38.c
+++ b/res/res_pjsip_t38.c
@@ -485,10 +485,20 @@
 {
 	struct ast_sip_channel_pvt *channel = ast_channel_tech_pvt(chan);
 
-	if (event == AST_FRAMEHOOK_EVENT_READ) {
+	switch (event) {
+	case AST_FRAMEHOOK_EVENT_READ:
 		f = t38_framehook_read(chan, channel->session, f);
-	} else if (event == AST_FRAMEHOOK_EVENT_WRITE) {
+		break;
+	case AST_FRAMEHOOK_EVENT_WRITE:
 		f = t38_framehook_write(chan, channel->session, f);
+		break;
+	case AST_FRAMEHOOK_EVENT_DETACHED:
+		if (channel->session && channel->session->media) {
+			ao2_ref(channel->session->media, -1);
+		}
+		break;
+	case AST_FRAMEHOOK_EVENT_ATTACHED:
+		break;
 	}
 
 	return f;
@@ -562,6 +572,9 @@
 	}
 
 	ast_channel_datastore_add(session->channel, datastore);
+
+	/* bump media container to be used by pbx and bridge threads */
+	ao2_bump(session->media);
 	ast_channel_unlock(session->channel);
 }
 

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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia92e2389b8d804bf205473e92ec06217e87ce237
Gerrit-Change-Number: 10601
Gerrit-PatchSet: 1
Gerrit-Owner: Paulo Vicentini <paulo.vicentini at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20181108/0a878050/attachment.html>