On 2 August 2014 23:13, <stef...@apache.org> wrote:
> Author: stefan2
> Date: Sat Aug 2 19:13:14 2014
> New Revision: 1615349
>
> URL: http://svn.apache.org/r1615349
> Log:
> Teach checked svn_mutex__t instances to detect unlock attempts on mutexes
> that are not locked.
>
> * subversion/include/svn_error_codes.h
> (SVN_ERR_INVALID_UNLOCK): Declare new error code.
>
> * subversion/libsvn_subr/mutex.c
> (svn_mutex__unlock): Verify that there was actually a lock on this mutex.
>
> Modified:
> subversion/trunk/subversion/include/svn_error_codes.h
> subversion/trunk/subversion/libsvn_subr/mutex.c
>
[..]
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/mutex.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/mutex.c Sat Aug 2 19:13:14 2014
> @@ -184,6 +184,17 @@ svn_mutex__unlock(svn_mutex__t *mutex,
> different but equivalent IDs for the same thread). */
> void *lock_owner = apr_atomic_casptr(&mutex->owner, NULL, NULL);
>
> + /* Check for double unlock. */
> + if (lock_owner == NULL)
> + {
> + /* There seems to be no guarantee that NULL is _not_ a valid
> + thread ID. Double check to be sure. */
> + if (!apr_os_thread_equal((apr_os_thread_t)lock_owner,
> + apr_os_thread_current()))
> + return svn_error_create(SVN_ERR_INVALID_UNLOCK, NULL,
> + _("Tried to release a non-locked mutex"));
> + }
> +
> /* Now, set it to NULL. */
> apr_atomic_casptr(&mutex->owner, NULL, lock_owner);
> }
> @@ -195,7 +206,9 @@ svn_mutex__unlock(svn_mutex__t *mutex,
> #else
> /* Update lock counter. */
> if (mutex->checked)
> - --mutex->count;
> + if (--mutex->count)
> + return svn_error_create(SVN_ERR_INVALID_UNLOCK, NULL,
> + _("Tried to release a non-locked mutex"));
1. Why you are using non-atomic decrement here while we're using
casptr() for mutex->owner?
2. You're leaving mutex object in invalid state on attempt to release
non-locked mutex.
I also find checked mutexes a 'false sense of security' thing: we
should be more careful when writing multi-threaded code instead of
relying on unreliable protection mechanism. That is because many
multi-threading problems do not have stable reproduction scripts
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
