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

[Michael Niedermayer]

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


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

applied

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- 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/20130318/c1b5ac5c/attachment.asc>