[Patch] Fix multiple reporting of the same lock in FSFS.

[Sergey Raevskiy <se...@visualsvn.com>]

Hi!

I've discovered interesting thing about the svn_fs_fs__get_locks() function.

In some unusual, but not impossible scenarios this function could report the
same lock multiple times: there should be the path with lock, and one of its
children should be locked as well (see the test in attached patch).  Current
callers in the Subversion codebase (such as svn_repos_fs_get_locks2()) are not
affected by this bug, since they store collected locks in apr_hash_t before
reporting them to the caller.

A bit of history:

First implementation of locking in FSFS used 'recursive' approach to store
the lock indexes (see 'trunk/subversion/libsvn_fs_fs/structure'@r853645):
[[[
...

To answer the question, "Does path FOO have a lock associate with
it?", one need only generate the MD5 digest of FOO's
absolute-in-the-FS path (say, 3b1b011fed614a263986b5c4869604e8), lock
for a file located like so:

   /path/to/repos/locks/3b1/3b1b011fed614a263986b5c4869604e8

And then see if that file contains lock information.

To inquire about locks on children of the path FOO, you would
reference the same path as above, but look for a list of children in
that file (instead of lock information).  Children as listed as MD5
digests, too, so you would simply iterate over those digests and
consult the files they reference, and so on, recursively.

...
]]]

Since r852750 (r12682) [1] this approach was changed to 'flat' indexes
storage (see 'trunk/subversion/libsvn_fs_fs/structure'@r956921):
[[[
 Also stored in the digest file for a given FS path are pointers to
 other digest files which contain information associated with other FS
-paths that are our path's immediate children.
+paths that are beneath our path (an immediate child thereof, or a
+grandchild, or a great-grandchild, ...).

 ...

 reference the same path as above, but look for a list of children in
 that file (instead of lock information).  Children are listed as MD5
 digests, too, so you would simply iterate over those digests and
-consult the files they reference, and so on, recursively.
+consult the files they reference for lock information.
]]]

This change happened *before* locking feature has been released in Subversion
1.2.  So, currently all existing FSFS repositories use 'flat' indexes for lock
storage.

However, the walk_digest_files() function wasn't updated to reflect the changes
from [1] and still iterates the locks in a 'recursive' way.  I suggest removing
recursion from this function, since it can produce unexpected results for the
caller.  I've attached a patch with the test and the fix for this issue.

Log message:
[[[
Fix multiple reporting of the same lock in FSFS.

In some unusual (but not impossible) scenarios this function could report the
same lock multiple times: there should be the path with lock, and one of its
children should be locked as well.

* subversion/libsvn_fs_fs/lock.c
  (walk_digests_callback_t): Remove unused argument and update comment.
  (locks_walker): Update callback.
  (walk_digest_files): Don't walk digest files recursively.

* subversion/tests/libsvn_fs/locks-test.c
  (get_locks_callback): Check if a lock was already reported.
  (parent_and_child_lock): New.
  (test_funcs): Add new test.

Patch by: sergey.raevskiy{_AT_}visualsvn.com
]]]

[1] http://svn.apache.org/r852750

Re: [Patch] Fix multiple reporting of the same lock in FSFS.

[Philip Martin <ph...@wandisco.com>]

Sergey Raevskiy <se...@visualsvn.com> writes:

> Follow-up to r1658482: Refactor walk_locks() function: remove the redundant
> complexity of walk_digest_files() / walk_digests_callback_t.

Committed in r1659217.  Thanks!

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*

Re: [Patch] Fix multiple reporting of the same lock in FSFS.

[Sergey Raevskiy <se...@visualsvn.com>]

>> I suspect the implementation is now more complicated than necessary.
>> walk_locks and walk_locks_baton could be removed, walk_digest_files
>> could be renamed to indicate that only a single digest file is accessed.
>> The callers of walk_locks would call the renamed function directly.
>
> Yes. I'm going to look into this and maybe will come up with a new patch.

I preferred to keep the walk_locks() function and inline walk_digest_files()
and locks_walker() instead.  The patch is attached.

Log message:
[[[
Follow-up to r1658482: Refactor walk_locks() function: remove the redundant
complexity of walk_digest_files() / walk_digests_callback_t.

* subversion/libsvn_fs_fs/lock.c
  (lock_expired): New helper function.
  (get_lock): Use the new function.
  (struct walk_locks_baton,
   walk_digests_callback_t,
   locks_walker,
   walk_digest_files): Remove.
  (walk_locks): Inline code from locks_walker() and walk_digest_files() with
    use of new helper function.

Patch by: sergey.raevskiy{_AT_}visualsvn.com
]]]

Re: [Patch] Fix multiple reporting of the same lock in FSFS.

[Sergey Raevskiy <se...@visualsvn.com>]

> I suspect the implementation is now more complicated than necessary.
> walk_locks and walk_locks_baton could be removed, walk_digest_files
> could be renamed to indicate that only a single digest file is accessed.
> The callers of walk_locks would call the renamed function directly.

Yes. I'm going to look into this and maybe will come up with a new patch.

Re: [Patch] Fix multiple reporting of the same lock in FSFS.

[Philip Martin <ph...@wandisco.com>]

Sergey Raevskiy <se...@visualsvn.com> writes:

> To reproduce this bug in test, I've emulated nested locks by creating a file
> ('/A'), locking it and then replacing by directory with same name.  This looks
> like a 'directory lock', and I am trying to say that if this happens in real
> life, the FS API would behave incorrectly.

I added a few extra comments to the test and committed, thanks!

I suspect the implementation is now more complicated than necessary.
walk_locks and walk_locks_baton could be removed, walk_digest_files
could be renamed to indicate that only a single digest file is accessed.
The callers of walk_locks would call the renamed function directly.

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*

Re: [Patch] Fix multiple reporting of the same lock in FSFS.

[Sergey Raevskiy <se...@visualsvn.com>]

> This is issue 2507
>
> http://subversion.tigris.org/issues/show_bug.cgi?id=2507
>
> Commit does not remove locks, lock removal is a separate step after
> commit.  However that doesn't work well when a commit removes a locked
> item, after such a commit the remaining lock is invalid and should not
> apply.

This is another issue.  My initial post is about a bug in FSFS that could lead
to unexpected behavior of svn_fs_get_locks() function.  What I'm talking about
is:

> However, the walk_digest_files() function wasn't updated to reflect the
> changes from [1] and still iterates the locks in a 'recursive' way.  I
> suggest removing recursion from this function, since it can produce
> unexpected results for the caller.  I've attached a patch with the test
> and the fix for this issue.

To reproduce this bug in test, I've emulated nested locks by creating a file
('/A'), locking it and then replacing by directory with same name.  This looks
like a 'directory lock', and I am trying to say that if this happens in real
life, the FS API would behave incorrectly.

> What happens if you remove /A and resinstate /A as a file, does the lock
> come back?

Yes.

Re: [Patch] Fix multiple reporting of the same lock in FSFS.

[Philip Martin <ph...@wandisco.com>]

Sergey Raevskiy <se...@visualsvn.com> writes:

> +  /* Make a file '/A'. */
> +  SVN_ERR(svn_fs_begin_txn(&txn, fs, 0, pool));
> +  SVN_ERR(svn_fs_txn_root(&root, txn, pool));
> +  SVN_ERR(svn_fs_make_file(root, "/A", pool));
> +  SVN_ERR(svn_fs_commit_txn(&conflict, &newrev, txn, pool));
> +
> +  /* Obtain a lock on '/A'. */
> +  SVN_ERR(svn_fs_lock(&lock, fs, "/A", NULL, NULL, FALSE, 0, newrev, FALSE,
> +                      pool));
> +
> +  /* Add a lock token to FS access context. */
> +  SVN_ERR(svn_fs_access_add_lock_token(access, lock->token));
> +
> +  /* Make some weird change: replace file '/A' by a directory with a child. */
> +  SVN_ERR(svn_fs_begin_txn(&txn, fs, newrev, pool));
> +  SVN_ERR(svn_fs_txn_root(&root, txn, pool));
> +  SVN_ERR(svn_fs_delete(root, "/A", pool));
> +  SVN_ERR(svn_fs_make_dir(root, "/A", pool));
> +  SVN_ERR(svn_fs_make_file(root, "/A/b", pool));
> +  SVN_ERR(svn_fs_commit_txn(&conflict, &newrev, txn, pool));
> +
> +  /* Obtain a lock on '/A/b'. */
> +  SVN_ERR(svn_fs_lock(&lock, fs, "/A/b", NULL, NULL, FALSE, 0, newrev, FALSE,
> +                      pool));

This is issue 2507

http://subversion.tigris.org/issues/show_bug.cgi?id=2507

Commit does not remove locks, lock removal is a separate step after
commit.  However that doesn't work well when a commit removes a locked
item, after such a commit the remaining lock is invalid and should not
apply.

What happens if you remove /A and resinstate /A as a file, does the lock
come back?

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*