fetch: adjust refspec->raw_nr when filtering prefetch refspecs

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@epic.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 80a64d0d269ed60708d7533977f9ba5c51a4b084..1c59e1c256c151f905053c2e89ba6cceb838171f 100644 (file)
--- 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 7fa54a4029ad4443ee78ff9de44068c637feeec8..b21bf2057d60fe12f97a00b37cbf986a8ce94fe3 100755 (executable)
--- 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