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 ]]]
Index: subversion/libsvn_repos/hooks.c =================================================================== --- subversion/libsvn_repos/hooks.c (revision 1657614) +++ 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 1657614) +++ subversion/tests/libsvn_repos/repos-test.c (working copy) @@ -3562,6 +3562,45 @@ test_repos_fs_type(const svn_test_opts_t *opts, return SVN_NO_ERROR; } +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)); + + /* 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)); + + 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 };