[PATCH] Race condition in svnsync can wedge a mirror repository

Hi,

I think I've found a bug in svnsync's locking code, that can cause
a stale "svn:sync-lock" revprop to be left in the repository. This
will prevent svnsync from being run again, until an administrator
manually deletes that property.

This is triggered by another process having the lock, and releasing
it between 8 and 9 seconds after svnsync is started. (Any earlier
and svnsync will successfully take the lock. Any later and svnsync
will give up and not try to take the lock).

This bug was found by code inspection. I have confirmed it can be
reproduced with the test script attached, using Subversion 1.6.6 on
Linux. After the script is run, the "svn:sync-lock" revprop should
not exist, but it does. (It is a timing-related problem, so you may
need to try a few times. With a hot cache, I can reproduce it 100%
of the time using this script).

The issue is with the get_lock() function in
subversion/svnsync/main.c. The algorithm used is:
> for (i = 0; i < 10; ++i)
> {
> [...]
> SVN_ERR(svn_ra_rev_prop(session, 0, "svn:sync-lock",
> &reposlocktoken, subpool));
>
> if (reposlocktoken)
> [...]
> else
> {
> SVN_ERR(svn_ra_change_rev_prop(session, 0,
> "svn:sync-lock",
> mylocktoken, subpool));
> // BUG: if i == 9 then we're about to return failure.
> // But we just took the lock!
> }
> }
>
> return svn_error_createf(APR_EINVAL, NULL,
> "Couldn't get lock on destination "
> "repos after %d attempts\n", i);

If the "else" branch is entered on the last iteration of the loop,
then the "svn:sync-lock" revprop is set but the function returns
failure. This wedges the repository.

A patch is attached. The patch is against 1.6.6. I've tried to
keep the changes minimal, because I think this might be a candidate
for a Subversion 1.6.7 patch release.

[[[
Fix svnserve bug that could leave repository locked.

* subversion/svnsync/main.c
  (get_lock): Move loop exit point so we cannot drop out of the
   loop (and return failure) immediately after successfully
   locking the mirror.
]]]

Kind regards,

Jon Foster

**********************************************************************
This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Cabot Communications Ltd.

If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone.

Cabot Communications Limited
Verona House, Filwood Road, Bristol BS16 3RY, UK
+44 (0) 1179584232

Co. Registered in England number 02817269

Please contact the sender if you believe you have received this email in error.

**********************************************************************

______________________________________________________________________
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email
______________________________________________________________________

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2424345

Please start new threads on the <dev_at_subversion.apache.org> mailing list.
To subscribe to the new list, send an empty e-mail to <dev-subscribe_at_subversion.apache.org>.