Re: [PATCH] Fix potential crash in libsvn_repos when executing pre-commit hook

Sergey Raevskiy <sergey.raevskiy_at_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*