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>
Tamir Duberstein committed
Jun 12, 2026 at 17:49 UTC
5bd39784cda151203aa6d97e24c21bf7fddc11de
3 files changed
+29
-4
commit-reach.c
+9
-3
@@ -757,7 +757,8 @@ static int in_commit_list(const struct commit_list *want, struct commit *c)
757
758
/*
759
* Test whether the candidate is contained in the list.
760
- * Do not recurse to find out, though, but return -1 if inconclusive.
760
+ * Do not recurse to find out, though, but return CONTAINS_UNKNOWN if
761
+ * inconclusive.
762
*/
763
static enum contains_result contains_test(struct commit *candidate,
764
const struct commit_list *want,
@@ -814,6 +815,7 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
815
if (result != CONTAINS_UNKNOWN)
816
return result;
817
818
+ *contains_cache_at(cache, candidate) = CONTAINS_IN_PROGRESS;
819
push_to_contains_stack(candidate, &contains_stack);
820
while (contains_stack.nr) {
821
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,
827
contains_stack.nr--;
828
}
829
/*
828
- * If we just popped the stack, parents->item has been marked,
829
- * therefore contains_test will return a meaningful yes/no.
830
+ * A parent may have just been popped and marked, or may still
831
+ * be active when replacement refs create a cycle.
832
*/
833
else switch (contains_test(parents->item, want, cache, cutoff)) {
834
case CONTAINS_YES:
@@ -836,7 +838,11 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
838
case CONTAINS_NO:
839
entry->parents = parents->next;
840
break;
841
+ case CONTAINS_IN_PROGRESS:
842
+ die(_("commit ancestry contains a cycle"));
843
case CONTAINS_UNKNOWN:
844
+ *contains_cache_at(cache, parents->item) =
845
+ CONTAINS_IN_PROGRESS;
846
push_to_contains_stack(parents->item, &contains_stack);
847
break;
848
}
commit-reach.h
+2
-1
@@ -73,7 +73,8 @@ int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid);
73
enum contains_result {
74
CONTAINS_UNKNOWN = 0,
75
CONTAINS_NO,
76
- CONTAINS_YES
76
+ CONTAINS_YES,
77
+ CONTAINS_IN_PROGRESS
78
};
79
80
define_commit_slab(contains_cache, enum contains_result);
t/t7004-tag.sh
+18
@@ -1611,6 +1611,24 @@ test_expect_success 'checking that first commit is in all tags (hash)' '
1611
test_cmp expected actual
1612
'
1613
1614
+test_expect_success 'tag --contains rejects cyclic replacement histories' '
1615
+ first=$(git rev-parse HEAD~2) &&
1616
+ second=$(git rev-parse HEAD~) &&
1617
+ third=$(git rev-parse HEAD) &&
1618
+ test_when_finished "
1619
+ git replace -d $first &&
1620
+ git replace -d $third &&
1621
+ git tag -d cycle-a cycle-b
1622
+ " &&
1623
+ git tag cycle-a "$first" &&
1624
+ git tag cycle-b "$third" &&
1625
+ git replace --graft "$first" "$third" "$second" &&
1626
+ git replace --graft "$third" "$first" &&
1627
+ test_must_fail git tag --contains="$second" --list "cycle-*" \
1628
+ >/dev/null 2>err &&
1629
+ test_grep "fatal: commit ancestry contains a cycle" err
1630
+'
1631
+
1632
# other ways of specifying the commit
1633
test_expect_success 'checking that first commit is in all tags (tag)' '
1634
cat >expected <<-\EOF &&