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

[Manfred Georg]

I took a crack at the more general change (it will be arriving shortly).
It's split into two patches, the first is a simplified version of this
change and the second patch more strongly defines the behavior on failure
of both the user supplied function and av_lockmgr_register.  Please
consider this patch dead.

Manfred

On Tue, Sep 30, 2014 at 12:13 PM, Michael Niedermayer <michaelni at gmx.at>
wrote:

> On Mon, Sep 29, 2014 at 09:57:02PM -0700, Manfred Georg wrote:
> > 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
>
> i meant this:
>
> failure = lockmgr_cb(&codec_mutex, AV_LOCK_DESTROY);
> failure |= lockmgr_cb(&avformat_mutex, AV_LOCK_DESTROY);
> codec_mutex = NULL;
> avformat_mutex = NULL;
> if (failure)
>     return -1;
>
>
> > 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.
>
> hopefully...probably ?
>
> also before the patch failure means either both locks havnt been
> created or the 2nd hasnt been
>
> after the patch a failure means either both locks havnt been
> created or the 1st or the 2nd hasnt been
>
> maybe its just me but i think its better to have the locks in a
> deterministic state on failure
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> it is not once nor twice but times without number that the same ideas make
> their appearance in the world. -- Aristotle
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>