Segfault in libsvn_client/conflicts.c
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. -- Joshua Kordani Senior Engineer Robotic Research, LLC jkord...@roboticresearch.com CONFIDENTIALITY NOTICE: This communication may contain private, confidential and privileged material for the sole use of the intended recipient. If you are not the intended recipient, please delete this e-mail and any attachments permanently. 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,
Re: Segfault in libsvn_client/conflicts.c
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);
Re: Segfault in libsvn_client/conflicts.c
Thank you for explaining the convention, it makes sense now that you
describe it like that.
It was delightfully short work to hunt down with the rr debugger ;-)
I'm having trouble testing it right now but I suspect it will work, I'll
get back to you on this.
There is another caller of find_related_node, and again In the few
minutes of looking it wasn't obvious to me if that callsite also needed
to correctly track the first argument remaining NULL after the call?
Line 5148.
On 8/7/21 04:48, Stefan Sperling wrote:
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);
--
Joshua Kordani
Senior Engineer
Robotic Research, LLC
jkord...@roboticresearch.com
CONFIDENTIALITY NOTICE: This communication may contain private, confidential
and privileged material for the sole use of the intended recipient. If you are
not the intended recipient, p
Re: Segfault in libsvn_client/conflicts.c
Yes it does appear to fix the issue. Thank you!
On 8/7/21 06:32, Joshua Kordani wrote:
Thank you for explaining the convention, it makes sense now that you
describe it like that.
It was delightfully short work to hunt down with the rr debugger ;-)
I'm having trouble testing it right now but I suspect it will work,
I'll get back to you on this.
There is another caller of find_related_node, and again In the few
minutes of looking it wasn't obvious to me if that callsite also
needed to correctly track the first argument remaining NULL after the
call? Line 5148.
On 8/7/21 04:48, Stefan Sperling wrote:
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);
--
Joshua Kordani
Senior Engineer
Robotic Research, LLC
jkord...@roboticresearch.com
CONFIDENTIALITY NOTICE: This communication may contain private, confidential
and privileged materia
Re: Segfault in libsvn_client/conflicts.c
On Sat, Aug 07, 2021 at 06:32:34AM -0400, Joshua Kordani wrote: > Thank you for explaining the convention, it makes sense now that you > describe it like that. > > It was delightfully short work to hunt down with the rr debugger ;-) > > I'm having trouble testing it right now but I suspect it will work, I'll get > back to you on this. > > There is another caller of find_related_node, and again In the few minutes > of looking it wasn't obvious to me if that callsite also needed to correctly > track the first argument remaining NULL after the call? Line 5148. The other caller should be fine because find_revision_for_suspected_deletion() will deal with related_repos_relpath being NULL. The problem in the case at hand is that we're passing related_repos_relpath to svn_path_url_add_component2() which cannot handle a NULL argument.
Re: Segfault in libsvn_client/conflicts.c
On 2021-08-07 17:14, Joshua Kordani wrote:
Yes it does appear to fix the issue. Thank you!
Hi Joshua (and list),
I've been trying to reproduce your issue both to have test case for
backporting the fix and possibly also adding to the test suite. However
I can't reproduce it, possibly I misunderstand some of your steps.
Do you know the exact steps to reproduce (when running without the
patch, obviously)?
Kind regards,
Daniel Sahlberg
On 8/7/21 06:32, Joshua Kordani wrote:
Thank you for explaining the convention, it makes sense now that you
describe it like that.
It was delightfully short work to hunt down with the rr debugger ;-)
I'm having trouble testing it right now but I suspect it will work,
I'll get back to you on this.
There is another caller of find_related_node, and again In the few
minutes of looking it wasn't obvious to me if that callsite also
needed to correctly track the first argument remaining NULL after
the call? Line 5148.
On 8/7/21 04:48, Stefan Sperling wrote:
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
Re: Segfault in libsvn_client/conflicts.c
On Sun, Aug 08, 2021 at 10:26:43AM +0200, Daniel Sahlberg wrote: > On 2021-08-07 17:14, Joshua Kordani wrote: > > Yes it does appear to fix the issue. Thank you! > > Hi Joshua (and list), > > I've been trying to reproduce your issue both to have test case for > backporting the fix and possibly also adding to the test suite. However I > can't reproduce it, possibly I misunderstand some of your steps. > > Do you know the exact steps to reproduce (when running without the patch, > obviously)? > > Kind regards, > > Daniel Sahlberg I have committed my patch in https://svn.apache.org/r1892118 I agree that having a test case would be nice to prevent this issue from resurfacing accidentally. It could also help us to identify other similar scenarios that don't work right. If we had a shell script which reproduced the crash I could write a corresponding C test for the resolver. What we lack is the sequence of changes and merges which trigger the issue. Joshua, do you think you would be able to provide this sequence of steps, ideally as a self-contained shell script? We offer a suitable shell script template here: https://subversion.apache.org/docs/community-guide/repro-template.sh Regards, Stefan
Re: Segfault in libsvn_client/conflicts.c
The shell script is easy. I run resolve on my codebase :-). The trick will be recreating the repo history that reproduces this problem, and that has always been gnarly to me. I could use some advice for this. Basically, what I will try to do is create a folder with files. create a branch at this point, move a file in the branch, cherry pick this commit with the move into trunk, and then try a top level merge of the branch into trunk. Those are the things I did to the repo, I'm not sure if others mucked with it. If that doesn't work, I'll have to sleuth my repo and I'll need help to do that On 8/8/21 16:38, Stefan Sperling wrote: On Sun, Aug 08, 2021 at 10:26:43AM +0200, Daniel Sahlberg wrote: On 2021-08-07 17:14, Joshua Kordani wrote: Yes it does appear to fix the issue. Thank you! Hi Joshua (and list), I've been trying to reproduce your issue both to have test case for backporting the fix and possibly also adding to the test suite. However I can't reproduce it, possibly I misunderstand some of your steps. Do you know the exact steps to reproduce (when running without the patch, obviously)? Kind regards, Daniel Sahlberg I have committed my patch in https://svn.apache.org/r1892118 I agree that having a test case would be nice to prevent this issue from resurfacing accidentally. It could also help us to identify other similar scenarios that don't work right. If we had a shell script which reproduced the crash I could write a corresponding C test for the resolver. What we lack is the sequence of changes and merges which trigger the issue. Joshua, do you think you would be able to provide this sequence of steps, ideally as a self-contained shell script? We offer a suitable shell script template here: https://subversion.apache.org/docs/community-guide/repro-template.sh Regards, Stefan -- Joshua Kordani Senior Engineer Robotic Research, LLC jkord...@roboticresearch.com CONFIDENTIALITY NOTICE: This communication may contain private, confidential and privileged material for the sole use of the intended recipient. If you are not the intended recipient, please delete this e-mail and any attachments permanently.
Re: Segfault in libsvn_client/conflicts.c
So unfortunately that simple case didn't reproduce the problem. So I need help investigating the state of things... On 8/9/21 14:09, Joshua Kordani wrote: The shell script is easy. I run resolve on my codebase :-). The trick will be recreating the repo history that reproduces this problem, and that has always been gnarly to me. I could use some advice for this. Basically, what I will try to do is create a folder with files. create a branch at this point, move a file in the branch, cherry pick this commit with the move into trunk, and then try a top level merge of the branch into trunk. Those are the things I did to the repo, I'm not sure if others mucked with it. If that doesn't work, I'll have to sleuth my repo and I'll need help to do that On 8/8/21 16:38, Stefan Sperling wrote: On Sun, Aug 08, 2021 at 10:26:43AM +0200, Daniel Sahlberg wrote: On 2021-08-07 17:14, Joshua Kordani wrote: Yes it does appear to fix the issue. Thank you! Hi Joshua (and list), I've been trying to reproduce your issue both to have test case for backporting the fix and possibly also adding to the test suite. However I can't reproduce it, possibly I misunderstand some of your steps. Do you know the exact steps to reproduce (when running without the patch, obviously)? Kind regards, Daniel Sahlberg I have committed my patch in https://svn.apache.org/r1892118 I agree that having a test case would be nice to prevent this issue from resurfacing accidentally. It could also help us to identify other similar scenarios that don't work right. If we had a shell script which reproduced the crash I could write a corresponding C test for the resolver. What we lack is the sequence of changes and merges which trigger the issue. Joshua, do you think you would be able to provide this sequence of steps, ideally as a self-contained shell script? We offer a suitable shell script template here: https://subversion.apache.org/docs/community-guide/repro-template.sh Regards, Stefan -- Joshua Kordani Senior Engineer Robotic Research, LLC jkord...@roboticresearch.com CONFIDENTIALITY NOTICE: This communication may contain private, confidential and privileged material for the sole use of the intended recipient. If you are not the intended recipient, please delete this e-mail and any attachments permanently.
Re: Segfault in libsvn_client/conflicts.c
Actually I was able to trace more of what happened in the repo, and I found a new crash ;-) I will try to script up these steps. So, I created trunk, a feature branch, and a test branch. On the test branch I created a file. I merged the test with the feature. On the feature, I moved the file to a new folder. Then I cherry pick merged that last revision onto trunk. Automatic merge resolution core dumped on that file. I run the final command with the version of svn with the patch not applied. --- Merging r12 into '.': C module1/m1file3new A module2/m1file3new --- Recording mergeinfo for merge of r12 into '.': G . --- Recording mergeinfo for merge of r12 into 'module1': U module1 Summary of conflicts: Tree conflicts: 1 Searching tree conflict details for 'module1/m1file3new' in repository: Checking r7...subversion/libsvn_subr/token.c:40: (apr_err=SVN_ERR_ASSERTION_FAIL) svn: E235000: In file 'subversion/libsvn_subr/token.c' line 40: internal malfunction Aborted (core dumped) On 8/9/21 14:38, Joshua Kordani wrote: So unfortunately that simple case didn't reproduce the problem. So I need help investigating the state of things... On 8/9/21 14:09, Joshua Kordani wrote: The shell script is easy. I run resolve on my codebase :-). The trick will be recreating the repo history that reproduces this problem, and that has always been gnarly to me. I could use some advice for this. Basically, what I will try to do is create a folder with files. create a branch at this point, move a file in the branch, cherry pick this commit with the move into trunk, and then try a top level merge of the branch into trunk. Those are the things I did to the repo, I'm not sure if others mucked with it. If that doesn't work, I'll have to sleuth my repo and I'll need help to do that On 8/8/21 16:38, Stefan Sperling wrote: On Sun, Aug 08, 2021 at 10:26:43AM +0200, Daniel Sahlberg wrote: On 2021-08-07 17:14, Joshua Kordani wrote: Yes it does appear to fix the issue. Thank you! Hi Joshua (and list), I've been trying to reproduce your issue both to have test case for backporting the fix and possibly also adding to the test suite. However I can't reproduce it, possibly I misunderstand some of your steps. Do you know the exact steps to reproduce (when running without the patch, obviously)? Kind regards, Daniel Sahlberg I have committed my patch in https://svn.apache.org/r1892118 I agree that having a test case would be nice to prevent this issue from resurfacing accidentally. It could also help us to identify other similar scenarios that don't work right. If we had a shell script which reproduced the crash I could write a corresponding C test for the resolver. What we lack is the sequence of changes and merges which trigger the issue. Joshua, do you think you would be able to provide this sequence of steps, ideally as a self-contained shell script? We offer a suitable shell script template here: https://subversion.apache.org/docs/community-guide/repro-template.sh Regards, Stefan -- Joshua Kordani Senior Engineer Robotic Research, LLC jkord...@roboticresearch.com CONFIDENTIALITY NOTICE: This communication may contain private, confidential and privileged material for the sole use of the intended recipient. If you are not the intended recipient, please delete this e-mail and any attachments permanently.
Re: Segfault in libsvn_client/conflicts.c
On Mon, Aug 09, 2021 at 02:09:40PM -0400, Joshua Kordani wrote: > The shell script is easy. I run resolve on my codebase :-). The trick will > be recreating the repo history that reproduces this problem, and that has > always been gnarly to me. I could use some advice for this. > > Basically, what I will try to do is create a folder with files. create a > branch at this point, move a file in the branch, cherry pick this commit > with the move into trunk, and then try a top level merge of the branch into > trunk. Those are the things I did to the repo, I'm not sure if others > mucked with it. If that doesn't work, I'll have to sleuth my repo and I'll > need help to do that svn log -v can help you figure out what copies and deletions have occurred. I suspect you have a case where some parent directory of the affected file has also been deleted or replaced somewhere along the line. Otherwise we should not get a NULL result when looking for the deleted node in the parent directory's history.
Re: Segfault in libsvn_client/conflicts.c
So I've traced what happened to the path that causes a segfault, but I can't reproduce it with a simple example. I created develop branch, then a test branch from it. On the test branch I added a file, and merged the toplevel of the test branch back into devel I made a release branch from devel. On devel, I deleted the file I cherrypick merged this deletion at the top level of release I merged the toplevel of release back into devel On my work repo, this causes the crash. In a small repo where I have repeated these steps, it does not. On my work repo, svn can't figure out that the deletions on each side are the same, even though I've outlined the steps here. On my simple repo, the deletion is not performed, or else recognized to be related changes somehow. When I try to use the --ignore-ancestry merge on my test repo, I get the tree conflict but the resolution doesn't cause the crash. I'm not sure what else I can do at this point. I'd love to know why svn can't detect that these deletions are the same in my work repo... This kind of problem plagues us On 8/9/21 16:10, Stefan Sperling wrote: On Mon, Aug 09, 2021 at 02:09:40PM -0400, Joshua Kordani wrote: The shell script is easy. I run resolve on my codebase :-). The trick will be recreating the repo history that reproduces this problem, and that has always been gnarly to me. I could use some advice for this. Basically, what I will try to do is create a folder with files. create a branch at this point, move a file in the branch, cherry pick this commit with the move into trunk, and then try a top level merge of the branch into trunk. Those are the things I did to the repo, I'm not sure if others mucked with it. If that doesn't work, I'll have to sleuth my repo and I'll need help to do that svn log -v can help you figure out what copies and deletions have occurred. I suspect you have a case where some parent directory of the affected file has also been deleted or replaced somewhere along the line. Otherwise we should not get a NULL result when looking for the deleted node in the parent directory's history. -- Joshua Kordani Senior Engineer Robotic Research, LLC jkord...@roboticresearch.com CONFIDENTIALITY NOTICE: This communication may contain private, confidential and privileged material for the sole use of the intended recipient. If you are not the intended recipient, please delete this e-mail and any attachments permanently.
Re: Segfault in libsvn_client/conflicts.c
On Mon, Aug 09, 2021 at 07:56:27PM -0400, Joshua Kordani wrote: > On my work repo, this causes the crash. In a small repo where I have > repeated these steps, it does not. Most likely in your test repository the history is arranged such that the deletions can be properly correllated by logging history of the various paths involved. Your test script probably creates history which "makes sense", and your work repository probably contains something unexpected which isn't aligned with "best practices" and might be difficult to spot. You will need to dig deeper into why the resolver is being mislead in your work repository. If all else fails, you could try printing URLs and revisions which are being logged by the resolver during its search. That might help with understanding why the resolver fails to correlate the deletions with one another.
