diff mbox series

[FFmpeg-devel] avutils/video_enc_params: remove unnecessary check to fix compile warning

Message ID 20210214062656.27221-1-nuomi2021@gmail.com
State New
Headers show
Series [FFmpeg-devel] avutils/video_enc_params: remove unnecessary check to fix compile warning | 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:26 a.m. UTC 
This will fix following compile warning:

libavutil/video_enc_params.c: In function ‘av_video_enc_params_alloc’:                                                                                                                                                                     libavutil/video_enc_params.c:36:19: warning: comparison is always false due to limited range of data type [-Wtype-limits]                                                                                                                       36 |     if (nb_blocks > SIZE_MAX / sizeof(AVVideoBlockParams) ||                                                                                                                                                                               |                   ^

Suppose a is "nb_blocks > SIZE_MAX / sizeof(AVVideoBlockParams))"
b is "nb_blocks * sizeof(AVVideoBlockParams) > SIZE_MAX - sizw"
If a is true, b is true.
If a is false, the expression depends on b.
No matter a is true or not, it we only need check b.
---
 libavutil/video_enc_params.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Anton Khirnov Feb. 14, 2021, 10:37 a.m. UTC | #1 
Quoting Nuo Mi (2021-02-14 07:26:56)
> This will fix following compile warning:
> 
> libavutil/video_enc_params.c: In function ‘av_video_enc_params_alloc’:                                                                                                                                                                     libavutil/video_enc_params.c:36:19: warning: comparison is always false due to limited range of data type [-Wtype-limits]                                                                                                                       36 |     if (nb_blocks > SIZE_MAX / sizeof(AVVideoBlockParams) ||                                                                                                                                                                               |                   ^
> 
> Suppose a is "nb_blocks > SIZE_MAX / sizeof(AVVideoBlockParams))"
> b is "nb_blocks * sizeof(AVVideoBlockParams) > SIZE_MAX - sizw"
> If a is true, b is true.
> If a is false, the expression depends on b.
> No matter a is true or not, it we only need check b.

Check a is needed to prevent overflow in b.

IMO the warning is bogus and we should consider disabling it.
Nuo Mi Feb. 14, 2021, 4:57 p.m. UTC | #2 
On Mon, Feb 15, 2021 at 12:41 AM Nuo Mi <nuomi2021@gmail.com> wrote:

> This will fix following compile warning:
>
>     libavutil/video_enc_params.c: In function ‘av_video_enc_params_alloc:
>
>
>            libavutil/video_enc_params.c:36:19: warning: comparison is
> always false due to limited range of data type [-Wtype-limits]
>
>                              36 |     if (nb_blocks > SIZE_MAX /
> sizeof(AVVideoBlockParams) ||
>
>                                                      |                   ^
> ---
>  libavutil/video_enc_params.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/libavutil/video_enc_params.c b/libavutil/video_enc_params.c
> index c46c0f1dc6..2606b5589a 100644
> --- a/libavutil/video_enc_params.c
> +++ b/libavutil/video_enc_params.c
> @@ -33,7 +33,11 @@ AVVideoEncParams *av_video_enc_params_alloc(enum
> AVVideoEncParamsType type,
>      size_t size;
>
>      size = sizeof(*par);
> -    if (nb_blocks > SIZE_MAX / sizeof(AVVideoBlockParams) ||
> +    if (
> +#if SIZE_MAX <= UINT_MAX
> +        //check the overflow
> +        nb_blocks > SIZE_MAX / sizeof(AVVideoBlockParams) ||
> +#endif
>          nb_blocks * sizeof(AVVideoBlockParams) > SIZE_MAX - size)
>          return NULL;
>      size += sizeof(AVVideoBlockParams) * nb_blocks;

 Hi Anton,
The code never overflow on a 64 bits machine.
How about we add a #ifdef like this?

thanks

>
Carl Eugen Hoyos Feb. 14, 2021, 5:10 p.m. UTC | #3 
Am So., 14. Feb. 2021 um 17:57 Uhr schrieb Nuo Mi <nuomi2021@gmail.com>:
>
> On Mon, Feb 15, 2021 at 12:41 AM Nuo Mi <nuomi2021@gmail.com> wrote:
>
> > This will fix following compile warning:
> >
> >     libavutil/video_enc_params.c: In function ‘av_video_enc_params_alloc:
> >
> >
> >            libavutil/video_enc_params.c:36:19: warning: comparison is
> > always false due to limited range of data type [-Wtype-limits]
> >
> >                              36 |     if (nb_blocks > SIZE_MAX /
> > sizeof(AVVideoBlockParams) ||
> >
> >                                                      |                   ^
> > ---
> >  libavutil/video_enc_params.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavutil/video_enc_params.c b/libavutil/video_enc_params.c
> > index c46c0f1dc6..2606b5589a 100644
> > --- a/libavutil/video_enc_params.c
> > +++ b/libavutil/video_enc_params.c
> > @@ -33,7 +33,11 @@ AVVideoEncParams *av_video_enc_params_alloc(enum
> > AVVideoEncParamsType type,
> >      size_t size;
> >
> >      size = sizeof(*par);
> > -    if (nb_blocks > SIZE_MAX / sizeof(AVVideoBlockParams) ||
> > +    if (
> > +#if SIZE_MAX <= UINT_MAX
> > +        //check the overflow
> > +        nb_blocks > SIZE_MAX / sizeof(AVVideoBlockParams) ||
> > +#endif
> >          nb_blocks * sizeof(AVVideoBlockParams) > SIZE_MAX - size)
> >          return NULL;
> >      size += sizeof(AVVideoBlockParams) * nb_blocks;

Apart from the ugliness (that may be unavoidable, I get the warning
here for both "if (0 && condition)" and "if (0) if (condition)"), the
comment is unnecessary.

Not sure if the condition is guaranteed to be sufficient, a sizeof() trick
might be necessary.

Carl Eugen
diff mbox series

Patch

diff --git a/libavutil/video_enc_params.c b/libavutil/video_enc_params.c
index c46c0f1dc6..31bce6a277 100644
--- a/libavutil/video_enc_params.c
+++ b/libavutil/video_enc_params.c
@@ -33,8 +33,7 @@  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 * sizeof(AVVideoBlockParams) > SIZE_MAX - size)
         return NULL;
     size += sizeof(AVVideoBlockParams) * nb_blocks;