Re: svn commit: r1612405 - in /subversion/trunk/subversion: include/private/svn_io_private.h libsvn_subr/io.c libsvn_subr/named_atomic.c

On 7 August 2014 00:47, Stefan Fuhrmann <stefan.fuhrmann_at_wandisco.com> wrote:
> 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.
>
Obvious problems are not rare in multithreading code and other complex
areas like FSFS, that's why we should be conservative about changing
them.

I've reviewed your fix and converted my veto to +1. Thanks!

-- 
Ivan Zhakov