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
Index: subversion/libsvn_fs_fs/lock.c
===================================================================
--- subversion/libsvn_fs_fs/lock.c (revision 1658384)
+++ subversion/libsvn_fs_fs/lock.c (working copy)
@@ -539,7 +539,6 @@ static svn_error_t *
locks_walker(void *baton,
const char *fs_path,
const char *digest_path,
- apr_hash_t *children,
svn_lock_t *lock,
svn_boolean_t have_write_lock,
apr_pool_t *pool)
@@ -569,12 +568,11 @@ locks_walker(void *baton,
/* Callback type for walk_digest_files().
*
- * CHILDREN and LOCK come from a read_digest_file(digest_path) call.
+ * LOCK come from a read_digest_file(digest_path) call.
*/
typedef svn_error_t *(*walk_digests_callback_t)(void *baton,
const char *fs_path,
const char *digest_path,
- apr_hash_t *children,
svn_lock_t *lock,
svn_boolean_t have_write_lock,
apr_pool_t *pool);
@@ -599,11 +597,10 @@ walk_digest_files(const char *fs_path,
/* First, send up any locks in the current digest file. */
SVN_ERR(read_digest_file(&children, &lock, fs_path, digest_path, pool));
- SVN_ERR(walk_digests_func(walk_digests_baton, fs_path, digest_path,
- children, lock,
+ SVN_ERR(walk_digests_func(walk_digests_baton, fs_path, digest_path, lock,
have_write_lock, pool));
- /* Now, recurse on this thing's child entries (if any; bail otherwise). */
+ /* Now, report all the child entries (if any; bail otherwise). */
if (! apr_hash_count(children))
return SVN_NO_ERROR;
subpool = svn_pool_create(pool);
@@ -611,9 +608,13 @@ walk_digest_files(const char *fs_path,
{
const char *digest = apr_hash_this_key(hi);
svn_pool_clear(subpool);
- SVN_ERR(walk_digest_files
- (fs_path, digest_path_from_digest(fs_path, digest, subpool),
- walk_digests_func, walk_digests_baton, have_write_lock,
subpool));
+
+ SVN_ERR(read_digest_file
+ (NULL, &lock, fs_path,
+ digest_path_from_digest(fs_path, digest, subpool), subpool));
+
+ SVN_ERR(walk_digests_func(walk_digests_baton, fs_path, digest_path, lock,
+ have_write_lock, subpool));
}
svn_pool_destroy(subpool);
return SVN_NO_ERROR;
Index: subversion/tests/libsvn_fs/locks-test.c
===================================================================
--- subversion/tests/libsvn_fs/locks-test.c (revision 1658384)
+++ subversion/tests/libsvn_fs/locks-test.c (working copy)
@@ -53,9 +53,19 @@ get_locks_callback(void *baton,
struct get_locks_baton_t *b = baton;
apr_pool_t *hash_pool = apr_hash_pool_get(b->locks);
svn_string_t *lock_path = svn_string_create(lock->path, hash_pool);
- apr_hash_set(b->locks, lock_path->data, lock_path->len,
- svn_lock_dup(lock, hash_pool));
- return SVN_NO_ERROR;
+
+ if (!apr_hash_get(b->locks, lock_path->data, lock_path->len))
+ {
+ apr_hash_set(b->locks, lock_path->data, lock_path->len,
+ svn_lock_dup(lock, hash_pool));
+ return SVN_NO_ERROR;
+ }
+ else
+ {
+ return svn_error_createf(SVN_ERR_TEST_FAILED, NULL,
+ "Lock for path '%s' is being reported twice.",
+ lock->path);
+ }
}
/* A factory function. */
@@ -1134,6 +1144,66 @@ obtain_write_lock_failure(const svn_test_opts_t *o
return SVN_NO_ERROR;
}
+static svn_error_t *
+parent_and_child_lock(const svn_test_opts_t *opts,
+ apr_pool_t *pool)
+{
+ svn_fs_t *fs;
+ svn_fs_access_t *access;
+ svn_fs_txn_t *txn;
+ svn_fs_root_t *root;
+ const char *conflict;
+ svn_revnum_t newrev;
+ svn_lock_t *lock;
+ struct get_locks_baton_t *get_locks_baton;
+ apr_size_t num_expected_paths;
+
+ SVN_ERR(svn_test__create_fs(&fs, "test-parent-and-child-lock", opts, pool));
+ SVN_ERR(svn_fs_create_access(&access, "bubba", pool));
+ SVN_ERR(svn_fs_set_access(fs, access));
+
+ /* 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));
+
+ /* Verify the locked paths. */
+ {
+ static const char *expected_paths[] = {
+ "/A",
+ "/A/b",
+ };
+ num_expected_paths = sizeof(expected_paths) / sizeof(const char *);
+ get_locks_baton = make_get_locks_baton(pool);
+ SVN_ERR(svn_fs_get_locks(fs, "/", get_locks_callback,
+ get_locks_baton, pool));
+ SVN_ERR(verify_matching_lock_paths(get_locks_baton, expected_paths,
+ num_expected_paths, pool));
+ }
+
+ return SVN_NO_ERROR;
+}
+
/* ------------------------------------------------------------------------ */
/* The test table. */
@@ -1171,6 +1241,8 @@ static struct svn_test_descriptor_t test_funcs[] =
"lock callback error"),
SVN_TEST_OPTS_PASS(obtain_write_lock_failure,
"lock/unlock when 'write-lock' couldn't be obtained"),
+ SVN_TEST_OPTS_PASS(parent_and_child_lock,
+ "lock parent and it's child"),
SVN_TEST_NULL
};
