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

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Saturday, May 7, 2022 8:49 PM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at 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 at 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 at 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