[PATCH v3] promisor-remote: prevent lazy-fetch recursion in child fetch

[PATCH v3] promisor-remote: prevent lazy-fetch recursion in child fetch

[Paul Tarjan via GitGitGadget]

From: Paul Tarjan <github@paulisageek.com>

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, calls
promisor_remote_get_direct() -- which is fetch_objects() again.

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.
A server that nevertheless sends REF_DELTA against a base the
client does not have is misbehaving; however the client should
not recurse unboundedly in response.

Propagate GIT_NO_LAZY_FETCH=1 into the child fetch's environment
so that if the child's index-pack encounters such a REF_DELTA, it
hits the existing guard at the top of fetch_objects() 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.

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>
---
    promisor-remote: prevent recursive lazy-fetch during index-pack
    
    Propagate GIT_NO_LAZY_FETCH=1 into the child fetch spawned by
    fetch_objects() so that index-pack cannot recurse back into lazy-fetch
    when resolving REF_DELTA bases.
    
    We hit this in production: 276 GB of promisor packs written in 90
    minutes against a ~10 GB monorepo with ~61K stale prefetch refs pointing
    at GC'd commits.
    
    Changes since v2:
    
     * Replaced env-var-proxy test with behavioral test that injects a thin
       pack containing a REF_DELTA against a missing base via HTTP,
       triggering the actual recursion path through index-pack's
       promisor_remote_get_direct() call
     * Moved test from t0411-clone-from-partial.sh to t5616-partial-clone.sh
       (requires HTTP infrastructure)
     * Dropped commit-graph comparison from commit message per review
       feedback
    
    Changes since v1:
    
     * Dropped CC: trailers from commit message (moved here for
       GitGitGadget)
     * Moved test into t0411-clone-from-partial.sh instead of a new file
     * Removed duplicate commit-message summary from PR description

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2224%2Fptarjan%2Fclaude%2Ffix-lazy-fetch-recursion-KP9Hl-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2224/ptarjan/claude/fix-lazy-fetch-recursion-KP9Hl-v3
Pull-Request: https://github.com/git/git/pull/2224

Range-diff vs v2:

 1:  907ca7a0ac ! 1:  d58ccb858f promisor-remote: prevent lazy-fetch recursion in child fetch
     @@ Commit message
      
          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.
     +    containing a REF_DELTA against a still-missing base, calls
     +    promisor_remote_get_direct() -- which is fetch_objects() again.
      
     -    The GIT_NO_LAZY_FETCH guard (introduced by e6d5479e7a (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.
     +    un-deltified or deltified only against objects in the same pack.
     +    A server that nevertheless sends REF_DELTA against a base the
     +    client does not have is misbehaving; however the client should
     +    not recurse unboundedly in response.
      
     -    This is the same bug shape that 3a1ea94a49 (commit-graph.c: no lazy
     -    fetch in lookup_commit_in_graph(), 2022-07-01) addressed at a
     -    different entry point.
     +    Propagate GIT_NO_LAZY_FETCH=1 into the child fetch's environment
     +    so that if the child's index-pack encounters such a REF_DELTA, it
     +    hits the existing guard at the top of fetch_objects() 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.
      
     -    Add a test that verifies the child fetch environment contains
     -    GIT_NO_LAZY_FETCH=1 via a reference-transaction hook.
     +    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>
      
     @@ promisor-remote.c: static int fetch_objects(struct repository *repo,
       		     "fetch", remote_name, "--no-tags",
       		     "--no-write-fetch-head", "--recurse-submodules=no",
      
     - ## t/t0411-clone-from-partial.sh ##
     -@@ t/t0411-clone-from-partial.sh: test_expect_success 'promisor lazy-fetching can be re-enabled' '
     - 	test_path_is_file script-executed
     + ## t/t5616-partial-clone.sh ##
     +@@ t/t5616-partial-clone.sh: test_expect_success PERL_TEST_HELPERS 'tolerate server sending REF_DELTA against
     + 	! test -e "$HTTPD_ROOT_PATH/one-time-script"
       '
       
     -+test_expect_success 'lazy-fetch child has GIT_NO_LAZY_FETCH=1' '
     -+	test_create_repo nolazy-server &&
     -+	test_commit -C nolazy-server foo &&
     -+	git -C nolazy-server repack -a -d --write-bitmap-index &&
     ++test_expect_success PERL_TEST_HELPERS 'lazy-fetch of REF_DELTA with missing base does not recurse' '
     ++	SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server" &&
     ++	rm -rf "$SERVER" repo &&
     ++	test_create_repo "$SERVER" &&
     ++	test_config -C "$SERVER" uploadpack.allowfilter 1 &&
     ++	test_config -C "$SERVER" uploadpack.allowanysha1inwant 1 &&
     ++
     ++	# Create a commit with 2 blobs to be used as delta base and content.
     ++	for i in $(test_seq 10)
     ++	do
     ++		echo "this is a line" >>"$SERVER/foo.txt" &&
     ++		echo "this is another line" >>"$SERVER/bar.txt" || return 1
     ++	done &&
     ++	git -C "$SERVER" add foo.txt bar.txt &&
     ++	git -C "$SERVER" commit -m initial &&
     ++	BLOB_FOO=$(git -C "$SERVER" rev-parse HEAD:foo.txt) &&
     ++	BLOB_BAR=$(git -C "$SERVER" rev-parse HEAD:bar.txt) &&
      +
     -+	git clone "file://$(pwd)/nolazy-server" nolazy-client &&
     -+	HASH=$(git -C nolazy-client rev-parse foo) &&
     -+	rm -rf nolazy-client/.git/objects/* &&
     ++	# Partial clone with blob:none. The client has commits and
     ++	# trees but no blobs.
     ++	test_config -C "$SERVER" protocol.version 2 &&
     ++	git -c protocol.version=2 clone --no-checkout \
     ++		--filter=blob:none $HTTPD_URL/one_time_script/server repo &&
      +
     -+	git -C nolazy-client config core.repositoryformatversion 1 &&
     -+	git -C nolazy-client config extensions.partialclone "origin" &&
     ++	# Sanity check: client does not have either blob locally.
     ++	git -C repo rev-list --objects --ignore-missing \
     ++		-- $BLOB_FOO >objlist &&
     ++	test_line_count = 0 objlist &&
      +
     -+	# Install a reference-transaction hook to record the env var
     -+	# as seen by processes inside the child fetch.
     -+	test_hook -C nolazy-client reference-transaction <<-\EOF &&
     -+	echo "$GIT_NO_LAZY_FETCH" >>../env-in-child
     ++	# Craft a thin pack where BLOB_FOO is a REF_DELTA against
     ++	# BLOB_BAR. Since the client has neither blob (blob:none
     ++	# filter), the delta base will be missing. This simulates a
     ++	# misbehaving server that sends REF_DELTA against an object
     ++	# the client does not have.
     ++	test-tool -C "$SERVER" pack-deltas --num-objects=1 >thin.pack <<-EOF &&
     ++	REF_DELTA $BLOB_FOO $BLOB_BAR
      +	EOF
      +
     -+	rm -f env-in-child &&
     -+	git -C nolazy-client cat-file -p "$HASH" &&
     ++	replace_packfile thin.pack &&
      +
     -+	# The hook runs inside the child fetch, which should have
     -+	# GIT_NO_LAZY_FETCH=1 in its environment.
     -+	grep "^1$" env-in-child
     ++	# Trigger a lazy fetch for BLOB_FOO. The child fetch spawned
     ++	# by fetch_objects() receives our crafted thin pack. Its
     ++	# index-pack encounters the missing delta base (BLOB_BAR) and
     ++	# tries to lazy-fetch it via promisor_remote_get_direct().
     ++	#
     ++	# With the fix: fetch_objects() propagates GIT_NO_LAZY_FETCH=1
     ++	# to the child, so the depth-2 fetch is blocked and we see the
     ++	# "lazy fetching disabled" warning. The object cannot be
     ++	# resolved, so cat-file fails.
     ++	#
     ++	# Without the fix: the depth-2 fetch would proceed, potentially
     ++	# recursing unboundedly with a persistently misbehaving server.
     ++	test_must_fail git -C repo -c protocol.version=2 \
     ++		cat-file -p $BLOB_FOO 2>err &&
     ++	test_grep "lazy fetching disabled" err &&
     ++
     ++	# Ensure that the one-time-script was used.
     ++	! test -e "$HTTPD_ROOT_PATH/one-time-script"
      +'
      +
     - test_done
     + # DO NOT add non-httpd-specific tests here, because the last part of this
     + # test script is only executed when httpd is available and enabled.
     + 


 promisor-remote.c        |  7 +++++
 t/t5616-partial-clone.sh | 60 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/promisor-remote.c b/promisor-remote.c
index 96fa215b06..35c7aab93d 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -42,6 +42,13 @@ static int fetch_objects(struct repository *repo,
 	child.in = -1;
 	if (repo != the_repository)
 		prepare_other_repo_env(&child.env, repo->gitdir);
+	/*
+	 * Prevent the child's index-pack from recursing back into
+	 * fetch_objects() when resolving REF_DELTA bases it does not
+	 * have.  With noop negotiation the server should never need
+	 * to send such deltas, so a depth-2 fetch would not help.
+	 */
+	strvec_pushf(&child.env, "%s=1", NO_LAZY_FETCH_ENVIRONMENT);
 	strvec_pushl(&child.args, "-c", "fetch.negotiationAlgorithm=noop",
 		     "fetch", remote_name, "--no-tags",
 		     "--no-write-fetch-head", "--recurse-submodules=no",
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 1e354e057f..27f131c8d9 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -907,6 +907,66 @@ test_expect_success PERL_TEST_HELPERS 'tolerate server sending REF_DELTA against
 	! test -e "$HTTPD_ROOT_PATH/one-time-script"
 '
 
+test_expect_success PERL_TEST_HELPERS 'lazy-fetch of REF_DELTA with missing base does not recurse' '
+	SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server" &&
+	rm -rf "$SERVER" repo &&
+	test_create_repo "$SERVER" &&
+	test_config -C "$SERVER" uploadpack.allowfilter 1 &&
+	test_config -C "$SERVER" uploadpack.allowanysha1inwant 1 &&
+
+	# Create a commit with 2 blobs to be used as delta base and content.
+	for i in $(test_seq 10)
+	do
+		echo "this is a line" >>"$SERVER/foo.txt" &&
+		echo "this is another line" >>"$SERVER/bar.txt" || return 1
+	done &&
+	git -C "$SERVER" add foo.txt bar.txt &&
+	git -C "$SERVER" commit -m initial &&
+	BLOB_FOO=$(git -C "$SERVER" rev-parse HEAD:foo.txt) &&
+	BLOB_BAR=$(git -C "$SERVER" rev-parse HEAD:bar.txt) &&
+
+	# Partial clone with blob:none. The client has commits and
+	# trees but no blobs.
+	test_config -C "$SERVER" protocol.version 2 &&
+	git -c protocol.version=2 clone --no-checkout \
+		--filter=blob:none $HTTPD_URL/one_time_script/server repo &&
+
+	# Sanity check: client does not have either blob locally.
+	git -C repo rev-list --objects --ignore-missing \
+		-- $BLOB_FOO >objlist &&
+	test_line_count = 0 objlist &&
+
+	# Craft a thin pack where BLOB_FOO is a REF_DELTA against
+	# BLOB_BAR. Since the client has neither blob (blob:none
+	# filter), the delta base will be missing. This simulates a
+	# misbehaving server that sends REF_DELTA against an object
+	# the client does not have.
+	test-tool -C "$SERVER" pack-deltas --num-objects=1 >thin.pack <<-EOF &&
+	REF_DELTA $BLOB_FOO $BLOB_BAR
+	EOF
+
+	replace_packfile thin.pack &&
+
+	# Trigger a lazy fetch for BLOB_FOO. The child fetch spawned
+	# by fetch_objects() receives our crafted thin pack. Its
+	# index-pack encounters the missing delta base (BLOB_BAR) and
+	# tries to lazy-fetch it via promisor_remote_get_direct().
+	#
+	# With the fix: fetch_objects() propagates GIT_NO_LAZY_FETCH=1
+	# to the child, so the depth-2 fetch is blocked and we see the
+	# "lazy fetching disabled" warning. The object cannot be
+	# resolved, so cat-file fails.
+	#
+	# Without the fix: the depth-2 fetch would proceed, potentially
+	# recursing unboundedly with a persistently misbehaving server.
+	test_must_fail git -C repo -c protocol.version=2 \
+		cat-file -p $BLOB_FOO 2>err &&
+	test_grep "lazy fetching disabled" err &&
+
+	# Ensure that the one-time-script was used.
+	! test -e "$HTTPD_ROOT_PATH/one-time-script"
+'
+
 # DO NOT add non-httpd-specific tests here, because the last part of this
 # test script is only executed when httpd is available and enabled.
 

base-commit: 7b2bccb0d58d4f24705bf985de1f4612e4cf06e5
-- 
gitgitgadget

Re: [PATCH v3] promisor-remote: prevent lazy-fetch recursion in child fetch

[Paul Tarjan]

Jeff King <peff@peff.net> writes:

> It would be nice to confirm that's what's going on, though (and it isn't
> really a thin pack). If the problem can be reproduced, I don't suppose
> we have a GIT_TRACE_PACKET output from a failing instance? That would
> confirm that we're correctly using the noop negotiation.

Yeah. I couldn't capture one live during the incident, but I did a
clean-room reproduction afterward: empty bare repo, git remote add
origin pointing at the same GitHub monorepo, then the exact argv
from fetch_objects() with a tree SHA pulled from one of the
.promisor sidecars written during the incident:

  $ git -c fetch.negotiationAlgorithm=noop fetch origin \
        --no-tags --no-write-fetch-head --recurse-submodules=no \
        --filter=blob:none --stdin <<< "$TREE_OID"

  packet:        fetch< version 2
  packet:        fetch< agent=git/github-94ec3a8682aa-Linux
  packet:        fetch< ls-refs=unborn
  packet:        fetch< fetch=shallow wait-for-done filter
  packet:        fetch< server-option
  packet:        fetch< object-format=sha1
  packet:        fetch< 0000
  packet:        fetch> command=fetch
  packet:        fetch> agent=git/2.51.2-Linux
  packet:        fetch> object-format=sha1
  packet:        fetch> 0001
  packet:        fetch> thin-pack
  packet:        fetch> no-progress
  packet:        fetch> ofs-delta
  packet:        fetch> filter blob:none
  packet:        fetch> want 086faa42a038f2e4a1b5e395cde544f8f3c6a9a5
  packet:        fetch> done
  packet:        fetch> 0000
  packet:        fetch< packfile
  packet:     sideband< PACK ...
  packet:     sideband< 0000

Zero have lines, noop negotiation doing its thing.

Two interesting bits:

1. The client sends thin-pack unconditionally (fetch-pack.c doesn't
   check what negotiation algorithm is in use). So the server is
   allowed to send a thin pack. With zero haves it has nothing to
   thin against, but nothing on the wire actually forbids it.

2. filter blob:none is on the wire -- hardcoded by fetch_objects().
   Combined with thin-pack, this gives the server a way to end up
   omitting an object that happens to be a delta base.

For this particular tree GitHub responded cleanly (5 trees, no
deltas, 2701 bytes), so whatever triggers the REF_DELTA depends on
the specific objects being fetched and how they're stored
server-side.

> But it _can_ send deltas against objects which are part of the backfill
> itself. Normally we'd send those as OFS_DELTA, because they're both in
> the same output pack. But there are cases where we might not:
>
>   - the verbatim pack-reuse code will sometimes rewrite ofs-delta into
>     ref-delta. I don't remember all of the cases where this might
>     happen. Certainly if the client hasn't claimed to support
>     ofs-deltas, but I think maybe some other cases? I'd have to dig into
>     it.

I think it's (b). What's interesting is that Jonathan Tan flagged
exactly this risk when he introduced the prefetch call.
8a30a1efd1 ("index-pack: prefetch missing REF_DELTA bases",
2019-05-14):

    Support for lazy fetching should still generally be turned off
    in index-pack because it is used as part of the lazy fetching
    process itself (if not, infinite loops may occur), but we do
    need to fetch the REF_DELTA bases. (When fetching REF_DELTA
    bases, it is unlikely that those are REF_DELTA themselves,
    because we do not send "have" when making such fetches.)

So the "infinite loops may occur" door was left open deliberately,
on the argument that it's unlikely. My patch just closes it.

Here's what I think went wrong in practice: fetch_objects()
hardcodes --filter=blob:none. When the want is a tree, the server
walks tree reachability and assembles potentially millions of tree
objects with no blobs. If pack-reuse passes through an object
stored server-side as OFS_DELTA against a blob, it comes out as
REF_DELTA and the blob base gets filtered out by blob:none.

I have a live ps snapshot from the incident showing the chain:

  3719679  git -c fetch.negotiationAlgorithm=noop fetch origin \
               --no-tags --no-write-fetch-head \
               --recurse-submodules=no --filter=blob:none --stdin
  3725911  git index-pack --stdin --fix-thin --keep=fetch-pack \
               3719679 on <host> --promisor --pack_header=2,7535361
  3726936  git -c fetch.negotiationAlgorithm=noop fetch origin \
               --no-tags --no-write-fetch-head \
               --recurse-submodules=no --filter=blob:none --stdin

--keep embeds the parent PID, so the chain is clear: lazy-fetch
(3719679) -> index-pack (3725911, 7,535,361 objects) -> depth-2
lazy-fetch (3726936). 7.5M objects is about right for wanting a
single tree with no haves and filter=blob:none -- you get the
entire commit+tree closure minus blobs. One REF_DELTA against a
filtered blob in 7.5M objects and you're recursing.

> Now there's a catch: the pack is not really thin, and so index-pack
> should not need to do an extra backfill request in order to get the base
> object.

Actually, the pack IS thin in this case -- the REF_DELTA's base is
a blob that the server filtered out because of blob:none. The base
isn't in the pack at all. fix_unresolved_deltas() correctly sees it
as unresolved and calls promisor_remote_get_direct().

> If that's what is happening, then I _think_ Paul's patch will do the
> right thing. We'd say "no, we don't have that object" without doing the
> backfill, and then eventually find it as part of the final resolution.

Right, that's what happens. The child hits the guard at the top of
fetch_objects() and bails with "lazy fetching disabled". The
depth-1 index-pack can't resolve the delta, which is the correct
outcome -- retrying the same noop fetch would just get the same
response.

I also wrote a deterministic local reproducer (no network needed)
that shows the recursion directly. Two scripts, needs python3 + git,
works on Linux and macOS. Ran it just now:

repro-depth1.sh sets up a local bare server with two similar blobs,
a blob:none partial-clone client, and a hand-crafted 68-byte thin
pack with one REF_DELTA against a promised-but-absent blob:

  === without GIT_NO_LAZY_FETCH (current git) ===
  index-pack invocations: 2
  lazy-fetch spawns:      1

  === with GIT_NO_LAZY_FETCH=1 (what the patch sets) ===
  index-pack invocations: 1
  lazy-fetch spawns:      0

repro-unbounded.sh goes further: uploadpack.packObjectsHook always
returns the same thin pack, so every lazy-fetch spawns another.
Watchdog kills it at depth 50:

  === without patch ===
  recursion depth reached: 55
  index-pack invocations:  55
  tmp_pack_* on disk:      55

  === with GIT_NO_LAZY_FETCH=1 (the patch) ===
  recursion depth:         0
  index-pack invocations:  1
  tmp_pack_* on disk:      0

Those tmp_pack_* files are the disk growth -- each index-pack
streams to a tmpfile, blocks in fix_unresolved_deltas() to
lazy-fetch the base, and never gets to rename it. In the real
incident each was ~1.4 GB.

Scripts below.

--- >8 --- repro-depth1.sh --- >8 ---

#!/bin/bash
# Deterministic depth-1 recursion proof for promisor-remote lazy-fetch bug.
# No network. Runs in ~1 second.
#
# Mechanism:
#   - Local bare server with two similar blobs; client is a blob:none
#     partial clone (both blobs promised, neither local).
#   - Craft a 68-byte thin pack by hand: one REF_DELTA object whose base
#     SHA is one of the promised-but-absent blobs.
#   - Feed to `git -C client index-pack --stdin --fix-thin`.
#   - fix_unresolved_deltas() sees the unresolved REF_DELTA, calls
#     promisor_remote_get_direct() for the base, which spawns
#     `git -c fetch.negotiationAlgorithm=noop fetch ... --stdin`.
#   - That child fetch spawns its own index-pack --fix-thin.
#   - GIT_TRACE shows the nested spawn.

set -euo pipefail

W="${TMPDIR:-/tmp}/promisor-recursion-demo"
rm -rf "$W"; mkdir -p "$W"; cd "$W"

git -c init.defaultBranch=main init -q --bare server.git
git -C server.git config uploadpack.allowFilter true
git -C server.git config uploadpack.allowAnySHA1InWant true
git -c init.defaultBranch=main init -q work
(
    cd work
    python3 -c "print('\n'.join('shared line %d ' * 4 % (i,i,i,i) for i in range(200)))" > f
    git add f; git commit -qm base
    python3 -c "print('\n'.join('shared line %d ' * 4 % (i,i,i,i) for i in range(200))); print('extra')" > f
    git add f; git commit -qm delta
    git push -q ../server.git main
)

printf '%s\n%s\n' "$(git -C work rev-parse HEAD~1:f)" "$(git -C work rev-parse HEAD:f)" | \
    git -C server.git pack-objects --stdout --delta-base-offset > both.pack

python3 << 'PYEOF'
import struct, zlib, hashlib

with open('both.pack', 'rb') as f:
    data = f.read()
assert data[:4] == b'PACK' and struct.unpack('>I', data[8:12])[0] == 2

def parse_hdr(data, pos):
    b = data[pos]; otype = (b >> 4) & 7; sz = b & 0xf; sh = 4; p = pos
    while b & 0x80:
        p += 1; b = data[p]; sz |= (b & 0x7f) << sh; sh += 7
    return otype, sz, p + 1

def zlib_end(data, pos):
    d = zlib.decompressobj(); p = pos
    while p < len(data):
        chunk = data[p:p+512]; d.decompress(chunk)
        if d.unused_data: return p + len(chunk) - len(d.unused_data)
        if d.eof: return p + len(chunk)
        p += len(chunk)
    return p

t1, s1, d1 = parse_hdr(data, 12)
if t1 == 6:
    p = d1; b = data[p]; p += 1
    while b & 0x80: b = data[p]; p += 1
    e1 = zlib_end(data, p)
else:
    e1 = zlib_end(data, d1)
t2, s2, d2 = parse_hdr(data, e1)

if t1 == 3 and t2 == 6:
    base_raw = zlib.decompress(data[d1:e1])
    delta_sz, delta_hdr_end = s2, d2
elif t1 == 6 and t2 == 3:
    base_raw = zlib.decompress(data[d2:zlib_end(data, d2)])
    delta_sz, delta_hdr_end = s1, d1
else:
    raise SystemExit(f"expected one blob + one OFS_DELTA, got types {t1},{t2}")

base_sha = hashlib.sha1(f"blob {len(base_raw)}\0".encode() + base_raw).digest()

p = delta_hdr_end; b = data[p]; p += 1
while b & 0x80: b = data[p]; p += 1
delta_zlib = data[p:zlib_end(data, p)]

sz = delta_sz
hb = [(7 << 4) | (sz & 0xf)]; sz >>= 4
while sz: hb[-1] |= 0x80; hb.append(sz & 0x7f); sz >>= 7
thin = b'PACK' + struct.pack('>II', 2, 1) + bytes(hb) + base_sha + delta_zlib
thin += hashlib.sha1(thin).digest()
with open('thin.pack', 'wb') as f: f.write(thin)
print(f"thin.pack: {len(thin)} bytes, REF_DELTA base={base_sha.hex()}")
PYEOF

git clone -q --no-local --filter=blob:none --no-checkout "file://$W/server.git" client

echo "=== without GIT_NO_LAZY_FETCH (current git) ==="
GIT_TRACE="$W/trace.txt" \
    git -C client index-pack --stdin --fix-thin < thin.pack >/dev/null 2>&1 || true
grep -E 'built-in: git (index-pack|fetch)|run_command:.*negotiationAlgorithm' trace.txt
echo "index-pack invocations: $(grep -c 'built-in: git index-pack' trace.txt)"
echo "lazy-fetch spawns:      $(grep -c 'run_command:.*negotiationAlgorithm=noop' trace.txt)"

echo ""
echo "=== with GIT_NO_LAZY_FETCH=1 (what the patch sets) ==="
rm -rf client
git clone -q --no-local --filter=blob:none --no-checkout "file://$W/server.git" client
GIT_NO_LAZY_FETCH=1 GIT_TRACE="$W/trace2.txt" \
    git -C client index-pack --stdin --fix-thin < thin.pack 2>&1 || true
echo "index-pack invocations: $(grep -c 'built-in: git index-pack' trace2.txt)"
echo "lazy-fetch spawns:      $(grep -c 'run_command:.*negotiationAlgorithm=noop' trace2.txt || echo 0)"

--- >8 --- repro-unbounded.sh --- >8 ---

#!/bin/bash
# Unbounded recursion proof. Run repro-depth1.sh first.
# uploadpack.packObjectsHook always returns the same thin pack,
# so every lazy-fetch spawns another. Watchdog kills at depth 50.

set -uo pipefail

W="${TMPDIR:-/tmp}/promisor-recursion-demo"
cd "$W"
test -f thin.pack || { echo "run repro-depth1.sh first"; exit 1; }

cat > evil-pack-objects.sh << EOF
#!/bin/sh
cat > /dev/null
cat "$W/thin.pack"
EOF
chmod +x evil-pack-objects.sh

cat > fake-global.gitconfig << EOF
[uploadpack]
    packObjectsHook = $W/evil-pack-objects.sh
EOF

rm -rf client
GIT_CONFIG_GLOBAL= git clone -q --no-local --filter=blob:none --no-checkout \
    "file://$W/server.git" client

TRACE="$W/unbounded.trace"
rm -f "$TRACE"

echo "=== spawning (watchdog kills at depth 50) ==="
GIT_TRACE="$TRACE" GIT_CONFIG_GLOBAL="$W/fake-global.gitconfig" \
    git -C client index-pack --stdin --fix-thin < thin.pack >/dev/null 2>&1 &
ROOT=$!

LIMIT=50
while kill -0 "$ROOT" 2>/dev/null; do
    N=$(grep -c 'run_command:.*negotiationAlgorithm=noop' "$TRACE" 2>/dev/null || true)
    N=${N:-0}
    if (( N >= LIMIT )); then
        echo ">>> depth $N reached, killing process tree $ROOT <<<"
        pkill -KILL -f "promisor-recursion-demo/client" 2>/dev/null || true
        kill -KILL "$ROOT" 2>/dev/null || true
        sleep 0.2
        pkill -KILL -f "promisor-recursion-demo/client" 2>/dev/null || true
        break
    fi
    sleep 0.02
done
wait "$ROOT" 2>/dev/null
EC=$?

DEPTH=$(grep -c 'run_command:.*negotiationAlgorithm=noop' "$TRACE" 2>/dev/null || true)
DEPTH=${DEPTH:-0}
IPACKS=$(grep -c 'built-in: git index-pack' "$TRACE" 2>/dev/null || true)
IPACKS=${IPACKS:-0}
TMPS=$(ls client/.git/objects/pack/tmp_pack_* 2>/dev/null | wc -l | tr -d ' ')

echo ""
echo "=== without patch (root exit $EC) ==="
echo "recursion depth reached: $DEPTH"
echo "index-pack invocations:  $IPACKS"
echo "tmp_pack_* on disk:      $TMPS"
echo ""
echo "first 4 + last 2 spawns (identical line, no termination condition):"
grep -E 'built-in: git index-pack|run_command:.*negotiationAlgorithm=noop' "$TRACE" | head -4
echo "..."
grep -E 'built-in: git index-pack|run_command:.*negotiationAlgorithm=noop' "$TRACE" | tail -2

echo ""
echo "=== with GIT_NO_LAZY_FETCH=1 (the patch) ==="
rm -rf client; rm -f "$TRACE"
GIT_CONFIG_GLOBAL= git clone -q --no-local --filter=blob:none --no-checkout \
    "file://$W/server.git" client
GIT_NO_LAZY_FETCH=1 GIT_TRACE="$TRACE" GIT_CONFIG_GLOBAL="$W/fake-global.gitconfig" \
    git -C client index-pack --stdin --fix-thin < thin.pack 2>&1 || true
CTRL_DEPTH=$(grep -c 'run_command:.*negotiationAlgorithm=noop' "$TRACE" 2>/dev/null || true)
CTRL_IPACKS=$(grep -c 'built-in: git index-pack' "$TRACE" 2>/dev/null || true)
CTRL_TMPS=$(ls client/.git/objects/pack/tmp_pack_* 2>/dev/null | wc -l | tr -d ' ')
echo "recursion depth:         ${CTRL_DEPTH:-0}"
echo "index-pack invocations:  ${CTRL_IPACKS:-0}"
echo "tmp_pack_* on disk:      $CTRL_TMPS"

rm -rf "$W/client"

Paul

Re: [PATCH v3] promisor-remote: prevent lazy-fetch recursion in child fetch

[Paul Tarjan]

Patrick Steinhardt <ps@pks.im> writes:

> I must be misunderstanding something here, but how is it that a commit
> can be garbage collected if a ref points to it? That shouldn't ever
> happen, as reachable commits should not be pruned.
>
> Or do you mean to say that the commits don't exist on the server side
> anymore?

Sloppy wording on my part — "GC'd" is wrong. These refs pointed at
commits that were promised but never materialized on the partial
clone. The ~77K broken refs looked like:

  error: refs/prefetch/remotes/origin/claude/add-azure-dependencies-EaaDn \
         does not point to a valid object!

They were created by git maintenance --task=prefetch, which runs
git fetch --prefetch --prune and writes refs/prefetch/remotes/origin/<branch>
pointing at the remote tip. On a blob:none partial clone it fetches
commit/tree metadata, but some referenced commits were never
actually downloaded before the upstream branches (ephemeral
CI/automation branches, force-pushed and deleted within days)
disappeared.

This is a red herring for the patch though. The stale prefetch refs
explain why the outer fetch got a thin pack — the client advertised
haves from promised-but-absent commits. But the recursion (depth-1
to depth-2+) is entirely inside fetch_objects() with noop
negotiation, independent of any refs.

I'll fix the commit message wording in a v4 if you'd like.

> That's certainly curious. Do you maybe have multiple remotes attached to
> the repository, or are you dropping/modifying the object filter at some
> point?
>
> All subsequent fetches need to use the same object filter as you've used
> during the initial clone, otherwise you may run into a situation as you
> have described. But in theory, Git knows to continue using the filter.

Nobody intentionally changed the filter. What happened is the
lazy-fetch child kept re-writing it. fetch_objects() hardcodes
--filter=blob:none on the child argv, and the child's
builtin/fetch.c writes the active filter to config.

23547c40 ("fetch: do not override partial clone filter", 2020)
guards this write behind a check for an already-set filter. But I
was unsetting remote.origin.partialclonefilter manually trying to
stop the storm, so the guard passed and the next lazy-fetch child
wrote it right back:

  21:45  (unset)     manual git config --unset
  22:05  blob:none   re-written by a lazy-fetch child
  22:11  (unset)     manual unset again
  23:11  blob:none   re-written again
  23:13  (unset)     manual unset
  23:28  blob:none   re-written (caught live by a config-mtime trap)

The actual mitigation was unsetting remote.origin.promisor too —
with no promisor remote, fix_unresolved_deltas() skips the prefetch
entirely.

This is arguably a separate bug: fetch_objects() should probably
pass -c remote.<name>.partialclonefilter=blob:none to override for
the single invocation, rather than --filter=blob:none which
persists to config. Not in scope for this patch, but I could follow
up separately if there's interest.

Paul

Re: [PATCH v3] promisor-remote: prevent lazy-fetch recursion in child fetch

[Junio C Hamano]

Paul Tarjan <paul@paultarjan.com> writes:

> Patrick Steinhardt <ps@pks.im> writes:
>
>> I must be misunderstanding something here, but how is it that a commit
>> can be garbage collected if a ref points to it? That shouldn't ever
>> happen, as reachable commits should not be pruned.
>>
>> Or do you mean to say that the commits don't exist on the server side
>> anymore?
>
> Sloppy wording on my part — "GC'd" is wrong. These refs pointed at
> commits that were promised but never materialized on the partial
> clone. The ~77K broken refs looked like:
> ...
> This is arguably a separate bug: fetch_objects() should probably
> pass -c remote.<name>.partialclonefilter=blob:none to override for
> the single invocation, rather than --filter=blob:none which
> persists to config. Not in scope for this patch, but I could follow
> up separately if there's interest.

So, is this topic still viable?

At least I see that v3 was not satisfactory enough from the
discussion thread, but do we know what needs updating, how much more
work is needed, and where we want to go?

For now I'll drop the copy I have (from more than a month ago) from
my tree.

Re: [PATCH v3] promisor-remote: prevent lazy-fetch recursion in child fetch

[Paul Tarjan]

Junio C Hamano <gitster@pobox.com> writes:

> So, is this topic still viable?

Yes, still viable from my end. The patch fixed the production
problem I ran into.

I thought v3 had addressed the outstanding feedback (HTTP
behavioral test, dropped the commit-graph comparison, packet
trace and pack-reuse analysis sent on Mar 13 in response to
Peff's questions), but I may have missed something. Is there a
specific concern I should address?

Paul