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.
Index: subversion/libsvn_repos/hooks.c
===================================================================
--- subversion/libsvn_repos/hooks.c (revision 1657849)
+++ subversion/libsvn_repos/hooks.c (working copy)
@@ -522,10 +522,19 @@ lock_token_content(apr_file_t **handle, apr_hash_t
const char *token = apr_hash_this_key(hi);
const char *path = apr_hash_this_val(hi);
+ if (path == (const char *) 1)
+ {
+ /* Special handling for svn_fs_access_t * created by using deprecated
+ svn_fs_access_add_lock_token() function. */
+ path = "";
+ }
+ else
+ {
+ path = svn_path_uri_autoescape(path, pool);
+ }
+
svn_stringbuf_appendstr(lock_str,
- svn_stringbuf_createf(pool, "%s|%s\n",
- svn_path_uri_autoescape(path, pool),
- token));
+ svn_stringbuf_createf(pool, "%s|%s\n", path, token));
}
svn_stringbuf_appendcstr(lock_str, "\n");
Index: subversion/tests/libsvn_repos/repos-test.c
===================================================================
--- subversion/tests/libsvn_repos/repos-test.c (revision 1657849)
+++ subversion/tests/libsvn_repos/repos-test.c (working copy)
@@ -3562,6 +3562,53 @@ test_repos_fs_type(const svn_test_opts_t *opts,
return SVN_NO_ERROR;
}
+static svn_error_t *
+deprecated_access_context_api(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,
+ "test-repo-deprecated-access-context-api",
+ opts, pool));
+
+ /* 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
+
+ /* 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));
+
+ /* 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_repos_fs_commit_txn(&conflict, repos, &new_rev, txn, pool));
+
+ 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 +3662,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(deprecated_access_context_api,
+ "test deprecated access context api"),
SVN_TEST_NULL
};
