[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn segfault - related to issue 2821 - children_with_mergeinfo invariant not obeyed (with patch)

From: Mark Eichin <eichin_at_gmail.com>
Date: Tue, 17 Mar 2009 15:34:53 -0400

Confirmed that the svn produced by backing out my original patch and
merging in 36613,36615,36631 also performs the same merge cleanly and
without segfault.

On Tue, Mar 17, 2009 at 1:44 PM, Mark Eichin <eichin_at_gmail.com> wrote:
> I'll see what I can do; possibly retrying the merge against a copy of
> the target from just before we actually committed it would work.  (I
> didn't think I could come up with a small test case either, the
> branches in question have been touched with various 1.5.x versions...
> I wonder if it's worth trying to cook up a mergeinfo-fsck or
> mergeinfo-lint to try and massage an existing repository into
> more-correct mergeinfo...)
>
> On Tue, Mar 17, 2009 at 1:28 PM, Paul Burba <ptburba_at_gmail.com> wrote:
>> On Mon, Mar 16, 2009 at 8:24 PM, Paul Burba <ptburba_at_gmail.com> wrote:
>>> On Thu, Mar 12, 2009 at 11:59 PM, Mark Eichin <eichin_at_gmail.com> wrote:
>>>> A coworker of mine (Ken Baker) got a segfault merging a branch back
>>>> into trunk, under 1.5.5, reproduced with 1.5.6.  Diving into the code
>>>> turns up a straightforard invariant issue - some code assumes that all
>>>> elements of the children_with_mergeinfo array have values, but the use
>>>> of remove_children_with_deleted_mergeinfo makes that not universally
>>>> true.  Bug (as described below) exists on trunk as well.
>>>>
>>>> (I relate this to issue 2821 because some of the places that *do*
>>>> defend against NULL were added in r30464 and r26803; issue 3067 might
>>>> also apply.  That combined with the fact that it's mergeinfo related
>>>> suggests that Paul Burba should have a look at it...)
>>>
>>> Hi Mark,
>>>
>>> Thanks for the detailed bug report and analysis.  I fixed this in
>>> r36613, but instead of adding more checks for NULL
>>> children_with_mergeinfo elements I stopped setting these elements to
>>> NULL in the first place, and just deleted them.
>>>
>>> I'll nominate this change for backport to 1.5.x and 1.6.x.
>>
>> Made a mistake in r36613, fixed in r36631.
>>
>> Mark - I should have asked this before; I was unable to work up a test
>> case that provoked the segfault, so could you try the offending merge
>> again with trunk?
>>
>> Paul
>>
>>> Paul
>>>
>>>> The particular segfault was do_directory_merge calling
>>>> drive_merge_report_editor which calls find_nearest_ancestor, which
>>>> does
>>>>
>>>>      svn_client__merge_path_t *child =
>>>>        APR_ARRAY_IDX(children_with_mergeinfo, i, svn_client__merge_path_t *);
>>>>
>>>> and immediately uses child without checking it.  Many places in
>>>> merge.c *do* check that randomly selected values in
>>>> children_with_mergeinfo are not NULL first, because
>>>> drive_merge_report_editor and remove_children_with_deleted_mergeinfo
>>>> are called in a loop, so most things *can* be called with a
>>>> children_with_mergeinfo that has cleared slots in it; the patch is
>>>> based on replicating that existing pattern.
>>>>
>>>> Fixing the first one exposes a second: do_directory_merge calls
>>>> process_children_with_new_mergeinfo which calls
>>>> find_child_with_mergeinfo which calls find_child_or_parent, which also
>>>> lacks protection (of potential_child_or_parent, same thing.)
>>>>
>>>> The following patch fixes both, and causes our merge to complete
>>>> successfully (and by inspection, correctly.)
>>>>
>>>> Rule 3 suggests a scan of the rest of merge.c - the only suspicious
>>>> case remaining is near the top of drive_merge_report_editor, in the
>>>> "if (honor_mergeinfo)" clause, but since we haven't seen that one fail
>>>> I don't know if conditions allow it to - it's also not an obvious
>>>> "search loop" pattern like the others where NULL is an obvious "not
>>>> matched" case, so we didn't touch that one.
>>>>
>>>>                        _Mark_ <eichin_at_metacarta.com>
>>>>
>>>> ps. Rule 3 of bugs: once you've fixed the bug, look for other places
>>>> you made the same mistake...
>>>
>>
>
>
>
> --
> _Mark_ <eichin_at_thok.org> <eichin_at_gmail.com>
>

-- 
_Mark_ <eichin_at_thok.org> <eichin_at_gmail.com>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1343034
Received on 2009-03-17 20:35:18 CET

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.