[FFmpeg-devel] [PATCH v4 1/1] libavformat/asfdec: A collection of related fixes for asfdec
Soft Works
softworkz at hotmail.com
Tue Sep 28 23:10:02 EEST 2021
Fix 1: Commit c8140fe7324f264faacf7395b27e12531d1f13f7 had
introduced a check for value_len > UINT16_MAX.
As a consequence, attached images of sizes larger than
UINT16_MAX could no longer be read.
Fix 2: The value_len is an uint32 not an int32 per spec. That
value must not be truncated, neither by casting to int, nor by any
conditional checks, because at the end of get_tag, this value is
needed to move forward in parsing. When the len value gets
modified, the parsing may break.
Fix 3: get_value had a return type of int, which means that reading
QWORDS was broken due to truncation.
Fix 4: Parsing of GUID values wasn't implemented
Fix 5: In get_tag, the code was adding 22 bytes (in order to allow
it to hold 64bit numbers as string) to the value len for creating
creating a buffer. This was unnecessarily imposing a
size-constraint on the value_len parameter.
Fix 6: The code in get_tag, was limiting the maximum value_len to
half the size of INT32. This was applied for all value types, even
though it is required only in case of ASF_UNICODE, not for any
other ones (like ASCII).
Fix 7: get_tag was always allocating a buffer regardless of the
datatype, even though this isn't required in case of ASF_BYTE_ARRAY
Fix 8: The spec allows attachment sizes of up to UINT32_MAX while
we can handle only sizes up to INT32_MAX. The debug.assert didn't
really address this, and truncating the value_len in calling
methods cannot be used as the length value is required in order to
continue parsing. I have added a check with log message in
ff_asf_handle_byte_array to handle this.
Signed-off-by: softworkz <softworkz at hotmail.com>
---
v4: Added line-breaks to commit message
libavformat/asf.c | 12 +++--
libavformat/asf.h | 2 +-
libavformat/asfdec_f.c | 115 +++++++++++++++++++++++++----------------
3 files changed, 81 insertions(+), 48 deletions(-)
diff --git a/libavformat/asf.c b/libavformat/asf.c
index 1ac8b5f078..179b66a2b4 100644
--- a/libavformat/asf.c
+++ b/libavformat/asf.c
@@ -267,12 +267,18 @@ static int get_id3_tag(AVFormatContext *s, int len)
}
int ff_asf_handle_byte_array(AVFormatContext *s, const char *name,
- int val_len)
+ uint32_t val_len)
{
+ if (val_len > INT32_MAX) {
+ av_log(s, AV_LOG_VERBOSE, "Unable to handle byte arrays > INT32_MAX in tag %s.\n", name);
+ return 1;
+ }
+
if (!strcmp(name, "WM/Picture")) // handle cover art
- return asf_read_picture(s, val_len);
+ return asf_read_picture(s, (int)val_len);
else if (!strcmp(name, "ID3")) // handle ID3 tag
- return get_id3_tag(s, val_len);
+ return get_id3_tag(s, (int)val_len);
+ av_log(s, AV_LOG_VERBOSE, "Unsupported byte array in tag %s.\n", name);
return 1;
}
diff --git a/libavformat/asf.h b/libavformat/asf.h
index 01cc4f7a46..4d28560f56 100644
--- a/libavformat/asf.h
+++ b/libavformat/asf.h
@@ -111,7 +111,7 @@ extern const AVMetadataConv ff_asf_metadata_conv[];
* is unsupported by this function and 0 otherwise.
*/
int ff_asf_handle_byte_array(AVFormatContext *s, const char *name,
- int val_len);
+ uint32_t val_len);
#define ASF_PACKET_FLAG_ERROR_CORRECTION_PRESENT 0x80 //1000 0000
diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c
index 72ba8b32a0..f0940b5b08 100644
--- a/libavformat/asfdec_f.c
+++ b/libavformat/asfdec_f.c
@@ -149,7 +149,7 @@ static const ff_asf_guid asf_audio_conceal_none = {
};
#define PRINT_IF_GUID(g, cmp) \
- if (!ff_guidcmp(g, &cmp)) \
+ if (!ff_guidcmp(g, (&cmp))) \
av_log(NULL, AV_LOG_TRACE, "(GUID: %s) ", # cmp)
static void print_guid(ff_asf_guid *g)
@@ -202,71 +202,100 @@ static int asf_probe(const AVProbeData *pd)
/* size of type 2 (BOOL) is 32bit for "Extended Content Description Object"
* but 16 bit for "Metadata Object" and "Metadata Library Object" */
-static int get_value(AVIOContext *pb, int type, int type2_size)
+static uint64_t get_value(AVIOContext *pb, int type, int type2_size)
{
switch (type) {
- case 2:
+ case ASF_BOOL:
return (type2_size == 32) ? avio_rl32(pb) : avio_rl16(pb);
- case 3:
+ case ASF_DWORD:
return avio_rl32(pb);
- case 4:
+ case ASF_QWORD:
return avio_rl64(pb);
- case 5:
+ case ASF_WORD:
return avio_rl16(pb);
default:
return INT_MIN;
}
}
-static void get_tag(AVFormatContext *s, const char *key, int type, int len, int type2_size)
+static void get_tag(AVFormatContext *s, const char *key, int type, uint32_t len, int type2_size)
{
ASFContext *asf = s->priv_data;
- char *value = NULL;
int64_t off = avio_tell(s->pb);
-#define LEN 22
-
- av_assert0((unsigned)len < (INT_MAX - LEN) / 2);
+ char *buffer = NULL;
+ uint64_t required_bufferlen;
+ int buffer_len;
if (!asf->export_xmp && !strncmp(key, "xmp", 3))
goto finish;
- value = av_malloc(2 * len + LEN);
- if (!value)
- goto finish;
-
switch (type) {
case ASF_UNICODE:
- avio_get_str16le(s->pb, len, value, 2 * len + 1);
+ required_bufferlen = (uint64_t)len * 2 + 1;
break;
- case -1: // ASCI
- avio_read(s->pb, value, len);
- value[len]=0;
+ case -1: // ASCII
+ required_bufferlen = (uint64_t)len + 1;
break;
case ASF_BYTE_ARRAY:
- if (ff_asf_handle_byte_array(s, key, len) > 0)
- av_log(s, AV_LOG_VERBOSE, "Unsupported byte array in tag %s.\n", key);
+ ff_asf_handle_byte_array(s, key, len);
+ goto finish;
+ case ASF_BOOL:
+ case ASF_DWORD:
+ case ASF_QWORD:
+ case ASF_WORD:
+ required_bufferlen = 22;
+ break;
+ case ASF_GUID:
+ required_bufferlen = 33;
+ break;
+ default:
+ required_bufferlen = len;
+ break;
+ }
+
+ if (required_bufferlen > INT32_MAX) {
+ av_log(s, AV_LOG_VERBOSE, "Unable to handle values > INT32_MAX in tag %s.\n", key);
+ goto finish;
+ }
+
+ buffer_len = (int)required_bufferlen;
+
+ buffer = av_malloc(buffer_len);
+ if (!buffer)
goto finish;
+
+ switch (type) {
+ case ASF_UNICODE:
+ avio_get_str16le(s->pb, len, buffer, buffer_len);
+ break;
+ case -1: // ASCII
+ avio_read(s->pb, buffer, len);
+ buffer[len]=0;
+ break;
case ASF_BOOL:
case ASF_DWORD:
case ASF_QWORD:
case ASF_WORD: {
uint64_t num = get_value(s->pb, type, type2_size);
- snprintf(value, LEN, "%"PRIu64, num);
+ snprintf(buffer, buffer_len, "%"PRIu64, num);
+ break;
+ }
+ case ASF_GUID: {
+ ff_asf_guid g;
+ ff_get_guid(s->pb, &g);
+ snprintf(buffer, buffer_len, "%x", g[0]);
break;
}
- case ASF_GUID:
- av_log(s, AV_LOG_DEBUG, "Unsupported GUID value in tag %s.\n", key);
- goto finish;
default:
av_log(s, AV_LOG_DEBUG,
"Unsupported value type %d in tag %s.\n", type, key);
goto finish;
}
- if (*value)
- av_dict_set(&s->metadata, key, value, 0);
+ if (*buffer)
+ av_dict_set(&s->metadata, key, buffer, 0);
finish:
- av_freep(&value);
+ av_freep(&buffer);
avio_seek(s->pb, off + len, SEEK_SET);
}
@@ -528,7 +557,7 @@ static int asf_read_ext_stream_properties(AVFormatContext *s, int64_t size)
static int asf_read_content_desc(AVFormatContext *s, int64_t size)
{
AVIOContext *pb = s->pb;
- int len1, len2, len3, len4, len5;
+ uint32_t len1, len2, len3, len4, len5;
len1 = avio_rl16(pb);
len2 = avio_rl16(pb);
@@ -568,9 +597,9 @@ static int asf_read_ext_content_desc(AVFormatContext *s, int64_t size)
* ASF stream count starts at 1. I am using 0 to the container value
* since it's unused. */
if (!strcmp(name, "AspectRatioX"))
- asf->dar[0].num = get_value(s->pb, value_type, 32);
+ asf->dar[0].num = (int)get_value(s->pb, value_type, 32);
else if (!strcmp(name, "AspectRatioY"))
- asf->dar[0].den = get_value(s->pb, value_type, 32);
+ asf->dar[0].den = (int)get_value(s->pb, value_type, 32);
else
get_tag(s, name, value_type, value_len, 32);
}
@@ -602,25 +631,23 @@ static int asf_read_metadata(AVFormatContext *s, int64_t size)
{
AVIOContext *pb = s->pb;
ASFContext *asf = s->priv_data;
- int n, stream_num, name_len_utf16, name_len_utf8, value_len;
+ int n, name_len_utf8;
+ uint16_t stream_num, name_len_utf16, value_type;
+ uint32_t value_len;
int ret, i;
n = avio_rl16(pb);
for (i = 0; i < n; i++) {
uint8_t *name;
- int value_type;
avio_rl16(pb); // lang_list_index
- stream_num = avio_rl16(pb);
- name_len_utf16 = avio_rl16(pb);
- value_type = avio_rl16(pb); /* value_type */
- value_len = avio_rl32(pb);
-
- if (value_len < 0 || value_len > UINT16_MAX)
- return AVERROR_INVALIDDATA;
+ stream_num = (uint16_t)avio_rl16(pb);
+ name_len_utf16 = (uint16_t)avio_rl16(pb);
+ value_type = (uint16_t)avio_rl16(pb); /* value_type */
+ value_len = avio_rl32(pb);
- name_len_utf8 = 2*name_len_utf16 + 1;
- name = av_malloc(name_len_utf8);
+ name_len_utf8 = 2 * name_len_utf16 + 1;
+ name = av_malloc(name_len_utf8);
if (!name)
return AVERROR(ENOMEM);
@@ -630,11 +657,11 @@ static int asf_read_metadata(AVFormatContext *s, int64_t size)
i, stream_num, name_len_utf16, value_type, value_len, name);
if (!strcmp(name, "AspectRatioX")){
- int aspect_x = get_value(s->pb, value_type, 16);
+ int aspect_x = (int)get_value(s->pb, value_type, 16);
if(stream_num < 128)
asf->dar[stream_num].num = aspect_x;
} else if(!strcmp(name, "AspectRatioY")){
- int aspect_y = get_value(s->pb, value_type, 16);
+ int aspect_y = (int)get_value(s->pb, value_type, 16);
if(stream_num < 128)
asf->dar[stream_num].den = aspect_y;
} else {
--
2.30.2.windows.1
More information about the ffmpeg-devel
mailing list