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

[Andreas Rheinhardt]

James Almer:
> It will be removed in the next commit.
> 
> Signed-off-by: James Almer <jamrial at 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