| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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,
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
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
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,
> [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.
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,
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 --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;