diff mbox series

[FFmpeg-devel] avutil/video_enc_param: fix warning

Message ID 1590070874-8135-1-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel] avutil/video_enc_param: fix warning
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Limin Wang May 21, 2020, 2:21 p.m. UTC
From: Limin Wang <lance.lmwang@gmail.com>

warning: comparison is always false due to limited range of data type [-Wtype-limits]
Also nb_blocks is unsigned int, so nb_blocks * sizeof(AVVideoBlockParams) may overflow,
so force to size_t

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavutil/video_enc_params.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Nicolas George May 21, 2020, 2:34 p.m. UTC | #1
lance.lmwang@gmail.com (12020-05-21):
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> warning: comparison is always false due to limited range of data type [-Wtype-limits]

> Also nb_blocks is unsigned int, so nb_blocks * sizeof(AVVideoBlockParams) may overflow,
> so force to size_t

No it may not, the test just before prevents it.

> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavutil/video_enc_params.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavutil/video_enc_params.c b/libavutil/video_enc_params.c
> index c46c0f1..4a4c85f 100644
> --- a/libavutil/video_enc_params.c
> +++ b/libavutil/video_enc_params.c
> @@ -33,8 +33,8 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type,
>      size_t size;
>  
>      size = sizeof(*par);

> -    if (nb_blocks > SIZE_MAX / sizeof(AVVideoBlockParams) ||
> +    if (nb_blocks > UINT_MAX / sizeof(AVVideoBlockParams) ||

These tests are not equivalent.

> -        nb_blocks * sizeof(AVVideoBlockParams) > SIZE_MAX - size)
> +        (size_t)nb_blocks * sizeof(AVVideoBlockParams) > SIZE_MAX - size)

The cast is unnecessary due to C's promotion rules.

>          return NULL;
>      size += sizeof(AVVideoBlockParams) * nb_blocks;
>  

Regards,
Carl Eugen Hoyos May 21, 2020, 2:34 p.m. UTC | #2
Am Do., 21. Mai 2020 um 16:21 Uhr schrieb <lance.lmwang@gmail.com>:
>
> From: Limin Wang <lance.lmwang@gmail.com>
>
> warning: comparison is always false due to limited range of data type [-Wtype-limits]
> Also nb_blocks is unsigned int, so nb_blocks * sizeof(AVVideoBlockParams) may overflow,
> so force to size_t
>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavutil/video_enc_params.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libavutil/video_enc_params.c b/libavutil/video_enc_params.c
> index c46c0f1..4a4c85f 100644
> --- a/libavutil/video_enc_params.c
> +++ b/libavutil/video_enc_params.c
> @@ -33,8 +33,8 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type,
>      size_t size;
>
>      size = sizeof(*par);
> -    if (nb_blocks > SIZE_MAX / sizeof(AVVideoBlockParams) ||
> -        nb_blocks * sizeof(AVVideoBlockParams) > SIZE_MAX - size)
> +    if (nb_blocks > UINT_MAX / sizeof(AVVideoBlockParams) ||
> +        (size_t)nb_blocks * sizeof(AVVideoBlockParams) > SIZE_MAX - size)

I don't think this patch is ok.

Shouldn't this be an assert() about sizeof(AVVideoBlockParams) instead?

Carl Eugen
Limin Wang May 21, 2020, 2:46 p.m. UTC | #3
On Thu, May 21, 2020 at 04:34:39PM +0200, Nicolas George wrote:
> lance.lmwang@gmail.com (12020-05-21):
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > warning: comparison is always false due to limited range of data type [-Wtype-limits]
> 
> > Also nb_blocks is unsigned int, so nb_blocks * sizeof(AVVideoBlockParams) may overflow,
> > so force to size_t
> 
> No it may not, the test just before prevents it.
> 
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavutil/video_enc_params.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavutil/video_enc_params.c b/libavutil/video_enc_params.c
> > index c46c0f1..4a4c85f 100644
> > --- a/libavutil/video_enc_params.c
> > +++ b/libavutil/video_enc_params.c
> > @@ -33,8 +33,8 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type,
> >      size_t size;
> >  
> >      size = sizeof(*par);
> 
> > -    if (nb_blocks > SIZE_MAX / sizeof(AVVideoBlockParams) ||
> > +    if (nb_blocks > UINT_MAX / sizeof(AVVideoBlockParams) ||
> 
> These tests are not equivalent.
I'm not sure, Mac compile give below warning:

 warning: comparison of constant 922337203685477580 with expression of type 'unsigned int' is always false
      [-Wtautological-constant-out-of-range-compare]


> 
> > -        nb_blocks * sizeof(AVVideoBlockParams) > SIZE_MAX - size)
> > +        (size_t)nb_blocks * sizeof(AVVideoBlockParams) > SIZE_MAX - size)
> 
> The cast is unnecessary due to C's promotion rules.

Yes, It seems it's not necessary as the first test.

> 
> >          return NULL;
> >      size += sizeof(AVVideoBlockParams) * nb_blocks;
> >  
> 
> Regards,
> 
> -- 
>   Nicolas George
Nicolas George May 21, 2020, 2:50 p.m. UTC | #4
lance.lmwang@gmail.com (12020-05-21):
> I'm not sure, Mac compile give below warning:
> 
>  warning: comparison of constant 922337203685477580 with expression of type 'unsigned int' is always false
>       [-Wtautological-constant-out-of-range-compare]
> 

Certainly. And?

> > The cast is unnecessary due to C's promotion rules.
> Yes, It seems it's not necessary as the first test.

The cast is unnecessary, period. Do you know C's promotion rules in this
case?

What I am really asking, in both cases, is:

Do you understand what you are doing?

Regards,
Anton Khirnov May 21, 2020, 3:02 p.m. UTC | #5
> [FFmpeg-devel] [PATCH] avutil/video_enc_param: fix warning

This is wrong. We should not fix warnings, we should fix bugs. Warnings
suggest there may be a bug, but not all warnings are correct.

In this case, I believe the warnings is invalid and there is no problem
to fix. It's correct that the comparison is always false on some
platforms, but AFAIK no standard we adhere to guarantees that on all
platforms.
Nicolas George May 21, 2020, 3:10 p.m. UTC | #6
Anton Khirnov (12020-05-21):
> This is wrong. We should not fix warnings, we should fix bugs. Warnings
> suggest there may be a bug, but not all warnings are correct.
> 
> In this case, I believe the warnings is invalid and there is no problem
> to fix. It's correct that the comparison is always false on some
> platforms, but AFAIK no standard we adhere to guarantees that on all
> platforms.

Yet, we should strive to silence this warning, because invalid warnings
distract from valid ones.

It is tricky to do elegantly.

Maybe:

static inline int
check_overflow(size_t n, size_t s, size_t c)
{
    return n <= SIZE_MAX / s && n * s < SIZE_MAX - c;
}

It would avoid the warning because n is size_t instead of unsigned.

Regards,
Anton Khirnov May 22, 2020, 8:17 a.m. UTC | #7
Quoting Nicolas George (2020-05-21 17:10:00)
> Anton Khirnov (12020-05-21):
> > This is wrong. We should not fix warnings, we should fix bugs. Warnings
> > suggest there may be a bug, but not all warnings are correct.
> > 
> > In this case, I believe the warnings is invalid and there is no problem
> > to fix. It's correct that the comparison is always false on some
> > platforms, but AFAIK no standard we adhere to guarantees that on all
> > platforms.
> 
> Yet, we should strive to silence this warning, because invalid warnings
> distract from valid ones.
> 
> It is tricky to do elegantly.
> 
> Maybe:
> 
> static inline int
> check_overflow(size_t n, size_t s, size_t c)
> {
>     return n <= SIZE_MAX / s && n * s < SIZE_MAX - c;
> }
> 
> It would avoid the warning because n is size_t instead of unsigned.

I would lean towards disabling the warning instead.

It's only really useful for checking whether an unsigned value is
negative. In pretty much all other cases the warning is spurious because
even if the comparison is tautological on that specific system, it might
not be tautological elsewhere.
diff mbox series

Patch

diff --git a/libavutil/video_enc_params.c b/libavutil/video_enc_params.c
index c46c0f1..4a4c85f 100644
--- a/libavutil/video_enc_params.c
+++ b/libavutil/video_enc_params.c
@@ -33,8 +33,8 @@  AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type,
     size_t size;
 
     size = sizeof(*par);
-    if (nb_blocks > SIZE_MAX / sizeof(AVVideoBlockParams) ||
-        nb_blocks * sizeof(AVVideoBlockParams) > SIZE_MAX - size)
+    if (nb_blocks > UINT_MAX / sizeof(AVVideoBlockParams) ||
+        (size_t)nb_blocks * sizeof(AVVideoBlockParams) > SIZE_MAX - size)
         return NULL;
     size += sizeof(AVVideoBlockParams) * nb_blocks;