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

[Michael Niedermayer]

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


> 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?

yes, fixed

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130317/09d5e0e1/attachment.asc>