You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Sergey Raevskiy <se...@visualsvn.com> on 2015/02/04 20:12:58 UTC
[PATCH] Fix possible crash in svn_fs_fs__lock() / svn_fs_fs__unlock()
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>
]]]
Re: [PATCH] Fix possible crash in svn_fs_fs__lock() / svn_fs_fs__unlock()
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Evgeny Kotkov <ev...@visualsvn.com> writes:
>> + 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));
>
> Should probably be named "test-obtain-write-lock-failure".
>
> + /* 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));
>
> I suppose there is no reason to use ignore_enoent = TRUE here, right? The
> 'write-lock' is always there and if it is not, the test shouldn't give a
> false positive.
I tweaked these parts of the test in r1657530.
Regards,
Evgeny Kotkov
Re: [PATCH] Fix possible crash in svn_fs_fs__lock() / svn_fs_fs__unlock()
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Sergey Raevskiy <se...@visualsvn.com> writes:
> 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.
[...]
> I've attached the patch with crashing test and simple fix for this issue.
Comments inline.
> @@ -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));
The unlock_body() function has multiple calling sites — svn_fs_fs__unlock()
and unlock_single(). This patch moves the ub->infos initialization into
svn_fs_fs__unlock(), but leaves unlock_single() unchanged. Hence, we will
most likely see another segfault due to us accessing uninitialized memory:
Use of uninitialised value of size 8
at 0x59BA8D6: apr_array_push (...)
by 0x6413D1B: unlock_body (lock.c:1089)
by 0x6414BE6: get_lock (lock.c:1181)
by 0x6412CD1: svn_fs_fs__allow_locked_operation (lock.c:511)
by 0x642233B: commit_body (transaction.c:3251)
by 0x6408E9B: with_lock (fs_fs.c:221)
by 0x6422130: svn_fs_fs__commit (transaction.c:3613)
by 0x6425E0D: svn_fs_fs__commit_txn (tree.c:2224)
by 0x4019B4: lock_expiration (locks-test.c:659)
by 0x4E3DB34: test_thread (svn_test_main.c:525)
by 0x5BE1181: start_thread (pthread_create.c:312)
by 0x5EF230C: clone (clone.S:111)
Invalid write of size 8
at 0x6413D1C: unlock_body (lock.c:1089)
by 0x6414BE6: get_lock (lock.c:1181)
by 0x6412CD1: svn_fs_fs__allow_locked_operation (lock.c:511)
by 0x642233B: commit_body (transaction.c:3251)
by 0x6408E9B: with_lock (fs_fs.c:221)
by 0x6422130: svn_fs_fs__commit (transaction.c:3613)
by 0x6425E0D: svn_fs_fs__commit_txn (tree.c:2224)
by 0x4019B4: lock_expiration (locks-test.c:659)
by 0x4E3DB34: test_thread (svn_test_main.c:525)
by 0x5BE1181: start_thread (pthread_create.c:312)
by 0x5EF230C: clone (clone.S:111)
> + 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));
Should probably be named "test-obtain-write-lock-failure".
+ /* 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));
I suppose there is no reason to use ignore_enoent = TRUE here, right? The
'write-lock' is always there and if it is not, the test shouldn't give a
false positive.
Regards,
Evgeny Kotkov
Re: [PATCH] Fix possible crash in svn_fs_fs__lock() / svn_fs_fs__unlock()
Posted by Philip Martin <ph...@wandisco.com>.
Sergey Raevskiy <se...@visualsvn.com> writes:
> 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>
> ]]]
You missed a second unlock allocation in unlock_single. I fixed that
and applied it in r1657525. Thanks!
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*