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

[Michael Niedermayer]

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

Either 64bit should be supported and converted to a supported
ratio or it should produce some warning/error not silent truncation

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

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Modern terrorism, a quick summary: Need oil, start war with country that
has oil, kill hundread thousand in war. Let country fall into chaos,
be surprised about raise of fundamantalists. Drop more bombs, kill more
people, be surprised about them taking revenge and drop even more bombs
and strip your own citizens of their rights and freedoms. to be continued
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20211001/7cbbe321/attachment.sig>