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

Nuo Mi nuomi2021 at gmail.com
Sun Feb 14 19:57:16 EET 2021


On Mon, Feb 15, 2021 at 1:39 AM Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:

> 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.
>
You are right, the count is unsigned int, but
https://github.com/FFmpeg/FFmpeg/blob/21346672270ae723aa774a9c8b0749954a75b3df/libavcodec/movtextenc.c#L112
tells us the value never > 16 bits. so the multiple will never overflow(if
we check "count <= UINT16_MAX" somewhere).


> > > > 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
> _______________________________________________
> 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".


More information about the ffmpeg-devel mailing list