On Fri, Aug 06, 2021 at 07:41:42PM -0400, Joshua Kordani wrote:
I am merging two branches together, A into B. In A, I deleted a
file and
cherrypicked that onto B. Later, I want to merge at the toplevel
from A
into B, and I get a segfault when svn tries to figure out how to
resolve the
resulting tree conflict (file missing, incoming file deleted).
I have narrowed it down to this line. At first, the string pointed
to by
related_repos_relpath is set to null, and related_peg_rev is set to
SVN_INVALID_REVNUM. No matter the exit paths, related_peg_rev will
either
be a valid rev, or SVN_INVALID_REVNUM. But, if we can't find a
related node
in line 2473, we bail, but related_repos_relpath is set to null.
It seems
like there are only two ways that the function will exit. In
successful
cases, the related_repos_relpath is updated appropriately. Otherwise I
guess that when the search fails, the conflict resolution stops and
the
original value of related_repos_relpath is unimportant.
I release all rights to this patch, or assign the rights to the
project
license as appropriate.
Hi Joshua,
Thank you very much for tracking this down!
Index: subversion/libsvn_client/conflicts.c
===================================================================
--- subversion/libsvn_client/conflicts.c (revision 1875623)
+++ subversion/libsvn_client/conflicts.c (working copy)
@@ -2422,7 +2422,7 @@
svn_node_kind_t related_node_kind;
svn_ra_session_t *ra_session;
- *related_repos_relpath = NULL;
+ /* *related_repos_relpath = NULL; */
*related_peg_rev = SVN_INVALID_REVNUM;
SVN_ERR(svn_client_conflict_get_repos_info(&repos_root_url, NULL,
By convention, functions are supposed to initialize their output
arguments.
Otherwise we run a risk of callers using uninitialized variables which
could be even worse than a NULL pointer since it might clobber
unrelated
memory rather than crashing immediately.
I would introduce a separate variable in the caller such that the
original pointer is left untouched if we hit the case you have found.
Also, the caller must deal with the case where the search for the
related node fails.
Does this alternative patch fix the crash for you, too?
Index: subversion/libsvn_client/conflicts.c
===================================================================
--- subversion/libsvn_client/conflicts.c (revision 1892057)
+++ subversion/libsvn_client/conflicts.c (working copy)
@@ -2847,13 +2847,27 @@
conflict_tree_get_details_local_missing(svn_client
/* Make sure we're going to search the related node in a
revision where
* it exists. The younger incoming node might have been deleted
in HEAD. */
if (related_repos_relpath != NULL && related_peg_rev !=
SVN_INVALID_REVNUM)
- SVN_ERR(find_related_node(
- &related_repos_relpath, &related_peg_rev,
- related_repos_relpath, related_peg_rev,
- (old_rev < new_rev ? old_repos_relpath :
new_repos_relpath),
- (old_rev < new_rev ? old_rev : new_rev),
- conflict, ctx, scratch_pool, scratch_pool));
+ {
+ const char *older_related_repos_relpath;
+ svn_revnum_t older_related_peg_rev;
+ SVN_ERR(find_related_node(
+ &older_related_repos_relpath, &older_related_peg_rev,
+ related_repos_relpath, related_peg_rev,
+ (old_rev < new_rev ? old_repos_relpath :
new_repos_relpath),
+ (old_rev < new_rev ? old_rev : new_rev),
+ conflict, ctx, scratch_pool, scratch_pool));
+ if (older_related_repos_relpath != NULL &&
+ older_related_peg_rev != SVN_INVALID_REVNUM)
+ {
+ related_repos_relpath = older_related_repos_relpath;
+ related_peg_rev = older_related_peg_rev;
+ }
+ }
+ /* Bail if we are unable to find the related node. */
+ if (related_repos_relpath == NULL || related_peg_rev ==
SVN_INVALID_REVNUM)
+ return SVN_NO_ERROR;
+
/* Set END_REV to our best guess of the nearest YCA revision. */
url = svn_path_url_add_component2(repos_root_url,
related_repos_relpath,
scratch_pool);
