Skip to content

Commit e1a1ec7

Browse files
ptarjanptarjan
committed
promisor-remote: prevent lazy-fetch recursion in child fetch
fetch_objects() spawns a child `git fetch` to lazily fill in missing objects. That child's index-pack, when it receives a thin pack containing a REF_DELTA against a still-missing base, explicitly calls promisor_remote_get_direct() — which is fetch_objects() again. If the base is truly unavailable (e.g. because many refs in the local store point at objects that have been garbage-collected on the server), each recursive lazy-fetch can trigger another, leading to unbounded recursion with runaway disk and process consumption. The GIT_NO_LAZY_FETCH guard (introduced by e6d5479 (git: add --no-lazy-fetch option, 2021-08-31)) already exists at the top of fetch_objects(); the missing piece is propagating it into the child fetch's environment. Add that propagation so the child's index-pack, if it encounters a REF_DELTA against a missing base, hits the guard and fails fast instead of recursing. Depth-1 lazy fetch (the whole point of fetch_objects()) is unaffected: only the child and its descendants see the variable. With negotiationAlgorithm=noop the client advertises no "have" lines, so a well-behaved server sends requested objects un-deltified or deltified only against objects in the same pack; the child's index-pack should never need a depth-2 fetch. If it does, the server response was broken or the local store is already corrupt, and further fetching would not help. Add a test that injects a thin pack containing a REF_DELTA against a missing base via HTTP, triggering the recursion path through index-pack's promisor_remote_get_direct() call. With the fix, the child's fetch_objects() sees GIT_NO_LAZY_FETCH=1 and blocks the depth-2 fetch with a "lazy fetching disabled" warning. Signed-off-by: Paul Tarjan <github@paulisageek.com>
1 parent 7b2bccb commit e1a1ec7

File tree

2 files changed

+67
-0
lines changed

2 files changed

+67
-0
lines changed

‎promisor-remote.c‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,13 @@ static int fetch_objects(struct repository *repo,
4242
child.in = -1;
4343
if (repo != the_repository)
4444
prepare_other_repo_env(&child.env, repo->gitdir);
45+
/*
46+
* Prevent the child's index-pack from recursing back into
47+
* fetch_objects() when resolving REF_DELTA bases it does not
48+
* have. With noop negotiation the server should never need
49+
* to send such deltas, so a depth-2 fetch would not help.
50+
*/
51+
strvec_pushf(&child.env, "%s=1", NO_LAZY_FETCH_ENVIRONMENT);
4552
strvec_pushl(&child.args, "-c", "fetch.negotiationAlgorithm=noop",
4653
"fetch", remote_name, "--no-tags",
4754
"--no-write-fetch-head", "--recurse-submodules=no",

‎t/t5616-partial-clone.sh‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -907,6 +907,66 @@ test_expect_success PERL_TEST_HELPERS 'tolerate server sending REF_DELTA against
907907
! test -e "$HTTPD_ROOT_PATH/one-time-script"
908908
'
909909

910+
test_expect_success PERL_TEST_HELPERS 'lazy-fetch of REF_DELTA with missing base does not recurse' '
911+
SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server" &&
912+
rm -rf "$SERVER" repo &&
913+
test_create_repo "$SERVER" &&
914+
test_config -C "$SERVER" uploadpack.allowfilter 1 &&
915+
test_config -C "$SERVER" uploadpack.allowanysha1inwant 1 &&
916+
917+
# Create a commit with 2 blobs to be used as delta base and content.
918+
for i in $(test_seq 10)
919+
do
920+
echo "this is a line" >>"$SERVER/foo.txt" &&
921+
echo "this is another line" >>"$SERVER/bar.txt" || return 1
922+
done &&
923+
git -C "$SERVER" add foo.txt bar.txt &&
924+
git -C "$SERVER" commit -m initial &&
925+
BLOB_FOO=$(git -C "$SERVER" rev-parse HEAD:foo.txt) &&
926+
BLOB_BAR=$(git -C "$SERVER" rev-parse HEAD:bar.txt) &&
927+
928+
# Partial clone with blob:none. The client has commits and
929+
# trees but no blobs.
930+
test_config -C "$SERVER" protocol.version 2 &&
931+
git -c protocol.version=2 clone --no-checkout \
932+
--filter=blob:none $HTTPD_URL/one_time_script/server repo &&
933+
934+
# Sanity check: client does not have either blob locally.
935+
git -C repo rev-list --objects --ignore-missing \
936+
-- $BLOB_FOO >objlist &&
937+
test_line_count = 0 objlist &&
938+
939+
# Craft a thin pack where BLOB_FOO is a REF_DELTA against
940+
# BLOB_BAR. Since the client has neither blob (blob:none
941+
# filter), the delta base will be missing. This simulates a
942+
# misbehaving server that sends REF_DELTA against an object
943+
# the client does not have.
944+
test-tool -C "$SERVER" pack-deltas --num-objects=1 >thin.pack <<-EOF &&
945+
REF_DELTA $BLOB_FOO $BLOB_BAR
946+
EOF
947+
948+
replace_packfile thin.pack &&
949+
950+
# Trigger a lazy fetch for BLOB_FOO. The child fetch spawned
951+
# by fetch_objects() receives our crafted thin pack. Its
952+
# index-pack encounters the missing delta base (BLOB_BAR) and
953+
# tries to lazy-fetch it via promisor_remote_get_direct().
954+
#
955+
# With the fix: fetch_objects() propagates GIT_NO_LAZY_FETCH=1
956+
# to the child, so the depth-2 fetch is blocked and we see the
957+
# "lazy fetching disabled" warning. The object cannot be
958+
# resolved, so cat-file fails.
959+
#
960+
# Without the fix: the depth-2 fetch would proceed, potentially
961+
# recursing unboundedly with a persistently misbehaving server.
962+
test_must_fail git -C repo -c protocol.version=2 \
963+
cat-file -p $BLOB_FOO 2>err &&
964+
test_grep "lazy fetching disabled" err &&
965+
966+
# Ensure that the one-time-script was used.
967+
! test -e "$HTTPD_ROOT_PATH/one-time-script"
968+
'
969+
910970
# DO NOT add non-httpd-specific tests here, because the last part of this
911971
# test script is only executed when httpd is available and enabled.
912972

0 commit comments

Comments
 (0)