[FFmpeg-devel] [PATCH] [Trac ticket #3194] libavcodec/utils: Statically initialize global mutexes to avoid a leak.

[Michael Niedermayer]

On Sun, Dec 15, 2013 at 01:44:51PM +0100, Hendrik Leppkes wrote:
> On Sun, Dec 15, 2013 at 1:38 PM, wm4 <nfxjfg at googlemail.com> wrote:
> > On Sun, 15 Dec 2013 13:16:06 +0100
> > Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> >
> >> On Sun, Dec 15, 2013 at 1:10 PM, wm4 <nfxjfg at googlemail.com> wrote:
> >> > On Sun, 15 Dec 2013 12:59:58 +0100
> >> > Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> >> >
> >> >> On Sun, Dec 15, 2013 at 12:17 PM, Tomer Barletz <barletz at gmail.com> wrote:
> >> >> > ---
> >> >> >  libavcodec/utils.c | 25 +++++++------------------
> >> >> >  1 file changed, 7 insertions(+), 18 deletions(-)
> >> >> >
> >> >> > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> >> >> > index 73370fe..0653acc 100644
> >> >> > --- a/libavcodec/utils.c
> >> >> > +++ b/libavcodec/utils.c
> >> >> > @@ -74,20 +74,6 @@ static int default_lockmgr_cb(void **arg, enum AVLockOp op)
> >> >> >      case AV_LOCK_CREATE:
> >> >> >          return 0;
> >> >> >      case AV_LOCK_OBTAIN:
> >> >> > -        if (!*mutex) {
> >> >> > -            pthread_mutex_t *tmp = av_malloc(sizeof(pthread_mutex_t));
> >> >> > -            if (!tmp)
> >> >> > -                return AVERROR(ENOMEM);
> >> >> > -            if ((err = pthread_mutex_init(tmp, NULL))) {
> >> >> > -                av_free(tmp);
> >> >> > -                return AVERROR(err);
> >> >> > -            }
> >> >> > -            if (avpriv_atomic_ptr_cas(mutex, NULL, tmp)) {
> >> >> > -                pthread_mutex_destroy(tmp);
> >> >> > -                av_free(tmp);
> >> >> > -            }
> >> >> > -        }
> >> >> > -
> >> >> >          if ((err = pthread_mutex_lock(*mutex)))
> >> >> >              return AVERROR(err);
> >> >> >
> >> >> > @@ -100,8 +86,7 @@ static int default_lockmgr_cb(void **arg, enum AVLockOp op)
> >> >> >      case AV_LOCK_DESTROY:
> >> >> >          if (*mutex)
> >> >> >              pthread_mutex_destroy(*mutex);
> >> >> > -        av_free(*mutex);
> >> >> > -        avpriv_atomic_ptr_cas(mutex, *mutex, NULL);
> >> >> > +
> >> >> >          return 0;
> >> >> >      }
> >> >> >      return 1;
> >> >> > @@ -114,8 +99,12 @@ static int (*lockmgr_cb)(void **mutex, enum AVLockOp op) = NULL;
> >> >> >
> >> >> >  volatile int ff_avcodec_locked;
> >> >> >  static int volatile entangled_thread_counter = 0;
> >> >> > -static void *codec_mutex;
> >> >> > -static void *avformat_mutex;
> >> >> > +
> >> >> > +static pthread_mutex_t codec_mutex_real = PTHREAD_MUTEX_INITIALIZER;
> >> >> > +static void *codec_mutex = &codec_mutex_real;
> >> >> > +
> >> >> > +static pthread_mutex_t avformat_mutex_real = PTHREAD_MUTEX_INITIALIZER;
> >> >> > +static void *avformat_mutex = &avformat_mutex_real;
> >> >> >
> >> >> >  #if FF_API_FAST_MALLOC && CONFIG_SHARED && HAVE_SYMVER
> >> >> >  FF_SYMVER(void*, av_fast_realloc, (void *ptr, unsigned int *size, size_t min_size), "LIBAVCODEC_55")
> >> >>
> >> >> Static initialization doesn't work with the w32threads compat wrapper,
> >> >> and as such, this patch would break this use-case.
> >> >
> >> > So why doesn't libavcodec use one of the pthread implementations for
> >> > windows?
> >>
> >> It can, but it also can use native win32 threads.
> >> The only complaint is lack of static initializers, no reason to force
> >> pthreads just for that.
> >
> > Maybe the w32threads wrapper could implement static initializers
> > instead of requiring dirty hacks in libavcodec? I'm not sure how to do
> > that, though. But maybe implementing pthread_once() would be possible.
> > That brings me to the question, how do Windows DLLs handle locking on
> > initialization?
> 
> The real problem here is that apparently you can't rely on the close
> callback being called on the lockmgr. If an user app provides its own,
> it could also leak.
> So why is this not called?

how / where do you want to call it ?

explicitly by the user app only ?
in that case libs that use libavcodec will leak

by every lib that uses libavcodec ?
this will lead to a races

so it first would need some kind of reference counting
where all libs & apps using libavcodec would call some function at
the end and when the count reaches 0 the mutexes could be
deallocated, but thats still tricky as one has to consider races where
another lib starts using libavcodec in that very moment

so 100% safe and robust deallocation seems tricky

if these corner cases dont bother you, you can just register
NULL as lock manager at the end to cause the mutexes to be freed

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- 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/20131215/58737523/attachment.asc>