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


You missed this part:

    if (required_bufferlen > INT32_MAX) {
        av_log(s, AV_LOG_VERBOSE, "Unable to handle values > INT32_MAX  in tag %s.\n", key);
        goto finish;
    }

which comes before avio_read.

goto finish doesn't mean to error out. It just means that value cannot 
be processed (even though it's valid from the ASF spec).

But parsing continues now without erroring, because now we can handle 
those values > INT32_MAX because we supply the correct value to
avio_seek() (the last statement).

Also:

The division by two applies to Unicode only, that's why I'm applying it
only to the Unicode case. The subtraction of '22' was not required, that's
why it's removed.


Kind regards,
softworkz