commit-reach: reject cycles in contains walk

The memoized contains traversal used by git tag assumes that commit
ancestry is acyclic. Replacement refs can violate that assumption,
causing it to keep pushing an already active commit until memory is
exhausted.

Mark commits while they are active and die if the traversal encounters
an active commit. Other failures in this walk already die through
parse_commit_or_die(); using a second reachability walk would only add
a separate policy for malformed history.

Suggested-by: Kristofer Karlsson <krka@spotify.com>
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/commit-reach.c b/commit-reach.c
index 5df471a313cf6be855b61fd9c06d2499e97bc7af..58553dff05692eaa09e219186e5d4995426aed0d 100644 (file)
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -757,7 +757,8 @@ static int in_commit_list(const struct commit_list *want, struct commit *c)
 
 /*
  * Test whether the candidate is contained in the list.
- * Do not recurse to find out, though, but return -1 if inconclusive.
+ * Do not recurse to find out, though, but return CONTAINS_UNKNOWN if
+ * inconclusive.
  */
 static enum contains_result contains_test(struct commit *candidate,
                                          const struct commit_list *want,
@@ -814,6 +815,7 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
        if (result != CONTAINS_UNKNOWN)
                return result;
 
+       *contains_cache_at(cache, candidate) = CONTAINS_IN_PROGRESS;
        push_to_contains_stack(candidate, &contains_stack);
        while (contains_stack.nr) {
                struct contains_stack_entry *entry = &contains_stack.contains_stack[contains_stack.nr - 1];
@@ -825,8 +827,8 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
                        contains_stack.nr--;
                }
                /*
-                * If we just popped the stack, parents->item has been marked,
-                * therefore contains_test will return a meaningful yes/no.
+                * A parent may have just been popped and marked, or may still
+                * be active when replacement refs create a cycle.
                 */
                else switch (contains_test(parents->item, want, cache, cutoff)) {
                case CONTAINS_YES:
@@ -836,7 +838,11 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
                case CONTAINS_NO:
                        entry->parents = parents->next;
                        break;
+               case CONTAINS_IN_PROGRESS:
+                       die(_("commit ancestry contains a cycle"));
                case CONTAINS_UNKNOWN:
+                       *contains_cache_at(cache, parents->item) =
+                               CONTAINS_IN_PROGRESS;
                        push_to_contains_stack(parents->item, &contains_stack);
                        break;
                }

diff --git a/commit-reach.h b/commit-reach.h
index 3f3a563d8a5dd11cc928beee481e914f20cd3aea..f908d305b169029a25fca81670f6f12d026284cd 100644 (file)
--- a/commit-reach.h
+++ b/commit-reach.h
@@ -73,7 +73,8 @@ int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid);
 enum contains_result {
        CONTAINS_UNKNOWN = 0,
        CONTAINS_NO,
-       CONTAINS_YES
+       CONTAINS_YES,
+       CONTAINS_IN_PROGRESS
 };
 
 define_commit_slab(contains_cache, enum contains_result);

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index d918005dd9859a3892bcccb278c8c16365c269da..67309494d283c76b0cb3fc8c3e2b2c12672bac42 100755 (executable)
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1611,6 +1611,24 @@ test_expect_success 'checking that first commit is in all tags (hash)' '
        test_cmp expected actual
 '
 
+test_expect_success 'tag --contains rejects cyclic replacement histories' '
+       first=$(git rev-parse HEAD~2) &&
+       second=$(git rev-parse HEAD~) &&
+       third=$(git rev-parse HEAD) &&
+       test_when_finished "
+               git replace -d $first &&
+               git replace -d $third &&
+               git tag -d cycle-a cycle-b
+       " &&
+       git tag cycle-a "$first" &&
+       git tag cycle-b "$third" &&
+       git replace --graft "$first" "$third" "$second" &&
+       git replace --graft "$third" "$first" &&
+       test_must_fail git tag --contains="$second" --list "cycle-*" \
+               >/dev/null 2>err &&
+       test_grep "fatal: commit ancestry contains a cycle" err
+'
+
 # other ways of specifying the commit
 test_expect_success 'checking that first commit is in all tags (tag)' '
        cat >expected <<-\EOF &&