[Asterisk-code-review] ast framehook attach() must be called with the channel locked. (asterisk[master])

[Anonymous Coward]

Anonymous Coward #1000019 has submitted this change and it was merged.

Change subject: ast_framehook_attach() must be called with the channel locked.
......................................................................


ast_framehook_attach() must be called with the channel locked.

The framehook container could become corrupted if the channel lock is not
held before calling.

Change-Id: I1a6b957a1f7b899eb29a186915f8cccab886a438
---
M main/bridge_basic.c
M res/res_pjsip_refer.c
M res/res_pjsip_t38.c
3 files changed, 14 insertions(+), 7 deletions(-)

Approvals:
  George Joseph: Looks good to me, but someone else must approve
  Anonymous Coward #1000019: Verified
  Joshua Colp: Looks good to me, approved



diff --git a/main/bridge_basic.c b/main/bridge_basic.c
index 8d7fbae..6c411fb 100644
--- a/main/bridge_basic.c
+++ b/main/bridge_basic.c
@@ -3088,7 +3088,9 @@
 	ao2_ref(props, +1);
 	target_interface.data = props;
 
+	ast_channel_lock(channel);
 	props->target_framehook_id = ast_framehook_attach(channel, &target_interface);
+	ast_channel_unlock(channel);
 	if (props->target_framehook_id == -1) {
 		ao2_ref(props, -1);
 		return -1;
diff --git a/res/res_pjsip_refer.c b/res/res_pjsip_refer.c
index 1043a1e..78d6e23 100644
--- a/res/res_pjsip_refer.c
+++ b/res/res_pjsip_refer.c
@@ -607,7 +607,10 @@
 		ao2_ref(refer->progress, +1);
 
 		/* If we can't attach a frame hook for whatever reason send a notification of success immediately */
-		if ((refer->progress->framehook = ast_framehook_attach(chan, &hook)) < 0) {
+		ast_channel_lock(chan);
+		refer->progress->framehook = ast_framehook_attach(chan, &hook);
+		ast_channel_unlock(chan);
+		if (refer->progress->framehook < 0) {
 			struct refer_progress_notification *notification = refer_progress_notification_alloc(refer->progress, 200,
 				PJSIP_EVSUB_STATE_TERMINATED);
 
diff --git a/res/res_pjsip_t38.c b/res/res_pjsip_t38.c
index 76720ac..150336a 100644
--- a/res/res_pjsip_t38.c
+++ b/res/res_pjsip_t38.c
@@ -501,25 +501,27 @@
 		return;
 	}
 
-	/* Skip attaching the framehook if the T.38 datastore already exists for the channel */
 	ast_channel_lock(session->channel);
-	if ((datastore = ast_channel_datastore_find(session->channel, &t38_framehook_datastore, NULL))) {
+
+	/* Skip attaching the framehook if the T.38 datastore already exists for the channel */
+	datastore = ast_channel_datastore_find(session->channel, &t38_framehook_datastore,
+		NULL);
+	if (datastore) {
 		ast_channel_unlock(session->channel);
 		return;
 	}
-	ast_channel_unlock(session->channel);
 
 	framehook_id = ast_framehook_attach(session->channel, &hook);
 	if (framehook_id < 0) {
-		ast_log(LOG_WARNING, "Could not attach T.38 Frame hook to channel, T.38 will be unavailable on '%s'\n",
+		ast_log(LOG_WARNING, "Could not attach T.38 Frame hook, T.38 will be unavailable on '%s'\n",
 			ast_channel_name(session->channel));
+		ast_channel_unlock(session->channel);
 		return;
 	}
 
-	ast_channel_lock(session->channel);
 	datastore = ast_datastore_alloc(&t38_framehook_datastore, NULL);
 	if (!datastore) {
-		ast_log(LOG_ERROR, "Could not attach T.38 Frame hook to channel, T.38 will be unavailable on '%s'\n",
+		ast_log(LOG_ERROR, "Could not alloc T.38 Frame hook datastore, T.38 will be unavailable on '%s'\n",
 			ast_channel_name(session->channel));
 		ast_framehook_detach(session->channel, framehook_id);
 		ast_channel_unlock(session->channel);

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I1a6b957a1f7b899eb29a186915f8cccab886a438
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>