You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@subversion.apache.org by Joshua Kordani <jk...@roboticresearch.com> on 2021/08/06 23:41:42 UTC
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
jkordani@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
Posted by Joshua Kordani <jk...@roboticresearch.com>.
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
jkordani@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
Posted by Joshua Kordani <jk...@roboticresearch.com>.
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
jkordani@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
Posted by Stefan Sperling <st...@elego.de>.
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.
Re: Segfault in libsvn_client/conflicts.c
Posted by Joshua Kordani <jk...@roboticresearch.com>.
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
jkordani@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
Posted by Stefan Sperling <st...@elego.de>.
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
Posted by Joshua Kordani <jk...@roboticresearch.com>.
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
jkordani@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
Posted by Stefan Sperling <st...@elego.de>.
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
Posted by Daniel Sahlberg <da...@gmail.com>.
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 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
Posted by Joshua Kordani <jk...@roboticresearch.com>.
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
jkordani@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
Posted by Stefan Sperling <st...@elego.de>.
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
Posted by Joshua Kordani <jk...@roboticresearch.com>.
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
jkordani@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
Posted by Stefan Sperling <st...@elego.de>.
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);