[Asterisk-code-review] channel: ast write frame wrongly freed after call to audiohooks (asterisk[14])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/5758 )

Change subject: channel: ast_write frame wrongly freed after call to audiohooks
......................................................................


channel: ast_write frame wrongly freed after call to audiohooks

ASTERISK-26419 introduced a bug when calling ast_audiohook_write_list in
ast_write. It would free the frame given to ast_write if the frame returned
by ast_audiohook_write_list was different than the given one. The frame
give to ast_write should never be freed within that function. It is the
caller's resposibility to free the frame after writing (or when it its done
with it). By freeing it within ast_write this of course led to some memory
corruption problems.

This patch makes it so the frame given to ast_write is no longer freed within
the function. The frame returned by ast_audiohook_write_list is now subsequently
used in ast_write and is freed later. It is freed either after translate if the
frame returned by translate is different, or near the end of ast_write prior
to function exit.

ASTERISK-26973 #close

Change-Id: I463d4ac3b736ced95de986ee74a489c7c7ab103b
---
M main/channel.c
1 file changed, 18 insertions(+), 10 deletions(-)

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



diff --git a/main/channel.c b/main/channel.c
index af49111..a4e1dd5 100644
--- a/main/channel.c
+++ b/main/channel.c
@@ -5234,26 +5234,21 @@
 			apply_plc(chan, fr);
 		}
 
+		f = fr;
+
 		/*
 		 * Send frame to audiohooks if present, if frametype is linear (else, later as per
 		 * previous behavior)
 		 */
 		if (ast_channel_audiohooks(chan)) {
 			if (ast_format_cache_is_slinear(fr->subclass.format)) {
-				struct ast_frame *old_frame;
 				hooked = 1;
-				old_frame = fr;
-				fr = ast_audiohook_write_list(chan, ast_channel_audiohooks(chan), AST_AUDIOHOOK_DIRECTION_WRITE, fr);
-				if (old_frame != fr) {
-					ast_frfree(old_frame);
-				}
+				f = ast_audiohook_write_list(chan, ast_channel_audiohooks(chan), AST_AUDIOHOOK_DIRECTION_WRITE, fr);
 			}
 		}
 
 		/* If the frame is in the raw write format, then it's easy... just use the frame - otherwise we will have to translate */
-		if (ast_format_cmp(fr->subclass.format, ast_channel_rawwriteformat(chan)) == AST_FORMAT_CMP_EQUAL) {
-			f = fr;
-		} else {
+		if (ast_format_cmp(fr->subclass.format, ast_channel_rawwriteformat(chan)) != AST_FORMAT_CMP_EQUAL) {
 			if (ast_format_cmp(ast_channel_writeformat(chan), fr->subclass.format) != AST_FORMAT_CMP_EQUAL) {
 				struct ast_str *codec_buf = ast_str_alloca(AST_FORMAT_CAP_NAMES_LEN);
 
@@ -5279,7 +5274,20 @@
 					break;
 				}
 			}
-			f = ast_channel_writetrans(chan) ? ast_translate(ast_channel_writetrans(chan), fr, 0) : fr;
+
+			if (ast_channel_writetrans(chan)) {
+				struct ast_frame *trans_frame = ast_translate(ast_channel_writetrans(chan), f, 0);
+
+				if (trans_frame != f && f != fr) {
+					/*
+					 * If translate gives us a new frame and so did the audio
+					 * hook then we need to free the one from the audio hook.
+					 */
+					ast_frfree(f);
+				}
+				f = trans_frame;
+			}
+
 		}
 
 		if (!f) {

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I463d4ac3b736ced95de986ee74a489c7c7ab103b
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 14
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>