[PATCH v3 4/5] promisor-remote: prevent infinite recursion when lazy fetching

[Christian Couder <christian.couder@gmail.com>]

If a repository R is configured to lazy fetch from a promisor remote P
which is also configured to in turn lazy fetch from R, there is an
infinite recursion: R asks P for a missing object, P asks R for it,
and so on. The simplest case of this is a repository configured as its
own promisor remote.

This is not reachable when serving a repository by default, as
`upload-pack` sets `GIT_NO_LAZY_FETCH` to 1, which makes the nested
`upload-pack` refuse to lazily fetch. A following commit will let
server operators allow lazy fetching for repositories they trust
though, and as `GIT_NO_LAZY_FETCH` is then set to 0 and passed down to
child processes, nothing stops the recursion anymore.

It does not recurse forever in practice, but only because each level
adds one more variable to the environment of the child process, so
after a while `exec()` fails with:

    fatal: cannot exec 'git-upload-pack ...': Argument list too long
    fatal: unable to fork

To avoid this pathological case altogether, let's use a new
`GIT_INTERNAL_LAZY_FETCH_DEPTH` to count the recursion depth, and let's
check that it doesn't exceed a MAX_LAZY_FETCH_DEPTH limit (set to 5 for
now).

Note that some nesting is legitimate: when `git fetch` runs
`index-pack`, it can lazily fetch REF_DELTA bases that are missing
locally, so the limit should not be 1.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 environment.h            |  8 ++++++++
 promisor-remote.c        | 26 ++++++++++++++++++++++----
 t/t0410-partial-clone.sh | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/environment.h b/environment.h
index e7ec5b0437..f2833be9fe 100644
--- a/environment.h
+++ b/environment.h
@@ -52,6 +52,14 @@
  */
 #define GIT_ADVICE_ENVIRONMENT "GIT_ADVICE"
 
+/*
+ * Environment variable used to detect that a lazy fetch is already in
+ * progress in a parent process, to prevent infinite recursion when a
+ * promisor remote resolves back to the repository being served.
+ * This is an internal variable that should not be set by the user.
+ */
+#define LAZY_FETCH_DEPTH_ENVIRONMENT "GIT_INTERNAL_LAZY_FETCH_DEPTH"
+
 /*
  * Environment variable used in handshaking the wire protocol.
  * Contains a colon ':' separated list of keys with optional values
diff --git a/promisor-remote.c b/promisor-remote.c
index df17fec3bb..e9c5b413f1 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -24,7 +24,7 @@ struct promisor_remote_config {
 static int fetch_objects(struct repository *repo,
 			 const char *remote_name,
 			 const struct object_id *oids,
-			 int oid_nr)
+			 int oid_nr, unsigned long depth)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
 	int i;
@@ -41,6 +41,7 @@ static int fetch_objects(struct repository *repo,
 		     "--filter=blob:none", "--stdin", NULL);
 	if (!repo_config_get_bool(repo, "promisor.quiet", &quiet) && quiet)
 		strvec_push(&child.args, "--quiet");
+	strvec_pushf(&child.env, "%s=%lu", LAZY_FETCH_DEPTH_ENVIRONMENT, depth + 1);
 	if (start_command(&child))
 		die(_("promisor-remote: unable to fork off fetch subprocess"));
 	child_in = xfdopen(child.in, "w");
@@ -269,6 +270,7 @@ static bool try_promisor_remotes(struct repository *repo,
 				 struct object_id **remaining_oids,
 				 int *remaining_nr,
 				 int *to_free,
+				 unsigned long depth,
 				 bool accepted_only)
 {
 	struct promisor_remote *r = repo->promisor_remote_config->promisors;
@@ -276,7 +278,8 @@ static bool try_promisor_remotes(struct repository *repo,
 	for (; r; r = r->next) {
 		if (accepted_only != r->accepted)
 			continue;
-		if (fetch_objects(repo, r->name, *remaining_oids, *remaining_nr) < 0) {
+		if (fetch_objects(repo, r->name,
+				  *remaining_oids, *remaining_nr, depth) < 0) {
 			if (*remaining_nr == 1)
 				continue;
 			*remaining_nr = remove_fetched_oids(repo, remaining_oids,
@@ -291,6 +294,8 @@ static bool try_promisor_remotes(struct repository *repo,
 	return false;
 }
 
+#define MAX_LAZY_FETCH_DEPTH 5
+
 /*
  * Return 'true' if all the objects could be fetched, 'false' otherwise.
  */
@@ -299,6 +304,8 @@ static bool lazy_fetch_objects(struct repository *repo,
 			       int *remaining_nr,
 			       int *to_free)
 {
+	unsigned long depth = git_env_ulong(LAZY_FETCH_DEPTH_ENVIRONMENT, 0);
+
 	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
 		static int warning_shown;
 		if (!warning_shown) {
@@ -308,13 +315,24 @@ static bool lazy_fetch_objects(struct repository *repo,
 		return false;
 	}
 
+	if (depth >= MAX_LAZY_FETCH_DEPTH) {
+		static int warning_shown;
+		if (!warning_shown) {
+			warning_shown = 1;
+			warning(_("too many nested lazy fetches (%lu); "
+				  "is a promisor remote pointing at the repository itself?"),
+				depth);
+		}
+		return false;
+	}
+
 	promisor_remote_init(repo);
 
 	/* Try accepted remotes first (those the server told us to use) */
 	return try_promisor_remotes(repo, remaining_oids, remaining_nr,
-				    to_free, true) ||
+				    to_free, depth, true) ||
 		try_promisor_remotes(repo, remaining_oids, remaining_nr,
-				     to_free, false);
+				     to_free, depth, false);
 }
 
 void promisor_remote_get_direct(struct repository *repo,
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 788e9a1631..a54685e3c7 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -709,6 +709,39 @@ test_expect_success 'lazy-fetch when accessing object not in the_repository' '
 	test_grep ! "[?]$FILE_HASH" out
 '
 
+test_expect_success 'lazy-fetch does not recurse infinitely between two promisor remotes' '
+	rm -rf full partial1.git partial2.git &&
+
+	# Create a repo with a blob
+	test_create_repo full &&
+	test_config -C full uploadpack.allowfilter 1 &&
+	test_config -C full uploadpack.allowanysha1inwant 1 &&
+	test_commit -C full create-a-file file.txt &&
+	FILE_HASH=$(git -C full rev-parse HEAD:file.txt) &&
+
+	# Create partial clone repos without blobs
+	git clone --filter=blob:none --bare "file://$(pwd)/full" partial1.git &&
+	git clone --filter=blob:none --bare "file://$(pwd)/full" partial2.git &&
+	test_config -C partial1.git uploadpack.allowfilter 1 &&
+	test_config -C partial1.git uploadpack.allowanysha1inwant 1 &&
+	test_config -C partial2.git uploadpack.allowfilter 1 &&
+	test_config -C partial2.git uploadpack.allowanysha1inwant 1 &&
+
+	# Configure the partial repos as remotes of each other
+	git -C partial2.git remote set-url origin "file://$(pwd)/partial1.git" &&
+	git -C partial1.git remote set-url origin "file://$(pwd)/partial2.git" &&
+
+	# Make sure lazy fetching fails
+	test_must_fail env GIT_TRACE="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 \
+		git -C partial1.git cat-file -e "$FILE_HASH" 2>err &&
+	test_grep "too many nested lazy fetches" err &&
+
+	# Make sure the recursion was bounded, i.e. that only
+	# MAX_LAZY_FETCH_DEPTH "git fetch" subprocesses were spawned
+	grep "run_command: GIT_INTERNAL_LAZY_FETCH_DEPTH" trace >fetches &&
+	test_line_count = 5 fetches
+'
+
 test_expect_success 'push should not fetch new commit objects' '
 	rm -rf server client &&
 	test_create_repo server &&
-- 
2.55.0.792.ged91fccac1.dirty