[PATCH 00/22] Memory leak fixes (pt.6)

[PATCH 00/22] Memory leak fixes (pt.6)

[Patrick Steinhardt]

Hi,

here's another round of memory leak fixes. This is the second-last round
of "general" leak fixes -- so once this and the next series are merged,
I have made a full pass over all failing test suites. There will still
be a bunch of memory leaks, namely ~86 failing test suites.

I have documented a bunch of remaining leaks at [1]. Most of them are a
bit more involved to fix, so they will likely require separate series.
These issues relate to:

  - Simplifying commit history causes us to rewrite their parents, but
    we don't free the old parents in some cases. Hit e.g. by t6012.

  - The describe atom infrastructure as used by git-for-each-ref(1) is
    leaky. Hit e.g. by t6300, t6302.

  - Breaking rewrites causes memory leaks. Hit e.g. by t7524.

  - Cloning repositories causes us to re-initialize `the_repository`.
    This surfaces lifetime management issues with the `struct remote`
    that we use, which reuses storage held by `the_repository`. Hit e.g.
    by t5558.

  - The GPG interface is leaky. Hit e.g. by t7510, t7528.

If you want to do some puzzling, please feel free to pick up any of
these leaks :) I won't start working on them before I have the final
part of memory leak fixes merged.

This patch series builds on top of 6a09c36371 (The eighth batch,
2024-08-23) with Junio's ps/leakfixes-part-5 at 13b23d2da5 (transport:
fix leaking negotiation tips, 2024-08-22) merged into it.

Patrick

[1]: https://gitlab.com/groups/gitlab-org/-/epics/14943

Patrick Steinhardt (22):
  t/test-lib: allow skipping leak checks for passing tests
  fetch-pack: fix memory leaks on fetch negotiation
  send-pack: fix leaking common object IDs
  builtin/push: fix leaking refspec query result
  upload-pack: fix leaking child process data on reachability checks
  submodule: fix leaking fetch task data
  builtin/submodule--helper: fix leaking refs on push-check
  remote: fix leaking tracking refs
  remote: fix leak in reachability check of a remote-tracking ref
  send-pack: fix leaking push cert nonce
  gpg-interface: fix misdesigned signing key interfaces
  object: clear grafts when clearing parsed object pool
  shallow: free grafts when unregistering them
  shallow: fix leaking members of `struct shallow_info`
  negotiator/skipping: fix leaking commit entries
  builtin/repack: fix leaking line buffer when packing promisors
  builtin/pack-objects: plug leaking list of keep-packs
  builtin/grep: fix leaking object context
  builtin/fmt-merge-msg: fix leaking buffers
  match-trees: fix leaking prefixes in `shift_tree()`
  merge-ort: fix two leaks when handling directory rename modifications
  builtin/repack: fix leaking keep-pack list

 builtin/fmt-merge-msg.c              |  2 ++
 builtin/grep.c                       |  1 +
 builtin/pack-objects.c               |  1 +
 builtin/push.c                       |  8 +++--
 builtin/repack.c                     |  3 ++
 builtin/submodule--helper.c          |  2 ++
 builtin/tag.c                        |  3 +-
 commit.c                             | 23 ++++--------
 commit.h                             |  3 +-
 fetch-pack.c                         |  3 ++
 gpg-interface.c                      | 26 ++++++++------
 gpg-interface.h                      |  4 +--
 match-trees.c                        | 10 ++++--
 merge-ort.c                          |  4 ++-
 negotiator/skipping.c                |  7 ++--
 object.c                             | 14 +++++++-
 object.h                             |  4 ++-
 remote.c                             |  6 +++-
 repository.c                         |  2 +-
 send-pack.c                          | 52 ++++++++++++++++++----------
 shallow.c                            | 15 ++++++--
 submodule.c                          |  2 ++
 t/t5516-fetch-push.sh                |  1 +
 t/t5526-fetch-submodules.sh          |  1 +
 t/t5531-deep-submodule-push.sh       |  1 +
 t/t5533-push-cas.sh                  |  1 +
 t/t5534-push-signed.sh               |  1 +
 t/t5537-fetch-shallow.sh             |  1 +
 t/t5538-push-shallow.sh              |  1 +
 t/t5549-fetch-push-http.sh           |  1 +
 t/t5552-skipping-fetch-negotiator.sh |  2 ++
 t/t5616-partial-clone.sh             |  1 +
 t/t6132-pathspec-exclude.sh          |  1 +
 t/t6135-pathspec-with-attrs.sh       |  2 ++
 t/t6200-fmt-merge-msg.sh             |  1 +
 t/t6409-merge-subtree.sh             |  1 +
 t/t6423-merge-rename-directories.sh  |  1 +
 t/t6500-gc.sh                        |  1 +
 t/t7703-repack-geometric.sh          |  1 +
 t/test-lib.sh                        | 11 +++++-
 upload-pack.c                        | 22 ++++++++----
 41 files changed, 175 insertions(+), 72 deletions(-)

-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 01/22] t/test-lib: allow skipping leak checks for passing tests

[Patrick Steinhardt]

With `GIT_TEST_PASSING_SANITIZE_LEAK=check`, one can double check
whether a memory leak fix caused some test suites to become leak free.
It is somewhat slow to execute though because it runs all of our test
suites with the leak sanitizer enabled. It is also pointless in most
cases, because the only test suites that need to be checked are those
which _aren't_ yet marked with `TEST_PASSES_SANITIZE_LEAK=true`.

Introduce a new value "check-failing". If set, we will only check those
tests which are not yet marked as leak free.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/test-lib.sh | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 54247604cbc..64bd36531c1 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1558,8 +1558,16 @@ then
 		passes_sanitize_leak=t
 	fi
 
-	if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check"
+	if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" ||
+	   test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing"
 	then
+		if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" &&
+		   test -n "$passes_sanitize_leak"
+		then
+			skip_all="skipping leak-free $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=check-failing"
+			test_done
+		fi
+
 		sanitize_leak_check=t
 		if test -n "$invert_exit_code"
 		then
@@ -1597,6 +1605,7 @@ then
 	export LSAN_OPTIONS
 
 elif test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" ||
+     test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" ||
      test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
 then
 	BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_PASSING_SANITIZE_LEAK=true"
-- 
2.46.0.164.g477ce5ccd6.dirty

Re: [PATCH 01/22] t/test-lib: allow skipping leak checks for passing tests

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> With `GIT_TEST_PASSING_SANITIZE_LEAK=check`, one can double check
> whether a memory leak fix caused some test suites to become leak free.
> It is somewhat slow to execute though because it runs all of our test
> suites with the leak sanitizer enabled. It is also pointless in most
> cases, because the only test suites that need to be checked are those
> which _aren't_ yet marked with `TEST_PASSES_SANITIZE_LEAK=true`.
>
> Introduce a new value "check-failing". If set, we will only check those
> tests which are not yet marked as leak free.

A very welcome addition.  I am already liking it while running
locally.

Thanks.

>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/test-lib.sh | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 54247604cbc..64bd36531c1 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1558,8 +1558,16 @@ then
>  		passes_sanitize_leak=t
>  	fi
>  
> -	if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check"
> +	if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" ||
> +	   test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing"
>  	then
> +		if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" &&
> +		   test -n "$passes_sanitize_leak"
> +		then
> +			skip_all="skipping leak-free $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=check-failing"
> +			test_done
> +		fi
> +
>  		sanitize_leak_check=t
>  		if test -n "$invert_exit_code"
>  		then
> @@ -1597,6 +1605,7 @@ then
>  	export LSAN_OPTIONS
>  
>  elif test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" ||
> +     test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" ||
>       test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
>  then
>  	BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_PASSING_SANITIZE_LEAK=true"

Re: [PATCH 01/22] t/test-lib: allow skipping leak checks for passing tests

[Toon claes]

Patrick Steinhardt <ps@pks.im> writes:

> With `GIT_TEST_PASSING_SANITIZE_LEAK=check`, one can double check
> whether a memory leak fix caused some test suites to become leak free.
> It is somewhat slow to execute though because it runs all of our test
> suites with the leak sanitizer enabled. It is also pointless in most
> cases, because the only test suites that need to be checked are those
> which _aren't_ yet marked with `TEST_PASSES_SANITIZE_LEAK=true`.

What I understand from `t/README` the "check" value is used to test
whether the presence or absence of `TEST_PASSES_SANITIZE_LEAK=true`
matches the expectations. I think it's better to express that in the
first sentence, because it sounds a bit misleading at the moment if you
don't know that.

> Introduce a new value "check-failing". If set, we will only check those
> tests which are not yet marked as leak free.

I would also mention this still has the effect that tests which *are*
leak-free but do not have `TEST_PASSES_SANITIZE_LEAK=true` fail due to
the use of "--invert-exit-code".

Also, can you add a short paragraph about this value in "t/README"?

--
Toon

Re: [PATCH 01/22] t/test-lib: allow skipping leak checks for passing tests

[Patrick Steinhardt]

On Thu, Aug 29, 2024 at 04:15:58PM +0200, Toon claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > With `GIT_TEST_PASSING_SANITIZE_LEAK=check`, one can double check
> > whether a memory leak fix caused some test suites to become leak free.
> > It is somewhat slow to execute though because it runs all of our test
> > suites with the leak sanitizer enabled. It is also pointless in most
> > cases, because the only test suites that need to be checked are those
> > which _aren't_ yet marked with `TEST_PASSES_SANITIZE_LEAK=true`.
> 
> What I understand from `t/README` the "check" value is used to test
> whether the presence or absence of `TEST_PASSES_SANITIZE_LEAK=true`
> matches the expectations. I think it's better to express that in the
> first sentence, because it sounds a bit misleading at the moment if you
> don't know that.
> 
> > Introduce a new value "check-failing". If set, we will only check those
> > tests which are not yet marked as leak free.
> 
> I would also mention this still has the effect that tests which *are*
> leak-free but do not have `TEST_PASSES_SANITIZE_LEAK=true` fail due to
> the use of "--invert-exit-code".

I don't want to go too much into the implementation details here, but
will note that we continue to behave the same as with "check", except
that we skip already-leak-free tests.

> Also, can you add a short paragraph about this value in "t/README"?

Yup, will do.

Patrick

[PATCH 02/22] fetch-pack: fix memory leaks on fetch negotiation

[Patrick Steinhardt]

We leak both the `nt_object_array` and `negotiator` structures in
`negotiate_using_fetch()`. Plug both of these leaks.

These leaks were exposed by t5516, but fixing them is not sufficient to
make the whole test suite leak free.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 fetch-pack.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fetch-pack.c b/fetch-pack.c
index 58b4581ad80..0ed82feda14 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -2227,7 +2227,10 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
 	trace2_region_leave("fetch-pack", "negotiate_using_fetch", the_repository);
 	trace2_data_intmax("negotiate_using_fetch", the_repository,
 			   "total_rounds", negotiation_round);
+
 	clear_common_flag(acked_commits);
+	object_array_clear(&nt_object_array);
+	negotiator.release(&negotiator);
 	strbuf_release(&req_buf);
 }
 
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 03/22] send-pack: fix leaking common object IDs

[Patrick Steinhardt]

We're leaking the array of common object IDs in `send_pack()`. Fix this
by creating a common exit path where we free the leaking data. While at
it, unify some other cleanups now that we have a central place to put
them.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 send-pack.c                | 34 ++++++++++++++++++++++------------
 t/t5549-fetch-push-http.sh |  1 +
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index fa2f5eec17b..b224ef9fc5e 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -508,7 +508,8 @@ int send_pack(struct send_pack_args *args,
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
 			"Perhaps you should specify a branch.\n");
-		return 0;
+		ret = 0;
+		goto out;
 	}
 
 	git_config_get_bool("push.negotiate", &push_negotiate);
@@ -615,12 +616,11 @@ int send_pack(struct send_pack_args *args,
 			 * atomically, abort the whole operation.
 			 */
 			if (use_atomic) {
-				strbuf_release(&req_buf);
-				strbuf_release(&cap_buf);
 				reject_atomic_push(remote_refs, args->send_mirror);
 				error("atomic push failed for ref %s. status: %d\n",
 				      ref->name, ref->status);
-				return args->porcelain ? 0 : -1;
+				ret = args->porcelain ? 0 : -1;
+				goto out;
 			}
 			/* else fallthrough */
 		default:
@@ -682,8 +682,6 @@ int send_pack(struct send_pack_args *args,
 		write_or_die(out, req_buf.buf, req_buf.len);
 		packet_flush(out);
 	}
-	strbuf_release(&req_buf);
-	strbuf_release(&cap_buf);
 
 	if (use_sideband && cmds_sent) {
 		memset(&demux, 0, sizeof(demux));
@@ -721,7 +719,9 @@ int send_pack(struct send_pack_args *args,
 				finish_async(&demux);
 			}
 			fd[1] = -1;
-			return -1;
+
+			ret = -1;
+			goto out;
 		}
 		if (!args->stateless_rpc)
 			/* Closed by pack_objects() via start_command() */
@@ -746,10 +746,12 @@ int send_pack(struct send_pack_args *args,
 	}
 
 	if (ret < 0)
-		return ret;
+		goto out;
 
-	if (args->porcelain)
-		return 0;
+	if (args->porcelain) {
+		ret = 0;
+		goto out;
+	}
 
 	for (ref = remote_refs; ref; ref = ref->next) {
 		switch (ref->status) {
@@ -758,8 +760,16 @@ int send_pack(struct send_pack_args *args,
 		case REF_STATUS_OK:
 			break;
 		default:
-			return -1;
+			ret = -1;
+			goto out;
 		}
 	}
-	return 0;
+
+	ret = 0;
+
+out:
+	oid_array_clear(&commons);
+	strbuf_release(&req_buf);
+	strbuf_release(&cap_buf);
+	return ret;
 }
diff --git a/t/t5549-fetch-push-http.sh b/t/t5549-fetch-push-http.sh
index 2cdebcb7356..6377fb6d993 100755
--- a/t/t5549-fetch-push-http.sh
+++ b/t/t5549-fetch-push-http.sh
@@ -5,6 +5,7 @@ test_description='fetch/push functionality using the HTTP protocol'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 04/22] builtin/push: fix leaking refspec query result

[Patrick Steinhardt]

When appending a refspec via `refspec_append_mapped()` we leak the
result of `query_refspecs()`. The overall logic around refspec queries
is quite weird, as callers are expected to either set the `src` or `dst`
pointers, and then the (allocated) result will be in the respective
other struct member.

As we have the `src` member set, plugging the memory leak is thus as
easy as just freeing the `dst` member. While at it, use designated
initializers to initialize the structure.

This leak was exposed by t5516, but fixing it is not sufficient to make
the whole test suite leak free.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/push.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 7a67398124f..0b123eb9c1e 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -72,13 +72,15 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,
 	const char *branch_name;
 
 	if (remote->push.nr) {
-		struct refspec_item query;
-		memset(&query, 0, sizeof(struct refspec_item));
-		query.src = matched->name;
+		struct refspec_item query = {
+			.src = matched->name,
+		};
+
 		if (!query_refspecs(&remote->push, &query) && query.dst) {
 			refspec_appendf(refspec, "%s%s:%s",
 					query.force ? "+" : "",
 					query.src, query.dst);
+			free(query.dst);
 			return;
 		}
 	}
-- 
2.46.0.164.g477ce5ccd6.dirty

Re: [PATCH 04/22] builtin/push: fix leaking refspec query result

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> When appending a refspec via `refspec_append_mapped()` we leak the
> result of `query_refspecs()`. The overall logic around refspec queries
> is quite weird, as callers are expected to either set the `src` or `dst`
> pointers, and then the (allocated) result will be in the respective
> other struct member.

Hmph, is it necessary to say "quite weird" for the purpose of this
change?  The query interface is designed to be usable to query both
ways and within that constraints, I find it designed very nicely
(but I do not think that is necessary to be said for the purpose of
this change, either)..

> As we have the `src` member set, plugging the memory leak is thus as
> easy as just freeing the `dst` member. While at it, use designated
> initializers to initialize the structure.

In order to understand this paragraph, of course, it helps for a
reader to understand that the query_refspecs() gives an answer by
populating the side other than the query side, and the answers are
what we want to release.

> This leak was exposed by t5516, but fixing it is not sufficient to make
> the whole test suite leak free.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/push.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index 7a67398124f..0b123eb9c1e 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -72,13 +72,15 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,
>  	const char *branch_name;
>  
>  	if (remote->push.nr) {
> -		struct refspec_item query;
> -		memset(&query, 0, sizeof(struct refspec_item));
> -		query.src = matched->name;
> +		struct refspec_item query = {
> +			.src = matched->name,
> +		};

This is "while at it" change that does not contribute to the leak or
the leakfix; the resulting code is easier to read.

>  		if (!query_refspecs(&remote->push, &query) && query.dst) {
>  			refspec_appendf(refspec, "%s%s:%s",
>  					query.force ? "+" : "",
>  					query.src, query.dst);
> +			free(query.dst);

And this is the real fix, which looks good.

Thanks.

>  			return;
>  		}
>  	}

Re: [PATCH 04/22] builtin/push: fix leaking refspec query result

[Patrick Steinhardt]

On Fri, Aug 30, 2024 at 02:59:01PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > When appending a refspec via `refspec_append_mapped()` we leak the
> > result of `query_refspecs()`. The overall logic around refspec queries
> > is quite weird, as callers are expected to either set the `src` or `dst`
> > pointers, and then the (allocated) result will be in the respective
> > other struct member.
> 
> Hmph, is it necessary to say "quite weird" for the purpose of this
> change?  The query interface is designed to be usable to query both
> ways and within that constraints, I find it designed very nicely
> (but I do not think that is necessary to be said for the purpose of
> this change, either)..

I don't quite agree that it's nicely designed -- I find it rather hard
to use and reason about, and the fact that so many callsites get this
interface wrong seems to indicate that there is at least some sort of
truth to this assessment.

But I don't mind removing this subjective opinion from the commit
message. I'll do that in case I end up rerolling this patch series.

Thanks!

Patrick

[PATCH 05/22] upload-pack: fix leaking child process data on reachability checks

[Patrick Steinhardt]

We spawn a git-rev-list(1) command to perform reachability checks in
"upload-pack.c". We do not release memory associated with the process
in error cases though, thus leaking memory.

Fix these by calling `child_process_clear()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5516-fetch-push.sh |  1 +
 upload-pack.c         | 22 ++++++++++++++++------
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 9d693eb57f7..331778bd42c 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -19,6 +19,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 TEST_CREATE_REPO_NO_TEMPLATE=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 D=$(pwd)
diff --git a/upload-pack.c b/upload-pack.c
index f03ba3e98be..c84c3c3b1f5 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -709,10 +709,13 @@ static int get_reachable_list(struct upload_pack_data *data,
 	struct object *o;
 	char namebuf[GIT_MAX_HEXSZ + 2]; /* ^ + hash + LF */
 	const unsigned hexsz = the_hash_algo->hexsz;
+	int ret;
 
 	if (do_reachable_revlist(&cmd, &data->shallows, reachable,
-				 data->allow_uor) < 0)
-		return -1;
+				 data->allow_uor) < 0) {
+		ret = -1;
+		goto out;
+	}
 
 	while ((i = read_in_full(cmd.out, namebuf, hexsz + 1)) == hexsz + 1) {
 		struct object_id oid;
@@ -736,10 +739,16 @@ static int get_reachable_list(struct upload_pack_data *data,
 	}
 	close(cmd.out);
 
-	if (finish_command(&cmd))
-		return -1;
+	if (finish_command(&cmd)) {
+		ret = -1;
+		goto out;
+	}
 
-	return 0;
+	ret = 0;
+
+out:
+	child_process_clear(&cmd);
+	return ret;
 }
 
 static int has_unreachable(struct object_array *src, enum allow_uor allow_uor)
@@ -749,7 +758,7 @@ static int has_unreachable(struct object_array *src, enum allow_uor allow_uor)
 	int i;
 
 	if (do_reachable_revlist(&cmd, src, NULL, allow_uor) < 0)
-		return 1;
+		goto error;
 
 	/*
 	 * The commits out of the rev-list are not ancestors of
@@ -775,6 +784,7 @@ static int has_unreachable(struct object_array *src, enum allow_uor allow_uor)
 error:
 	if (cmd.out >= 0)
 		close(cmd.out);
+	child_process_clear(&cmd);
 	return 1;
 }
 
-- 
2.46.0.164.g477ce5ccd6.dirty

Re: [PATCH 05/22] upload-pack: fix leaking child process data on reachability checks

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> We spawn a git-rev-list(1) command to perform reachability checks in
> "upload-pack.c". We do not release memory associated with the process
> in error cases though, thus leaking memory.

Thanks for plugging this leak dating back in ages.

Both changes look sensible to me.

[PATCH 06/22] submodule: fix leaking fetch task data

[Patrick Steinhardt]

The `submodule_parallel_fetch` structure contains various data
structures that we use to set up parallel fetches of submodules. We do
not free some of its data though, causing memory leaks. Plug those.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 submodule.c                 | 2 ++
 t/t5526-fetch-submodules.sh | 1 +
 2 files changed, 3 insertions(+)

diff --git a/submodule.c b/submodule.c
index 97516b0fec1..97d0d47b561 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1883,6 +1883,8 @@ int fetch_submodules(struct repository *r,
 out:
 	free_submodules_data(&spf.changed_submodule_names);
 	string_list_clear(&spf.seen_submodule_names, 0);
+	strbuf_release(&spf.submodules_with_errors);
+	free(spf.oid_fetch_tasks);
 	return spf.result;
 }
 
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 5e566205ba4..2cfb5bd6bb1 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -6,6 +6,7 @@ test_description='Recursive "git fetch" for submodules'
 GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1
 export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 pwd=$(pwd)
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 07/22] builtin/submodule--helper: fix leaking refs on push-check

[Patrick Steinhardt]

In the push-check subcommand of the submodule helper we acquire a list
of local refs, but never free that list. Fix this memory leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/submodule--helper.c    | 2 ++
 t/t5531-deep-submodule-push.sh | 1 +
 2 files changed, 3 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 85fb23dee84..642a0edabf0 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2958,7 +2958,9 @@ static int push_check(int argc, const char **argv, const char *prefix UNUSED)
 				    rs->src);
 			}
 		}
+
 		refspec_clear(&refspec);
+		free_refs(local_refs);
 	}
 	free(head);
 
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index f3fff557447..135823630a3 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -8,6 +8,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1
 export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 08/22] remote: fix leaking tracking refs

[Patrick Steinhardt]

When computing the remote tracking ref we cause two memory leaks:

  - We leak when `remote_tracking()` fails.

  - We leak when the call to `remote_tracking()` succeeds and sets
    `ref->tracking_ref()`.

Fix both of these leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 remote.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index 8f3dee13186..240311619ab 100644
--- a/remote.c
+++ b/remote.c
@@ -1123,6 +1123,7 @@ void free_one_ref(struct ref *ref)
 		return;
 	free_one_ref(ref->peer_ref);
 	free(ref->remote_status);
+	free(ref->tracking_ref);
 	free(ref->symref);
 	free(ref);
 }
@@ -2620,8 +2621,10 @@ static int remote_tracking(struct remote *remote, const char *refname,
 	dst = apply_refspecs(&remote->fetch, refname);
 	if (!dst)
 		return -1; /* no tracking ref for refname at remote */
-	if (refs_read_ref(get_main_ref_store(the_repository), dst, oid))
+	if (refs_read_ref(get_main_ref_store(the_repository), dst, oid)) {
+		free(dst);
 		return -1; /* we know what the tracking ref is but we cannot read it */
+	}
 
 	*dst_refname = dst;
 	return 0;
-- 
2.46.0.164.g477ce5ccd6.dirty

Re: [PATCH 08/22] remote: fix leaking tracking refs

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> @@ -1123,6 +1123,7 @@ void free_one_ref(struct ref *ref)
>  		return;
>  	free_one_ref(ref->peer_ref);
>  	free(ref->remote_status);
> +	free(ref->tracking_ref);
>  	free(ref->symref);
>  	free(ref);
>  }
> @@ -2620,8 +2621,10 @@ static int remote_tracking(struct remote *remote, const char *refname,
>  	dst = apply_refspecs(&remote->fetch, refname);
>  	if (!dst)
>  		return -1; /* no tracking ref for refname at remote */
> -	if (refs_read_ref(get_main_ref_store(the_repository), dst, oid))
> +	if (refs_read_ref(get_main_ref_store(the_repository), dst, oid)) {
> +		free(dst);
>  		return -1; /* we know what the tracking ref is but we cannot read it */
> +	}
>  
>  	*dst_refname = dst;
>  	return 0;

Looking good.

[PATCH 09/22] remote: fix leak in reachability check of a remote-tracking ref

[Patrick Steinhardt]

In `check_if_includes_upstream()` we retrieve the local ref
corresponding to a remote-tracking ref we want to check reachability
for. We never free that local ref and thus cause a memory leak. Fix
this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 remote.c            | 1 +
 t/t5533-push-cas.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/remote.c b/remote.c
index 240311619ab..bff54046b2c 100644
--- a/remote.c
+++ b/remote.c
@@ -2774,6 +2774,7 @@ static void check_if_includes_upstream(struct ref *remote)
 
 	if (is_reachable_in_reflog(local->name, remote) <= 0)
 		remote->unreachable = 1;
+	free_one_ref(local);
 }
 
 static void apply_cas(struct push_cas_option *cas,
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index cba26a872dd..6365d99777e 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -5,6 +5,7 @@ test_description='compare & swap push force/delete safety'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 setup_srcdst_basic () {
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 10/22] send-pack: fix leaking push cert nonce

[Patrick Steinhardt]

When retrieving the push cert nonce from the server, we first store the
constant returned by `server_feature_value()` and then, if the nonce is
valid, we duplicate the nonce memory to extend its lifetime. We never
free the latter and thus cause a memory leak.

Fix this by storing the limited-lifetime nonce into a scope-local
variable such that the long-lived, allocated nonce can be easily freed
without having to cast away its constness.

This leak was exposed by t5534, but fixing it is not sufficient to make
the whole test suite leak free.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 send-pack.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index b224ef9fc5e..c37f6ab3c07 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -501,7 +501,7 @@ int send_pack(struct send_pack_args *args,
 	unsigned cmds_sent = 0;
 	int ret;
 	struct async demux;
-	const char *push_cert_nonce = NULL;
+	char *push_cert_nonce = NULL;
 	struct packet_reader reader;
 	int use_bitmaps;
 
@@ -550,10 +550,11 @@ int send_pack(struct send_pack_args *args,
 
 	if (args->push_cert != SEND_PACK_PUSH_CERT_NEVER) {
 		size_t len;
-		push_cert_nonce = server_feature_value("push-cert", &len);
-		if (push_cert_nonce) {
-			reject_invalid_nonce(push_cert_nonce, len);
-			push_cert_nonce = xmemdupz(push_cert_nonce, len);
+		const char *nonce = server_feature_value("push-cert", &len);
+
+		if (nonce) {
+			reject_invalid_nonce(nonce, len);
+			push_cert_nonce = xmemdupz(nonce, len);
 		} else if (args->push_cert == SEND_PACK_PUSH_CERT_ALWAYS) {
 			die(_("the receiving end does not support --signed push"));
 		} else if (args->push_cert == SEND_PACK_PUSH_CERT_IF_ASKED) {
@@ -771,5 +772,6 @@ int send_pack(struct send_pack_args *args,
 	oid_array_clear(&commons);
 	strbuf_release(&req_buf);
 	strbuf_release(&cap_buf);
+	free(push_cert_nonce);
 	return ret;
 }
-- 
2.46.0.164.g477ce5ccd6.dirty

Re: [PATCH 10/22] send-pack: fix leaking push cert nonce

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> When retrieving the push cert nonce from the server, we first store the
> constant returned by `server_feature_value()` and then, if the nonce is
> valid, we duplicate the nonce memory to extend its lifetime. We never
> free the latter and thus cause a memory leak.

"to extend its lifetime" -> "to a NUL-terminated string, so that we
can pass it to generate_push_cert()".

What is going on in this code path is this.

The other side may send a nonce in the server capability.  We die if
that nonce is bogus.  Otherwise we make a xmemdupz() copy because we
need to pass the nonce to generate_push_cert() that expects nonce to
be a NUL terminated string (the original server capability is a long
concatenation of capabilities and we learn the <ptr, len> for the
nonce).  The function cryptographically signs the ref update request
we have, together with the nonce we got from the server, so that the
other side can validate that it is signed by us, and the nonce serves
as a protection against replay attacks.

> diff --git a/send-pack.c b/send-pack.c
> index b224ef9fc5e..c37f6ab3c07 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -501,7 +501,7 @@ int send_pack(struct send_pack_args *args,
>  	unsigned cmds_sent = 0;
>  	int ret;
>  	struct async demux;
> -	const char *push_cert_nonce = NULL;
> +	char *push_cert_nonce = NULL;
>  	struct packet_reader reader;
>  	int use_bitmaps;

This is a change necessary to avoid having to cast the parameter to
free().

> @@ -550,10 +550,11 @@ int send_pack(struct send_pack_args *args,
>  
>  	if (args->push_cert != SEND_PACK_PUSH_CERT_NEVER) {
>  		size_t len;
> -		push_cert_nonce = server_feature_value("push-cert", &len);
> -		if (push_cert_nonce) {
> -			reject_invalid_nonce(push_cert_nonce, len);
> -			push_cert_nonce = xmemdupz(push_cert_nonce, len);
> +		const char *nonce = server_feature_value("push-cert", &len);
> +
> +		if (nonce) {
> +			reject_invalid_nonce(nonce, len);
> +			push_cert_nonce = xmemdupz(nonce, len);

And this hunk become needed because push_cert_nonce cannot receive
the return value from server_feature_value() without stripping
constness.

>  		} else if (args->push_cert == SEND_PACK_PUSH_CERT_ALWAYS) {
>  			die(_("the receiving end does not support --signed push"));
>  		} else if (args->push_cert == SEND_PACK_PUSH_CERT_IF_ASKED) {
> @@ -771,5 +772,6 @@ int send_pack(struct send_pack_args *args,
>  	oid_array_clear(&commons);
>  	strbuf_release(&req_buf);
>  	strbuf_release(&cap_buf);
> +	free(push_cert_nonce);

And this is my fault to forget freeing.  Thanks for spotting and fixing.

>  	return ret;
>  }

[PATCH 11/22] gpg-interface: fix misdesigned signing key interfaces

[Patrick Steinhardt]

The interfaces to retrieve signing keys and their IDs are misdesigned as
they return string constants even though they indeed allocate memory,
which leads to memory leaks. Refactor the code to instead always return
allocated strings and let the callers free them accordingly.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/tag.c          |  3 ++-
 commit.c               |  9 ++++++---
 gpg-interface.c        | 26 +++++++++++++++-----------
 gpg-interface.h        |  4 ++--
 send-pack.c            |  6 ++++--
 t/t5534-push-signed.sh |  1 +
 6 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index a1fb218512c..ab3b500543d 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -160,7 +160,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid,
 	const struct git_hash_algo *compat = the_repository->compat_hash_algo;
 	struct strbuf sig = STRBUF_INIT, compat_sig = STRBUF_INIT;
 	struct strbuf compat_buf = STRBUF_INIT;
-	const char *keyid = get_signing_key();
+	char *keyid = get_signing_key();
 	int ret = -1;
 
 	if (sign_buffer(buffer, &sig, keyid))
@@ -190,6 +190,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid,
 	strbuf_release(&sig);
 	strbuf_release(&compat_sig);
 	strbuf_release(&compat_buf);
+	free(keyid);
 	return ret;
 }
 
diff --git a/commit.c b/commit.c
index 24ab5c1b509..ec9efc189d5 100644
--- a/commit.c
+++ b/commit.c
@@ -1150,11 +1150,14 @@ int add_header_signature(struct strbuf *buf, struct strbuf *sig, const struct gi
 
 static int sign_commit_to_strbuf(struct strbuf *sig, struct strbuf *buf, const char *keyid)
 {
+	char *keyid_to_free = NULL;
+	int ret = 0;
 	if (!keyid || !*keyid)
-		keyid = get_signing_key();
+		keyid = keyid_to_free = get_signing_key();
 	if (sign_buffer(buf, sig, keyid))
-		return -1;
-	return 0;
+		ret = -1;
+	free(keyid_to_free);
+	return ret;
 }
 
 int parse_signed_commit(const struct commit *commit,
diff --git a/gpg-interface.c b/gpg-interface.c
index 6587085cd19..cf6126b5aa0 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -45,8 +45,8 @@ struct gpg_format {
 				    size_t signature_size);
 	int (*sign_buffer)(struct strbuf *buffer, struct strbuf *signature,
 			   const char *signing_key);
-	const char *(*get_default_key)(void);
-	const char *(*get_key_id)(void);
+	char *(*get_default_key)(void);
+	char *(*get_key_id)(void);
 };
 
 static const char *openpgp_verify_args[] = {
@@ -86,9 +86,9 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
 static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
 			   const char *signing_key);
 
-static const char *get_default_ssh_signing_key(void);
+static char *get_default_ssh_signing_key(void);
 
-static const char *get_ssh_key_id(void);
+static char *get_ssh_key_id(void);
 
 static struct gpg_format gpg_format[] = {
 	{
@@ -847,7 +847,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key)
 }
 
 /* Returns the first public key from an ssh-agent to use for signing */
-static const char *get_default_ssh_signing_key(void)
+static char *get_default_ssh_signing_key(void)
 {
 	struct child_process ssh_default_key = CHILD_PROCESS_INIT;
 	int ret = -1;
@@ -899,12 +899,16 @@ static const char *get_default_ssh_signing_key(void)
 	return default_key;
 }
 
-static const char *get_ssh_key_id(void) {
-	return get_ssh_key_fingerprint(get_signing_key());
+static char *get_ssh_key_id(void)
+{
+	char *signing_key = get_signing_key();
+	char *key_id = get_ssh_key_fingerprint(signing_key);
+	free(signing_key);
+	return key_id;
 }
 
 /* Returns a textual but unique representation of the signing key */
-const char *get_signing_key_id(void)
+char *get_signing_key_id(void)
 {
 	gpg_interface_lazy_init();
 
@@ -916,17 +920,17 @@ const char *get_signing_key_id(void)
 	return get_signing_key();
 }
 
-const char *get_signing_key(void)
+char *get_signing_key(void)
 {
 	gpg_interface_lazy_init();
 
 	if (configured_signing_key)
-		return configured_signing_key;
+		return xstrdup(configured_signing_key);
 	if (use_format->get_default_key) {
 		return use_format->get_default_key();
 	}
 
-	return git_committer_info(IDENT_STRICT | IDENT_NO_DATE);
+	return xstrdup(git_committer_info(IDENT_STRICT | IDENT_NO_DATE));
 }
 
 const char *gpg_trust_level_to_str(enum signature_trust_level level)
diff --git a/gpg-interface.h b/gpg-interface.h
index 7cd98161f7f..e09f12e8d04 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -80,13 +80,13 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
 const char *gpg_trust_level_to_str(enum signature_trust_level level);
 
 void set_signing_key(const char *);
-const char *get_signing_key(void);
+char *get_signing_key(void);
 
 /*
  * Returns a textual unique representation of the signing key in use
  * Either a GPG KeyID or a SSH Key Fingerprint
  */
-const char *get_signing_key_id(void);
+char *get_signing_key_id(void);
 int check_signature(struct signature_check *sigc,
 		    const char *signature, size_t slen);
 void print_signature_buffer(const struct signature_check *sigc,
diff --git a/send-pack.c b/send-pack.c
index c37f6ab3c07..31a62e6a98c 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -348,7 +348,8 @@ static int generate_push_cert(struct strbuf *req_buf,
 {
 	const struct ref *ref;
 	struct string_list_item *item;
-	char *signing_key_id = xstrdup(get_signing_key_id());
+	char *signing_key_id = get_signing_key_id();
+	char *signing_key = get_signing_key();
 	const char *cp, *np;
 	struct strbuf cert = STRBUF_INIT;
 	int update_seen = 0;
@@ -381,7 +382,7 @@ static int generate_push_cert(struct strbuf *req_buf,
 	if (!update_seen)
 		goto free_return;
 
-	if (sign_buffer(&cert, &cert, get_signing_key()))
+	if (sign_buffer(&cert, &cert, signing_key))
 		die(_("failed to sign the push certificate"));
 
 	packet_buf_write(req_buf, "push-cert%c%s", 0, cap_string);
@@ -394,6 +395,7 @@ static int generate_push_cert(struct strbuf *req_buf,
 
 free_return:
 	free(signing_key_id);
+	free(signing_key);
 	strbuf_release(&cert);
 	return update_seen;
 }
diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index c91a62b77af..d43aee0c327 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -5,6 +5,7 @@ test_description='signed push'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-gpg.sh
 
-- 
2.46.0.164.g477ce5ccd6.dirty

Re: [PATCH 11/22] gpg-interface: fix misdesigned signing key interfaces

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> The interfaces to retrieve signing keys and their IDs are misdesigned as
> they return string constants even though they indeed allocate memory,
> which leads to memory leaks. Refactor the code to instead always return
> allocated strings and let the callers free them accordingly.

Makes sense.

[PATCH 12/22] object: clear grafts when clearing parsed object pool

[Patrick Steinhardt]

We do not clear grafts part of the parsed object pool when clearing the
pool itself, which can lead to memory leaks when a repository is being
cleared.

Fix this by moving `reset_commit_grafts()` into "object.c" and making it
part of the `struct parsed_object_pool` interface such that we can call
it from `parsed_object_pool_clear()`. Adapt `parsed_object_pool_new()`
to take and store a reference to its owning repository, which is needed
by `unparse_commit()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 commit.c     | 14 +-------------
 commit.h     |  3 ++-
 object.c     | 14 +++++++++++++-
 object.h     |  4 +++-
 repository.c |  2 +-
 shallow.c    |  2 +-
 6 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/commit.c b/commit.c
index ec9efc189d5..bbef0e81c65 100644
--- a/commit.c
+++ b/commit.c
@@ -177,7 +177,7 @@ int commit_graft_pos(struct repository *r, const struct object_id *oid)
 		       commit_graft_oid_access);
 }
 
-static void unparse_commit(struct repository *r, const struct object_id *oid)
+void unparse_commit(struct repository *r, const struct object_id *oid)
 {
 	struct commit *c = lookup_commit(r, oid);
 
@@ -318,18 +318,6 @@ int for_each_commit_graft(each_commit_graft_fn fn, void *cb_data)
 	return ret;
 }
 
-void reset_commit_grafts(struct repository *r)
-{
-	int i;
-
-	for (i = 0; i < r->parsed_objects->grafts_nr; i++) {
-		unparse_commit(r, &r->parsed_objects->grafts[i]->oid);
-		free(r->parsed_objects->grafts[i]);
-	}
-	r->parsed_objects->grafts_nr = 0;
-	r->parsed_objects->commit_graft_prepared = 0;
-}
-
 struct commit_buffer {
 	void *buffer;
 	unsigned long size;
diff --git a/commit.h b/commit.h
index d62b1d93f95..5ba0f77b1eb 100644
--- a/commit.h
+++ b/commit.h
@@ -108,6 +108,8 @@ static inline int repo_parse_commit_no_graph(struct repository *r,
 
 void parse_commit_or_die(struct commit *item);
 
+void unparse_commit(struct repository *r, const struct object_id *oid);
+
 struct buffer_slab;
 struct buffer_slab *allocate_commit_buffer_slab(void);
 void free_commit_buffer_slab(struct buffer_slab *bs);
@@ -240,7 +242,6 @@ int commit_graft_pos(struct repository *r, const struct object_id *oid);
 int register_commit_graft(struct repository *r, struct commit_graft *, int);
 void prepare_commit_graft(struct repository *r);
 struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid);
-void reset_commit_grafts(struct repository *r);
 
 struct commit *get_fork_point(const char *refname, struct commit *commit);
 
diff --git a/object.c b/object.c
index d756c7f2ea3..94ea8fb8d2c 100644
--- a/object.c
+++ b/object.c
@@ -545,11 +545,12 @@ void repo_clear_commit_marks(struct repository *r, unsigned int flags)
 	}
 }
 
-struct parsed_object_pool *parsed_object_pool_new(void)
+struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)
 {
 	struct parsed_object_pool *o = xmalloc(sizeof(*o));
 	memset(o, 0, sizeof(*o));
 
+	o->repo = repo;
 	o->blob_state = allocate_alloc_state();
 	o->tree_state = allocate_alloc_state();
 	o->commit_state = allocate_alloc_state();
@@ -628,6 +629,16 @@ void raw_object_store_clear(struct raw_object_store *o)
 	hashmap_clear(&o->pack_map);
 }
 
+void parsed_object_pool_reset_commit_grafts(struct parsed_object_pool *o)
+{
+	for (int i = 0; i < o->grafts_nr; i++) {
+		unparse_commit(o->repo, &o->grafts[i]->oid);
+		free(o->grafts[i]);
+	}
+	o->grafts_nr = 0;
+	o->commit_graft_prepared = 0;
+}
+
 void parsed_object_pool_clear(struct parsed_object_pool *o)
 {
 	/*
@@ -659,6 +670,7 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)
 	free_commit_buffer_slab(o->buffer_slab);
 	o->buffer_slab = NULL;
 
+	parsed_object_pool_reset_commit_grafts(o);
 	clear_alloc_state(o->blob_state);
 	clear_alloc_state(o->tree_state);
 	clear_alloc_state(o->commit_state);
diff --git a/object.h b/object.h
index 05691486ebf..17f32f1103e 100644
--- a/object.h
+++ b/object.h
@@ -7,6 +7,7 @@ struct buffer_slab;
 struct repository;
 
 struct parsed_object_pool {
+	struct repository *repo;
 	struct object **obj_hash;
 	int nr_objs, obj_hash_size;
 
@@ -31,8 +32,9 @@ struct parsed_object_pool {
 	struct buffer_slab *buffer_slab;
 };
 
-struct parsed_object_pool *parsed_object_pool_new(void);
+struct parsed_object_pool *parsed_object_pool_new(struct repository *repo);
 void parsed_object_pool_clear(struct parsed_object_pool *o);
+void parsed_object_pool_reset_commit_grafts(struct parsed_object_pool *o);
 
 struct object_list {
 	struct object *item;
diff --git a/repository.c b/repository.c
index 9825a308993..e6fc2c6aa9d 100644
--- a/repository.c
+++ b/repository.c
@@ -54,7 +54,7 @@ void initialize_repository(struct repository *repo)
 {
 	repo->objects = raw_object_store_new();
 	repo->remote_state = remote_state_new();
-	repo->parsed_objects = parsed_object_pool_new();
+	repo->parsed_objects = parsed_object_pool_new(repo);
 	ALLOC_ARRAY(repo->index, 1);
 	index_state_init(repo->index, repo);
 
diff --git a/shallow.c b/shallow.c
index b8cd051e3b6..a10cf9e9d5d 100644
--- a/shallow.c
+++ b/shallow.c
@@ -97,7 +97,7 @@ static void reset_repository_shallow(struct repository *r)
 {
 	r->parsed_objects->is_shallow = -1;
 	stat_validity_clear(r->parsed_objects->shallow_stat);
-	reset_commit_grafts(r);
+	parsed_object_pool_reset_commit_grafts(r->parsed_objects);
 }
 
 int commit_shallow_file(struct repository *r, struct shallow_lock *lk)
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 13/22] shallow: free grafts when unregistering them

[Patrick Steinhardt]

When removing a graft via `unregister_shallow()` we remove it from the
grafts array, but do not free the structure. Fix this to plug the leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 shallow.c                | 4 +++-
 t/t5537-fetch-shallow.sh | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/shallow.c b/shallow.c
index a10cf9e9d5d..7e0ee96ead9 100644
--- a/shallow.c
+++ b/shallow.c
@@ -51,10 +51,12 @@ int unregister_shallow(const struct object_id *oid)
 	int pos = commit_graft_pos(the_repository, oid);
 	if (pos < 0)
 		return -1;
-	if (pos + 1 < the_repository->parsed_objects->grafts_nr)
+	if (pos + 1 < the_repository->parsed_objects->grafts_nr) {
+		free(the_repository->parsed_objects->grafts[pos]);
 		MOVE_ARRAY(the_repository->parsed_objects->grafts + pos,
 			   the_repository->parsed_objects->grafts + pos + 1,
 			   the_repository->parsed_objects->grafts_nr - pos - 1);
+	}
 	the_repository->parsed_objects->grafts_nr--;
 	return 0;
 }
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 37f7547a4ca..cae4d400f32 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -5,6 +5,7 @@ test_description='fetch/clone from a shallow clone'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 commit() {
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 14/22] shallow: fix leaking members of `struct shallow_info`

[Patrick Steinhardt]

We do not free several struct members in `clear_shallow_info()`. Fix
this to plug the resulting leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 shallow.c               | 9 +++++++++
 t/t5538-push-shallow.sh | 1 +
 2 files changed, 10 insertions(+)

diff --git a/shallow.c b/shallow.c
index 7e0ee96ead9..dcebc263d70 100644
--- a/shallow.c
+++ b/shallow.c
@@ -489,6 +489,15 @@ void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa)
 
 void clear_shallow_info(struct shallow_info *info)
 {
+	if (info->used_shallow) {
+		for (size_t i = 0; i < info->shallow->nr; i++)
+			free(info->used_shallow[i]);
+		free(info->used_shallow);
+	}
+
+	free(info->need_reachability_test);
+	free(info->reachable);
+	free(info->shallow_ref);
 	free(info->ours);
 	free(info->theirs);
 }
diff --git a/t/t5538-push-shallow.sh b/t/t5538-push-shallow.sh
index e91fcc173e8..6adc3a20a45 100755
--- a/t/t5538-push-shallow.sh
+++ b/t/t5538-push-shallow.sh
@@ -5,6 +5,7 @@ test_description='push from/to a shallow clone'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 commit() {
-- 
2.46.0.164.g477ce5ccd6.dirty

Re: [PATCH 14/22] shallow: fix leaking members of `struct shallow_info`

[Toon claes]

Patrick Steinhardt <ps@pks.im> writes:

> We do not free several struct members in `clear_shallow_info()`. Fix
> this to plug the resulting leaks.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  shallow.c               | 9 +++++++++
>  t/t5538-push-shallow.sh | 1 +
>  2 files changed, 10 insertions(+)
>
> diff --git a/shallow.c b/shallow.c
> index 7e0ee96ead9..dcebc263d70 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -489,6 +489,15 @@ void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa)
>  
>  void clear_shallow_info(struct shallow_info *info)
>  {
> +	if (info->used_shallow) {
> +		for (size_t i = 0; i < info->shallow->nr; i++)
> +			free(info->used_shallow[i]);
> +		free(info->used_shallow);
> +	}
> +
> +	free(info->need_reachability_test);
> +	free(info->reachable);
> +	free(info->shallow_ref);
>  	free(info->ours);
>  	free(info->theirs);
>  }

Recently was agreed in the CodingGuidelines `S_clear()` functions do a
`S_release()` + `S_init()`. I see we're not initializing the struct for
future use (i.e. we don't reset the `nr_*` fields to 0). But we cannot
really do an init, because that would be calling
`prepare_shallow_info()`, which allocates new memory. So would it be
worth to rename this function to `release_shallow_info()`?

--
Toon

Re: [PATCH 14/22] shallow: fix leaking members of `struct shallow_info`

[Junio C Hamano]

Toon claes <toon@iotcl.com> writes:

>>  void clear_shallow_info(struct shallow_info *info)
>>  {
>> +	if (info->used_shallow) {
>> +		for (size_t i = 0; i < info->shallow->nr; i++)
>> +			free(info->used_shallow[i]);
>> +		free(info->used_shallow);
>> +	}
>> +
>> +	free(info->need_reachability_test);
>> +	free(info->reachable);
>> +	free(info->shallow_ref);
>>  	free(info->ours);
>>  	free(info->theirs);
>>  }
>
> `prepare_shallow_info()`, which allocates new memory. So would it be
> worth to rename this function to `release_shallow_info()`?

In the longer term in a separate "renaming everything" effort, yes.
In the context of "plug many resource leaks" series, probably no.

Thanks.

Re: [PATCH 14/22] shallow: fix leaking members of `struct shallow_info`

[Patrick Steinhardt]

On Thu, Aug 29, 2024 at 09:07:47AM -0700, Junio C Hamano wrote:
> Toon claes <toon@iotcl.com> writes:
> 
> >>  void clear_shallow_info(struct shallow_info *info)
> >>  {
> >> +	if (info->used_shallow) {
> >> +		for (size_t i = 0; i < info->shallow->nr; i++)
> >> +			free(info->used_shallow[i]);
> >> +		free(info->used_shallow);
> >> +	}
> >> +
> >> +	free(info->need_reachability_test);
> >> +	free(info->reachable);
> >> +	free(info->shallow_ref);
> >>  	free(info->ours);
> >>  	free(info->theirs);
> >>  }
> >
> > `prepare_shallow_info()`, which allocates new memory. So would it be
> > worth to rename this function to `release_shallow_info()`?
> 
> In the longer term in a separate "renaming everything" effort, yes.
> In the context of "plug many resource leaks" series, probably no.

Yeah, agreed. And note that `release_shallow_info()` does not match our
style guide, either: it should be `shallow_info_release()`.

Patrick

[PATCH 15/22] negotiator/skipping: fix leaking commit entries

[Patrick Steinhardt]

When releasing the skipping negotiator we free its priority queue, but
not the contained entries. Fix this to plug a memory leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 negotiator/skipping.c                | 7 +++++--
 t/t5552-skipping-fetch-negotiator.sh | 2 ++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/negotiator/skipping.c b/negotiator/skipping.c
index f65d47858b4..b738fe4faef 100644
--- a/negotiator/skipping.c
+++ b/negotiator/skipping.c
@@ -247,8 +247,11 @@ static int ack(struct fetch_negotiator *n, struct commit *c)
 
 static void release(struct fetch_negotiator *n)
 {
-	clear_prio_queue(&((struct data *)n->data)->rev_list);
-	FREE_AND_NULL(n->data);
+	struct data *data = n->data;
+	for (int i = 0; i < data->rev_list.nr; i++)
+		free(data->rev_list.array[i].data);
+	clear_prio_queue(&data->rev_list);
+	FREE_AND_NULL(data);
 }
 
 void skipping_negotiator_init(struct fetch_negotiator *negotiator)
diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh
index b55a9f65e6b..4f2e5ae8dfa 100755
--- a/t/t5552-skipping-fetch-negotiator.sh
+++ b/t/t5552-skipping-fetch-negotiator.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test skipping fetch negotiator'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'fetch.negotiationalgorithm config' '
-- 
2.46.0.164.g477ce5ccd6.dirty

Re: [PATCH 15/22] negotiator/skipping: fix leaking commit entries

[Calvin Wan]

Patrick Steinhardt <ps@pks.im> writes:
> When releasing the skipping negotiator we free its priority queue, but
> not the contained entries. Fix this to plug a memory leak.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  negotiator/skipping.c                | 7 +++++--
>  t/t5552-skipping-fetch-negotiator.sh | 2 ++
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/negotiator/skipping.c b/negotiator/skipping.c
> index f65d47858b4..b738fe4faef 100644
> --- a/negotiator/skipping.c
> +++ b/negotiator/skipping.c
> @@ -247,8 +247,11 @@ static int ack(struct fetch_negotiator *n, struct commit *c)
>  
>  static void release(struct fetch_negotiator *n)
>  {
> -	clear_prio_queue(&((struct data *)n->data)->rev_list);
> -	FREE_AND_NULL(n->data);
> +	struct data *data = n->data;
> +	for (int i = 0; i < data->rev_list.nr; i++)
> +		free(data->rev_list.array[i].data);
> +	clear_prio_queue(&data->rev_list);
> +	FREE_AND_NULL(data);
>  }
>  

It seems unintuitive that clear_prio_queue() doesn't also free the data
underneath and that a caller would have to know to free that as well to
avoid leaking memory. Would it make more sense to add this change to
clear_prio_queue() instead? Patch 14 has that pattern already.

Thanks again for the cleanups -- I'm tempted to take a stab at some of
the other memory leaks you mentioned during our biweekly hackathon. All
of the other patches look reasonable to me.

Re: [PATCH 15/22] negotiator/skipping: fix leaking commit entries

[Josh Steadmon]

On 2024.08.28 20:29, Calvin Wan wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > When releasing the skipping negotiator we free its priority queue, but
> > not the contained entries. Fix this to plug a memory leak.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  negotiator/skipping.c                | 7 +++++--
> >  t/t5552-skipping-fetch-negotiator.sh | 2 ++
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/negotiator/skipping.c b/negotiator/skipping.c
> > index f65d47858b4..b738fe4faef 100644
> > --- a/negotiator/skipping.c
> > +++ b/negotiator/skipping.c
> > @@ -247,8 +247,11 @@ static int ack(struct fetch_negotiator *n, struct commit *c)
> >  
> >  static void release(struct fetch_negotiator *n)
> >  {
> > -	clear_prio_queue(&((struct data *)n->data)->rev_list);
> > -	FREE_AND_NULL(n->data);
> > +	struct data *data = n->data;
> > +	for (int i = 0; i < data->rev_list.nr; i++)
> > +		free(data->rev_list.array[i].data);
> > +	clear_prio_queue(&data->rev_list);
> > +	FREE_AND_NULL(data);
> >  }
> >  
> 
> It seems unintuitive that clear_prio_queue() doesn't also free the data
> underneath and that a caller would have to know to free that as well to
> avoid leaking memory. Would it make more sense to add this change to
> clear_prio_queue() instead? Patch 14 has that pattern already.

I'm assuming the reasoning is that clear_prio_queue() can't know if its
items need more complicated cleanup of their own, so if the caller
(potentially) needs to clean up items individually anyway, the caller
can also free them at the same time?

> Thanks again for the cleanups -- I'm tempted to take a stab at some of
> the other memory leaks you mentioned during our biweekly hackathon. All
> of the other patches look reasonable to me.

The series also looks good to me, thanks!

Reviewed-by: Josh Steadmon <steadmon@google.com>

Re: [PATCH 15/22] negotiator/skipping: fix leaking commit entries

[Patrick Steinhardt]

On Wed, Aug 28, 2024 at 03:19:10PM -0700, Josh Steadmon wrote:
> On 2024.08.28 20:29, Calvin Wan wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> > > When releasing the skipping negotiator we free its priority queue, but
> > > not the contained entries. Fix this to plug a memory leak.
> > > 
> > > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > > ---
> > >  negotiator/skipping.c                | 7 +++++--
> > >  t/t5552-skipping-fetch-negotiator.sh | 2 ++
> > >  2 files changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/negotiator/skipping.c b/negotiator/skipping.c
> > > index f65d47858b4..b738fe4faef 100644
> > > --- a/negotiator/skipping.c
> > > +++ b/negotiator/skipping.c
> > > @@ -247,8 +247,11 @@ static int ack(struct fetch_negotiator *n, struct commit *c)
> > >  
> > >  static void release(struct fetch_negotiator *n)
> > >  {
> > > -	clear_prio_queue(&((struct data *)n->data)->rev_list);
> > > -	FREE_AND_NULL(n->data);
> > > +	struct data *data = n->data;
> > > +	for (int i = 0; i < data->rev_list.nr; i++)
> > > +		free(data->rev_list.array[i].data);
> > > +	clear_prio_queue(&data->rev_list);
> > > +	FREE_AND_NULL(data);
> > >  }
> > >  
> > 
> > It seems unintuitive that clear_prio_queue() doesn't also free the data
> > underneath and that a caller would have to know to free that as well to
> > avoid leaking memory. Would it make more sense to add this change to
> > clear_prio_queue() instead? Patch 14 has that pattern already.
> 
> I'm assuming the reasoning is that clear_prio_queue() can't know if its
> items need more complicated cleanup of their own, so if the caller
> (potentially) needs to clean up items individually anyway, the caller
> can also free them at the same time?

Yeah, that's mostly the reason. We have e.g. `string_list_clear_func()`
that works around this issue by making the caller provide the cleanup
function, and we could use the same pattern here. But it seems as if
most of the callers of `clear_prio_queue()` don't need this because they
already drain the queue during normal operations anyway.

With patch 14 you probably refer to `clear_shallow_info()`? We're not
using a priority queue there, so it is not quite related to the prio
queue we have here. So I'm inclined to leave this as-is, and if we ever
see that we have more callsites that want to clean up the prio queue and
its contents we can introduce `prio_queue_clear_func()`. Does that work
for you?

> > Thanks again for the cleanups -- I'm tempted to take a stab at some of
> > the other memory leaks you mentioned during our biweekly hackathon. All
> > of the other patches look reasonable to me.

I'd certainly be happy to shed some of the load here. If you do, please
give me a quick ping so that we don't duplicate any work by accident.

> The series also looks good to me, thanks!
> 
> Reviewed-by: Josh Steadmon <steadmon@google.com>

Thanks!

Patrick

Re: [PATCH 15/22] negotiator/skipping: fix leaking commit entries

[Calvin Wan]

On Thu, Aug 29, 2024 at 1:41 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> Yeah, that's mostly the reason. We have e.g. `string_list_clear_func()`
> that works around this issue by making the caller provide the cleanup
> function, and we could use the same pattern here. But it seems as if
> most of the callers of `clear_prio_queue()` don't need this because they
> already drain the queue during normal operations anyway.
>
> With patch 14 you probably refer to `clear_shallow_info()`? We're not
> using a priority queue there, so it is not quite related to the prio
> queue we have here. So I'm inclined to leave this as-is, and if we ever
> see that we have more callsites that want to clean up the prio queue and
> its contents we can introduce `prio_queue_clear_func()`. Does that work
> for you?

Yes it does! Thanks for the clarification

[PATCH 16/22] builtin/repack: fix leaking line buffer when packing promisors

[Patrick Steinhardt]

In `repack_promisor_objects()` we read output from git-pack-objects(1)
line by line, using `strbuf_getline_lf()`. We never free the line
buffer, causing a memory leak. Plug it.

This leak is being hit in t5616, but plugging it alone is not
sufficient to make the whole test suite leak free.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/repack.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/repack.c b/builtin/repack.c
index 8bb875532b4..a382754feee 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -425,9 +425,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
 
 		free(promisor_name);
 	}
+
 	fclose(out);
 	if (finish_command(&cmd))
 		die(_("could not finish pack-objects to repack promisor objects"));
+	strbuf_release(&line);
 }
 
 struct pack_geometry {
-- 
2.46.0.164.g477ce5ccd6.dirty

Re: [PATCH 16/22] builtin/repack: fix leaking line buffer when packing promisors

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> In `repack_promisor_objects()` we read output from git-pack-objects(1)
> line by line, using `strbuf_getline_lf()`. We never free the line
> buffer, causing a memory leak. Plug it.
>
> This leak is being hit in t5616, but plugging it alone is not
> sufficient to make the whole test suite leak free.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/repack.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 8bb875532b4..a382754feee 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -425,9 +425,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
>  
>  		free(promisor_name);
>  	}
> +
>  	fclose(out);
>  	if (finish_command(&cmd))
>  		die(_("could not finish pack-objects to repack promisor objects"));
> +	strbuf_release(&line);
>  }

Obviously correct.

Thanks.

[PATCH 17/22] builtin/pack-objects: plug leaking list of keep-packs

[Patrick Steinhardt]

The `--keep-pack` option of git-pack-objects(1) populates the arguments
into a string list. And while the list is marked as `NODUP` and thus
won't duplicate the strings, the list entries themselves still need to
be free'd. We don't though, causing a leak.

Plug it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/pack-objects.c   | 1 +
 t/t5616-partial-clone.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c481feadbfa..ab78d72e273 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4641,6 +4641,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 cleanup:
 	clear_packing_data(&to_pack);
 	list_objects_filter_release(&filter_options);
+	string_list_clear(&keep_pack_list, 0);
 	strvec_clear(&rp);
 
 	return 0;
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 2da7291e379..467c46dccf6 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -5,6 +5,7 @@ test_description='git partial clone'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # create a normal "src" repo where we can later create new commits.
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 18/22] builtin/grep: fix leaking object context

[Patrick Steinhardt]

Even when `get_oid_with_context()` fails it may have allocated some data
in tthe object context. But we do not release it in git-grep(1) when the
call fails, leading to a memory leak. Plug it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/grep.c                 | 1 +
 t/t6132-pathspec-exclude.sh    | 1 +
 t/t6135-pathspec-with-attrs.sh | 2 ++
 3 files changed, 4 insertions(+)

diff --git a/builtin/grep.c b/builtin/grep.c
index dfc3c3e8bd2..dda4582d646 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1133,6 +1133,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 					 &oid, &oc)) {
 			if (seen_dashdash)
 				die(_("unable to resolve revision: %s"), arg);
+			object_context_release(&oc);
 			break;
 		}
 
diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh
index 9fdafeb1e90..f31c09c056f 100755
--- a/t/t6132-pathspec-exclude.sh
+++ b/t/t6132-pathspec-exclude.sh
@@ -2,6 +2,7 @@
 
 test_description='test case exclude pathspec'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index 120dcd74a51..794bc7daf05 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test labels in pathspecs'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup a tree' '
-- 
2.46.0.164.g477ce5ccd6.dirty

Re: [PATCH 18/22] builtin/grep: fix leaking object context

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Even when `get_oid_with_context()` fails it may have allocated some data
> in tthe object context. But we do not release it in git-grep(1) when the

"tthe" -> "the".

> call fails, leading to a memory leak. Plug it.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/grep.c                 | 1 +
>  t/t6132-pathspec-exclude.sh    | 1 +
>  t/t6135-pathspec-with-attrs.sh | 2 ++
>  3 files changed, 4 insertions(+)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index dfc3c3e8bd2..dda4582d646 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1133,6 +1133,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  					 &oid, &oc)) {
>  			if (seen_dashdash)
>  				die(_("unable to resolve revision: %s"), arg);
> +			object_context_release(&oc);
>  			break;
>  		}

OK.  This is the "oh, this is not a revision argument" codepath.  It
is perfectly normal for get_oid_with_context() to fail here, and we
should make sure we clear the context variable.

Thanks.

[PATCH 19/22] builtin/fmt-merge-msg: fix leaking buffers

[Patrick Steinhardt]

Fix leaking input and output buffers in git-fmt-merge-msg(1).

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fmt-merge-msg.c  | 2 ++
 t/t6200-fmt-merge-msg.sh | 1 +
 2 files changed, 3 insertions(+)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 957786d1b3a..0b162f8fab1 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -67,6 +67,8 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)
 		return ret;
 	write_in_full(STDOUT_FILENO, output.buf, output.len);
 
+	strbuf_release(&input);
+	strbuf_release(&output);
 	free(inpath);
 	return 0;
 }
diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index 5a221f8ef1f..ac57b0e4ae3 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -8,6 +8,7 @@ test_description='fmt-merge-msg test'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY/lib-gpg.sh"
 
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 20/22] match-trees: fix leaking prefixes in `shift_tree()`

[Patrick Steinhardt]

In `shift_tree()` we allocate two empty strings that we end up
passing to `match_trees()`. If that function finds a better match it
will update these pointers to point to a newly allocated strings,
freeing the old strings. We never free the final results though, neither
the ones we have allocated ourselves, nor the one that `match_trees()`
might've returned to us.

Fix the resulting memory leaks by creating a common exit path where we
free them.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 match-trees.c            | 10 +++++++---
 t/t6409-merge-subtree.sh |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/match-trees.c b/match-trees.c
index f17c74d483f..147b03abf18 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -294,18 +294,22 @@ void shift_tree(struct repository *r,
 		unsigned short mode;
 
 		if (!*del_prefix)
-			return;
+			goto out;
 
 		if (get_tree_entry(r, hash2, del_prefix, shifted, &mode))
 			die("cannot find path %s in tree %s",
 			    del_prefix, oid_to_hex(hash2));
-		return;
+		goto out;
 	}
 
 	if (!*add_prefix)
-		return;
+		goto out;
 
 	splice_tree(hash1, add_prefix, hash2, shifted);
+
+out:
+	free(add_prefix);
+	free(del_prefix);
 }
 
 /*
diff --git a/t/t6409-merge-subtree.sh b/t/t6409-merge-subtree.sh
index e9ba6f1690d..528615b981f 100755
--- a/t/t6409-merge-subtree.sh
+++ b/t/t6409-merge-subtree.sh
@@ -5,6 +5,7 @@ test_description='subtree merge strategy'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
-- 
2.46.0.164.g477ce5ccd6.dirty

Re: [PATCH 20/22] match-trees: fix leaking prefixes in `shift_tree()`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> In `shift_tree()` we allocate two empty strings that we end up
> passing to `match_trees()`. If that function finds a better match it
> will update these pointers to point to a newly allocated strings,
> freeing the old strings. We never free the final results though, neither
> the ones we have allocated ourselves, nor the one that `match_trees()`
> might've returned to us.
>
> Fix the resulting memory leaks by creating a common exit path where we
> free them.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  match-trees.c            | 10 +++++++---
>  t/t6409-merge-subtree.sh |  1 +
>  2 files changed, 8 insertions(+), 3 deletions(-)

We are not going to take the "best_match" out of this
function, so somebody ought to free it, and that somebody must be
this function.

Makes sense.

[PATCH 21/22] merge-ort: fix two leaks when handling directory rename modifications

[Patrick Steinhardt]

There are two leaks in `apply_directory_rename_modifications()`:

  - We do not release the `dirs_to_insert` string list.

  - We do not release some `conflict_info` we put into the
    `opt->priv->paths` string map.

The former is trivial to fix. The latter is a bit less straight forward:
the `util` pointer of the string map may sometimes point to data that
has been allocated via `CALLOC()`, while at other times it may point to
data that has been allocated via a `mem_pool`.

It very much seems like an oversight that we didn't also allocate the
conflict info in this code path via the memory pool, though. So let's
fix that, which will also plug the memory leak for us.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 merge-ort.c                         | 4 +++-
 t/t6423-merge-rename-directories.sh | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/merge-ort.c b/merge-ort.c
index 3752c7e595d..0ed3cd06b1a 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2710,7 +2710,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
 		struct conflict_info *dir_ci;
 		char *cur_dir = dirs_to_insert.items[i].string;
 
-		CALLOC_ARRAY(dir_ci, 1);
+		dir_ci = mem_pool_calloc(&opt->priv->pool, 1, sizeof(*dir_ci));
 
 		dir_ci->merged.directory_name = parent_name;
 		len = strlen(parent_name);
@@ -2838,6 +2838,8 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
 	 * Finally, record the new location.
 	 */
 	pair->two->path = new_path;
+
+	string_list_clear(&dirs_to_insert, 0);
 }
 
 /*** Function Grouping: functions related to regular rename detection ***/
diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
index 88d1cf2cde9..4aaaf38be68 100755
--- a/t/t6423-merge-rename-directories.sh
+++ b/t/t6423-merge-rename-directories.sh
@@ -25,6 +25,7 @@ test_description="recursive merge with directory renames"
 #                     underscore notation is to differentiate different
 #                     files that might be renamed into each other's paths.)
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-merge.sh
 
-- 
2.46.0.164.g477ce5ccd6.dirty

Re: [PATCH 21/22] merge-ort: fix two leaks when handling directory rename modifications

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> There are two leaks in `apply_directory_rename_modifications()`:
>
>   - We do not release the `dirs_to_insert` string list.
>
>   - We do not release some `conflict_info` we put into the
>     `opt->priv->paths` string map.
>
> The former is trivial to fix. The latter is a bit less straight forward:
> the `util` pointer of the string map may sometimes point to data that
> has been allocated via `CALLOC()`, while at other times it may point to
> data that has been allocated via a `mem_pool`.
>
> It very much seems like an oversight that we didn't also allocate the
> conflict info in this code path via the memory pool, though. So let's
> fix that, which will also plug the memory leak for us.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---

May be nice if we can hear from the original author and the area
expert.

>  merge-ort.c                         | 4 +++-
>  t/t6423-merge-rename-directories.sh | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 3752c7e595d..0ed3cd06b1a 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -2710,7 +2710,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
>  		struct conflict_info *dir_ci;
>  		char *cur_dir = dirs_to_insert.items[i].string;
>  
> -		CALLOC_ARRAY(dir_ci, 1);
> +		dir_ci = mem_pool_calloc(&opt->priv->pool, 1, sizeof(*dir_ci));
>  
>  		dir_ci->merged.directory_name = parent_name;
>  		len = strlen(parent_name);
> @@ -2838,6 +2838,8 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
>  	 * Finally, record the new location.
>  	 */
>  	pair->two->path = new_path;
> +
> +	string_list_clear(&dirs_to_insert, 0);
>  }
>  
>  /*** Function Grouping: functions related to regular rename detection ***/
> diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
> index 88d1cf2cde9..4aaaf38be68 100755
> --- a/t/t6423-merge-rename-directories.sh
> +++ b/t/t6423-merge-rename-directories.sh
> @@ -25,6 +25,7 @@ test_description="recursive merge with directory renames"
>  #                     underscore notation is to differentiate different
>  #                     files that might be renamed into each other's paths.)
>  
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>  . "$TEST_DIRECTORY"/lib-merge.sh

Re: [PATCH 21/22] merge-ort: fix two leaks when handling directory rename modifications

[Elijah Newren]

On Wed, Sep 4, 2024 at 3:56 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Patrick Steinhardt <ps@pks.im> writes:
>
> > There are two leaks in `apply_directory_rename_modifications()`:
> >
> >   - We do not release the `dirs_to_insert` string list.
> >
> >   - We do not release some `conflict_info` we put into the
> >     `opt->priv->paths` string map.
> >
> > The former is trivial to fix. The latter is a bit less straight forward:
> > the `util` pointer of the string map may sometimes point to data that
> > has been allocated via `CALLOC()`, while at other times it may point to
> > data that has been allocated via a `mem_pool`.

Oops.

> > It very much seems like an oversight that we didn't also allocate the
> > conflict info in this code path via the memory pool, though.

Yep.

> > So let's fix that, which will also plug the memory leak for us.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
>
> May be nice if we can hear from the original author and the area
> expert.
>
> >  merge-ort.c                         | 4 +++-
> >  t/t6423-merge-rename-directories.sh | 1 +
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/merge-ort.c b/merge-ort.c
> > index 3752c7e595d..0ed3cd06b1a 100644
> > --- a/merge-ort.c
> > +++ b/merge-ort.c
> > @@ -2710,7 +2710,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
> >               struct conflict_info *dir_ci;
> >               char *cur_dir = dirs_to_insert.items[i].string;
> >
> > -             CALLOC_ARRAY(dir_ci, 1);
> > +             dir_ci = mem_pool_calloc(&opt->priv->pool, 1, sizeof(*dir_ci));
> >
> >               dir_ci->merged.directory_name = parent_name;
> >               len = strlen(parent_name);
> > @@ -2838,6 +2838,8 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
> >        * Finally, record the new location.
> >        */
> >       pair->two->path = new_path;
> > +
> > +     string_list_clear(&dirs_to_insert, 0);
> >  }
> >
> >  /*** Function Grouping: functions related to regular rename detection ***/
> > diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
> > index 88d1cf2cde9..4aaaf38be68 100755
> > --- a/t/t6423-merge-rename-directories.sh
> > +++ b/t/t6423-merge-rename-directories.sh
> > @@ -25,6 +25,7 @@ test_description="recursive merge with directory renames"
> >  #                     underscore notation is to differentiate different
> >  #                     files that might be renamed into each other's paths.)
> >
> > +TEST_PASSES_SANITIZE_LEAK=true
> >  . ./test-lib.sh
> >  . "$TEST_DIRECTORY"/lib-merge.sh

Looks good to me; thanks for fixing up my bugs.

Reviewed-by: Elijah Newren <newren@gmail.com>

[PATCH 22/22] builtin/repack: fix leaking keep-pack list

[Patrick Steinhardt]

The list of packs to keep is populated via a command line option but
never free'd. Plug this memory leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/repack.c            | 1 +
 t/t6500-gc.sh               | 1 +
 t/t7703-repack-geometric.sh | 1 +
 3 files changed, 3 insertions(+)

diff --git a/builtin/repack.c b/builtin/repack.c
index a382754feee..3ee8cfa732f 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -1543,6 +1543,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	}
 
 cleanup:
+	string_list_clear(&keep_pack_list, 0);
 	string_list_clear(&names, 1);
 	existing_packs_release(&existing);
 	free_pack_geometry(&geometry);
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 1b5909d1b70..58654b3437e 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -3,6 +3,7 @@
 test_description='basic git gc tests
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh
index 9fc1626fbfd..8877aea98ba 100755
--- a/t/t7703-repack-geometric.sh
+++ b/t/t7703-repack-geometric.sh
@@ -2,6 +2,7 @@
 
 test_description='git repack --geometric works correctly'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 GIT_TEST_MULTI_PACK_INDEX=0
-- 
2.46.0.164.g477ce5ccd6.dirty

Re: [PATCH 00/22] Memory leak fixes (pt.6)

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> here's another round of memory leak fixes. This is the second-last round
> of "general" leak fixes -- so once this and the next series are merged,
> I have made a full pass over all failing test suites. There will still
> be a bunch of memory leaks, namely ~86 failing test suites.

I've read (or at least briefly scanned) most of the changes in this
series over the course of several days, and did not spot anything
glaringly wrong, other than a small typo or two whose fix should be
trivial.

Thanks.  

[PATCH v2 00/22] Memory leak fixes (pt.6)

[Patrick Steinhardt]

Hi,

this is the second version of this round of memory leak fixes.

There are only some smallish changes compared to v1:

  - Explain leak checking a bit more carefully and document the new
    `GIT_TEST_PASSING_SANITIZE_LEAK=check-failing` value in t/README.

  - Some more trivial commit message improvements.

Thanks!

Patrick

Patrick Steinhardt (22):
  t/test-lib: allow skipping leak checks for passing tests
  fetch-pack: fix memory leaks on fetch negotiation
  send-pack: fix leaking common object IDs
  builtin/push: fix leaking refspec query result
  upload-pack: fix leaking child process data on reachability checks
  submodule: fix leaking fetch task data
  builtin/submodule--helper: fix leaking refs on push-check
  remote: fix leaking tracking refs
  remote: fix leak in reachability check of a remote-tracking ref
  send-pack: fix leaking push cert nonce
  gpg-interface: fix misdesigned signing key interfaces
  object: clear grafts when clearing parsed object pool
  shallow: free grafts when unregistering them
  shallow: fix leaking members of `struct shallow_info`
  negotiator/skipping: fix leaking commit entries
  builtin/repack: fix leaking line buffer when packing promisors
  builtin/pack-objects: plug leaking list of keep-packs
  builtin/grep: fix leaking object context
  builtin/fmt-merge-msg: fix leaking buffers
  match-trees: fix leaking prefixes in `shift_tree()`
  merge-ort: fix two leaks when handling directory rename modifications
  builtin/repack: fix leaking keep-pack list

 builtin/fmt-merge-msg.c              |  2 ++
 builtin/grep.c                       |  1 +
 builtin/pack-objects.c               |  1 +
 builtin/push.c                       |  8 +++--
 builtin/repack.c                     |  3 ++
 builtin/submodule--helper.c          |  2 ++
 builtin/tag.c                        |  3 +-
 commit.c                             | 23 ++++--------
 commit.h                             |  3 +-
 fetch-pack.c                         |  3 ++
 gpg-interface.c                      | 26 ++++++++------
 gpg-interface.h                      |  4 +--
 match-trees.c                        | 10 ++++--
 merge-ort.c                          |  4 ++-
 negotiator/skipping.c                |  7 ++--
 object.c                             | 14 +++++++-
 object.h                             |  4 ++-
 remote.c                             |  6 +++-
 repository.c                         |  2 +-
 send-pack.c                          | 52 ++++++++++++++++++----------
 shallow.c                            | 15 ++++++--
 submodule.c                          |  2 ++
 t/README                             |  3 ++
 t/t5516-fetch-push.sh                |  1 +
 t/t5526-fetch-submodules.sh          |  1 +
 t/t5531-deep-submodule-push.sh       |  1 +
 t/t5533-push-cas.sh                  |  1 +
 t/t5534-push-signed.sh               |  1 +
 t/t5537-fetch-shallow.sh             |  1 +
 t/t5538-push-shallow.sh              |  1 +
 t/t5549-fetch-push-http.sh           |  1 +
 t/t5552-skipping-fetch-negotiator.sh |  2 ++
 t/t5616-partial-clone.sh             |  1 +
 t/t6132-pathspec-exclude.sh          |  1 +
 t/t6135-pathspec-with-attrs.sh       |  2 ++
 t/t6200-fmt-merge-msg.sh             |  1 +
 t/t6409-merge-subtree.sh             |  1 +
 t/t6423-merge-rename-directories.sh  |  1 +
 t/t6500-gc.sh                        |  1 +
 t/t7703-repack-geometric.sh          |  1 +
 t/test-lib.sh                        | 11 +++++-
 upload-pack.c                        | 22 ++++++++----
 42 files changed, 178 insertions(+), 72 deletions(-)

Range-diff against v1:
 1:  7c158acadf4 !  1:  fa62d0106a5 t/test-lib: allow skipping leak checks for passing tests
    @@ Commit message
     
         With `GIT_TEST_PASSING_SANITIZE_LEAK=check`, one can double check
         whether a memory leak fix caused some test suites to become leak free.
    +    This is done by running all tests with the leak checker enabled. If a
    +    test suite does not declare `TEST_PASSES_SANITIZE_LEAK=true` but still
    +    finishes successfully with the leak checker enabled, then this indicates
    +    that the test is leak free and thus missing the annotation.
    +
         It is somewhat slow to execute though because it runs all of our test
         suites with the leak sanitizer enabled. It is also pointless in most
         cases, because the only test suites that need to be checked are those
         which _aren't_ yet marked with `TEST_PASSES_SANITIZE_LEAK=true`.
     
    -    Introduce a new value "check-failing". If set, we will only check those
    -    tests which are not yet marked as leak free.
    +    Introduce a new value "check-failing". When set, we behave the same as
    +    if "check" was passed, except that we only check those tests which do
    +    not have `TEST_PASSES_SANITIZE_LEAK=true` set. This is significantly
    +    faster than running all test suites but still fulfills the usecase of
    +    finding newly-leak-free test suites.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    + ## t/README ##
    +@@ t/README: GIT_TEST_PASSING_SANITIZE_LEAK=check when combined with "--immediate"
    + will run to completion faster, and result in the same failing
    + tests.
    + 
    ++GIT_TEST_PASSING_SANITIZE_LEAK=check-failing behaves the same as "check",
    ++but skips all tests which are already marked as leak-free.
    ++
    + GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version'
    + default to n.
    + 
    +
      ## t/test-lib.sh ##
     @@ t/test-lib.sh: then
      		passes_sanitize_leak=t
 2:  33bc728a345 =  2:  611a29d1ca3 fetch-pack: fix memory leaks on fetch negotiation
 3:  a13db9777f0 =  3:  0d969962a39 send-pack: fix leaking common object IDs
 4:  92fc97b3db8 =  4:  834a184c855 builtin/push: fix leaking refspec query result
 5:  77023421e18 =  5:  17b0c4b577a upload-pack: fix leaking child process data on reachability checks
 6:  b4b65c3783c =  6:  872f39faece submodule: fix leaking fetch task data
 7:  e3d1ac1712f =  7:  3cbd6fe808e builtin/submodule--helper: fix leaking refs on push-check
 8:  7fafcc53d23 =  8:  90647301de5 remote: fix leaking tracking refs
 9:  1446e42f0ba =  9:  bb845fc9ff1 remote: fix leak in reachability check of a remote-tracking ref
10:  138a5ded35a ! 10:  a1baba39bc5 send-pack: fix leaking push cert nonce
    @@ Commit message
     
         When retrieving the push cert nonce from the server, we first store the
         constant returned by `server_feature_value()` and then, if the nonce is
    -    valid, we duplicate the nonce memory to extend its lifetime. We never
    -    free the latter and thus cause a memory leak.
    +    valid, we duplicate the nonce memory to a NUL-terminated string, so that
    +    we can pass it to `generate_push_cert()`. We never free the latter and
    +    thus cause a memory leak.
     
         Fix this by storing the limited-lifetime nonce into a scope-local
         variable such that the long-lived, allocated nonce can be easily freed
11:  a1fca8104b3 = 11:  ddebe2f6f6b gpg-interface: fix misdesigned signing key interfaces
12:  922b8640a55 = 12:  242f6b76db3 object: clear grafts when clearing parsed object pool
13:  ec9a5143241 = 13:  4747042cb6a shallow: free grafts when unregistering them
14:  2a63030ff09 = 14:  d3996c92d80 shallow: fix leaking members of `struct shallow_info`
15:  920db3a2912 = 15:  66ed1151449 negotiator/skipping: fix leaking commit entries
16:  19eb9073482 = 16:  78c1e5e1772 builtin/repack: fix leaking line buffer when packing promisors
17:  017713f5a9c = 17:  d2e108040fd builtin/pack-objects: plug leaking list of keep-packs
18:  0722cb38ea9 ! 18:  9fd891f5222 builtin/grep: fix leaking object context
    @@ Commit message
         builtin/grep: fix leaking object context
     
         Even when `get_oid_with_context()` fails it may have allocated some data
    -    in tthe object context. But we do not release it in git-grep(1) when the
    +    in the object context. But we do not release it in git-grep(1) when the
         call fails, leading to a memory leak. Plug it.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
19:  b37391c0d6b = 19:  84b68affa0d builtin/fmt-merge-msg: fix leaking buffers
20:  05461e3b1c0 = 20:  bbb8ab20229 match-trees: fix leaking prefixes in `shift_tree()`
21:  da1c23a9ccf = 21:  db0e7a8733a merge-ort: fix two leaks when handling directory rename modifications
22:  3d30c727fbc = 22:  2368924995f builtin/repack: fix leaking keep-pack list

base-commit: 098ca6ba562006675df6a6d0948400d2d955458b
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 01/22] t/test-lib: allow skipping leak checks for passing tests

[Patrick Steinhardt]

With `GIT_TEST_PASSING_SANITIZE_LEAK=check`, one can double check
whether a memory leak fix caused some test suites to become leak free.
This is done by running all tests with the leak checker enabled. If a
test suite does not declare `TEST_PASSES_SANITIZE_LEAK=true` but still
finishes successfully with the leak checker enabled, then this indicates
that the test is leak free and thus missing the annotation.

It is somewhat slow to execute though because it runs all of our test
suites with the leak sanitizer enabled. It is also pointless in most
cases, because the only test suites that need to be checked are those
which _aren't_ yet marked with `TEST_PASSES_SANITIZE_LEAK=true`.

Introduce a new value "check-failing". When set, we behave the same as
if "check" was passed, except that we only check those tests which do
not have `TEST_PASSES_SANITIZE_LEAK=true` set. This is significantly
faster than running all test suites but still fulfills the usecase of
finding newly-leak-free test suites.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/README      |  3 +++
 t/test-lib.sh | 11 ++++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/t/README b/t/README
index 44c02d81298..8dcb778e260 100644
--- a/t/README
+++ b/t/README
@@ -386,6 +386,9 @@ GIT_TEST_PASSING_SANITIZE_LEAK=check when combined with "--immediate"
 will run to completion faster, and result in the same failing
 tests.
 
+GIT_TEST_PASSING_SANITIZE_LEAK=check-failing behaves the same as "check",
+but skips all tests which are already marked as leak-free.
+
 GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version'
 default to n.
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 54247604cbc..64bd36531c1 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1558,8 +1558,16 @@ then
 		passes_sanitize_leak=t
 	fi
 
-	if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check"
+	if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" ||
+	   test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing"
 	then
+		if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" &&
+		   test -n "$passes_sanitize_leak"
+		then
+			skip_all="skipping leak-free $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=check-failing"
+			test_done
+		fi
+
 		sanitize_leak_check=t
 		if test -n "$invert_exit_code"
 		then
@@ -1597,6 +1605,7 @@ then
 	export LSAN_OPTIONS
 
 elif test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" ||
+     test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" ||
      test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
 then
 	BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_PASSING_SANITIZE_LEAK=true"
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 02/22] fetch-pack: fix memory leaks on fetch negotiation

[Patrick Steinhardt]

We leak both the `nt_object_array` and `negotiator` structures in
`negotiate_using_fetch()`. Plug both of these leaks.

These leaks were exposed by t5516, but fixing them is not sufficient to
make the whole test suite leak free.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 fetch-pack.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fetch-pack.c b/fetch-pack.c
index 58b4581ad80..0ed82feda14 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -2227,7 +2227,10 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
 	trace2_region_leave("fetch-pack", "negotiate_using_fetch", the_repository);
 	trace2_data_intmax("negotiate_using_fetch", the_repository,
 			   "total_rounds", negotiation_round);
+
 	clear_common_flag(acked_commits);
+	object_array_clear(&nt_object_array);
+	negotiator.release(&negotiator);
 	strbuf_release(&req_buf);
 }
 
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 03/22] send-pack: fix leaking common object IDs

[Patrick Steinhardt]

We're leaking the array of common object IDs in `send_pack()`. Fix this
by creating a common exit path where we free the leaking data. While at
it, unify some other cleanups now that we have a central place to put
them.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 send-pack.c                | 34 ++++++++++++++++++++++------------
 t/t5549-fetch-push-http.sh |  1 +
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index fa2f5eec17b..b224ef9fc5e 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -508,7 +508,8 @@ int send_pack(struct send_pack_args *args,
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
 			"Perhaps you should specify a branch.\n");
-		return 0;
+		ret = 0;
+		goto out;
 	}
 
 	git_config_get_bool("push.negotiate", &push_negotiate);
@@ -615,12 +616,11 @@ int send_pack(struct send_pack_args *args,
 			 * atomically, abort the whole operation.
 			 */
 			if (use_atomic) {
-				strbuf_release(&req_buf);
-				strbuf_release(&cap_buf);
 				reject_atomic_push(remote_refs, args->send_mirror);
 				error("atomic push failed for ref %s. status: %d\n",
 				      ref->name, ref->status);
-				return args->porcelain ? 0 : -1;
+				ret = args->porcelain ? 0 : -1;
+				goto out;
 			}
 			/* else fallthrough */
 		default:
@@ -682,8 +682,6 @@ int send_pack(struct send_pack_args *args,
 		write_or_die(out, req_buf.buf, req_buf.len);
 		packet_flush(out);
 	}
-	strbuf_release(&req_buf);
-	strbuf_release(&cap_buf);
 
 	if (use_sideband && cmds_sent) {
 		memset(&demux, 0, sizeof(demux));
@@ -721,7 +719,9 @@ int send_pack(struct send_pack_args *args,
 				finish_async(&demux);
 			}
 			fd[1] = -1;
-			return -1;
+
+			ret = -1;
+			goto out;
 		}
 		if (!args->stateless_rpc)
 			/* Closed by pack_objects() via start_command() */
@@ -746,10 +746,12 @@ int send_pack(struct send_pack_args *args,
 	}
 
 	if (ret < 0)
-		return ret;
+		goto out;
 
-	if (args->porcelain)
-		return 0;
+	if (args->porcelain) {
+		ret = 0;
+		goto out;
+	}
 
 	for (ref = remote_refs; ref; ref = ref->next) {
 		switch (ref->status) {
@@ -758,8 +760,16 @@ int send_pack(struct send_pack_args *args,
 		case REF_STATUS_OK:
 			break;
 		default:
-			return -1;
+			ret = -1;
+			goto out;
 		}
 	}
-	return 0;
+
+	ret = 0;
+
+out:
+	oid_array_clear(&commons);
+	strbuf_release(&req_buf);
+	strbuf_release(&cap_buf);
+	return ret;
 }
diff --git a/t/t5549-fetch-push-http.sh b/t/t5549-fetch-push-http.sh
index 2cdebcb7356..6377fb6d993 100755
--- a/t/t5549-fetch-push-http.sh
+++ b/t/t5549-fetch-push-http.sh
@@ -5,6 +5,7 @@ test_description='fetch/push functionality using the HTTP protocol'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 04/22] builtin/push: fix leaking refspec query result

[Patrick Steinhardt]

When appending a refspec via `refspec_append_mapped()` we leak the
result of `query_refspecs()`. The overall logic around refspec queries
is quite weird, as callers are expected to either set the `src` or `dst`
pointers, and then the (allocated) result will be in the respective
other struct member.

As we have the `src` member set, plugging the memory leak is thus as
easy as just freeing the `dst` member. While at it, use designated
initializers to initialize the structure.

This leak was exposed by t5516, but fixing it is not sufficient to make
the whole test suite leak free.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/push.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 7a67398124f..0b123eb9c1e 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -72,13 +72,15 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,
 	const char *branch_name;
 
 	if (remote->push.nr) {
-		struct refspec_item query;
-		memset(&query, 0, sizeof(struct refspec_item));
-		query.src = matched->name;
+		struct refspec_item query = {
+			.src = matched->name,
+		};
+
 		if (!query_refspecs(&remote->push, &query) && query.dst) {
 			refspec_appendf(refspec, "%s%s:%s",
 					query.force ? "+" : "",
 					query.src, query.dst);
+			free(query.dst);
 			return;
 		}
 	}
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 05/22] upload-pack: fix leaking child process data on reachability checks

[Patrick Steinhardt]

We spawn a git-rev-list(1) command to perform reachability checks in
"upload-pack.c". We do not release memory associated with the process
in error cases though, thus leaking memory.

Fix these by calling `child_process_clear()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5516-fetch-push.sh |  1 +
 upload-pack.c         | 22 ++++++++++++++++------
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 9d693eb57f7..331778bd42c 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -19,6 +19,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 TEST_CREATE_REPO_NO_TEMPLATE=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 D=$(pwd)
diff --git a/upload-pack.c b/upload-pack.c
index f03ba3e98be..c84c3c3b1f5 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -709,10 +709,13 @@ static int get_reachable_list(struct upload_pack_data *data,
 	struct object *o;
 	char namebuf[GIT_MAX_HEXSZ + 2]; /* ^ + hash + LF */
 	const unsigned hexsz = the_hash_algo->hexsz;
+	int ret;
 
 	if (do_reachable_revlist(&cmd, &data->shallows, reachable,
-				 data->allow_uor) < 0)
-		return -1;
+				 data->allow_uor) < 0) {
+		ret = -1;
+		goto out;
+	}
 
 	while ((i = read_in_full(cmd.out, namebuf, hexsz + 1)) == hexsz + 1) {
 		struct object_id oid;
@@ -736,10 +739,16 @@ static int get_reachable_list(struct upload_pack_data *data,
 	}
 	close(cmd.out);
 
-	if (finish_command(&cmd))
-		return -1;
+	if (finish_command(&cmd)) {
+		ret = -1;
+		goto out;
+	}
 
-	return 0;
+	ret = 0;
+
+out:
+	child_process_clear(&cmd);
+	return ret;
 }
 
 static int has_unreachable(struct object_array *src, enum allow_uor allow_uor)
@@ -749,7 +758,7 @@ static int has_unreachable(struct object_array *src, enum allow_uor allow_uor)
 	int i;
 
 	if (do_reachable_revlist(&cmd, src, NULL, allow_uor) < 0)
-		return 1;
+		goto error;
 
 	/*
 	 * The commits out of the rev-list are not ancestors of
@@ -775,6 +784,7 @@ static int has_unreachable(struct object_array *src, enum allow_uor allow_uor)
 error:
 	if (cmd.out >= 0)
 		close(cmd.out);
+	child_process_clear(&cmd);
 	return 1;
 }
 
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 06/22] submodule: fix leaking fetch task data

[Patrick Steinhardt]

The `submodule_parallel_fetch` structure contains various data
structures that we use to set up parallel fetches of submodules. We do
not free some of its data though, causing memory leaks. Plug those.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 submodule.c                 | 2 ++
 t/t5526-fetch-submodules.sh | 1 +
 2 files changed, 3 insertions(+)

diff --git a/submodule.c b/submodule.c
index 97516b0fec1..97d0d47b561 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1883,6 +1883,8 @@ int fetch_submodules(struct repository *r,
 out:
 	free_submodules_data(&spf.changed_submodule_names);
 	string_list_clear(&spf.seen_submodule_names, 0);
+	strbuf_release(&spf.submodules_with_errors);
+	free(spf.oid_fetch_tasks);
 	return spf.result;
 }
 
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 5e566205ba4..2cfb5bd6bb1 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -6,6 +6,7 @@ test_description='Recursive "git fetch" for submodules'
 GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1
 export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 pwd=$(pwd)
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 07/22] builtin/submodule--helper: fix leaking refs on push-check

[Patrick Steinhardt]

In the push-check subcommand of the submodule helper we acquire a list
of local refs, but never free that list. Fix this memory leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/submodule--helper.c    | 2 ++
 t/t5531-deep-submodule-push.sh | 1 +
 2 files changed, 3 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 85fb23dee84..642a0edabf0 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2958,7 +2958,9 @@ static int push_check(int argc, const char **argv, const char *prefix UNUSED)
 				    rs->src);
 			}
 		}
+
 		refspec_clear(&refspec);
+		free_refs(local_refs);
 	}
 	free(head);
 
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index f3fff557447..135823630a3 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -8,6 +8,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1
 export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 08/22] remote: fix leaking tracking refs

[Patrick Steinhardt]

When computing the remote tracking ref we cause two memory leaks:

  - We leak when `remote_tracking()` fails.

  - We leak when the call to `remote_tracking()` succeeds and sets
    `ref->tracking_ref()`.

Fix both of these leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 remote.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index 8f3dee13186..240311619ab 100644
--- a/remote.c
+++ b/remote.c
@@ -1123,6 +1123,7 @@ void free_one_ref(struct ref *ref)
 		return;
 	free_one_ref(ref->peer_ref);
 	free(ref->remote_status);
+	free(ref->tracking_ref);
 	free(ref->symref);
 	free(ref);
 }
@@ -2620,8 +2621,10 @@ static int remote_tracking(struct remote *remote, const char *refname,
 	dst = apply_refspecs(&remote->fetch, refname);
 	if (!dst)
 		return -1; /* no tracking ref for refname at remote */
-	if (refs_read_ref(get_main_ref_store(the_repository), dst, oid))
+	if (refs_read_ref(get_main_ref_store(the_repository), dst, oid)) {
+		free(dst);
 		return -1; /* we know what the tracking ref is but we cannot read it */
+	}
 
 	*dst_refname = dst;
 	return 0;
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 09/22] remote: fix leak in reachability check of a remote-tracking ref

[Patrick Steinhardt]

In `check_if_includes_upstream()` we retrieve the local ref
corresponding to a remote-tracking ref we want to check reachability
for. We never free that local ref and thus cause a memory leak. Fix
this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 remote.c            | 1 +
 t/t5533-push-cas.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/remote.c b/remote.c
index 240311619ab..bff54046b2c 100644
--- a/remote.c
+++ b/remote.c
@@ -2774,6 +2774,7 @@ static void check_if_includes_upstream(struct ref *remote)
 
 	if (is_reachable_in_reflog(local->name, remote) <= 0)
 		remote->unreachable = 1;
+	free_one_ref(local);
 }
 
 static void apply_cas(struct push_cas_option *cas,
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index cba26a872dd..6365d99777e 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -5,6 +5,7 @@ test_description='compare & swap push force/delete safety'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 setup_srcdst_basic () {
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 10/22] send-pack: fix leaking push cert nonce

[Patrick Steinhardt]

When retrieving the push cert nonce from the server, we first store the
constant returned by `server_feature_value()` and then, if the nonce is
valid, we duplicate the nonce memory to a NUL-terminated string, so that
we can pass it to `generate_push_cert()`. We never free the latter and
thus cause a memory leak.

Fix this by storing the limited-lifetime nonce into a scope-local
variable such that the long-lived, allocated nonce can be easily freed
without having to cast away its constness.

This leak was exposed by t5534, but fixing it is not sufficient to make
the whole test suite leak free.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 send-pack.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index b224ef9fc5e..c37f6ab3c07 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -501,7 +501,7 @@ int send_pack(struct send_pack_args *args,
 	unsigned cmds_sent = 0;
 	int ret;
 	struct async demux;
-	const char *push_cert_nonce = NULL;
+	char *push_cert_nonce = NULL;
 	struct packet_reader reader;
 	int use_bitmaps;
 
@@ -550,10 +550,11 @@ int send_pack(struct send_pack_args *args,
 
 	if (args->push_cert != SEND_PACK_PUSH_CERT_NEVER) {
 		size_t len;
-		push_cert_nonce = server_feature_value("push-cert", &len);
-		if (push_cert_nonce) {
-			reject_invalid_nonce(push_cert_nonce, len);
-			push_cert_nonce = xmemdupz(push_cert_nonce, len);
+		const char *nonce = server_feature_value("push-cert", &len);
+
+		if (nonce) {
+			reject_invalid_nonce(nonce, len);
+			push_cert_nonce = xmemdupz(nonce, len);
 		} else if (args->push_cert == SEND_PACK_PUSH_CERT_ALWAYS) {
 			die(_("the receiving end does not support --signed push"));
 		} else if (args->push_cert == SEND_PACK_PUSH_CERT_IF_ASKED) {
@@ -771,5 +772,6 @@ int send_pack(struct send_pack_args *args,
 	oid_array_clear(&commons);
 	strbuf_release(&req_buf);
 	strbuf_release(&cap_buf);
+	free(push_cert_nonce);
 	return ret;
 }
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 11/22] gpg-interface: fix misdesigned signing key interfaces

[Patrick Steinhardt]

The interfaces to retrieve signing keys and their IDs are misdesigned as
they return string constants even though they indeed allocate memory,
which leads to memory leaks. Refactor the code to instead always return
allocated strings and let the callers free them accordingly.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/tag.c          |  3 ++-
 commit.c               |  9 ++++++---
 gpg-interface.c        | 26 +++++++++++++++-----------
 gpg-interface.h        |  4 ++--
 send-pack.c            |  6 ++++--
 t/t5534-push-signed.sh |  1 +
 6 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index a1fb218512c..ab3b500543d 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -160,7 +160,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid,
 	const struct git_hash_algo *compat = the_repository->compat_hash_algo;
 	struct strbuf sig = STRBUF_INIT, compat_sig = STRBUF_INIT;
 	struct strbuf compat_buf = STRBUF_INIT;
-	const char *keyid = get_signing_key();
+	char *keyid = get_signing_key();
 	int ret = -1;
 
 	if (sign_buffer(buffer, &sig, keyid))
@@ -190,6 +190,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid,
 	strbuf_release(&sig);
 	strbuf_release(&compat_sig);
 	strbuf_release(&compat_buf);
+	free(keyid);
 	return ret;
 }
 
diff --git a/commit.c b/commit.c
index 24ab5c1b509..ec9efc189d5 100644
--- a/commit.c
+++ b/commit.c
@@ -1150,11 +1150,14 @@ int add_header_signature(struct strbuf *buf, struct strbuf *sig, const struct gi
 
 static int sign_commit_to_strbuf(struct strbuf *sig, struct strbuf *buf, const char *keyid)
 {
+	char *keyid_to_free = NULL;
+	int ret = 0;
 	if (!keyid || !*keyid)
-		keyid = get_signing_key();
+		keyid = keyid_to_free = get_signing_key();
 	if (sign_buffer(buf, sig, keyid))
-		return -1;
-	return 0;
+		ret = -1;
+	free(keyid_to_free);
+	return ret;
 }
 
 int parse_signed_commit(const struct commit *commit,
diff --git a/gpg-interface.c b/gpg-interface.c
index 6587085cd19..cf6126b5aa0 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -45,8 +45,8 @@ struct gpg_format {
 				    size_t signature_size);
 	int (*sign_buffer)(struct strbuf *buffer, struct strbuf *signature,
 			   const char *signing_key);
-	const char *(*get_default_key)(void);
-	const char *(*get_key_id)(void);
+	char *(*get_default_key)(void);
+	char *(*get_key_id)(void);
 };
 
 static const char *openpgp_verify_args[] = {
@@ -86,9 +86,9 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
 static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
 			   const char *signing_key);
 
-static const char *get_default_ssh_signing_key(void);
+static char *get_default_ssh_signing_key(void);
 
-static const char *get_ssh_key_id(void);
+static char *get_ssh_key_id(void);
 
 static struct gpg_format gpg_format[] = {
 	{
@@ -847,7 +847,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key)
 }
 
 /* Returns the first public key from an ssh-agent to use for signing */
-static const char *get_default_ssh_signing_key(void)
+static char *get_default_ssh_signing_key(void)
 {
 	struct child_process ssh_default_key = CHILD_PROCESS_INIT;
 	int ret = -1;
@@ -899,12 +899,16 @@ static const char *get_default_ssh_signing_key(void)
 	return default_key;
 }
 
-static const char *get_ssh_key_id(void) {
-	return get_ssh_key_fingerprint(get_signing_key());
+static char *get_ssh_key_id(void)
+{
+	char *signing_key = get_signing_key();
+	char *key_id = get_ssh_key_fingerprint(signing_key);
+	free(signing_key);
+	return key_id;
 }
 
 /* Returns a textual but unique representation of the signing key */
-const char *get_signing_key_id(void)
+char *get_signing_key_id(void)
 {
 	gpg_interface_lazy_init();
 
@@ -916,17 +920,17 @@ const char *get_signing_key_id(void)
 	return get_signing_key();
 }
 
-const char *get_signing_key(void)
+char *get_signing_key(void)
 {
 	gpg_interface_lazy_init();
 
 	if (configured_signing_key)
-		return configured_signing_key;
+		return xstrdup(configured_signing_key);
 	if (use_format->get_default_key) {
 		return use_format->get_default_key();
 	}
 
-	return git_committer_info(IDENT_STRICT | IDENT_NO_DATE);
+	return xstrdup(git_committer_info(IDENT_STRICT | IDENT_NO_DATE));
 }
 
 const char *gpg_trust_level_to_str(enum signature_trust_level level)
diff --git a/gpg-interface.h b/gpg-interface.h
index 7cd98161f7f..e09f12e8d04 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -80,13 +80,13 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
 const char *gpg_trust_level_to_str(enum signature_trust_level level);
 
 void set_signing_key(const char *);
-const char *get_signing_key(void);
+char *get_signing_key(void);
 
 /*
  * Returns a textual unique representation of the signing key in use
  * Either a GPG KeyID or a SSH Key Fingerprint
  */
-const char *get_signing_key_id(void);
+char *get_signing_key_id(void);
 int check_signature(struct signature_check *sigc,
 		    const char *signature, size_t slen);
 void print_signature_buffer(const struct signature_check *sigc,
diff --git a/send-pack.c b/send-pack.c
index c37f6ab3c07..31a62e6a98c 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -348,7 +348,8 @@ static int generate_push_cert(struct strbuf *req_buf,
 {
 	const struct ref *ref;
 	struct string_list_item *item;
-	char *signing_key_id = xstrdup(get_signing_key_id());
+	char *signing_key_id = get_signing_key_id();
+	char *signing_key = get_signing_key();
 	const char *cp, *np;
 	struct strbuf cert = STRBUF_INIT;
 	int update_seen = 0;
@@ -381,7 +382,7 @@ static int generate_push_cert(struct strbuf *req_buf,
 	if (!update_seen)
 		goto free_return;
 
-	if (sign_buffer(&cert, &cert, get_signing_key()))
+	if (sign_buffer(&cert, &cert, signing_key))
 		die(_("failed to sign the push certificate"));
 
 	packet_buf_write(req_buf, "push-cert%c%s", 0, cap_string);
@@ -394,6 +395,7 @@ static int generate_push_cert(struct strbuf *req_buf,
 
 free_return:
 	free(signing_key_id);
+	free(signing_key);
 	strbuf_release(&cert);
 	return update_seen;
 }
diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index c91a62b77af..d43aee0c327 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -5,6 +5,7 @@ test_description='signed push'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-gpg.sh
 
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 12/22] object: clear grafts when clearing parsed object pool

[Patrick Steinhardt]

We do not clear grafts part of the parsed object pool when clearing the
pool itself, which can lead to memory leaks when a repository is being
cleared.

Fix this by moving `reset_commit_grafts()` into "object.c" and making it
part of the `struct parsed_object_pool` interface such that we can call
it from `parsed_object_pool_clear()`. Adapt `parsed_object_pool_new()`
to take and store a reference to its owning repository, which is needed
by `unparse_commit()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 commit.c     | 14 +-------------
 commit.h     |  3 ++-
 object.c     | 14 +++++++++++++-
 object.h     |  4 +++-
 repository.c |  2 +-
 shallow.c    |  2 +-
 6 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/commit.c b/commit.c
index ec9efc189d5..bbef0e81c65 100644
--- a/commit.c
+++ b/commit.c
@@ -177,7 +177,7 @@ int commit_graft_pos(struct repository *r, const struct object_id *oid)
 		       commit_graft_oid_access);
 }
 
-static void unparse_commit(struct repository *r, const struct object_id *oid)
+void unparse_commit(struct repository *r, const struct object_id *oid)
 {
 	struct commit *c = lookup_commit(r, oid);
 
@@ -318,18 +318,6 @@ int for_each_commit_graft(each_commit_graft_fn fn, void *cb_data)
 	return ret;
 }
 
-void reset_commit_grafts(struct repository *r)
-{
-	int i;
-
-	for (i = 0; i < r->parsed_objects->grafts_nr; i++) {
-		unparse_commit(r, &r->parsed_objects->grafts[i]->oid);
-		free(r->parsed_objects->grafts[i]);
-	}
-	r->parsed_objects->grafts_nr = 0;
-	r->parsed_objects->commit_graft_prepared = 0;
-}
-
 struct commit_buffer {
 	void *buffer;
 	unsigned long size;
diff --git a/commit.h b/commit.h
index d62b1d93f95..5ba0f77b1eb 100644
--- a/commit.h
+++ b/commit.h
@@ -108,6 +108,8 @@ static inline int repo_parse_commit_no_graph(struct repository *r,
 
 void parse_commit_or_die(struct commit *item);
 
+void unparse_commit(struct repository *r, const struct object_id *oid);
+
 struct buffer_slab;
 struct buffer_slab *allocate_commit_buffer_slab(void);
 void free_commit_buffer_slab(struct buffer_slab *bs);
@@ -240,7 +242,6 @@ int commit_graft_pos(struct repository *r, const struct object_id *oid);
 int register_commit_graft(struct repository *r, struct commit_graft *, int);
 void prepare_commit_graft(struct repository *r);
 struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid);
-void reset_commit_grafts(struct repository *r);
 
 struct commit *get_fork_point(const char *refname, struct commit *commit);
 
diff --git a/object.c b/object.c
index d756c7f2ea3..94ea8fb8d2c 100644
--- a/object.c
+++ b/object.c
@@ -545,11 +545,12 @@ void repo_clear_commit_marks(struct repository *r, unsigned int flags)
 	}
 }
 
-struct parsed_object_pool *parsed_object_pool_new(void)
+struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)
 {
 	struct parsed_object_pool *o = xmalloc(sizeof(*o));
 	memset(o, 0, sizeof(*o));
 
+	o->repo = repo;
 	o->blob_state = allocate_alloc_state();
 	o->tree_state = allocate_alloc_state();
 	o->commit_state = allocate_alloc_state();
@@ -628,6 +629,16 @@ void raw_object_store_clear(struct raw_object_store *o)
 	hashmap_clear(&o->pack_map);
 }
 
+void parsed_object_pool_reset_commit_grafts(struct parsed_object_pool *o)
+{
+	for (int i = 0; i < o->grafts_nr; i++) {
+		unparse_commit(o->repo, &o->grafts[i]->oid);
+		free(o->grafts[i]);
+	}
+	o->grafts_nr = 0;
+	o->commit_graft_prepared = 0;
+}
+
 void parsed_object_pool_clear(struct parsed_object_pool *o)
 {
 	/*
@@ -659,6 +670,7 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)
 	free_commit_buffer_slab(o->buffer_slab);
 	o->buffer_slab = NULL;
 
+	parsed_object_pool_reset_commit_grafts(o);
 	clear_alloc_state(o->blob_state);
 	clear_alloc_state(o->tree_state);
 	clear_alloc_state(o->commit_state);
diff --git a/object.h b/object.h
index 05691486ebf..17f32f1103e 100644
--- a/object.h
+++ b/object.h
@@ -7,6 +7,7 @@ struct buffer_slab;
 struct repository;
 
 struct parsed_object_pool {
+	struct repository *repo;
 	struct object **obj_hash;
 	int nr_objs, obj_hash_size;
 
@@ -31,8 +32,9 @@ struct parsed_object_pool {
 	struct buffer_slab *buffer_slab;
 };
 
-struct parsed_object_pool *parsed_object_pool_new(void);
+struct parsed_object_pool *parsed_object_pool_new(struct repository *repo);
 void parsed_object_pool_clear(struct parsed_object_pool *o);
+void parsed_object_pool_reset_commit_grafts(struct parsed_object_pool *o);
 
 struct object_list {
 	struct object *item;
diff --git a/repository.c b/repository.c
index 9825a308993..e6fc2c6aa9d 100644
--- a/repository.c
+++ b/repository.c
@@ -54,7 +54,7 @@ void initialize_repository(struct repository *repo)
 {
 	repo->objects = raw_object_store_new();
 	repo->remote_state = remote_state_new();
-	repo->parsed_objects = parsed_object_pool_new();
+	repo->parsed_objects = parsed_object_pool_new(repo);
 	ALLOC_ARRAY(repo->index, 1);
 	index_state_init(repo->index, repo);
 
diff --git a/shallow.c b/shallow.c
index b8cd051e3b6..a10cf9e9d5d 100644
--- a/shallow.c
+++ b/shallow.c
@@ -97,7 +97,7 @@ static void reset_repository_shallow(struct repository *r)
 {
 	r->parsed_objects->is_shallow = -1;
 	stat_validity_clear(r->parsed_objects->shallow_stat);
-	reset_commit_grafts(r);
+	parsed_object_pool_reset_commit_grafts(r->parsed_objects);
 }
 
 int commit_shallow_file(struct repository *r, struct shallow_lock *lk)
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 13/22] shallow: free grafts when unregistering them

[Patrick Steinhardt]

When removing a graft via `unregister_shallow()` we remove it from the
grafts array, but do not free the structure. Fix this to plug the leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 shallow.c                | 4 +++-
 t/t5537-fetch-shallow.sh | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/shallow.c b/shallow.c
index a10cf9e9d5d..7e0ee96ead9 100644
--- a/shallow.c
+++ b/shallow.c
@@ -51,10 +51,12 @@ int unregister_shallow(const struct object_id *oid)
 	int pos = commit_graft_pos(the_repository, oid);
 	if (pos < 0)
 		return -1;
-	if (pos + 1 < the_repository->parsed_objects->grafts_nr)
+	if (pos + 1 < the_repository->parsed_objects->grafts_nr) {
+		free(the_repository->parsed_objects->grafts[pos]);
 		MOVE_ARRAY(the_repository->parsed_objects->grafts + pos,
 			   the_repository->parsed_objects->grafts + pos + 1,
 			   the_repository->parsed_objects->grafts_nr - pos - 1);
+	}
 	the_repository->parsed_objects->grafts_nr--;
 	return 0;
 }
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 37f7547a4ca..cae4d400f32 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -5,6 +5,7 @@ test_description='fetch/clone from a shallow clone'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 commit() {
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 14/22] shallow: fix leaking members of `struct shallow_info`

[Patrick Steinhardt]

We do not free several struct members in `clear_shallow_info()`. Fix
this to plug the resulting leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 shallow.c               | 9 +++++++++
 t/t5538-push-shallow.sh | 1 +
 2 files changed, 10 insertions(+)

diff --git a/shallow.c b/shallow.c
index 7e0ee96ead9..dcebc263d70 100644
--- a/shallow.c
+++ b/shallow.c
@@ -489,6 +489,15 @@ void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa)
 
 void clear_shallow_info(struct shallow_info *info)
 {
+	if (info->used_shallow) {
+		for (size_t i = 0; i < info->shallow->nr; i++)
+			free(info->used_shallow[i]);
+		free(info->used_shallow);
+	}
+
+	free(info->need_reachability_test);
+	free(info->reachable);
+	free(info->shallow_ref);
 	free(info->ours);
 	free(info->theirs);
 }
diff --git a/t/t5538-push-shallow.sh b/t/t5538-push-shallow.sh
index e91fcc173e8..6adc3a20a45 100755
--- a/t/t5538-push-shallow.sh
+++ b/t/t5538-push-shallow.sh
@@ -5,6 +5,7 @@ test_description='push from/to a shallow clone'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 commit() {
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 15/22] negotiator/skipping: fix leaking commit entries

[Patrick Steinhardt]

When releasing the skipping negotiator we free its priority queue, but
not the contained entries. Fix this to plug a memory leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 negotiator/skipping.c                | 7 +++++--
 t/t5552-skipping-fetch-negotiator.sh | 2 ++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/negotiator/skipping.c b/negotiator/skipping.c
index f65d47858b4..b738fe4faef 100644
--- a/negotiator/skipping.c
+++ b/negotiator/skipping.c
@@ -247,8 +247,11 @@ static int ack(struct fetch_negotiator *n, struct commit *c)
 
 static void release(struct fetch_negotiator *n)
 {
-	clear_prio_queue(&((struct data *)n->data)->rev_list);
-	FREE_AND_NULL(n->data);
+	struct data *data = n->data;
+	for (int i = 0; i < data->rev_list.nr; i++)
+		free(data->rev_list.array[i].data);
+	clear_prio_queue(&data->rev_list);
+	FREE_AND_NULL(data);
 }
 
 void skipping_negotiator_init(struct fetch_negotiator *negotiator)
diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh
index b55a9f65e6b..4f2e5ae8dfa 100755
--- a/t/t5552-skipping-fetch-negotiator.sh
+++ b/t/t5552-skipping-fetch-negotiator.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test skipping fetch negotiator'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'fetch.negotiationalgorithm config' '
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 16/22] builtin/repack: fix leaking line buffer when packing promisors

[Patrick Steinhardt]

In `repack_promisor_objects()` we read output from git-pack-objects(1)
line by line, using `strbuf_getline_lf()`. We never free the line
buffer, causing a memory leak. Plug it.

This leak is being hit in t5616, but plugging it alone is not
sufficient to make the whole test suite leak free.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/repack.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/repack.c b/builtin/repack.c
index 8bb875532b4..a382754feee 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -425,9 +425,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
 
 		free(promisor_name);
 	}
+
 	fclose(out);
 	if (finish_command(&cmd))
 		die(_("could not finish pack-objects to repack promisor objects"));
+	strbuf_release(&line);
 }
 
 struct pack_geometry {
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 17/22] builtin/pack-objects: plug leaking list of keep-packs

[Patrick Steinhardt]

The `--keep-pack` option of git-pack-objects(1) populates the arguments
into a string list. And while the list is marked as `NODUP` and thus
won't duplicate the strings, the list entries themselves still need to
be free'd. We don't though, causing a leak.

Plug it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/pack-objects.c   | 1 +
 t/t5616-partial-clone.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c481feadbfa..ab78d72e273 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4641,6 +4641,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 cleanup:
 	clear_packing_data(&to_pack);
 	list_objects_filter_release(&filter_options);
+	string_list_clear(&keep_pack_list, 0);
 	strvec_clear(&rp);
 
 	return 0;
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 2da7291e379..467c46dccf6 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -5,6 +5,7 @@ test_description='git partial clone'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # create a normal "src" repo where we can later create new commits.
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 18/22] builtin/grep: fix leaking object context

[Patrick Steinhardt]

Even when `get_oid_with_context()` fails it may have allocated some data
in the object context. But we do not release it in git-grep(1) when the
call fails, leading to a memory leak. Plug it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/grep.c                 | 1 +
 t/t6132-pathspec-exclude.sh    | 1 +
 t/t6135-pathspec-with-attrs.sh | 2 ++
 3 files changed, 4 insertions(+)

diff --git a/builtin/grep.c b/builtin/grep.c
index dfc3c3e8bd2..dda4582d646 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1133,6 +1133,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 					 &oid, &oc)) {
 			if (seen_dashdash)
 				die(_("unable to resolve revision: %s"), arg);
+			object_context_release(&oc);
 			break;
 		}
 
diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh
index 9fdafeb1e90..f31c09c056f 100755
--- a/t/t6132-pathspec-exclude.sh
+++ b/t/t6132-pathspec-exclude.sh
@@ -2,6 +2,7 @@
 
 test_description='test case exclude pathspec'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index 120dcd74a51..794bc7daf05 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test labels in pathspecs'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup a tree' '
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 19/22] builtin/fmt-merge-msg: fix leaking buffers

[Patrick Steinhardt]

Fix leaking input and output buffers in git-fmt-merge-msg(1).

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fmt-merge-msg.c  | 2 ++
 t/t6200-fmt-merge-msg.sh | 1 +
 2 files changed, 3 insertions(+)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 957786d1b3a..0b162f8fab1 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -67,6 +67,8 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)
 		return ret;
 	write_in_full(STDOUT_FILENO, output.buf, output.len);
 
+	strbuf_release(&input);
+	strbuf_release(&output);
 	free(inpath);
 	return 0;
 }
diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index 5a221f8ef1f..ac57b0e4ae3 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -8,6 +8,7 @@ test_description='fmt-merge-msg test'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY/lib-gpg.sh"
 
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 20/22] match-trees: fix leaking prefixes in `shift_tree()`

[Patrick Steinhardt]

In `shift_tree()` we allocate two empty strings that we end up
passing to `match_trees()`. If that function finds a better match it
will update these pointers to point to a newly allocated strings,
freeing the old strings. We never free the final results though, neither
the ones we have allocated ourselves, nor the one that `match_trees()`
might've returned to us.

Fix the resulting memory leaks by creating a common exit path where we
free them.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 match-trees.c            | 10 +++++++---
 t/t6409-merge-subtree.sh |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/match-trees.c b/match-trees.c
index f17c74d483f..147b03abf18 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -294,18 +294,22 @@ void shift_tree(struct repository *r,
 		unsigned short mode;
 
 		if (!*del_prefix)
-			return;
+			goto out;
 
 		if (get_tree_entry(r, hash2, del_prefix, shifted, &mode))
 			die("cannot find path %s in tree %s",
 			    del_prefix, oid_to_hex(hash2));
-		return;
+		goto out;
 	}
 
 	if (!*add_prefix)
-		return;
+		goto out;
 
 	splice_tree(hash1, add_prefix, hash2, shifted);
+
+out:
+	free(add_prefix);
+	free(del_prefix);
 }
 
 /*
diff --git a/t/t6409-merge-subtree.sh b/t/t6409-merge-subtree.sh
index e9ba6f1690d..528615b981f 100755
--- a/t/t6409-merge-subtree.sh
+++ b/t/t6409-merge-subtree.sh
@@ -5,6 +5,7 @@ test_description='subtree merge strategy'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 21/22] merge-ort: fix two leaks when handling directory rename modifications

[Patrick Steinhardt]

There are two leaks in `apply_directory_rename_modifications()`:

  - We do not release the `dirs_to_insert` string list.

  - We do not release some `conflict_info` we put into the
    `opt->priv->paths` string map.

The former is trivial to fix. The latter is a bit less straight forward:
the `util` pointer of the string map may sometimes point to data that
has been allocated via `CALLOC()`, while at other times it may point to
data that has been allocated via a `mem_pool`.

It very much seems like an oversight that we didn't also allocate the
conflict info in this code path via the memory pool, though. So let's
fix that, which will also plug the memory leak for us.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 merge-ort.c                         | 4 +++-
 t/t6423-merge-rename-directories.sh | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/merge-ort.c b/merge-ort.c
index 3752c7e595d..0ed3cd06b1a 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2710,7 +2710,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
 		struct conflict_info *dir_ci;
 		char *cur_dir = dirs_to_insert.items[i].string;
 
-		CALLOC_ARRAY(dir_ci, 1);
+		dir_ci = mem_pool_calloc(&opt->priv->pool, 1, sizeof(*dir_ci));
 
 		dir_ci->merged.directory_name = parent_name;
 		len = strlen(parent_name);
@@ -2838,6 +2838,8 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
 	 * Finally, record the new location.
 	 */
 	pair->two->path = new_path;
+
+	string_list_clear(&dirs_to_insert, 0);
 }
 
 /*** Function Grouping: functions related to regular rename detection ***/
diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
index 88d1cf2cde9..4aaaf38be68 100755
--- a/t/t6423-merge-rename-directories.sh
+++ b/t/t6423-merge-rename-directories.sh
@@ -25,6 +25,7 @@ test_description="recursive merge with directory renames"
 #                     underscore notation is to differentiate different
 #                     files that might be renamed into each other's paths.)
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-merge.sh
 
-- 
2.46.0.519.g2e7b89e038.dirty

[PATCH v2 22/22] builtin/repack: fix leaking keep-pack list

[Patrick Steinhardt]

The list of packs to keep is populated via a command line option but
never free'd. Plug this memory leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/repack.c            | 1 +
 t/t6500-gc.sh               | 1 +
 t/t7703-repack-geometric.sh | 1 +
 3 files changed, 3 insertions(+)

diff --git a/builtin/repack.c b/builtin/repack.c
index a382754feee..3ee8cfa732f 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -1543,6 +1543,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	}
 
 cleanup:
+	string_list_clear(&keep_pack_list, 0);
 	string_list_clear(&names, 1);
 	existing_packs_release(&existing);
 	free_pack_geometry(&geometry);
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 1b5909d1b70..58654b3437e 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -3,6 +3,7 @@
 test_description='basic git gc tests
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh
index 9fc1626fbfd..8877aea98ba 100755
--- a/t/t7703-repack-geometric.sh
+++ b/t/t7703-repack-geometric.sh
@@ -2,6 +2,7 @@
 
 test_description='git repack --geometric works correctly'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 GIT_TEST_MULTI_PACK_INDEX=0
-- 
2.46.0.519.g2e7b89e038.dirty

Re: [PATCH v2 00/22] Memory leak fixes (pt.6)

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> There are only some smallish changes compared to v1:
>
>   - Explain leak checking a bit more carefully and document the new
>     `GIT_TEST_PASSING_SANITIZE_LEAK=check-failing` value in t/README.
>
>   - Some more trivial commit message improvements.

These changes looked trivially good ;-)  Shall we mark the topic for
'next' soonish, or are there others who want to comment?

Thanks.

Re: [PATCH v2 00/22] Memory leak fixes (pt.6)

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Hi,
>
> this is the second version of this round of memory leak fixes.
>
> There are only some smallish changes compared to v1:
>
>   - Explain leak checking a bit more carefully and document the new
>     `GIT_TEST_PASSING_SANITIZE_LEAK=check-failing` value in t/README.
>
>   - Some more trivial commit message improvements.
>
> Thanks!

Looking good.  Let me mark the topic for 'next' soonish.

Thanks.