From 1177d16193c6308c4cdffb9f0ea69ce731c8b1c1 Mon Sep 17 00:00:00 2001
From: Andre Noll <maan@tuebingen.mpg.de>
Date: Fri, 20 Aug 2021 14:23:04 +0200
Subject: [PATCH] mp4: Improve parse_tag().

* Merge tag_add_field() and read_string() into parse_tag() since they
are simple enough and have only one caller.

* Avoid memory leaks in the error case.

* Let the function return an error code (rather than -1) in all cases,
and check the return value in the callers.

* Add a sanity check for the subsize.

* Avoid creating two copies of the tag value.

* Rename the variable for the tag value.
---
 mp4.c | 85 ++++++++++++++++++++++++-----------------------------------
 1 file changed, 35 insertions(+), 50 deletions(-)

diff --git a/mp4.c b/mp4.c
index 479a2b69..02dd5a19 100644
--- a/mp4.c
+++ b/mp4.c
@@ -620,39 +620,6 @@ static int read_stsd(struct mp4 *f)
 	return 1;
 }
 
-static int32_t tag_add_field(struct mp4_metadata *meta, const char *item,
-		const char *value, int32_t len)
-{
-	meta->tags = para_realloc(meta->tags,
-		(meta->count + 1) * sizeof(struct mp4_tag));
-	meta->tags[meta->count].item = para_strdup(item);
-	meta->tags[meta->count].len = len;
-	if (len >= 0) {
-		meta->tags[meta->count].value = para_malloc(len + 1);
-		memcpy(meta->tags[meta->count].value, value, len);
-		meta->tags[meta->count].value[len] = 0;
-	} else {
-		meta->tags[meta->count].value = para_strdup(value);
-	}
-	meta->count++;
-	return 1;
-}
-
-static int read_string(struct mp4 *f, uint32_t length, char **result)
-{
-	char *str = para_malloc(length + 1);
-	int ret = read_data(f, str, length);
-
-	if (ret <= 0) {
-		free(str);
-		*result = NULL;
-	} else {
-		str[length] = '\0';
-		*result = str;
-	}
-	return ret;
-}
-
 static const char *get_metadata_name(uint8_t atom_type)
 {
 	switch (atom_type) {
@@ -669,9 +636,10 @@ static int parse_tag(struct mp4 *f, uint8_t parent, int32_t size)
 {
 	int ret;
 	uint64_t subsize, sumsize;
-	char *data = NULL;
+	char *value = NULL;
 	uint32_t len = 0;
 	uint64_t destpos;
+	struct mp4_tag *tag;
 
 	for (
 		sumsize = 0;
@@ -682,30 +650,43 @@ static int parse_tag(struct mp4 *f, uint8_t parent, int32_t size)
 		uint8_t header_size = 0;
 		ret = atom_read_header(f, &atom_type, &header_size, &subsize);
 		if (ret <= 0)
-			return ret;
+			goto fail;
 		destpos = get_position(f) + subsize - header_size;
 		if (atom_type != ATOM_DATA)
 			continue;
 		ret = read_int8(f, NULL); /* version */
 		if (ret <= 0)
-			return ret;
+			goto fail;
 		ret = read_int24(f, NULL); /* flags */
 		if (ret <= 0)
-			return ret;
+			goto fail;
 		ret = read_int32(f, NULL); /* reserved */
 		if (ret <= 0)
-			return ret;
-		free(data);
-		ret = read_string(f, subsize - (header_size + 8), &data);
-		if (ret <= 0)
-			return ret;
+			goto fail;
+		ret = -ERRNO_TO_PARA_ERROR(EINVAL);
+		if (subsize < header_size + 8 || subsize > UINT_MAX)
+			goto fail;
 		len = subsize - (header_size + 8);
+		free(value);
+		value = para_malloc(len + 1);
+		ret = read_data(f, value, len);
+		if (ret <= 0)
+			goto fail;
+		value[len] = '\0';
 	}
-	if (!data)
-		return -1;
-	tag_add_field(&f->meta, get_metadata_name(parent), data, len);
-	free(data);
+	if (!value)
+		return -ERRNO_TO_PARA_ERROR(EINVAL);
+	f->meta.tags = para_realloc(f->meta.tags, (f->meta.count + 1)
+		* sizeof(struct mp4_tag));
+	tag = f->meta.tags + f->meta.count;
+	tag->item = para_strdup(get_metadata_name(parent));
+	tag->value = value;
+	tag->len = len;
+	f->meta.count++;
 	return 1;
+fail:
+	free(value);
+	return ret;
 }
 
 static int read_mdhd(struct mp4 *f)
@@ -780,7 +761,9 @@ static int32_t read_ilst(struct mp4 *f, int32_t size)
 		case ATOM_ALBUM:
 		case ATOM_COMMENT:
 		case ATOM_DATE:
-			parse_tag(f, atom_type, subsize - header_size);
+			ret = parse_tag(f, atom_type, subsize - header_size);
+			if (ret <= 0)
+				return ret;
 		}
 		set_position(f, destpos);
 		sumsize += subsize;
@@ -807,9 +790,11 @@ static int32_t read_meta(struct mp4 *f, uint64_t size)
 			return ret;
 		if (subsize <= header_size + 4)
 			return 1;
-		if (atom_type == ATOM_ILST)
-			read_ilst(f, subsize - (header_size + 4));
-		else
+		if (atom_type == ATOM_ILST) {
+			ret = read_ilst(f, subsize - (header_size + 4));
+			if (ret <= 0)
+				return ret;
+		} else
 			set_position(f, get_position(f) + subsize - header_size);
 		sumsize += subsize;
 	}
-- 
2.39.2