| Message ID | 20241112083400.GA3529122@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | double-free with git fetch --prefetch | expand |
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
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> --- builtin/fetch.c | 1 + t/t5582-fetch-negative-refspec.sh | 4 ++++ 2 files changed, 5 insertions(+)