[Asterisk-code-review] res_rtp_asterisk: Add frame list cleanups to ast_rtp_read (asterisk[16])

[George Joseph]

George Joseph has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/13412 )


Change subject: res_rtp_asterisk:  Add frame list cleanups to ast_rtp_read
......................................................................

res_rtp_asterisk:  Add frame list cleanups to ast_rtp_read

In Asterisk 16+, there are a few places in ast_rtp_read where we've
allocated a frame list but return a null frame instead of the list.
In these cases, any frames left in the list won't be freed.  In the
vast majority of the cases, the list is empty when we return so
there's nothing to free but there have been leaks reported in the
wild that can be traced back to frames left in the list before
returning.

The escape paths now all have logic to free frames left in the
list.

ASTERISK-28609
Reported by: Ted G

Change-Id: Ia1d7075857ebd26b47183c44b1aebb0d8f985f7a
---
M res/res_rtp_asterisk.c
1 file changed, 15 insertions(+), 6 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/12/13412/1

diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c
index c870fce..4429187 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -7520,6 +7520,7 @@
 
 	AST_LIST_HEAD_INIT_NOLOCK(&frames);
 
+
 	/* Only non-bundled instances can change/learn the remote's SSRC implicitly. */
 	if (!child && !AST_VECTOR_SIZE(&rtp->ssrc_mapping)) {
 		/* Force a marker bit and change SSRC if the SSRC changes */
@@ -7561,7 +7562,7 @@
 
 	if (!rtp->recv_buffer) {
 		/* If there is no receive buffer then we can pass back the frame directly */
-		return ast_rtp_interpret(instance, srtp, &addr, read_area, res, prev_seqno);
+		goto free_list_and_interpret;
 	} else if (rtp->expectedrxseqno == -1 || seqno == rtp->expectedrxseqno) {
 		rtp->expectedrxseqno = seqno + 1;
 
@@ -7569,7 +7570,7 @@
 		 * return it directly without duplicating it.
 		 */
 		if (!ast_data_buffer_count(rtp->recv_buffer)) {
-			return ast_rtp_interpret(instance, srtp, &addr, read_area, res, prev_seqno);
+			goto free_list_and_interpret;
 		}
 
 		if (!AST_VECTOR_REMOVE_CMP_ORDERED(&rtp->missing_seqno, seqno, find_by_value,
@@ -7582,7 +7583,7 @@
 		 * chance it will be overwritten.
 		 */
 		if (!ast_data_buffer_get(rtp->recv_buffer, seqno + 1)) {
-			return ast_rtp_interpret(instance, srtp, &addr, read_area, res, prev_seqno);
+			goto free_list_and_interpret;
 		}
 
 		/* Otherwise we need to dupe the frame so that the potential processing of frames placed after
@@ -7696,7 +7697,12 @@
 		AST_VECTOR_RESET(&rtp->missing_seqno, AST_VECTOR_ELEM_CLEANUP_NOOP);
 
 		return AST_LIST_FIRST(&frames);
-	} else if (seqno < rtp->expectedrxseqno) {
+	}
+
+	/* We're finished with the frames list */
+	ast_frame_free(AST_LIST_FIRST(&frames), 0);
+
+	if (seqno < rtp->expectedrxseqno) {
 		/* If this is a packet from the past then we have received a duplicate packet, so just drop it */
 		ast_debug(2, "Received an old packet with sequence number '%d' on RTP instance '%p', dropping it\n",
 			seqno, instance);
@@ -7807,11 +7813,14 @@
 				ast_rtcp_calculate_sr_rr_statistics(instance, rtcp_report, remote_address, ice, sr);
 			}
 		}
-
-		return &ast_null_frame;
 	}
 
 	return &ast_null_frame;
+
+free_list_and_interpret:
+	ast_frame_free(AST_LIST_FIRST(&frames), 0);
+	return ast_rtp_interpret(instance, srtp, &addr, read_area, res, prev_seqno);
+
 }
 
 /*! \pre instance is locked */

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

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Ia1d7075857ebd26b47183c44b1aebb0d8f985f7a
Gerrit-Change-Number: 13412
Gerrit-PatchSet: 1
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20191209/b04d5bd0/attachment.html>