[Asterisk-code-review] frame.c: Fix off-nominal format ref leaks. (asterisk[14])

[George Joseph]

George Joseph has submitted this change and it was merged. ( https://gerrit.asterisk.org/4793 )

Change subject: frame.c: Fix off-nominal format ref leaks.
......................................................................


frame.c: Fix off-nominal format ref leaks.

* ast_frisolate() could leak frame format refs on allocation
failures.

* Similified code in ast_frisolate() and code used by
ast_frisolate().

Change-Id: I79566d4d36b3d7801bf0c8294fcd3e9a86a2ed6d
---
M main/frame.c
1 file changed, 43 insertions(+), 26 deletions(-)

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



diff --git a/main/frame.c b/main/frame.c
index 92b92b6..8334080 100644
--- a/main/frame.c
+++ b/main/frame.c
@@ -84,9 +84,9 @@
 	if ((frames = ast_threadstorage_get(&frame_cache, sizeof(*frames)))) {
 		if ((f = AST_LIST_REMOVE_HEAD(&frames->list, frame_list))) {
 			size_t mallocd_len = f->mallocd_hdr_len;
+
 			memset(f, 0, sizeof(*f));
 			f->mallocd_hdr_len = mallocd_len;
-			f->mallocd = AST_MALLOCD_HDR;
 			frames->size--;
 			return f;
 		}
@@ -141,12 +141,12 @@
 #endif
 
 	if (fr->mallocd & AST_MALLOCD_DATA) {
-		if (fr->data.ptr)
+		if (fr->data.ptr) {
 			ast_free(fr->data.ptr - fr->offset);
+		}
 	}
 	if (fr->mallocd & AST_MALLOCD_SRC) {
-		if (fr->src)
-			ast_free((void *) fr->src);
+		ast_free((void *) fr->src);
 	}
 	if (fr->mallocd & AST_MALLOCD_HDR) {
 		if ((fr->frametype == AST_FRAME_VOICE) || (fr->frametype == AST_FRAME_VIDEO) ||
@@ -208,14 +208,14 @@
 			return NULL;
 		}
 		out->frametype = fr->frametype;
+		out->subclass = fr->subclass;
 		if ((fr->frametype == AST_FRAME_VOICE) || (fr->frametype == AST_FRAME_VIDEO) ||
 			(fr->frametype == AST_FRAME_IMAGE)) {
-			out->subclass.format = ao2_bump(fr->subclass.format);
-		} else {
-			memcpy(&out->subclass, &fr->subclass, sizeof(out->subclass));
+			ao2_bump(out->subclass.format);
 		}
 		out->datalen = fr->datalen;
 		out->samples = fr->samples;
+		out->mallocd = AST_MALLOCD_HDR;
 		out->offset = fr->offset;
 		/* Copy the timing data */
 		ast_copy_flags(out, fr, AST_FLAGS_ALL);
@@ -228,46 +228,63 @@
 		out = fr;
 	}
 
-	if (!(fr->mallocd & AST_MALLOCD_SRC) && fr->src) {
-		if (!(out->src = ast_strdup(fr->src))) {
-			if (out != fr) {
-				ast_free(out);
+	if (fr->src) {
+		/* The original frame has a source string */
+		if (!(fr->mallocd & AST_MALLOCD_SRC)) {
+			/*
+			 * The original frame has a non-malloced source string.
+			 *
+			 * Duplicate the string and put it into the isolated frame
+			 * which may also be the original frame.
+			 */
+			newdata = ast_strdup(fr->src);
+			if (!newdata) {
+				if (out != fr) {
+					ast_frame_free(out, 0);
+				}
+				return NULL;
 			}
-			return NULL;
+			out->src = newdata;
+			out->mallocd |= AST_MALLOCD_SRC;
+		} else if (out != fr) {
+			/* Steal the source string from the original frame. */
+			out->src = fr->src;
+			fr->src = NULL;
+			fr->mallocd &= ~AST_MALLOCD_SRC;
+			out->mallocd |= AST_MALLOCD_SRC;
 		}
-	} else {
-		out->src = fr->src;
-		fr->src = NULL;
-		fr->mallocd &= ~AST_MALLOCD_SRC;
 	}
 
 	if (!(fr->mallocd & AST_MALLOCD_DATA))  {
+		/* The original frame has a non-malloced data buffer. */
 		if (!fr->datalen) {
+			/* Actually it's just an int so we can simply copy it. */
 			out->data.uint32 = fr->data.uint32;
-			out->mallocd = AST_MALLOCD_HDR | AST_MALLOCD_SRC;
 			return out;
 		}
-		if (!(newdata = ast_malloc(fr->datalen + AST_FRIENDLY_OFFSET))) {
-			if (out->src != fr->src) {
-				ast_free((void *) out->src);
-			}
+		/*
+		 * Duplicate the data buffer and put it into the isolated frame
+		 * which may also be the original frame.
+		 */
+		newdata = ast_malloc(fr->datalen + AST_FRIENDLY_OFFSET);
+		if (!newdata) {
 			if (out != fr) {
-				ast_free(out);
+				ast_frame_free(out, 0);
 			}
 			return NULL;
 		}
 		newdata += AST_FRIENDLY_OFFSET;
 		out->offset = AST_FRIENDLY_OFFSET;
-		out->datalen = fr->datalen;
 		memcpy(newdata, fr->data.ptr, fr->datalen);
 		out->data.ptr = newdata;
-	} else {
+		out->mallocd |= AST_MALLOCD_DATA;
+	} else if (out != fr) {
+		/* Steal the data buffer from the original frame. */
 		out->data = fr->data;
 		memset(&fr->data, 0, sizeof(fr->data));
 		fr->mallocd &= ~AST_MALLOCD_DATA;
+		out->mallocd |= AST_MALLOCD_DATA;
 	}
-
-	out->mallocd = AST_MALLOCD_HDR | AST_MALLOCD_SRC | AST_MALLOCD_DATA;
 
 	return out;
 }

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

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