diff mbox series

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

Message ID 20210214062739.28181-1-nuomi2021@gmail.com
State New
Headers show
Series [FFmpeg-devel] avcodec/movtextenc: fix compile warning for type-limits | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Nuo Mi Feb. 14, 2021, 6:27 a.m. UTC 
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(-)

Comments

Anton Khirnov Feb. 14, 2021, 10:34 a.m. UTC | #1 
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?
Nuo Mi Feb. 14, 2021, 4:30 p.m. UTC | #2 
On Sun, Feb 14, 2021 at 6:35 PM Anton Khirnov <anton@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. This why the compiler has warning on
my 64 bits system.

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.


> --
> Anton Khirnov
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Carl Eugen Hoyos Feb. 14, 2021, 4:35 p.m. UTC | #3 
Am So., 14. Feb. 2021 um 17:30 Uhr schrieb Nuo Mi <nuomi2021@gmail.com>:
>
> On Sun, Feb 14, 2021 at 6:35 PM Anton Khirnov <anton@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?

Carl Eugen
Nuo Mi Feb. 14, 2021, 5:15 p.m. UTC | #4 
On Mon, Feb 15, 2021 at 1:05 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> Am So., 14. Feb. 2021 um 17:30 Uhr schrieb Nuo Mi <nuomi2021@gmail.com>:
> >
> > On Sun, Feb 14, 2021 at 6:35 PM Anton Khirnov <anton@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Carl Eugen Hoyos Feb. 14, 2021, 5:38 p.m. UTC | #5 
Am So., 14. Feb. 2021 um 18:15 Uhr schrieb Nuo Mi <nuomi2021@gmail.com>:
>
> On Mon, Feb 15, 2021 at 1:05 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> > Am So., 14. Feb. 2021 um 17:30 Uhr schrieb Nuo Mi <nuomi2021@gmail.com>:
> > >
> > > On Sun, Feb 14, 2021 at 6:35 PM Anton Khirnov <anton@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
Nuo Mi Feb. 14, 2021, 5:57 p.m. UTC | #6 
On Mon, Feb 15, 2021 at 1:39 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> Am So., 14. Feb. 2021 um 18:15 Uhr schrieb Nuo Mi <nuomi2021@gmail.com>:
> >
> > On Mon, Feb 15, 2021 at 1:05 AM Carl Eugen Hoyos <ceffmpeg@gmail.com>
> wrote:
> >
> > > Am So., 14. Feb. 2021 um 17:30 Uhr schrieb Nuo Mi <nuomi2021@gmail.com
> >:
> > > >
> > > > On Sun, Feb 14, 2021 at 6:35 PM Anton Khirnov <anton@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Carl Eugen Hoyos Feb. 14, 2021, 6:01 p.m. UTC | #7 
Am So., 14. Feb. 2021 um 18:57 Uhr schrieb Nuo Mi <nuomi2021@gmail.com>:

> > 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

No, this is not correct:
Line 112 does not know how often line 369 was called.

Carl Eugen
Nuo Mi Feb. 14, 2021, 6:08 p.m. UTC | #8 
On Mon, Feb 15, 2021 at 2:02 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> Am So., 14. Feb. 2021 um 18:57 Uhr schrieb Nuo Mi <nuomi2021@gmail.com>:
>
> > >
> 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
>
> No, this is not correct:
> Line 112 does not know how often line 369 was called.
>
Yes, we can check s->count <= UINT16_MAX before 369. It will make sure we
never overflow.

>
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Andreas Rheinhardt Feb. 14, 2021, 6:15 p.m. UTC | #9 
Nuo Mi:
> On Mon, Feb 15, 2021 at 2:02 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 
>> Am So., 14. Feb. 2021 um 18:57 Uhr schrieb Nuo Mi <nuomi2021@gmail.com>:
>>
>>>>
>> 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
>>
>> No, this is not correct:
>> Line 112 does not know how often line 369 was called.
>>
> Yes, we can check s->count <= UINT16_MAX before 369. It will make sure we
> never overflow.
> 
No, it doesn't. There is nothing that guarantees that UINT16_MAX *
sizeof(StyleBox) is representable in a size_t. (Remember the compiler
can add arbitrary padding in a struct.)
But it is nevertheless possible to avoid these warnings on Clang; GCC is
a bit stupider, though, and I see no way to avoid the warnings for GCC
(the problem is that sizeof isn't available to the preprocessor, so one
can't #if this away). Will send patches soon.

- Andreas
diff mbox series

Patch

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 ||
             !(tmp = av_fast_realloc(s->style_attributes,
                                     &s->style_attributes_bytes_allocated,
                                     (s->count + 1) * sizeof(*s->style_attributes)))) {