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