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

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

From: Mark Eichin <eichin_at_gmail.com>
Date: Thu, 12 Mar 2009 23:59:08 -0400

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...)

The particular segfault was do_directory_merge calling
drive_merge_report_editor which calls find_nearest_ancestor, which

      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...

Index: subversion/src/subversion/libsvn_client/merge.c
--- subversion/src/subversion/libsvn_client/merge.c (revision 93056)
+++ subversion/src/subversion/libsvn_client/merge.c (working copy)
@@ -1609,10 +1609,15 @@
       svn_client__merge_path_t *child =
         APR_ARRAY_IDX(children_with_mergeinfo, i, svn_client__merge_path_t *);
+ // might want to replace this with if (!child || child->absent)
+ if (!child)
+ continue;
       if (svn_path_is_ancestor(child->path, path)
           && (svn_path_compare_paths(child->path, path) != 0
               || path_is_own_ancestor))
         ancestor_index = i;
   return ancestor_index;
@@ -3888,14 +3893,20 @@
            looking_for_child ? j < children_with_mergeinfo->nelts : j >= 0;
            j = looking_for_child ? j + 1 : j - 1)
+ int cmp;
           /* If this potential child is neither the child we are looking for
              or another one of PARENT's children then CHILD_PATH doesn't
              exist in CHILDREN_WITH_MERGEINFO. */
           svn_client__merge_path_t *potential_child_or_parent =
             APR_ARRAY_IDX(children_with_mergeinfo, j,
                           svn_client__merge_path_t *);
- int cmp = svn_path_compare_paths(path,
- potential_child_or_parent->path);
+ if (!potential_child_or_parent)
+ continue;
+ cmp = svn_path_compare_paths(path,
+ potential_child_or_parent->path);
           if (cmp == 0)
               /* Found child or parent. */

_Mark_ <eichin_at_thok.org> <eichin_at_gmail.com>
Received on 2009-03-13 04:59:24 CET

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