diff mbox series

[FFmpeg-devel,v2,01/11] libavformat/asf: fix handling of byte array length values

Message ID 0056a93a347829e72cd6d09d48062978ca4ac6e0.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

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

The spec allows attachment sizes of up to UINT32_MAX while
we can handle only sizes up to INT32_MAX (in downstream
code)

The debug.assert in get_tag didn't really address this,
and truncating the value_len in calling methods cannot
be used because the length value is required in order to
continue parsing. This adds a check with log message in
ff_asf_handle_byte_array to handle those (rare) cases.

Signed-off-by: softworkz <softworkz@hotmail.com>
---
 libavformat/asf.c | 12 +++++++++---
 libavformat/asf.h |  2 +-
 2 files changed, 10 insertions(+), 4 deletions(-)

Comments

Michael Niedermayer May 7, 2022, 6:48 p.m. UTC | #1
On Sat, May 07, 2022 at 09:36:34AM +0000, softworkz wrote:
> From: softworkz <softworkz@hotmail.com>
> 
> The spec allows attachment sizes of up to UINT32_MAX while
> we can handle only sizes up to INT32_MAX (in downstream
> code)
> 
> The debug.assert in get_tag didn't really address this,
> and truncating the value_len in calling methods cannot
> be used because the length value is required in order to
> continue parsing. This adds a check with log message in
> ff_asf_handle_byte_array to handle those (rare) cases.
> 
> Signed-off-by: softworkz <softworkz@hotmail.com>
> ---
>  libavformat/asf.c | 12 +++++++++---
>  libavformat/asf.h |  2 +-
>  2 files changed, 10 insertions(+), 4 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);

unneeded


>  
> +    av_log(s, AV_LOG_VERBOSE, "Unsupported byte array in tag %s.\n", name);

Probably this should be DEBUG


thx

[...]
Soft Works May 8, 2022, 2:27 a.m. UTC | #2
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Saturday, May 7, 2022 8:49 PM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v2 01/11] libavformat/asf: fix
> handling of byte array length values
> 
> On Sat, May 07, 2022 at 09:36:34AM +0000, softworkz wrote:
> > From: softworkz <softworkz@hotmail.com>
> >
> > The spec allows attachment sizes of up to UINT32_MAX while
> > we can handle only sizes up to INT32_MAX (in downstream
> > code)
> >
> > The debug.assert in get_tag didn't really address this,
> > and truncating the value_len in calling methods cannot
> > be used because the length value is required in order to
> > continue parsing. This adds a check with log message in
> > ff_asf_handle_byte_array to handle those (rare) cases.
> >
> > Signed-off-by: softworkz <softworkz@hotmail.com>
> > ---
> >  libavformat/asf.c | 12 +++++++++---
> >  libavformat/asf.h |  2 +-
> >  2 files changed, 10 insertions(+), 4 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);
> 
> unneeded

Hi Michael,

thanks a lot for reviewing!

I think we had talked about this a while ago. From my point of view,
that explicit cast to int, tells me, every other reader of the code
as well as any static analysis or linting tools that the developer
has been aware of the data type mismatch between supplied variable
and the parameter type and that the conversion is intended rather
than accidental.
But I don't want to insist - I have removed it.


 
> >
> > +    av_log(s, AV_LOG_VERBOSE, "Unsupported byte array in tag
> %s.\n", name);
> 
> Probably this should be DEBUG

The problem with DEBUG is that some components are spitting out
so many lines with log level DEBUG that you'd hardly ever see it
unless you'd explicitly search for that exact line, while that
line is rather of the kind that you don't expect it.

I've changed it to DEBUG, though.


> >          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

Good idea, didn't know that av_reduce() can take larger numbers.
Done.


Thanks again,
softworkz
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