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.
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...
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1336234
Received on 2009-03-17 01:24:49 CET