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

[Hendrik Leppkes]

On Mon, Mar 18, 2013 at 7:39 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Sun, Mar 17, 2013 at 07:14:58PM +0100, Michael Niedermayer wrote:
>> On Sun, Mar 17, 2013 at 07:05:57PM +0100, Hendrik Leppkes wrote:
>> > 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?
>>
>> the only reason is that its faster
>
> btw, if someone thinks this is a problem i can change it ...
>

Probably not a problem, i hope.
In the long run i would favor using a mutex for the pool anyway, it
would just feel cleaner, and requires less headaches to be sure it
works.