[FFmpeg-devel] [PATCH v5 2/7] libavformat/asfdec: Fix get_value return type

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Friday, 1 October 2021 19:56
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v5 2/7] libavformat/asfdec: Fix
> get_value return type
> 
> On Thu, Sep 30, 2021 at 12:11:13PM +0000, Soft Works wrote:
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> > > Michael Niedermayer
> > > Sent: Thursday, 30 September 2021 14:04
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel at ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH v5 2/7] libavformat/asfdec:
> Fix
> > > get_value return type
> > >
> > > On Thu, Sep 30, 2021 at 01:45:46PM +0200, Michael Niedermayer
> wrote:
> > > > On Thu, Sep 30, 2021 at 02:58:30AM +0000, Soft Works wrote:
> > > > > 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 at hotmail.com>
> > > > > ---
> > > > > v5: Split into pieces as requested
> > > > >
> > > > >  libavformat/asfdec_f.c | 10 +++++-----
> > > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c
> > > > > index 72ba8b32a0..076b5ab147 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 2:
> > > > > @@ -568,9 +568,9 @@ static int
> > > asf_read_ext_content_desc(AVFormatContext *s, int64_t size)
> > > > >           * 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);
> > > > > +            asf->dar[0].num = (int)get_value(s->pb,
> value_type,
> > > 32);
> > > > >          else if (!strcmp(name, "AspectRatioY"))
> > > > > -            asf->dar[0].den = get_value(s->pb, value_type,
> 32);
> > > > > +            asf->dar[0].den = (int)get_value(s->pb,
> value_type,
> > > 32);
> > > > >          else
> > > > >              get_tag(s, name, value_type, value_len, 32);
> > > > >      }
> > > > > @@ -630,11 +630,11 @@ 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);
> > > > > +            int aspect_x = (int)get_value(s->pb, value_type,
> > > 16);
> > > > >              if(stream_num < 128)
> > > > >                  asf->dar[stream_num].num = aspect_x;
> > > > >          } else if(!strcmp(name, "AspectRatioY")){
> > > > > -            int aspect_y = get_value(s->pb, value_type, 16);
> > > > > +            int aspect_y = (int)get_value(s->pb, value_type,
> > > 16);
> > > > >              if(stream_num < 128)
> > > > >                  asf->dar[stream_num].den = aspect_y;
> > > > >          } else {
> > > >
> > > > a 64bit/64bit aspect does not work after this, it still just
> > > silently
> > > > truncates the value to 32bit, the truncation just happens later
> > >
> > > forgot: see av_reduce()
> >
> > These lines are not about making any change. The get_value() calls
> > for AspectRatioX/Y are returning 16bit values, before and after.
> >
> > I added the explicit cast to (int) because get_value() doesn't
> return
> > int anymore and as such, to indicate that the cast is intentional.
> > But the values in this case are limited to 16bit integers anyway.
> 
> I dont see what would stop a file that encodes a 64bit aspect from
> being read as 64bit and then randomly and silently truncated
> neither before nor after this patch. The type is read and passed
> to get_value() its not checked or i have missed it

This is correct. AspectX/Y > INT32_MAX isn't handled correctly,
neither before nor after this patch.

> I dont understand why adding a explicit cast to int would improve
> anything.

Before this patch, the return type of get_value() was int, now it's
uint64_t. This causes a truncation of the return value that didn't
happen before (the truncation happened in get_value()). 

This leads to a compiler/linter warning that hasn't existed before.
Adding an explicit cast avoids that and indicates that the developer
was aware of the truncation. The less warnings a code file generates,
the better can a developer focus on those that might be actually 
relevant. That's why I've added the explicit cast.

> Either 64bit should be supported and converted to a supported

We can't do this because AVRational's .num and .den are of type int.

> ratio or it should produce some warning/error not silent truncation

The width and height fields in the ASF Specification are coded as 
32bit (see Advanced Systems Format Specification, Revision 1.20.03,
pages 82 and 85), so it would require a really weird encoder
that emits non-reduced AspectX/Y ration component values as QWORDS.

If we take such unlikely cases as a general measure, there would
be quite a number of additional checks that we would need to add.
This patchset deals with incorrect assumptions about realistic cases,
I can add checks for the less realistic ones as well, if you wish.

Thanks for looking into it,
softworkz