In filter_prefetch_refspecs(), we may remove one or more refspecs if
they point into refs/tags/. When we do, we remove the item from the
refspec->items array, shifting subsequent items down, and then decrement
the refspec->nr count.
We also remove the item from the refspec->raw array, but fail to
decrement refspec->raw_nr. This leaves us with a count that is too high,
and anybody looking at the "raw" array will erroneously see either:
1. The removed entry, if there were no subsequent items to shift down.
2. A duplicate of the final entry, as everything is shifted down but
there was nothing to overwrite the final item.
The obvious culprit to run into this is calling refspec_clear(), which
will try to free the removed entry (case 1) or double-free the final
entry (case 2). But even though the bug has existed since the function
was added in 2e03115d0c (fetch: add --prefetch option, 2021-04-16), we
did not trigger it in the test suite. The --prefetch option is normally
only used with configured refspecs, and we never bother to call
refspec_clear() on those (they are stored as part of a struct remote,
which is held in a global variable).
But you could trigger case 2 manually like:
git fetch --prefetch . refs/tags/foo refs/tags/bar
Ironically you couldn't trigger case 1, because the code accidentally
leaked the string in the raw array, and the two bugs (the leak and the
double-free) cancelled out. But when we fixed the leak in ea4780307c
(fetch: free "raw" string when shrinking refspec, 2024-09-24), it became
possible to trigger that, too, with a single item:
git fetch --prefetch . refs/tags/foo
We can fix both cases by just correctly decrementing "raw_nr" when we
shrink the array. Even though we don't expect people to use --prefetch
with command-line refspecs, we'll add a test to make sure it behaves
well (like the test just before it, we're just confirming that the
filtered prefetch succeeds at all).
Reported-by: Eric Mills <ermills@xxxxxxxx>
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
builtin/fetch.c | 1 +
t/t5582-fetch-negative-refspec.sh | 4 ++++
2 files changed, 5 insertions(+)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index d9027e4dc9..9e0cabebe7 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -463,6 +463,7 @@ static void filter_prefetch_refspec(struct refspec *rs)
rs->raw[j - 1] = rs->raw[j];
}
rs->nr--;
+ rs->raw_nr--;
i--;
continue;
}
diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh
index 7fa54a4029..b21bf2057d 100755
--- a/t/t5582-fetch-negative-refspec.sh
+++ b/t/t5582-fetch-negative-refspec.sh
@@ -283,4 +283,8 @@ test_expect_success '--prefetch succeeds when refspec becomes empty' '
git -C one fetch --prefetch
'
+test_expect_success '--prefetch succeeds with empty command line refspec' '
+ git -C one fetch --prefetch origin +refs/tags/extra
+'
+
test_done
--
2.47.0.508.g57228aee23
