[Asterisk-code-review] res pjsip t38.c: Fix deadlock in T.38 framehook. (asterisk[14])

[Jenkins2]

Jenkins2 has submitted this change and it was merged. ( https://gerrit.asterisk.org/5562 )

Change subject: res_pjsip_t38.c: Fix deadlock in T.38 framehook.
......................................................................


res_pjsip_t38.c: Fix deadlock in T.38 framehook.

A deadlock can happen between a channel lock and a pjsip session media
container lock.  One thread is processing a reINVITE's SDP and walking
through the session's media container when it waits for the channel lock
to put the determined format capabilities onto the channel.  The other
thread is writing a frame to the channel and processing the T.38 frame
hook.  The T.38 frame hook then waits for the pjsip session's media
container lock.  The two threads are now deadlocked.

* Made the T.38 frame hook release the channel lock before searching the
session's media container.  This fix has been done to several other
frame hooks to fix deadlocks.

ASTERISK-26974 #close

Change-Id: Ie984a76ce00bef6ec9aa239010e51e8dd74c8186
---
M res/res_pjsip_t38.c
1 file changed, 20 insertions(+), 10 deletions(-)

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



diff --git a/res/res_pjsip_t38.c b/res/res_pjsip_t38.c
index 346f824..7643492 100644
--- a/res/res_pjsip_t38.c
+++ b/res/res_pjsip_t38.c
@@ -400,7 +400,8 @@
 }
 
 /*! \brief Frame hook callback for writing */
-static struct ast_frame *t38_framehook_write(struct ast_sip_session *session, struct ast_frame *f)
+static struct ast_frame *t38_framehook_write(struct ast_channel *chan,
+	struct ast_sip_session *session, struct ast_frame *f)
 {
 	if (f->frametype == AST_FRAME_CONTROL && f->subclass.integer == AST_CONTROL_T38_PARAMETERS &&
 		session->endpoint->media.t38.enabled) {
@@ -414,27 +415,36 @@
 			ao2_ref(data, -1);
 		}
 	} else if (f->frametype == AST_FRAME_MODEM) {
-		RAII_VAR(struct ast_sip_session_media *, session_media, NULL, ao2_cleanup);
+		struct ast_sip_session_media *session_media;
 
-		if ((session_media = ao2_find(session->media, "image", OBJ_KEY)) &&
-			session_media->udptl) {
+		/* Avoid deadlock between chan and the session->media container lock */
+		ast_channel_unlock(chan);
+		session_media = ao2_find(session->media, "image", OBJ_SEARCH_KEY);
+		ast_channel_lock(chan);
+		if (session_media && session_media->udptl) {
 			ast_udptl_write(session_media->udptl, f);
 		}
+		ao2_cleanup(session_media);
 	}
 
 	return f;
 }
 
 /*! \brief Frame hook callback for reading */
-static struct ast_frame *t38_framehook_read(struct ast_sip_session *session, struct ast_frame *f)
+static struct ast_frame *t38_framehook_read(struct ast_channel *chan,
+	struct ast_sip_session *session, struct ast_frame *f)
 {
 	if (ast_channel_fdno(session->channel) == 5) {
-		RAII_VAR(struct ast_sip_session_media *, session_media, NULL, ao2_cleanup);
+		struct ast_sip_session_media *session_media;
 
-		if ((session_media = ao2_find(session->media, "image", OBJ_KEY)) &&
-			session_media->udptl) {
+		/* Avoid deadlock between chan and the session->media container lock */
+		ast_channel_unlock(chan);
+		session_media = ao2_find(session->media, "image", OBJ_SEARCH_KEY);
+		ast_channel_lock(chan);
+		if (session_media && session_media->udptl) {
 			f = ast_udptl_read(session_media->udptl);
 		}
+		ao2_cleanup(session_media);
 	}
 
 	return f;
@@ -447,9 +457,9 @@
 	struct ast_sip_channel_pvt *channel = ast_channel_tech_pvt(chan);
 
 	if (event == AST_FRAMEHOOK_EVENT_READ) {
-		f = t38_framehook_read(channel->session, f);
+		f = t38_framehook_read(chan, channel->session, f);
 	} else if (event == AST_FRAMEHOOK_EVENT_WRITE) {
-		f = t38_framehook_write(channel->session, f);
+		f = t38_framehook_write(chan, channel->session, f);
 	}
 
 	return f;

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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie984a76ce00bef6ec9aa239010e51e8dd74c8186
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 14
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>