On Fri, Aug 1, 2014 at 7:37 PM, Ivan Zhakov <i...@visualsvn.com> wrote:
> On 22 July 2014 01:48, <stef...@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.
