[FFmpeg-devel] [PATCH] Reset mutex to NULL after mutex destruction.

Manfred Georg mgeorg at google.com
Tue Sep 30 06:57:02 CEST 2014


Answers inline.

On Mon, Sep 29, 2014 at 7:07 PM, Michael Niedermayer <michaelni at gmx.at>
wrote:

> On Mon, Sep 29, 2014 at 02:41:38PM -0700, Manfred Georg wrote:
> > A badly behaving user provided mutex manager (such as that in OpenCV)
> may not reset the mutex to NULL on destruction.  This can cause a problem
> for a later mutex manager (which may assert that the mutex is NULL before
> creating).
> > ---
> >  libavcodec/utils.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> > index 9eb2b5b..a1f7cfc 100644
> > --- a/libavcodec/utils.c
> > +++ b/libavcodec/utils.c
> > @@ -3457,18 +3457,21 @@ AVHWAccel *av_hwaccel_next(const AVHWAccel
> *hwaccel)
> >  int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op))
> >  {
> >      if (lockmgr_cb) {
> > -        if (lockmgr_cb(&codec_mutex, AV_LOCK_DESTROY))
> > -            return -1;
> > -        if (lockmgr_cb(&avformat_mutex, AV_LOCK_DESTROY))
> > +        void *old_codec_mutex = codec_mutex;
> > +        void *old_avformat_mutex = avformat_mutex;
> > +        int failure;
> > +        codec_mutex = NULL;
> > +        avformat_mutex = NULL;
> > +        failure = lockmgr_cb(&old_codec_mutex, AV_LOCK_DESTROY);
> > +        if (lockmgr_cb(&old_avformat_mutex, AV_LOCK_DESTROY) || failure)
> >              return -1;
>
> why do you use temporary variables ?
> wouldnt simply setting them to NULL afterwards achieve the same ?
>
> The behavior on failure wouldn't be the same.  In the original code the
second call is never made if the first one fails.  I feel like this
implementation is at least a bit more symmetric.  Really, we'd want to
define some sane semantics on failure (both for this function and the lock
manager function provided by the user) and specify what happens in the
function comment in the header (without that we could argue in circles for
hours about which implementation is less incorrect), but that seemed out of
scope for this change.


>
> >      }
> >
> >      lockmgr_cb = cb;
> >
> >      if (lockmgr_cb) {
> > -        if (lockmgr_cb(&codec_mutex, AV_LOCK_CREATE))
> > -            return -1;
> > -        if (lockmgr_cb(&avformat_mutex, AV_LOCK_CREATE))
> > +        int failure = lockmgr_cb(&codec_mutex, AV_LOCK_CREATE);
> > +        if (lockmgr_cb(&avformat_mutex, AV_LOCK_CREATE) || failure)
> >              return -1;
>
> why, when the creation of the first lock manager fails you try to
> create the 2nd one ?
>
> isnt it more logic to leave the state as it was before the call on
> failure, instead of trying to half initialize things ?
> that would also require to first create the 2 new lock managers
> and then when both succeeded destroy the old
> though thats orthogonal to the stated intend of teh patch and
> should, if done, probably be in a seperate patch
>
>
As far as I know, it is never specified what state the lock manager
function should leave things in on failure.  You may assume that failure
means a mutex wasn't created while I may assume that it may have been
anyway.  This implementation seemed less likely to leave things in a bad
state given that we don't know what the lock manager function is actually
doing.  At least both mutex will have had the same thing done on
them...hopefully...probably.


> thanks
>

you're welcome.

Manfred

>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Its not that you shouldnt use gotos but rather that you should write
> readable code and code with gotos often but not always is less readable
>


More information about the ffmpeg-devel mailing list