[FFmpeg-devel] [PATCH] avcodec/movtextenc: fix compile warning for type-limits

[Carl Eugen Hoyos]

Am So., 14. Feb. 2021 um 18:15 Uhr schrieb Nuo Mi <nuomi2021 at gmail.com>:
>
> On Mon, Feb 15, 2021 at 1:05 AM Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
>
> > Am So., 14. Feb. 2021 um 17:30 Uhr schrieb Nuo Mi <nuomi2021 at gmail.com>:
> > >
> > > On Sun, Feb 14, 2021 at 6:35 PM Anton Khirnov <anton at khirnov.net> wrote:
> > >
> > > > Quoting Nuo Mi (2021-02-14 07:27:39)
> > > > > CC      libavcodec/mpegaudiodec_common.o
> > > > > libavcodec/movtextenc.c: In function ‘mov_text_style_start’:
> > > > > libavcodec/movtextenc.c:358:26: warning: comparison is always false
> > due
> > > > to limited range of data type [-Wtype-limits]
> > > > >   358 |         if (s->count + 1 > SIZE_MAX /
> > > > sizeof(*s->style_attributes) ||
> > > > > ---
> > > > >  libavcodec/movtextenc.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c
> > > > > index 1bef21e0b9..cd0e43a79b 100644
> > > > > --- a/libavcodec/movtextenc.c
> > > > > +++ b/libavcodec/movtextenc.c
> > > > > @@ -355,7 +355,7 @@ static int mov_text_style_start(MovTextContext
> > *s)
> > > > >          StyleBox *tmp;
> > > > >
> > > > >          // last style != defaults, end the style entry and start a
> > new
> > > > one
> > > > > -        if (s->count + 1 > SIZE_MAX / sizeof(*s->style_attributes)
> > ||
> > > > > +        if ((s->count + 1) *  sizeof(*s->style_attributes) >
> > SIZE_MAX ||
> > > >
> > > > What guarantees the multiplication does not overflow?
> > > >
> > > Hi Anton,
> > > Thanks for the review.
> > > It never overflows on 64bits system.
> >
> > But not every system is a 64bit system
> >
> > > This why the compiler has warning on
> > > my 64 bits system.
> >
> > Yes, we also see the warning on 64bit systems.
> >
> > > If you check
> > >
> > https://github.com/FFmpeg/FFmpeg/blob/21346672270ae723aa774a9c8b0749954a75b3df/libavcodec/movtextenc.c#L110
> > > s->count * sizeof(*s->style_attributes) never > 32 bits.

This is not correct afaict:
The relevant line is 369 not 110, count is of type unsigned and if you multiply
it with something >1, it can overflow.

> > > I can add some check for s->count check based on the code if you
> > > are preferred.
> >
> > Isn't this exactly the check that is already there?

> No, we only check the overflow, the overflow depends on the machine,

Yes.

> it never overflow on a 64 bits system.

Yes.

> And the limitation is not from the spec.

Yes.

> From this codec(
> https://github.com/FFmpeg/FFmpeg/blob/21346672270ae723aa774a9c8b0749954a75b3df/libavcodec/movtextenc.c#L112).
> s->count never > 16 bits.
> Instead of depends on system check, I think check s->count <= UINT16_MAX is
> more reasonable.

The rest of your comments do not look correct to me but I may miss
something.

I suspect that if it were simple to silence this warning it would be already
be silenced.

Carl Eugen