On Fri, Aug 1, 2014 at 7:37 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
> On 22 July 2014 01:48, <stefan2_at_apache.org> wrote:
> > Author: stefan2
> > Date: Mon Jul 21 21:48:35 2014
> > New Revision: 1612405
> >
> > URL: http://svn.apache.org/r1612405
> > Log:
> > Follow-up to r1611379: Correctly handle all intermittent errors.
> >
> > Since the same code sequence has been used for FSFS since 2007,
> > factor it out into some private API function.
> >
> > * subversion/include/private/svn_io_private.h
> > (svn_io__file_lock_autocreate): Declare new private API.
> >
> > * subversion/libsvn_subr/io.c
> > (svn_io__file_lock_autocreate): Move the file handling code from
> > the lock() function here and handle
> > all ordinary and racy errors.
> >
> > * subversion/libsvn_subr/named_atomic.c
> > (lock): Call the new function and ensure correct thread mutex use.
> >
> > Found by: brane
> >
> [...]
>
> >
> > Modified: subversion/trunk/subversion/libsvn_subr/named_atomic.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/named_atomic.c?rev=1612405&r1=1612404&r2=1612405&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_subr/named_atomic.c (original)
> > +++ subversion/trunk/subversion/libsvn_subr/named_atomic.c Mon Jul 21
> 21:48:35 2014
> > @@ -32,6 +32,7 @@
> > #include "svn_pools.h"
> > #include "svn_dirent_uri.h"
> > #include "svn_io.h"
> > +#include "private/svn_io_private.h"
> >
> > /* Implementation aspects.
> > *
> > @@ -274,28 +275,10 @@ init_thread_mutex(void *baton, apr_pool_
> > static svn_error_t *
> > lock(struct mutex_t *mutex)
> > {
> > - svn_error_t *err;
> > -
> > - /* Intra-process lock */
> > - SVN_ERR(svn_mutex__lock(thread_mutex));
> > -
> > - /* Inter-process lock. */
> > - err = svn_io_file_lock2(mutex->lock_name, TRUE, FALSE, mutex->pool);
> > - if (err && APR_STATUS_IS_ENOENT(err->apr_err))
> > - {
> > - /* No lock file? No big deal; these are just empty files anyway.
> > - Create it and try again. */
> > - svn_error_clear(err);
> > - err = NULL;
> > -
> > - SVN_ERR(svn_io_file_create_empty(mutex->lock_name, mutex->pool));
> > - SVN_ERR(svn_io_file_lock2(mutex->lock_name, TRUE, FALSE,
> mutex->pool));
> > - }
> > -
> > - /* Don't leave us in a semi-locked state ... */
> > - return err
> > - ? svn_mutex__unlock(thread_mutex, err)
> > - : err;
> > + SVN_MUTEX__WITH_LOCK(thread_mutex,
> > + svn_io__file_lock_autocreate(mutex->lock_name,
> > + mutex->pool));
> > + return SVN_NO_ERROR;
> > }
> >
> Stefan,
>
> It seems you broke the code (again): SVN_MUTEX__WITH_LOCK() will
> release THREAD_MUTEX immediately after file lock will be obtained.
> That means that access to shared memory will not be synchronized on
> posix platforms (!). And this also could cause undefined behavior when
> unlock() function will try to unlock THREAD_MUTEX without actually
> owning it [1].
>
Thank you for spotting this! It's one of those "too obvious to see" snafus.
The lock() is obviously supposed to keep the locks ... Fixed in r1615354.
-- Stefan^2.
Received on 2014-08-06 22:47:34 CEST