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

[Michael Niedermayer]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140930/ef36c4da/attachment.asc>