diff mbox series

[FFmpeg-devel] avutil/mem: use GCC builtins to check for overflow in av_size_mult()

Message ID 20210527150657.1199-1-jamrial@gmail.com
State Accepted
Commit baf5cc5b7a016efaa0d9bf3dfe62ad74ef4ffafe
Headers show
Series [FFmpeg-devel] avutil/mem: use GCC builtins to check for overflow in av_size_mult() | 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

James Almer May 27, 2021, 3:06 p.m. UTC 
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavutil/mem.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

James Almer May 30, 2021, 12:31 p.m. UTC | #1 
On 5/27/2021 12:06 PM, James Almer wrote:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>   libavutil/mem.h | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/libavutil/mem.h b/libavutil/mem.h
> index e21a1feaae..c876111afb 100644
> --- a/libavutil/mem.h
> +++ b/libavutil/mem.h
> @@ -674,11 +674,18 @@ void *av_dynarray2_add(void **tab_ptr, int *nb_ptr, size_t elem_size,
>    */
>   static inline int av_size_mult(size_t a, size_t b, size_t *r)
>   {
> -    size_t t = a * b;
> +    size_t t;
> +
> +#if (!defined(__INTEL_COMPILER) && AV_GCC_VERSION_AT_LEAST(5,1)) || AV_HAS_BUILTIN(__builtin_mul_overflow)
> +    if (__builtin_mul_overflow(a, b, &t))
> +        return AVERROR(EINVAL);
> +#else
> +    t = a * b;
>       /* Hack inspired from glibc: don't try the division if nelem and elsize
>        * are both less than sqrt(SIZE_MAX). */
>       if ((a | b) >= ((size_t)1 << (sizeof(size_t) * 4)) && a && t / a != b)
>           return AVERROR(EINVAL);
> +#endif
>       *r = t;
>       return 0;
>   }

Will apply.
Anton Khirnov May 31, 2021, 8:54 a.m. UTC | #2 
Quoting James Almer (2021-05-27 17:06:57)
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavutil/mem.h | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/libavutil/mem.h b/libavutil/mem.h
> index e21a1feaae..c876111afb 100644
> --- a/libavutil/mem.h
> +++ b/libavutil/mem.h
> @@ -674,11 +674,18 @@ void *av_dynarray2_add(void **tab_ptr, int *nb_ptr, size_t elem_size,
>   */
>  static inline int av_size_mult(size_t a, size_t b, size_t *r)
>  {
> -    size_t t = a * b;
> +    size_t t;
> +
> +#if (!defined(__INTEL_COMPILER) && AV_GCC_VERSION_AT_LEAST(5,1)) || AV_HAS_BUILTIN(__builtin_mul_overflow)
> +    if (__builtin_mul_overflow(a, b, &t))
> +        return AVERROR(EINVAL);
> +#else
> +    t = a * b;
>      /* Hack inspired from glibc: don't try the division if nelem and elsize
>       * are both less than sqrt(SIZE_MAX). */
>      if ((a | b) >= ((size_t)1 << (sizeof(size_t) * 4)) && a && t / a != b)
>          return AVERROR(EINVAL);
> +#endif
>      *r = t;
>      return 0;
>  }
> -- 
> 2.31.1

This is starting to look way too ugly for something exposed in a public
header.
Nicolas George May 31, 2021, 8:57 a.m. UTC | #3 
Anton Khirnov (12021-05-31):
> This is starting to look way too ugly for something exposed in a public
> header.

The fact it resides in a public header is just a limitation of C's
linking model. The code itself is internal source, users should not look
at it.

Regards,
Carl Eugen Hoyos May 31, 2021, 5:16 p.m. UTC | #4 
Am Do., 27. Mai 2021 um 17:08 Uhr schrieb James Almer <jamrial@gmail.com>:
>
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavutil/mem.h | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/libavutil/mem.h b/libavutil/mem.h
> index e21a1feaae..c876111afb 100644
> --- a/libavutil/mem.h
> +++ b/libavutil/mem.h
> @@ -674,11 +674,18 @@ void *av_dynarray2_add(void **tab_ptr, int *nb_ptr, size_t elem_size,
>   */
>  static inline int av_size_mult(size_t a, size_t b, size_t *r)
>  {
> -    size_t t = a * b;
> +    size_t t;
> +
> +#if (!defined(__INTEL_COMPILER) && AV_GCC_VERSION_AT_LEAST(5,1)) || AV_HAS_BUILTIN(__builtin_mul_overflow)
> +    if (__builtin_mul_overflow(a, b, &t))
> +        return AVERROR(EINVAL);
> +#else
> +    t = a * b;
>      /* Hack inspired from glibc: don't try the division if nelem and elsize
>       * are both less than sqrt(SIZE_MAX). */
>      if ((a | b) >= ((size_t)1 << (sizeof(size_t) * 4)) && a && t / a != b)
>          return AVERROR(EINVAL);
> +#endif

The commit message appears to be missing a hint about the performance
difference.

Carl Eugen
diff mbox series

Patch

diff --git a/libavutil/mem.h b/libavutil/mem.h
index e21a1feaae..c876111afb 100644
--- a/libavutil/mem.h
+++ b/libavutil/mem.h
@@ -674,11 +674,18 @@  void *av_dynarray2_add(void **tab_ptr, int *nb_ptr, size_t elem_size,
  */
 static inline int av_size_mult(size_t a, size_t b, size_t *r)
 {
-    size_t t = a * b;
+    size_t t;
+
+#if (!defined(__INTEL_COMPILER) && AV_GCC_VERSION_AT_LEAST(5,1)) || AV_HAS_BUILTIN(__builtin_mul_overflow)
+    if (__builtin_mul_overflow(a, b, &t))
+        return AVERROR(EINVAL);
+#else
+    t = a * b;
     /* Hack inspired from glibc: don't try the division if nelem and elsize
      * are both less than sqrt(SIZE_MAX). */
     if ((a | b) >= ((size_t)1 << (sizeof(size_t) * 4)) && a && t / a != b)
         return AVERROR(EINVAL);
+#endif
     *r = t;
     return 0;
 }