You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Sahlberg <da...@gmail.com> on 2021/08/21 19:38:56 UTC
Re: svn commit: r1892471 - in /subversion/trunk/subversion:
libsvn_client/conflicts.c libsvn_wc/wc_db.c tests/cmdline/merge_tree_conflict_tests.py
Den fre 20 aug. 2021 kl 14:39 skrev <st...@apache.org>:
> Author: stsp
> Date: Fri Aug 20 12:39:20 2021
> New Revision: 1892471
>
> URL: http://svn.apache.org/viewvc?rev=1892471&view=rev
> Log:
> Fix an assertion failure triggered by the conflict resolver.
>
> The resolver may need to deal with nodes of type 'none' but was using
> a token map which does not support the 'none' type. Switch to a token
> map which supports the 'none' type. This makes the regression test
> added in r1892470 pass.
>
> The above fix uncovered a different problem where the wc_move_targets
> pointer in conflict details for a "local missing" node was left as NULL.
> The assumption throughout the resolver is that this variable is initialized
> to an empty array instead. Do this to avoid a NULL-deref which would
> otherwise be triggered by the same regression test.
>
> * subversion/libsvn_client/conflicts.c
> (conflict_tree_get_details_local_missing): Always initialize the
> details->wc_move_targets variable.
>
This change seems to cause an error in conflicts-test#45:
# cat fails.log
[[[
subversion/tests/svn_test_main.c:475: (apr_err=SVN_ERR_TEST_FAILED)
svn_tests: E200006: Test crashed (run in debugger with '--allow-segfaults')
FAIL: conflicts-test 45: cherry-pick edit from moved directory
]]]
[[[
W: Tree conflict on
'svn-test-work/working_copies/tree_conflict_tests-26/A1/B2':
Changes destined for a directory arrived during merge of
'^/A/B2:4'.
No such file or directory was found in the merge target working copy.
The directory '^/A/B' was moved to '^/A/B2' in r3 by jrandom.
W: CMD: /home/daniel/svn_1.14.x_2/subversion/svn/svn merge -c4 '^/A'
svn-test-work/working_copies/tree_conflict_tests-26/A1 --config-dir
/home/daniel/svn_1.14.x_2/subversion/tests/cmdline/svn-test-work/local_tmp/config_tree_conflict_tests-26
--force-interactive --password rayjandom --no-auth-cache --username jrandom
terminated by signal 11
W: CWD: /home/daniel/svn_1.14.x_2/subversion/tests/cmdline
W: EXCEPTION: SVNProcessTerminatedBySignal
Traceback (most recent call last):
File
"/home/daniel/svn_1.14.x_2/subversion/tests/cmdline/svntest/main.py", line
1927, in run
rc = self.pred.run(sandbox)
File
"/home/daniel/svn_1.14.x_2/subversion/tests/cmdline/svntest/testcase.py",
line 178, in run
result = self.func(sandbox)
File
"/home/daniel/svn_1.14.x_2/subversion/tests/cmdline/tree_conflict_tests.py",
line 1537, in local_missing_dir_endless_loop
main.run_svn("Tree conflict on '%s'" % sbox.ospath("A1/B2"),
File
"/home/daniel/svn_1.14.x_2/subversion/tests/cmdline/svntest/main.py", line
817, in run_svn
return run_command(svn_binary, error_expected, False,
File
"/home/daniel/svn_1.14.x_2/subversion/tests/cmdline/svntest/main.py", line
444, in run_command
return run_command_stdin(command, error_expected, 0, binary_mode,
File
"/home/daniel/svn_1.14.x_2/subversion/tests/cmdline/svntest/main.py", line
616, in run_command_stdin
exit_code, stdout_lines, stderr_lines = spawn_process(command,
File
"/home/daniel/svn_1.14.x_2/subversion/tests/cmdline/svntest/main.py", line
591, in spawn_process
stdout_lines, stderr_lines, exit_code = wait_on_pipe(kid, binary_mode)
File
"/home/daniel/svn_1.14.x_2/subversion/tests/cmdline/svntest/main.py", line
557, in wait_on_pipe
raise SVNProcessTerminatedBySignal
svntest.main.SVNProcessTerminatedBySignal
FAIL: tree_conflict_tests.py 26: endless loop when resolving local-missing
dir
]]]
(END)
I have tried it both in branches/1.14.x and in trunk with the same result.
* subversion/libsvn_wc/wc_db.c
> (svn_wc__db_find_working_nodes_with_basename,
> svn_wc__db_find_copies_of_repos_path): Use kind_map_none instead of
> the regular kind_map which doesn't support the 'none' type.
>
> * subversion/tests/cmdline/merge_tree_conflict_tests.py
> (merge_local_missing_node_kind_none): Remove XFail marker.
>
> Reported by: Joshua Kordani (jkordani {AT} roboticresearch dot com)
>
> Modified:
> subversion/trunk/subversion/libsvn_client/conflicts.c
> subversion/trunk/subversion/libsvn_wc/wc_db.c
> subversion/trunk/subversion/tests/cmdline/merge_tree_conflict_tests.py
>
> Modified: subversion/trunk/subversion/libsvn_client/conflicts.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/conflicts.c?rev=1892471&r1=1892470&r2=1892471&view=diff
>
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/conflicts.c (original)
> +++ subversion/trunk/subversion/libsvn_client/conflicts.c Fri Aug 20
> 12:39:20 2021
> @@ -3028,12 +3028,12 @@ conflict_tree_get_details_local_missing(
> deleted_basename,
> conflict->pool);
> details->moves = moves;
> + details->wc_move_targets = apr_hash_make(conflict->pool);
> if (details->moves != NULL)
> {
> apr_pool_t *iterpool;
> int i;
>
> - details->wc_move_targets = apr_hash_make(conflict->pool);
> iterpool = svn_pool_create(scratch_pool);
> for (i = 0; i < details->moves->nelts; i++)
> {
>
I have not investigated further (ENOTIME right now) but I presume some
other part of the code expects wc_move_targets to be NULL.
> Modified: subversion/trunk/subversion/libsvn_wc/wc_db.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.c?rev=1892471&r1=1892470&r2=1892471&view=diff
>
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/wc_db.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/wc_db.c Fri Aug 20 12:39:20 2021
> @@ -16732,7 +16732,7 @@ svn_wc__db_find_working_nodes_with_basen
> SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
>
> STMT_SELECT_PRESENT_HIGHEST_WORKING_NODES_BY_BASENAME_AND_KIND));
> SVN_ERR(svn_sqlite__bindf(stmt, "ist", wcroot->wc_id, basename,
> - kind_map, kind));
> + kind_map_none, kind));
> SVN_ERR(svn_sqlite__step(&have_row, stmt));
>
> *local_abspaths = apr_array_make(result_pool, 1, sizeof(const char *));
> @@ -16776,7 +16776,7 @@ svn_wc__db_find_copies_of_repos_path(apr
> SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
> STMT_SELECT_COPIES_OF_REPOS_RELPATH));
> SVN_ERR(svn_sqlite__bindf(stmt, "ist", wcroot->wc_id, repos_relpath,
> - kind_map, kind));
> + kind_map_none, kind));
> SVN_ERR(svn_sqlite__step(&have_row, stmt));
>
> *local_abspaths = apr_array_make(result_pool, 1, sizeof(const char *));
>
> Modified:
> subversion/trunk/subversion/tests/cmdline/merge_tree_conflict_tests.py
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/merge_tree_conflict_tests.py?rev=1892471&r1=1892470&r2=1892471&view=diff
>
> ==============================================================================
> --- subversion/trunk/subversion/tests/cmdline/merge_tree_conflict_tests.py
> (original)
> +++ subversion/trunk/subversion/tests/cmdline/merge_tree_conflict_tests.py
> Fri Aug 20 12:39:20 2021
> @@ -2364,7 +2364,6 @@ def spurios_tree_conflict_with_added_fil
> [], False, True, '--reintegrate',
> sbox.ospath('A_branch'))
>
> -@XFail()
> def merge_local_missing_node_kind_none(sbox):
> "crash in resolver triggered by none-type node"
>
Kind regards,
Daniel Sahlberg
Re: svn commit: r1892471 - in /subversion/trunk/subversion:
libsvn_client/conflicts.c libsvn_wc/wc_db.c tests/cmdline/merge_tree_conflict_tests.py
Posted by Daniel Sahlberg <da...@gmail.com>.
Den mån 23 aug. 2021 kl 11:16 skrev Stefan Sperling <st...@elego.de>:
> On Sat, Aug 21, 2021 at 09:38:56PM +0200, Daniel Sahlberg wrote:
> > > @@ -3028,12 +3028,12 @@ conflict_tree_get_details_local_missing(
> > >
> deleted_basename,
> > > conflict->pool);
> > > details->moves = moves;
> > > + details->wc_move_targets = apr_hash_make(conflict->pool);
> > > if (details->moves != NULL)
> > > {
> > > apr_pool_t *iterpool;
> > > int i;
> > >
> > > - details->wc_move_targets = apr_hash_make(conflict->pool);
> > > iterpool = svn_pool_create(scratch_pool);
> > > for (i = 0; i < details->moves->nelts; i++)
> > > {
> > >
> >
> > I have not investigated further (ENOTIME right now) but I presume some
> > other part of the code expects wc_move_targets to be NULL.
>
> The problem is that some parts of the code try to search the now non-NULL
> hash map with a NULL key because they lack checks for NULL keys.
> I will commit a fix shortly.
>
Thanks!
I can confirm that the test suite now passes.
I'm going to upgrade my vote to +0, not because I have any concerns but
because I don't feel confident enough reviewing the C code to vote +1.
Kind regards,
Daniel Sahlberg
Re: svn commit: r1892471 - in /subversion/trunk/subversion:
libsvn_client/conflicts.c libsvn_wc/wc_db.c tests/cmdline/merge_tree_conflict_tests.py
Posted by Daniel Sahlberg <da...@gmail.com>.
Den mån 23 aug. 2021 kl 11:16 skrev Stefan Sperling <st...@elego.de>:
> On Sat, Aug 21, 2021 at 09:38:56PM +0200, Daniel Sahlberg wrote:
> > > @@ -3028,12 +3028,12 @@ conflict_tree_get_details_local_missing(
> > >
> deleted_basename,
> > > conflict->pool);
> > > details->moves = moves;
> > > + details->wc_move_targets = apr_hash_make(conflict->pool);
> > > if (details->moves != NULL)
> > > {
> > > apr_pool_t *iterpool;
> > > int i;
> > >
> > > - details->wc_move_targets = apr_hash_make(conflict->pool);
> > > iterpool = svn_pool_create(scratch_pool);
> > > for (i = 0; i < details->moves->nelts; i++)
> > > {
> > >
> >
> > I have not investigated further (ENOTIME right now) but I presume some
> > other part of the code expects wc_move_targets to be NULL.
>
> The problem is that some parts of the code try to search the now non-NULL
> hash map with a NULL key because they lack checks for NULL keys.
> I will commit a fix shortly.
>
Thanks!
I can confirm that the test suite now passes.
I'm going to upgrade my vote to +0, not because I have any concerns but
because I don't feel confident enough reviewing the C code to vote +1.
Kind regards,
Daniel Sahlberg
Re: svn commit: r1892471 - in /subversion/trunk/subversion:
libsvn_client/conflicts.c libsvn_wc/wc_db.c
tests/cmdline/merge_tree_conflict_tests.py
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Aug 21, 2021 at 09:38:56PM +0200, Daniel Sahlberg wrote:
> > @@ -3028,12 +3028,12 @@ conflict_tree_get_details_local_missing(
> > deleted_basename,
> > conflict->pool);
> > details->moves = moves;
> > + details->wc_move_targets = apr_hash_make(conflict->pool);
> > if (details->moves != NULL)
> > {
> > apr_pool_t *iterpool;
> > int i;
> >
> > - details->wc_move_targets = apr_hash_make(conflict->pool);
> > iterpool = svn_pool_create(scratch_pool);
> > for (i = 0; i < details->moves->nelts; i++)
> > {
> >
>
> I have not investigated further (ENOTIME right now) but I presume some
> other part of the code expects wc_move_targets to be NULL.
The problem is that some parts of the code try to search the now non-NULL
hash map with a NULL key because they lack checks for NULL keys.
I will commit a fix shortly.
Re: svn commit: r1892471 - in /subversion/trunk/subversion:
libsvn_client/conflicts.c libsvn_wc/wc_db.c
tests/cmdline/merge_tree_conflict_tests.py
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Aug 21, 2021 at 09:38:56PM +0200, Daniel Sahlberg wrote:
> > @@ -3028,12 +3028,12 @@ conflict_tree_get_details_local_missing(
> > deleted_basename,
> > conflict->pool);
> > details->moves = moves;
> > + details->wc_move_targets = apr_hash_make(conflict->pool);
> > if (details->moves != NULL)
> > {
> > apr_pool_t *iterpool;
> > int i;
> >
> > - details->wc_move_targets = apr_hash_make(conflict->pool);
> > iterpool = svn_pool_create(scratch_pool);
> > for (i = 0; i < details->moves->nelts; i++)
> > {
> >
>
> I have not investigated further (ENOTIME right now) but I presume some
> other part of the code expects wc_move_targets to be NULL.
The problem is that some parts of the code try to search the now non-NULL
hash map with a NULL key because they lack checks for NULL keys.
I will commit a fix shortly.