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/05 20:19:47 UTC
[PATCH] Fix potential crash in libsvn_repos when executing pre-commit hook
Hi!
I've discovered another crash in lock-related code. Usage of deprecated API
svn_fs_access_add_lock_token() function leads to crash when pre-commit hook is
getting executed.
Function svn_fs_access_add_lock_token() simply calls
svn_fs_access_add_lock_token2() and passes some magic value ((const char *) 1)
as PATH parameter. Such condition is not checked by function
lock_token_content() which is used to prepare STDIN contents for pre-commit
hook:
[[[
static svn_error_t *
lock_token_content(apr_file_t **handle, apr_hash_t *lock_tokens,
apr_pool_t *pool)
{
...
for (hi = apr_hash_first(pool, lock_tokens); hi;
hi = apr_hash_next(hi))
{
const char *token = apr_hash_this_key(hi);
const char *path = apr_hash_this_val(hi);
svn_stringbuf_appendstr(lock_str,
svn_stringbuf_createf(pool, "%s|%s\n",
svn_path_uri_autoescape(path, pool),
token));
}
...
}
]]]
I've attached the patch with crashing test and simple fix, but I'm not really
sure about my solution. A probably better approach would be to replace magic
pointer value by an empty string, but I'm not sure about binary compatibility
etc.
Log message:
[[[
Fix potential crash in libsvn_repos when executing pre-commit hook.
* subversion/subversion/libsvn_repos/hooks.c
(lock_token_content): Add special handling for 'magic' value
((const char *) 1).
* subversion/subversion/tests/libsvn_repos/repos-test.c
(pre_commit_hook_lock_token_without_path): New.
(test_funcs): Add new test.
Patch by: sergey.raevskiy{_AT_}visualsvn.com
]]]
Re: [PATCH] Fix potential crash in libsvn_repos when executing pre-commit hook
Posted by Philip Martin <ph...@wandisco.com>.
Sergey Raevskiy <se...@visualsvn.com> writes:
> I've attached the patch with crashing test and simple fix, but I'm not really
> sure about my solution. A probably better approach would be to replace magic
> pointer value by an empty string, but I'm not sure about binary compatibility
> etc.
The empty string is a valid filesystem path, although not one we can
currently lock. The documentation for svn_fs_access_t is wrong here,
it states the lock_tokens hash stores a mapping of UUID-->1 but it
really stores UUID-->path where path can be the magic value 1. At
present our backend does not check explicilty check the path but if a
backend did check the path then changing the magic 1 to an empty string
is probably the wrong thing to do.
I wonder if we should be checking the path? The modified test below
still passes, is that what we want?
Index: subversion/tests/libsvn_fs/locks-test.c
===================================================================
--- subversion/tests/libsvn_fs/locks-test.c (revision 1657760)
+++ subversion/tests/libsvn_fs/locks-test.c (working copy)
@@ -531,7 +531,8 @@ final_lock_check(const svn_test_opts_t *opts,
/* Supply correct username and token; commit should work. */
SVN_ERR(svn_fs_set_access(fs, access));
- SVN_ERR(svn_fs_access_add_lock_token(access, mylock->token));
+ /* Wrong path but commit still works! */
+ SVN_ERR(svn_fs_access_add_lock_token2(access, "/made/up", mylock->token));
SVN_ERR(svn_fs_commit_txn(&conflict, &newrev, txn, pool));
SVN_TEST_ASSERT(SVN_IS_VALID_REVNUM(newrev));
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: [PATCH] Fix potential crash in libsvn_repos when executing
pre-commit hook
Posted by Sergey Raevskiy <se...@visualsvn.com>.
> I committed the test using /bin/sh, anything else is a lot more work and
> possibly more likely to fail than to find a system without /bin/sh.
I'm agree, other options seems to be less reliable and much more complicated.
Finally, there is well-known practise of using /usr/bin/env (e.g.
'#!/usr/bin/env sh') but this option seems to be even less portable than
/bin/sh.
Re: [PATCH] Fix potential crash in libsvn_repos when executing pre-commit hook
Posted by Philip Martin <ph...@wandisco.com>.
Branko Čibej <br...@wandisco.com> writes:
>> I would expect this to work in practice as I've never seen a Unix
>> machine without /bin/sh, but I suppose strictly speaking we cannot rely
>> on /bin/sh. Is it portable enough? I suppose we could have a configure
>> variable based on CONFIG_SHELL. Or have a configure variable based on
>> PYTHON and write the hook in python. Or perhaps compile some C to
>> produce an executable and use that by copying it into the repository.
>
> That last would probably the safest thing to do; after all, if we can
> produce binaries, we can write portable hook scripts, too.
I committed the test using /bin/sh, anything else is a lot more work and
possibly more likely to fail than to find a system without /bin/sh.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: [PATCH] Fix potential crash in libsvn_repos when executing pre-commit
hook
Posted by Branko Čibej <br...@wandisco.com>.
On 06.02.2015 17:14, Philip Martin wrote:
> Sergey Raevskiy <se...@visualsvn.com> writes:
>
>> + /* Set an empty pre-commit hook. */
>> +#ifdef WIN32
>> + SVN_ERR(svn_io_file_create(
>> + "test-repo-deprecated-access-context-api/hooks/pre-commit.bat",
>> + "exit 0" APR_EOL_STR, pool));
>> +#else
>> + SVN_ERR(svn_io_file_create(
>> + "test-repo-deprecated-access-context-api/hooks/pre-commit",
>> + "#!/bin/sh" APR_EOL_STR "exit 0" APR_EOL_STR, pool));
>> + SVN_ERR(svn_io_set_file_executable(svn_repos_pre_commit_hook(repos, pool),
>> + TRUE, FALSE, pool));
>> +#endif
> I'd probably use svn_repos_pre_commit_hook in all three places (use
> apr_pstrcat to add the .bat) rather than hard-coding the hook name.
>
> I would expect this to work in practice as I've never seen a Unix
> machine without /bin/sh, but I suppose strictly speaking we cannot rely
> on /bin/sh. Is it portable enough? I suppose we could have a configure
> variable based on CONFIG_SHELL. Or have a configure variable based on
> PYTHON and write the hook in python. Or perhaps compile some C to
> produce an executable and use that by copying it into the repository.
That last would probably the safest thing to do; after all, if we can
produce binaries, we can write portable hook scripts, too.
-- Brane
Re: [PATCH] Fix potential crash in libsvn_repos when executing pre-commit hook
Posted by Philip Martin <ph...@wandisco.com>.
Sergey Raevskiy <se...@visualsvn.com> writes:
> + /* Set an empty pre-commit hook. */
> +#ifdef WIN32
> + SVN_ERR(svn_io_file_create(
> + "test-repo-deprecated-access-context-api/hooks/pre-commit.bat",
> + "exit 0" APR_EOL_STR, pool));
> +#else
> + SVN_ERR(svn_io_file_create(
> + "test-repo-deprecated-access-context-api/hooks/pre-commit",
> + "#!/bin/sh" APR_EOL_STR "exit 0" APR_EOL_STR, pool));
> + SVN_ERR(svn_io_set_file_executable(svn_repos_pre_commit_hook(repos, pool),
> + TRUE, FALSE, pool));
> +#endif
I'd probably use svn_repos_pre_commit_hook in all three places (use
apr_pstrcat to add the .bat) rather than hard-coding the hook name.
I would expect this to work in practice as I've never seen a Unix
machine without /bin/sh, but I suppose strictly speaking we cannot rely
on /bin/sh. Is it portable enough? I suppose we could have a configure
variable based on CONFIG_SHELL. Or have a configure variable based on
PYTHON and write the hook in python. Or perhaps compile some C to
produce an executable and use that by copying it into the repository.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: [PATCH] Fix potential crash in libsvn_repos when executing
pre-commit hook
Posted by Sergey Raevskiy <se...@visualsvn.com>.
Thanks for the review! I really messed up the test.
> How is that going to work? "exit 0" is not a valid hook script. It
> might work with an additional "/bin/sh" but that is not portable.
This will actually work on Windows, but yes, it isn't portable.
> Ah! The fs layer does not run any hooks, you need to use the repos
> layer's svn_repos_fs_commit_txn to run hooks. But that will not work
> because the hook is not portable.
I reworked the patch (see the attachment). The hook script setup code is
now rewritten in more portable way; test also uses the appropriate API
(svn_repos_fs_commit_txn()) to commit the transaction. The test crashes
in lock_token_content() without the fix.
I tested this on Windows and Linux machines.
Log message:
[[[
Fix potential crash in libsvn_repos when executing pre-commit hook.
* subversion/subversion/libsvn_repos/hooks.c
(lock_token_content): Add special handling for 'magic' value
((const char *) 1).
* subversion/subversion/tests/libsvn_repos/repos-test.c
(deprecated_access_context_api): New.
(test_funcs): Add new test.
Patch by: sergey.raevskiy{_AT_}visualsvn.com
]]]
> I wonder if we should be checking the path? The modified test below
> still passes, is that what we want?
There is at least one place where mod_dav_svn reconstructs svn_fs_access_t in a
hackish way:
[[[
do {
/* Note the path/lock pairs are only for lock token checking
in access, and the relative path is not actually accurate
as it contains the !svn bits. However, we're using only
the tokens anyway (for access control). */
serr = svn_fs_access_add_lock_token2(access_ctx, relative,
list->locktoken->uuid_str);
if (serr)
return dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
"Error pushing token into filesystem.",
r->pool);
list = list->next;
} while (list);
]]]
So, basically, we can't change this behaviour without patching mod_dav_svn.
I'm not really sure if there is a way to get right paths for lock tokens in
this place, but maybe we could improve this place in a separate patch.
Re: [PATCH] Fix potential crash in libsvn_repos when executing pre-commit hook
Posted by Philip Martin <ph...@wandisco.com>.
Sergey Raevskiy <se...@visualsvn.com> writes:
> +static svn_error_t *
> +pre_commit_hook_lock_token_without_path(const svn_test_opts_t *opts,
> + apr_pool_t *pool)
> +{
> + svn_repos_t *repos;
> + svn_fs_access_t *access;
> + svn_fs_txn_t *txn;
> + svn_fs_root_t *root;
> + const char *conflict;
> + svn_revnum_t new_rev;
> +
> + /* Create test repository. */
> + SVN_ERR(svn_test__create_repos(&repos, "pre_commit_hook_tokens_test",
> + opts, pool));
> +
> + /* Set an empty pre-commit hook. */
> + SVN_ERR(svn_io_file_create(svn_repos_pre_commit_hook(repos, pool),
> + "exit 0", pool));
> + SVN_ERR(svn_io_set_file_executable(svn_repos_pre_commit_hook(repos, pool),
> + TRUE, FALSE, pool));
How is that going to work? "exit 0" is not a valid hook script. It
might work with an additional "/bin/sh" but that is not portable.
> +
> + /* Set some access context using svn_fs_access_add_lock_token(). */
> + SVN_ERR(svn_fs_create_access(&access, "jrandom", pool));
> + SVN_ERR(svn_fs_access_add_lock_token(access, "opaquelocktoken:abc"));
> + SVN_ERR(svn_fs_set_access(svn_repos_fs(repos), access));
> +
> + /* Try to commit a new revision. */
> + SVN_ERR(svn_repos_fs_begin_txn_for_commit2(&txn, repos, 0,
> + apr_hash_make(pool), pool));
> + SVN_ERR(svn_fs_txn_root(&root, txn, pool));
> + SVN_ERR(svn_fs_make_dir(root, "/whatever", pool));
> + SVN_ERR(svn_fs_commit_txn(&conflict, &new_rev, txn, pool));
Ah! The fs layer does not run any hooks, you need to use the repos
layer's svn_repos_fs_commit_txn to run hooks. But that will not work
because the hook is not portable.
> +
> + SVN_TEST_STRING_ASSERT(conflict, NULL);
> + SVN_TEST_ASSERT(new_rev == 1);
> +
> + return SVN_NO_ERROR;
> +}
> +
> /* The test table. */
>
> static int max_threads = 4;
> @@ -3615,6 +3654,8 @@ static struct svn_test_descriptor_t test_funcs[] =
> "test svn_repos__config_pool_*"),
> SVN_TEST_OPTS_PASS(test_repos_fs_type,
> "test test_repos_fs_type"),
> + SVN_TEST_OPTS_PASS(pre_commit_hook_lock_token_without_path,
> + "test legacy access context api"),
> SVN_TEST_NULL
> };
>
>
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*