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

[Nuo Mi]

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.
>
> > 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?
>
Hi Carl,
thanks for the review.
No, we only check the overflow, the overflow depends on the machine, it
never overflow on a 64 bits system. And the limitation is not from the spec.
>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.


> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".