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

[Michael Niedermayer]

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 ?


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

thanks

[...]
-- 
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
-------------- 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/f6f1d5c6/attachment.asc>