diff mbox series

[FFmpeg-devel,v2,02/11] libavformat/asfdec: fix get_value return type and add checks for

Message ID a35b7c87d4548c761234b3e4fef60412f3d07837.1651916204.git.ffmpegagent@gmail.com
State New
Headers show
Series libavformat/asf: fix handling of byte array length values | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Aman Karmani May 7, 2022, 9:36 a.m. UTC 
From: softworkz <softworkz@hotmail.com>

unsupported values

get_value had a return type of int, which means that reading
QWORDS (case 4) was broken due to truncation of the result from
avio_rl64().

Signed-off-by: softworkz <softworkz@hotmail.com>
---
 libavformat/asfdec_f.c | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

Comments

Michael Niedermayer May 7, 2022, 6:57 p.m. UTC | #1 
On Sat, May 07, 2022 at 09:36:35AM +0000, softworkz wrote:
> From: softworkz <softworkz@hotmail.com>
> 
> unsupported values
> 
> get_value had a return type of int, which means that reading
> QWORDS (case 4) was broken due to truncation of the result from
> avio_rl64().
> 
> Signed-off-by: softworkz <softworkz@hotmail.com>
> ---
>  libavformat/asfdec_f.c | 38 +++++++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c
> index a8f36ed286..d31e1d581d 100644
> --- a/libavformat/asfdec_f.c
> +++ b/libavformat/asfdec_f.c
> @@ -202,7 +202,7 @@ 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 ASF_BOOL:
> @@ -567,10 +567,22 @@ static int asf_read_ext_content_desc(AVFormatContext *s, int64_t size)
>          /* My sample has that stream set to 0 maybe that mean the container.
>           * 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);
> -        else if (!strcmp(name, "AspectRatioY"))
> -            asf->dar[0].den = get_value(s->pb, value_type, 32);
> +        if (!strcmp(name, "AspectRatioX")) {
> +            const uint64_t value = get_value(s->pb, value_type, 32);
> +            if (value > INT32_MAX) {
> +                av_log(s, AV_LOG_DEBUG, "Unsupported AspectRatioX value: %"PRIu64"\n", value);
> +                return AVERROR(ENOTSUP);
> +            }
> +            asf->dar[0].num = (int)value;
> +        }
> +        else if (!strcmp(name, "AspectRatioY")) {
> +            const uint64_t value = get_value(s->pb, value_type, 32);
> +            if (value > INT32_MAX) {
> +                av_log(s, AV_LOG_DEBUG, "Unsupported AspectRatioY value: %"PRIu64"\n", value);
> +                return AVERROR(ENOTSUP);
> +            }
> +            asf->dar[0].den = (int)value;
> +        }
>          else
>              get_tag(s, name, value_type, value_len, 32);
>      }
> @@ -630,13 +642,21 @@ 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);
> +            const uint64_t aspect_x = get_value(s->pb, value_type, 16);
> +            if (aspect_x > INT32_MAX) {
> +                av_log(s, AV_LOG_DEBUG, "Unsupported AspectRatioX value: %"PRIu64"\n", aspect_x);
> +                return AVERROR(ENOTSUP);
> +            }
>              if(stream_num < 128)
> -                asf->dar[stream_num].num = aspect_x;
> +                asf->dar[stream_num].num = (int)aspect_x;
>          } else if(!strcmp(name, "AspectRatioY")){
> -            int aspect_y = get_value(s->pb, value_type, 16);
> +            const uint64_t aspect_y = get_value(s->pb, value_type, 16);
> +            if (aspect_y > INT32_MAX) {
> +                av_log(s, AV_LOG_DEBUG, "Unsupported AspectRatioY value: %"PRIu64"\n", aspect_y);
> +                return AVERROR(ENOTSUP);
> +            }
>              if(stream_num < 128)
> -                asf->dar[stream_num].den = aspect_y;
> +                asf->dar[stream_num].den = (int)aspect_y;
>          } else {

If you go to the length to do something with oddly huge aspect components
maybe change dar to 2 uint64_t and check it in one place instead of 2
also the av_reduce() can handle a wider range than int32

thx

[...]
diff mbox series

Patch

diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c
index a8f36ed286..d31e1d581d 100644
--- a/libavformat/asfdec_f.c
+++ b/libavformat/asfdec_f.c
@@ -202,7 +202,7 @@  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 ASF_BOOL:
@@ -567,10 +567,22 @@  static int asf_read_ext_content_desc(AVFormatContext *s, int64_t size)
         /* My sample has that stream set to 0 maybe that mean the container.
          * 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);
-        else if (!strcmp(name, "AspectRatioY"))
-            asf->dar[0].den = get_value(s->pb, value_type, 32);
+        if (!strcmp(name, "AspectRatioX")) {
+            const uint64_t value = get_value(s->pb, value_type, 32);
+            if (value > INT32_MAX) {
+                av_log(s, AV_LOG_DEBUG, "Unsupported AspectRatioX value: %"PRIu64"\n", value);
+                return AVERROR(ENOTSUP);
+            }
+            asf->dar[0].num = (int)value;
+        }
+        else if (!strcmp(name, "AspectRatioY")) {
+            const uint64_t value = get_value(s->pb, value_type, 32);
+            if (value > INT32_MAX) {
+                av_log(s, AV_LOG_DEBUG, "Unsupported AspectRatioY value: %"PRIu64"\n", value);
+                return AVERROR(ENOTSUP);
+            }
+            asf->dar[0].den = (int)value;
+        }
         else
             get_tag(s, name, value_type, value_len, 32);
     }
@@ -630,13 +642,21 @@  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);
+            const uint64_t aspect_x = get_value(s->pb, value_type, 16);
+            if (aspect_x > INT32_MAX) {
+                av_log(s, AV_LOG_DEBUG, "Unsupported AspectRatioX value: %"PRIu64"\n", aspect_x);
+                return AVERROR(ENOTSUP);
+            }
             if(stream_num < 128)
-                asf->dar[stream_num].num = aspect_x;
+                asf->dar[stream_num].num = (int)aspect_x;
         } else if(!strcmp(name, "AspectRatioY")){
-            int aspect_y = get_value(s->pb, value_type, 16);
+            const uint64_t aspect_y = get_value(s->pb, value_type, 16);
+            if (aspect_y > INT32_MAX) {
+                av_log(s, AV_LOG_DEBUG, "Unsupported AspectRatioY value: %"PRIu64"\n", aspect_y);
+                return AVERROR(ENOTSUP);
+            }
             if(stream_num < 128)
-                asf->dar[stream_num].den = aspect_y;
+                asf->dar[stream_num].den = (int)aspect_y;
         } else {
             get_tag(s, name, value_type, value_len, 16);
         }