diff mbox series

[FFmpeg-devel,v2] libavformat/asfdec: A collection of related fixes for asfdec

Message ID MN2PR04MB5981BD13603F38435D67E624BAF69@MN2PR04MB5981.namprd04.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel,v2] libavformat/asfdec: A collection of related fixes for asfdec
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Soft Works Aug. 9, 2021, 12:40 a.m. UTC
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@hotmail.com>
---
v2: Fix merge conflict
 libavformat/asf.c      |  12 +++--
 libavformat/asf.h      |   2 +-
 libavformat/asfdec_f.c | 115 +++++++++++++++++++++++++----------------
 3 files changed, 81 insertions(+), 48 deletions(-)
diff mbox series

Patch

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 ff6ddfb967..09040c4cb9 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);
 }
 
@@ -526,7 +555,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);
@@ -566,9 +595,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);
     }
@@ -600,25 +629,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);
 
@@ -628,11 +655,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 {