Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: ---
    • Fix Version/s: 1.7.0
    • Component/s: svnsync
    • Labels:

      Description

      This fixes the race condition where two svnsync processes can lock
      the repository at the same time (as noticed by Stefan Sperling
      earlier today).  I wanted to do this without changing the RA layer,
      so that it can work with older servers.
      
      Since the RA layer doesn't have a test-and-set primitive (other
      than a commit!), some raciness is inevitable.  This patch fixes the
      dangerous race, but introduces a much less dangerous race.  With
      this patch, two "svnsync" processes trying to lock the repository at
      the same time might both fail to get the lock.  To mitigate this,
      both processes will sleep for a random amount of time before
      retrying.  This should reduce the chance of them staying in lock-step
      and repeatedly failing.  (Hopefully the chance becomes negligible).
      
      This doesn't conflict with my --use-external-locking patch from
      earlier today; they change different parts of the code and have
      different use-cases. [Logged as Issue 3545]
      

      Original issue reported by gavinbaumanis

        Activity

        Hide
        danielsh Daniel Shahaf (äñ§€¥£¢) added a comment -

        The race condition also affects svnrdump with 1.6 (and
        earlier) servers.  (svnrdump itself is new in 1.7.)
        
        

        Show
        danielsh Daniel Shahaf (äñ§€¥£¢) added a comment - The race condition also affects svnrdump with 1.6 (and earlier) servers. (svnrdump itself is new in 1.7.)
        Hide
        danielsh Daniel Shahaf (äñ§€¥£¢) added a comment -

        Merged the branch to trunk in r1000693.
        
        http://mid.gmane.org/20100922115358.GB4790@lp-shahaf.local
        

        Show
        danielsh Daniel Shahaf (äñ§€¥£¢) added a comment - Merged the branch to trunk in r1000693. http://mid.gmane.org/20100922115358.GB4790@lp-shahaf.local
        Hide
        danielsh Daniel Shahaf (äñ§€¥£¢) added a comment -

        Started this and related work on the atomic-revprop branch.
        
        See this post for current status:
        http://mail-archives.apache.org/mod_mbox/subversion-dev/201008.mbox/<20100811005138.GC2294@daniel3.local>
        http://mid.gmane.org/20100811005138.GC2294@daniel3.local
        

        Show
        danielsh Daniel Shahaf (äñ§€¥£¢) added a comment - Started this and related work on the atomic-revprop branch. See this post for current status: http://mail-archives.apache.org/mod_mbox/subversion-dev/201008.mbox/<20100811005138.GC2294@daniel3.local> http://mid.gmane.org/20100811005138.GC2294@daniel3.local
        Hide
        philipm Philip Martin added a comment -

        No, that doesn't work.
        
        The pre-revprop-change hook cannot be used because revprops changes and hooks
        are executed without a transaction.  The ACTION parameter passed to the hook is
        not reliable.
        

        Show
        philipm Philip Martin added a comment - No, that doesn't work. The pre-revprop-change hook cannot be used because revprops changes and hooks are executed without a transaction. The ACTION parameter passed to the hook is not reliable.
        Hide
        philipm Philip Martin added a comment -

        Always test shell scripts :-(
        
        The script needs to validate both the user and the change. Reorganising so that
        it is easier to follow gives this:
        
        #!/bin/sh
        USER="$3"
        PROPNAME="$4"
        ACTION="$5"
        if [ "$USER" != "syncuser" ] ; then
          echo "Only the syncuser user may change revision properties" >&2
          exit 1
        fi
        if [ "$PROPNAME" = "svn:sync-lock" ] ; then
          if [ "$ACTION" != "A" ]  && [ "$ACTION" != "D" ] ; then 
            echo "Modifying svn:sync-lock not allowed" >&2
            exit 1
          fi
        fi
        exit 0
        
        

        Show
        philipm Philip Martin added a comment - Always test shell scripts :-( The script needs to validate both the user and the change. Reorganising so that it is easier to follow gives this: #!/bin/sh USER="$3" PROPNAME="$4" ACTION="$5" if [ "$USER" != "syncuser" ] ; then echo "Only the syncuser user may change revision properties" >&2 exit 1 fi if [ "$PROPNAME" = "svn:sync-lock" ] ; then if [ "$ACTION" != "A" ] && [ "$ACTION" != "D" ] ; then echo "Modifying svn:sync-lock not allowed" >&2 exit 1 fi fi exit 0
        Hide
        philipm Philip Martin added a comment -

        The reason this patch has not been applied is that it does not fix the race, it
        just makes it less likely.  See Stefan's comments Apr 15 2010. The problem is
        that changing a revprop does not provide an atomic test and set.
        
        The canonical pre-revprop-change hook for svnsync is:
        
        #!/bin/sh
        USER="$3"
        if [ "$USER" = "syncuser" ] ; then exit 0 ; fi
        echo "Only the syncuser user may change revision properties" >&2
        exit 1
        
        If we extend it to:
        
        #!/bin/sh
        USER="$3"
        PROPNAME="$4"
        ACTION="$5"
        if [ "$USER" = "syncuser" ] ; then exit 0 ; fi
        echo "Only the syncuser user may change revision properties" >&2
        if [ "$PROPNAME" = "svn:sync-lock" ] ; then
          if [ "$ACTION" = "A" ]  || [ "$ACTION" = "D" ] ; then exit 0 ; fi
          exit 1
        fi
        exit 1
        
        then svnsync can only set and delete the revprop, it cannot modify it. This
        effectively makes the revprop change into an atomic operation.
        
        This solves the race and allows svnsync to lock the repository reliably. The
        only problem is that svnsync does not expect a revprop change to fail, so if the
        race does occur then when the hook returns an error one of the svnsync commands
        will exit. However that is comparable to one of the svnsync commands failing
        after 10 attempts to get a lock.
        

        Show
        philipm Philip Martin added a comment - The reason this patch has not been applied is that it does not fix the race, it just makes it less likely. See Stefan's comments Apr 15 2010. The problem is that changing a revprop does not provide an atomic test and set. The canonical pre-revprop-change hook for svnsync is: #!/bin/sh USER="$3" if [ "$USER" = "syncuser" ] ; then exit 0 ; fi echo "Only the syncuser user may change revision properties" >&2 exit 1 If we extend it to: #!/bin/sh USER="$3" PROPNAME="$4" ACTION="$5" if [ "$USER" = "syncuser" ] ; then exit 0 ; fi echo "Only the syncuser user may change revision properties" >&2 if [ "$PROPNAME" = "svn:sync-lock" ] ; then if [ "$ACTION" = "A" ] || [ "$ACTION" = "D" ] ; then exit 0 ; fi exit 1 fi exit 1 then svnsync can only set and delete the revprop, it cannot modify it. This effectively makes the revprop change into an atomic operation. This solves the race and allows svnsync to lock the repository reliably. The only problem is that svnsync does not expect a revprop change to fail, so if the race does occur then when the hook returns an error one of the svnsync commands will exit. However that is comparable to one of the svnsync commands failing after 10 attempts to get a lock.
        Hide
        cmpilato C. Michael Pilato added a comment -

        Add 'patch' keyword to issues which have an attachment of the 'patch' type.
        

        Show
        cmpilato C. Michael Pilato added a comment - Add 'patch' keyword to issues which have an attachment of the 'patch' type.
        Hide
        cmpilato C. Michael Pilato added a comment -

        Fix issue type.  We're discontinuing the use of PATCH.
        

        Show
        cmpilato C. Michael Pilato added a comment - Fix issue type. We're discontinuing the use of PATCH.
        Hide
        stsp Stefan Sperling added a comment -

        See http://svn.haxx.se/dev/archive-2010-02/0363.shtml
        

        Show
        stsp Stefan Sperling added a comment - See http://svn.haxx.se/dev/archive-2010-02/0363.shtml
        Hide
        subversion-importer Subversion Importer added a comment -

        Looks like the log message got lost.  Here it is:
        [[[
        Fix race condition that allowed two svnsync processes to get the lock
        at the same time.
        
        * subversion/include/svn_props.h:
          (SVNSYNC_PROP_PRE_LOCK_PREFIX): New define.
        
        * subversion/svnsync/main.c:
          (get_local_host_name, generate_moderately_random_number,
           try_get_lock): New functions.
          (get_lock): Rewritten.
        
        Found by: stsp
                  Jon Foster <jon.foster@cabot.co.uk>
        (Independently discovered)
        Patch by: Jon Foster <jon.foster@cabot.co.uk>
        ]]]
        
        

        Original comment by jonfoster

        Show
        subversion-importer Subversion Importer added a comment - Looks like the log message got lost. Here it is: [[[ Fix race condition that allowed two svnsync processes to get the lock at the same time. * subversion/include/svn_props.h: (SVNSYNC_PROP_PRE_LOCK_PREFIX): New define. * subversion/svnsync/main.c: (get_local_host_name, generate_moderately_random_number, try_get_lock): New functions. (get_lock): Rewritten. Found by: stsp Jon Foster <jon.foster@cabot.co.uk> (Independently discovered) Patch by: Jon Foster <jon.foster@cabot.co.uk> ]]] Original comment by jonfoster
        Hide
        subversion-importer Subversion Importer added a comment -

        Stephan's explanation of the bug was here:
        
        http://mail-archives.apache.org/mod_mbox/subversion-dev/200911.mbox/%3C20091127115356.GC9302@jack.stsp.name%3E
        

        Original comment by jonfoster

        Show
        subversion-importer Subversion Importer added a comment - Stephan's explanation of the bug was here: http://mail-archives.apache.org/mod_mbox/subversion-dev/200911.mbox/%3C20091127115356.GC9302@jack.stsp.name%3E Original comment by jonfoster
        Hide
        julianfoad Julian Foad added a comment -

        The email thread I linked just now is on this subject, but the thread in which
        this patch was actually sent is
        <http://mail-archives.apache.org/mod_mbox/subversion-dev/200911.mbox/%3C4B6F0239C13D0245820603C036D180BC9A23AF@CABOTUKEXCH01.cabot.local%3E>
        

        Show
        julianfoad Julian Foad added a comment - The email thread I linked just now is on this subject, but the thread in which this patch was actually sent is <http://mail-archives.apache.org/mod_mbox/subversion-dev/200911.mbox/%3C4B6F0239C13D0245820603C036D180BC9A23AF@CABOTUKEXCH01.cabot.local%3E>
        Hide
        julianfoad Julian Foad added a comment -

        The email thread is at
        <http://mail-archives.apache.org/mod_mbox/subversion-dev/200911.mbox/%3C4B6F0239C13D0245820603C036D180BC9A22BB@CABOTUKEXCH01.cabot.local%3E>.
        

        Show
        julianfoad Julian Foad added a comment - The email thread is at <http://mail-archives.apache.org/mod_mbox/subversion-dev/200911.mbox/%3C4B6F0239C13D0245820603C036D180BC9A22BB@CABOTUKEXCH01.cabot.local%3E>.
        Hide
        subversion-importer Subversion Importer added a comment -

        Patch Submiited by Jon Foster.
        

        Original comment by gavinbaumanis

        Show
        subversion-importer Subversion Importer added a comment - Patch Submiited by Jon Foster. Original comment by gavinbaumanis
        Hide
        subversion-importer Subversion Importer added a comment -

        Created an attachment (id=1074)
        svnsync two step locking patch
        
        

        Original comment by gavinbaumanis

        Show
        subversion-importer Subversion Importer added a comment - Created an attachment (id=1074) svnsync two step locking patch Original comment by gavinbaumanis
        Hide
        subversion-importer Subversion Importer added a comment -

        Attachment 1_svnsync-two-step-locking.patch has been added with description: svnsync two step locking patch

        Show
        subversion-importer Subversion Importer added a comment - Attachment 1_svnsync-two-step-locking.patch has been added with description: svnsync two step locking patch

          People

          • Assignee:
            Unassigned
            Reporter:
            subversion-importer Subversion Importer
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development