Hi!
Recenty I've discovered possible crash in FSFS locking code. If, for some
reason, 'write-lock' cannot be obtained for lock/unlock operation, the FSFS
will SEGFAULT.
This happens beacuse lb.infos field is getting initialized only in function
lock_body() (see the code below). So, if svn_fs_fs__with_write_lock() fails
without actual invoking the lock_body(), lb.infos will be left uninitialized.
[[[
svn_error_t *
svn_fs_fs__lock(svn_fs_t *fs,
...
struct lock_baton lb;
...
lb.fs = fs;
lb.targets = sorted_targets;
lb.comment = comment;
lb.is_dav_comment = is_dav_comment;
lb.expiration_date = expiration_date;
lb.steal_lock = steal_lock;
lb.result_pool = result_pool;
err = svn_fs_fs__with_write_lock(fs, lock_body, &lb, scratch_pool);
for (i = 0; i < lb.infos->nelts; ++i)
{
...
]]]
The same thing with svn_fs_fs__unlock().
I've attached the patch with crashing test and simple fix for this issue.
Log message:
[[[
Fix possible crash in svn_fs_fs__lock() / svn_fs_fs__unlock().
* subversion/subversion/tests/libsvn_fs/locks-test.c
(obtain_write_lock_failure_test): New; test for the issue.
* subversion/subversion/libsvn_fs_fs/lock.c
(lock_body,
svn_fs_fs__lock): Initialize the lb.infos field before calling to
svn_fs_fs__with_write_lock().
(unlock_body,
svn_fs_fs__unlock): Same.
Patch by: Sergey Raevskiy <sergey.raevskiy{_AT_}visualsvn.com>
]]]
Index: subversion/libsvn_fs_fs/lock.c
===================================================================
--- subversion/libsvn_fs_fs/lock.c (revision 1657295)
+++ subversion/libsvn_fs_fs/lock.c (working copy)
@@ -853,9 +853,6 @@ lock_body(void *baton, apr_pool_t *pool)
int i, outstanding = 0;
apr_pool_t *iterpool = svn_pool_create(pool);
- lb->infos = apr_array_make(lb->result_pool, lb->targets->nelts,
- sizeof(struct lock_info_t));
-
/* Until we implement directory locks someday, we only allow locks
on files or non-existent paths. */
/* Use fs->vtable->foo instead of svn_fs_foo to avoid circular
@@ -1056,9 +1053,6 @@ unlock_body(void *baton, apr_pool_t *pool)
int i, max_components = 0, outstanding = 0;
apr_pool_t *iterpool = svn_pool_create(pool);
- ub->infos = apr_array_make(ub->result_pool, ub->targets->nelts,
- sizeof(struct unlock_info_t));
-
SVN_ERR(ub->fs->vtable->youngest_rev(&youngest, ub->fs, pool));
SVN_ERR(ub->fs->vtable->revision_root(&root, ub->fs, youngest, pool));
@@ -1240,6 +1234,8 @@ svn_fs_fs__lock(svn_fs_t *fs,
lb.fs = fs;
lb.targets = sorted_targets;
+ lb.infos = apr_array_make(result_pool, sorted_targets->nelts,
+ sizeof(struct lock_info_t));
lb.comment = comment;
lb.is_dav_comment = is_dav_comment;
lb.expiration_date = expiration_date;
@@ -1330,6 +1326,8 @@ svn_fs_fs__unlock(svn_fs_t *fs,
ub.fs = fs;
ub.targets = sorted_targets;
+ ub.infos = apr_array_make(result_pool, sorted_targets->nelts,
+ sizeof(struct unlock_info_t));
ub.skip_check = FALSE;
ub.break_lock = break_lock;
ub.result_pool = result_pool;
Index: subversion/tests/libsvn_fs/locks-test.c
===================================================================
--- subversion/tests/libsvn_fs/locks-test.c (revision 1657295)
+++ subversion/tests/libsvn_fs/locks-test.c (working copy)
@@ -1079,6 +1079,62 @@ lock_cb_error(const svn_test_opts_t *opts,
return SVN_NO_ERROR;
}
+static svn_error_t *
+obtain_write_lock_failure_test(const svn_test_opts_t *opts,
+ apr_pool_t *pool)
+{
+ svn_fs_t *fs;
+ const char *conflict;
+ svn_revnum_t newrev;
+ svn_fs_access_t *access;
+ svn_fs_lock_target_t *target;
+ struct lock_many_baton_t baton;
+ apr_hash_t *lock_paths, *unlock_paths;
+
+ /* The test makes sense only for FSFS. */
+ if (strcmp(opts->fs_type, SVN_FS_TYPE_FSFS) != 0)
+ return svn_error_create(SVN_ERR_TEST_SKIPPED, NULL,
+ "this will test FSFS repositories only");
+
+ SVN_ERR(create_greek_fs(&fs, &newrev, "obtain-write-lock-failure-test",
+ opts, pool));
+ SVN_ERR(svn_fs_create_access(&access, "bubba", pool));
+ SVN_ERR(svn_fs_set_access(fs, access));
+
+ /* Make a read only 'write-lock' file. This prevents any write operations
+ from being executed. */
+
SVN_ERR(svn_io_set_file_read_only("obtain-write-lock-failure-test/write-lock",
+ TRUE, pool));
+
+ baton.results = apr_hash_make(pool);
+ baton.pool = pool;
+ baton.count = 0;
+
+ /* Trying to lock some paths. We don't really care about error; the test
+ shouldn't crash. */
+ target = svn_fs_lock_target_create(NULL, newrev, pool);
+ lock_paths = apr_hash_make(pool);
+ svn_hash_sets(lock_paths, "/iota", target);
+ svn_hash_sets(lock_paths, "/A/mu", target);
+
+ apr_hash_clear(baton.results);
+ SVN_TEST_ASSERT_ANY_ERROR(svn_fs_lock_many(fs, lock_paths, "comment", 0, 0,
0,
+ lock_many_cb, &baton, pool,
pool));
+
+ /* Trying to unlock some paths. We don't really care about error; the test
+ shouldn't crash. */
+ unlock_paths = apr_hash_make(pool);
+ svn_hash_sets(unlock_paths, "/iota", "");
+ svn_hash_sets(unlock_paths, "/A/mu", "");
+
+ apr_hash_clear(baton.results);
+ SVN_TEST_ASSERT_ANY_ERROR(svn_fs_unlock_many(fs, unlock_paths, TRUE,
+ lock_many_cb, &baton, pool,
+ pool));
+
+ return SVN_NO_ERROR;
+}
+
/* ------------------------------------------------------------------------ */
/* The test table. */
@@ -1114,6 +1170,8 @@ static struct svn_test_descriptor_t test_funcs[] =
"lock multiple paths"),
SVN_TEST_OPTS_PASS(lock_cb_error,
"lock callback error"),
+ SVN_TEST_OPTS_PASS(obtain_write_lock_failure_test,
+ "lock/unlock when 'write-lock' couldn't be obtained"),
SVN_TEST_NULL
};
