[FFmpeg-devel] [PATCH v5 3/7] libavformat/asfdec: Fix type of value_len

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> James Almer
> Sent: Thursday, 30 September 2021 05:53
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v5 3/7] libavformat/asfdec: Fix
> type of value_len
> 
> On 9/29/2021 11:58 PM, Soft Works wrote:
> > 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.
> >
> > Signed-off-by: softworkz <softworkz at hotmail.com>
> > ---
> > v5: Split into pieces as requested
> >
> >   libavformat/asfdec_f.c | 24 +++++++++++-------------
> >   1 file changed, 11 insertions(+), 13 deletions(-)
> >
> > diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c
> > index 076b5ab147..d017fae019 100644
> > --- a/libavformat/asfdec_f.c
> > +++ b/libavformat/asfdec_f.c
> > @@ -218,7 +218,7 @@ static uint64_t get_value(AVIOContext *pb, int
> type, int type2_size)
> >       }
> >   }
> >
> > -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)
> 
> There's an av_assert0() in this function to ensure len will never be
> bigger than (INT_MAX - 22) / 2. And len is used as argument for
> avio_read(), which takes an int, not an unsigned int. So this change
> is
> not ok.

The general situation is this:

The ASF spec allows sizes of various elements in its format up to a 
maximum of UINT32_MAX.

As you have correctly noted: we can't process those sizes because 
some of the internal functions are taking int32 parameters rather than
uint32. (e.g. with avio_read, get_id3_tag, asf_read_picture, ...)


This patch can't fix that part. But what it fixes is that such values
between INT32_MAX and UINT32_MAX won't break ASF parsing anymore.
We emit a message that there's some metadata value that we can't
read, but we skip over it and continue parsing.

That's one part of what this patch does.

Kind regards,
softworkz