diff mbox series

[FFmpeg-devel,1/3] avcodec/utils: don't use ff_fast_mallocz in av_fast_padded_malloc()

Message ID 20210523145433.5426-1-jamrial@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel,1/3] avcodec/utils: don't use ff_fast_mallocz in av_fast_padded_malloc()
Related show

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 23, 2021, 2:54 p.m. UTC
It will be removed in the next commit.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/utils.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Andreas Rheinhardt May 24, 2021, 10:52 a.m. UTC | #1
James Almer:
> It will be removed in the next commit.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/utils.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index c08f9a7da3..5394a179b0 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -29,7 +29,7 @@
>  #include "libavutil/avassert.h"
>  #include "libavutil/avstring.h"
>  #include "libavutil/intreadwrite.h"
> -#include "libavutil/mem_internal.h"
> +#include "libavutil/mem.h"
>  #include "libavutil/pixdesc.h"
>  #include "libavutil/imgutils.h"
>  #include "libavutil/pixfmt.h"
> @@ -49,25 +49,31 @@
>  
>  void av_fast_padded_malloc(void *ptr, unsigned int *size, size_t min_size)
>  {
> -    uint8_t **p = ptr;
> +    uint8_t *tmp, **p = ptr;
>      if (min_size > SIZE_MAX - AV_INPUT_BUFFER_PADDING_SIZE) {
>          av_freep(p);
>          *size = 0;
>          return;
>      }
> -    if (!ff_fast_malloc(p, size, min_size + AV_INPUT_BUFFER_PADDING_SIZE, 1))
> +    tmp = *p;
> +    av_fast_mallocz(p, size, min_size + AV_INPUT_BUFFER_PADDING_SIZE);
> +    /* don't zero the padding if the buffer was reallocated */
> +    if (*p && *p == tmp)
>          memset(*p + min_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>  }
>  
>  void av_fast_padded_mallocz(void *ptr, unsigned int *size, size_t min_size)
>  {
> -    uint8_t **p = ptr;
> +    uint8_t *tmp, **p = ptr;
>      if (min_size > SIZE_MAX - AV_INPUT_BUFFER_PADDING_SIZE) {
>          av_freep(p);
>          *size = 0;
>          return;
>      }
> -    if (!ff_fast_malloc(p, size, min_size + AV_INPUT_BUFFER_PADDING_SIZE, 1))
> +    tmp = *p;
> +    av_fast_mallocz(p, size, min_size + AV_INPUT_BUFFER_PADDING_SIZE);
> +    /* don't zero the buffer if it was reallocated */
> +    if (*p && *p == tmp)
>          memset(*p, 0, min_size + AV_INPUT_BUFFER_PADDING_SIZE);
>  }
>  
> 
The checks "*p == tmp" are flawed: It might be that the buffer has been
reallocated and that *p == tmp is nevertheless true (namely if the
buffer could have just been extended at the end) which might lead to
unnecessary zeroing; even worse, "[t]he value of a pointer becomes
indeterminate when the object it points to reaches the end of its
lifetime" (C99, 6.2.4.2), so it is compliant with the standard that if
av_fast_mallocz() reallocates the buffer, the tmp pointer on the stack
of av_fast_padded_malloc[z] might suddenly change (I don't know a system
in which this happens, though).

This can be avoided by always zeroing the padding in
av_fast_padded_malloc() and zeroing the buffer in
av_fast_padded_mallocz() ourselves (in which case one should use
av_fast_malloc() in av_fast_padded_mallocz()).

- Andreas
diff mbox series

Patch

diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index c08f9a7da3..5394a179b0 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -29,7 +29,7 @@ 
 #include "libavutil/avassert.h"
 #include "libavutil/avstring.h"
 #include "libavutil/intreadwrite.h"
-#include "libavutil/mem_internal.h"
+#include "libavutil/mem.h"
 #include "libavutil/pixdesc.h"
 #include "libavutil/imgutils.h"
 #include "libavutil/pixfmt.h"
@@ -49,25 +49,31 @@ 
 
 void av_fast_padded_malloc(void *ptr, unsigned int *size, size_t min_size)
 {
-    uint8_t **p = ptr;
+    uint8_t *tmp, **p = ptr;
     if (min_size > SIZE_MAX - AV_INPUT_BUFFER_PADDING_SIZE) {
         av_freep(p);
         *size = 0;
         return;
     }
-    if (!ff_fast_malloc(p, size, min_size + AV_INPUT_BUFFER_PADDING_SIZE, 1))
+    tmp = *p;
+    av_fast_mallocz(p, size, min_size + AV_INPUT_BUFFER_PADDING_SIZE);
+    /* don't zero the padding if the buffer was reallocated */
+    if (*p && *p == tmp)
         memset(*p + min_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
 }
 
 void av_fast_padded_mallocz(void *ptr, unsigned int *size, size_t min_size)
 {
-    uint8_t **p = ptr;
+    uint8_t *tmp, **p = ptr;
     if (min_size > SIZE_MAX - AV_INPUT_BUFFER_PADDING_SIZE) {
         av_freep(p);
         *size = 0;
         return;
     }
-    if (!ff_fast_malloc(p, size, min_size + AV_INPUT_BUFFER_PADDING_SIZE, 1))
+    tmp = *p;
+    av_fast_mallocz(p, size, min_size + AV_INPUT_BUFFER_PADDING_SIZE);
+    /* don't zero the buffer if it was reallocated */
+    if (*p && *p == tmp)
         memset(*p, 0, min_size + AV_INPUT_BUFFER_PADDING_SIZE);
 }