[FFmpeg-devel] [PATCH 3/3] avutil/buffer: Fix race in pool.

[Hendrik Leppkes]

On Sun, Mar 17, 2013 at 6:46 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> This race will always happen sooner or later in a multi-threaded
> environment and it will over time lead to OOM.
> This fix works by spinning, there are other ways by which this
> can be fixed, like simply detecting the issue after it happened
> and freeing the over-allocated memory or simply using a mutex.
>
> Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> ---
>  libavutil/buffer.c          |    7 +++++++
>  libavutil/buffer_internal.h |    2 ++
>  2 files changed, 9 insertions(+)
>
> diff --git a/libavutil/buffer.c b/libavutil/buffer.c
> index 5c753ab..6639744 100644
> --- a/libavutil/buffer.c
> +++ b/libavutil/buffer.c
> @@ -307,6 +307,7 @@ static AVBufferRef *pool_alloc_buffer(AVBufferPool *pool)
>      ret->buffer->free   = pool_release_buffer;
>
>      avpriv_atomic_int_add_and_fetch(&pool->refcount, 1);
> +    avpriv_atomic_int_add_and_fetch(&pool->nb_allocated, 1);
>
>      return ret;
>  }
> @@ -318,6 +319,12 @@ AVBufferRef *av_buffer_pool_get(AVBufferPool *pool)
>
>      /* check whether the pool is empty */
>      buf = get_pool(pool);
> +    if (!buf && pool->refcount < pool->nb_allocated) {
> +        av_log(NULL, AV_LOG_DEBUG, "Pool race dectected, spining to avoid overallocation and eventual OOM\n");
> +        while (!buf && avpriv_atomic_int_get(&pool->refcount) < avpriv_atomic_int_get(&pool->nb_allocated))
> +            buf = get_pool(pool);
> +    }
> +

Any particular reason the first comparison in the if doesn't use the
atomic gets?
Also, reading the code, the pool holds a refcount onto itself as well,
so refcount is always one higher as nb_allocated if all buffers are in
use, so maybe that should be taken into account here?

>      if (!buf)
>          return pool_alloc_buffer(pool);
>
> diff --git a/libavutil/buffer_internal.h b/libavutil/buffer_internal.h
> index b2602f8..c291908 100644
> --- a/libavutil/buffer_internal.h
> +++ b/libavutil/buffer_internal.h
> @@ -85,6 +85,8 @@ struct AVBufferPool {
>       */
>      volatile int refcount;
>
> +    volatile int nb_allocated;
> +
>      int size;
>      AVBufferRef* (*alloc)(int size);
>  };
> --
> 1.7.9.5
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel