[FFmpeg-devel] [PATCH 1/3] avutil/mem: Fix *realloc() alignment with memalign hack

[wm4]

On Mon, 21 Nov 2016 16:18:56 +0100
Michael Niedermayer <michael at niedermayer.cc> wrote:

> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> ---
>  libavutil/mem.c | 44 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 12 deletions(-)
> 

"Fix" is not a good description. Was it broken? What was broken?

Also, I thought the memalign hack was generally not needed anymore.
Which platforms still need it?

> diff --git a/libavutil/mem.c b/libavutil/mem.c
> index 1a8fc21..6273d89 100644
> --- a/libavutil/mem.c
> +++ b/libavutil/mem.c
> @@ -86,12 +86,15 @@ void *av_malloc(size_t size)
>          return NULL;
>  
>  #if CONFIG_MEMALIGN_HACK
> -    ptr = malloc(size + ALIGN);
> +    ptr = malloc(size + ALIGN + 8);
>      if (!ptr)
>          return ptr;
> -    diff              = ((~(long)ptr)&(ALIGN - 1)) + 1;
> +
> +    diff = ((((uintptr_t)ptr) + 9 + ALIGN - 1) & ~(ALIGN - 1)) - (uintptr_t)ptr;
> +    av_assert0(diff>0 && diff<=ALIGN + 8);

Why 9?

>      ptr               = (char *)ptr + diff;
> -    ((char *)ptr)[-1] = diff;
> +    ((char *)ptr)[-9] = diff;
> +    ((int64_t *)ptr)[-1] = size;
>  #elif HAVE_POSIX_MEMALIGN
>      if (size) //OS X on SDK 10.6 has a broken posix_memalign implementation
>      if (posix_memalign(&ptr, ALIGN, size))
> @@ -146,6 +149,7 @@ void *av_realloc(void *ptr, size_t size)
>  {
>  #if CONFIG_MEMALIGN_HACK
>      int diff;
> +    void *ptr2;
>  #endif
>  
>      /* let's disallow possibly ambiguous cases */
> @@ -153,15 +157,31 @@ void *av_realloc(void *ptr, size_t size)
>          return NULL;
>  
>  #if CONFIG_MEMALIGN_HACK
> -    //FIXME this isn't aligned correctly, though it probably isn't needed
>      if (!ptr)
>          return av_malloc(size);
> -    diff = ((char *)ptr)[-1];
> -    av_assert0(diff>0 && diff<=ALIGN);  
> -    ptr = realloc((char *)ptr - diff, size + diff);
> -    if (ptr)
> -        ptr = (char *)ptr + diff;
> -    return ptr;
> +    diff = ((char *)ptr)[-9];
> +    av_assert0(diff>0 && diff<=ALIGN + 8);
> +
> +    // Quick path for apparently and likly aligned realloc()
> +    if (diff == ALIGN) {
> +        ptr = realloc((char *)ptr - diff, size + diff);
> +        if (ptr)
> +            ptr = (char *)ptr + diff;
> +        if (!(((uintptr_t)ptr) & (ALIGN - 1))) {
> +            if (ptr)
> +                ((int64_t *)ptr)[-1] = size;
> +            return ptr;
> +        }
> +    }
> +
> +    ptr2 = av_malloc(size);
> +    if (!ptr2)
> +        return NULL;
> +
> +    memcpy(ptr2, ptr, FFMIN(((int64_t *)ptr)[-1], size));
> +    ((int64_t *)ptr2)[-1] = size;
> +    av_free(ptr);
> +    return ptr2;

I thought av_realloc() never keeps alignment. It only needs to be sure
to not choke on the alignment hack, since calling av_realloc() on a
av_malloc() allocated block is still ok, even if it can lose the
alignment.

>  #elif HAVE_ALIGNED_MALLOC
>      return _aligned_realloc(ptr, size + !size, ALIGN);
>  #else
> @@ -229,8 +249,8 @@ void av_free(void *ptr)
>  {
>  #if CONFIG_MEMALIGN_HACK
>      if (ptr) {
> -        int v= ((char *)ptr)[-1];
> -        av_assert0(v>0 && v<=ALIGN);  
> +        int v= ((char *)ptr)[-9];
> +        av_assert0(v>0 && v<=ALIGN + 8);
>          free((char *)ptr - v);
>      }
>  #elif HAVE_ALIGNED_MALLOC