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