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
   };
 

Reply via email to