Author: kotkov Date: Tue Nov 27 18:10:01 2018 New Revision: 1847572 URL: http://svn.apache.org/viewvc?rev=1847572&view=rev Log: fsfs: Fix an issue with the DAG open_path() that was causing unexpected SVN_ERR_FS_NOT_DIRECTORY errors when attempting to open a path with `open_path_node_only | open_path_allow_null` flags.
The implication of this is that certain svn_fs_closest_copy() calls could be returing unexpected errors as well. For the end user, this means that such errors were possible, for example, during certain `svn update`s. The root cause of this behavior is the implementation of the optimization within the open_path() routine. The optimization attempts to do a cache lookup of the prospective parent directory of the path to be opened. If the cache lookup is successful, the parent node is assumed — but not checked — to be a directory. The absense of the check was causing unexpected errors instead of returning `NULL` in a case where: - open_path() is called with `open_path_node_only | open_path_allow_null` - the path to be opened points to a non-existing path, but its parent path is a file - the DAG node of the "parent" file is cached See https://lists.apache.org/thread.html/693a95b0da834387e78a7f08df2392b634397d32f37428c81c02f8c5@%3Cdev.subversion.apache.org%3E * subversion/libsvn_fs_fs/tree.c (err_not_directory): New helper function factored out from... (open_path): ...here. Check the kind of the DAG node for the prospective parent returned from cache in the `open_path_node_only` optimization. Handle the case where it is not a directory; utilize the new helper to construct the appropriate error. * subversion/tests/libsvn_fs/fs-test.c (test_closest_copy_file_replaced_with_dir): New regression test. (test_funcs): Run new test. Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c subversion/trunk/subversion/tests/libsvn_fs/fs-test.c Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1847572&r1=1847571&r2=1847572&view=diff ============================================================================== --- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original) +++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Tue Nov 27 18:10:01 2018 @@ -920,6 +920,25 @@ try_match_last_node(dag_node_t **node_p, return SVN_NO_ERROR; } +/* Helper for open_path() that constructs and returns an appropriate + SVN_ERR_FS_NOT_DIRECTORY error. */ +static svn_error_t * +err_not_directory(svn_fs_root_t *root, + const char *path, + apr_pool_t *scratch_pool) +{ + const char *msg; + + msg = root->is_txn_root + ? apr_psprintf(scratch_pool, + _("Failure opening '%s' in transaction '%s'"), + path, root->txn) + : apr_psprintf(scratch_pool, + _("Failure opening '%s' in revision %ld"), + path, root->rev); + + return svn_error_quick_wrap(SVN_FS__ERR_NOT_DIRECTORY(root->fs, path), msg); +} /* Open the node identified by PATH in ROOT, allocating in POOL. Set *PARENT_PATH_P to a path from the node up to ROOT. The resulting @@ -1016,12 +1035,26 @@ open_path(parent_path_t **parent_path_p, SVN_ERR(dag_node_cache_get(&here, root, directory, pool)); /* Did the shortcut work? */ - if (here) + if (here && svn_fs_fs__dag_node_kind(here) == svn_node_dir) { apr_size_t dirname_len = strlen(directory); path_so_far->len = dirname_len; rest = path + dirname_len + 1; } + else if (here) + { + /* The parent node is not a directory. We are looking for some + sub-path, so that sub-path will not exist. That will be o.k. + if we are just here to check for the path's existence, but + should result in an error otherwise. */ + if (flags & open_path_allow_null) + { + *parent_path_p = NULL; + return SVN_NO_ERROR; + } + else + return err_not_directory(root, directory, pool); + } } } @@ -1144,8 +1177,6 @@ open_path(parent_path_t **parent_path_p, /* The path isn't finished yet; we'd better be in a directory. */ if (svn_fs_fs__dag_node_kind(child) != svn_node_dir) { - const char *msg; - /* Since this is not a directory and we are looking for some sub-path, that sub-path will not exist. That will be o.k., if we are just here to check for the path's existence. */ @@ -1156,14 +1187,7 @@ open_path(parent_path_t **parent_path_p, } /* It's really a problem ... */ - msg = root->is_txn_root - ? apr_psprintf(iterpool, - _("Failure opening '%s' in transaction '%s'"), - path, root->txn) - : apr_psprintf(iterpool, - _("Failure opening '%s' in revision %ld"), - path, root->rev); - SVN_ERR_W(SVN_FS__ERR_NOT_DIRECTORY(fs, path_so_far->data), msg); + return err_not_directory(root, path_so_far->data, iterpool); } rest = next; Modified: subversion/trunk/subversion/tests/libsvn_fs/fs-test.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_fs/fs-test.c?rev=1847572&r1=1847571&r2=1847572&view=diff ============================================================================== --- subversion/trunk/subversion/tests/libsvn_fs/fs-test.c (original) +++ subversion/trunk/subversion/tests/libsvn_fs/fs-test.c Tue Nov 27 18:10:01 2018 @@ -7369,6 +7369,78 @@ closest_copy_test_svn_4677(const svn_tes return SVN_NO_ERROR; } +static svn_error_t * +test_closest_copy_file_replaced_with_dir(const svn_test_opts_t *opts, + apr_pool_t *pool) +{ + svn_fs_t *fs; + svn_fs_txn_t *txn; + svn_fs_root_t *txn_root; + svn_fs_root_t *rev_root; + svn_revnum_t youngest_rev; + svn_fs_root_t *copy_root; + const char *copy_path; + + /* Prepare a filesystem. */ + SVN_ERR(svn_test__create_fs(&fs, "test-closest-copy-file-replaced-with-dir", + opts, pool)); + + youngest_rev = 0; + + /* Modeled after the case described in the thread: + "[PATCH] A test for "Can't get entries" error" + https://lists.apache.org/thread.html/693a95b0da834387e78a7f08df2392b634397d32f37428c81c02f8c5@%3Cdev.subversion.apache.org%3E + */ + /* r1: Add a directory with a file. */ + SVN_ERR(svn_fs_begin_txn2(&txn, fs, youngest_rev, 0, pool)); + SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool)); + SVN_ERR(svn_fs_make_dir(txn_root, "/A", pool)); + SVN_ERR(svn_fs_make_file(txn_root, "/A/mu", pool)); + SVN_ERR(test_commit_txn(&youngest_rev, txn, NULL, pool)); + SVN_TEST_INT_ASSERT(youngest_rev, 1); + + /* r2: Copy the directory. */ + SVN_ERR(svn_fs_begin_txn2(&txn, fs, youngest_rev, 0, pool)); + SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool)); + SVN_ERR(svn_fs_revision_root(&rev_root, fs, 1, pool)); + SVN_ERR(svn_fs_copy(rev_root, "/A", txn_root, "/B", pool)); + SVN_ERR(test_commit_txn(&youngest_rev, txn, NULL, pool)); + SVN_TEST_INT_ASSERT(youngest_rev, 2); + + /* r3: Delete the file. */ + SVN_ERR(svn_fs_begin_txn2(&txn, fs, youngest_rev, 0, pool)); + SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool)); + SVN_ERR(svn_fs_delete(txn_root, "/B/mu", pool)); + SVN_ERR(test_commit_txn(&youngest_rev, txn, NULL, pool)); + SVN_TEST_INT_ASSERT(youngest_rev, 3); + + /* r4: Replace the file with a new directory containing a file. */ + SVN_ERR(svn_fs_begin_txn2(&txn, fs, youngest_rev, 0, pool)); + SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool)); + SVN_ERR(svn_fs_make_dir(txn_root, "/B/mu", pool)); + SVN_ERR(svn_fs_make_file(txn_root, "/B/mu/iota", pool)); + SVN_ERR(test_commit_txn(&youngest_rev, txn, NULL, pool)); + SVN_TEST_INT_ASSERT(youngest_rev, 4); + + /* Test a couple of svn_fs_closest_copy() calls; the second call used + to fail with an unexpected SVN_ERR_FS_NOT_DIRECTORY error. */ + + SVN_ERR(svn_fs_revision_root(&rev_root, fs, 2, pool)); + SVN_ERR(svn_fs_closest_copy(©_root, ©_path, rev_root, "/B/mu", pool)); + + SVN_TEST_ASSERT(copy_root != NULL); + SVN_TEST_INT_ASSERT(svn_fs_revision_root_revision(copy_root), 2); + SVN_TEST_STRING_ASSERT(copy_path, "/B"); + + SVN_ERR(svn_fs_revision_root(&rev_root, fs, 4, pool)); + SVN_ERR(svn_fs_closest_copy(©_root, ©_path, rev_root, "/B/mu/iota", pool)); + + SVN_TEST_ASSERT(copy_root == NULL); + SVN_TEST_ASSERT(copy_path == NULL); + + return SVN_NO_ERROR; +} + /* ------------------------------------------------------------------------ */ /* The test table. */ @@ -7513,6 +7585,8 @@ static struct svn_test_descriptor_t test "test rep-sharing on content rather than SHA1"), SVN_TEST_OPTS_PASS(closest_copy_test_svn_4677, "test issue SVN-4677 regression"), + SVN_TEST_OPTS_PASS(test_closest_copy_file_replaced_with_dir, + "svn_fs_closest_copy after replacing file with dir"), SVN_TEST_NULL };
