[PATCH 00/21] Memory leak fixes (pt.9)

[PATCH 00/21] Memory leak fixes (pt.9)

[Patrick Steinhardt]

Hi,

here's another round of memory leak fixes. With this series we're down
to 10 test suites which are failing with the leak sanitizer. There are
two patch series in flight [1][2] that fix three more test suites, so in
total we're down to 7 test suites that we'll still have to fix up. So:
we're almost done!

Patrick

[1]: <20240826083052.1542228-1-toon@iotcl.com>
[2]: <cover.1726556195.git.ps@pks.im>

Patrick Steinhardt (21):
  builtin/ls-remote: plug leaking server options
  t/helper: fix leaks in "reach" test tool
  grep: fix leak in `grep_splice_or()`
  builtin/grep: fix leak with `--max-count=0`
  revision: fix leaking bloom filters
  diff-lib: fix leaking diffopts in `do_diff_cache()`
  pretty: clear signature check
  upload-pack: fix leaking URI protocols
  builtin/commit: fix leaking change data contents
  trailer: fix leaking trailer values
  builtin/commit: fix leaking cleanup config
  transport-helper: fix leaking import/export marks
  builtin/tag: fix leaking key ID on failure to sign
  combine-diff: fix leaking lost lines
  dir: release untracked cache data
  sparse-index: correctly free EWAH contents
  t/helper: stop re-initialization of `the_repository`
  t/helper: fix leaking buffer in "dump-untracked-cache"
  dir: fix leak when parsing "status.showUntrackedFiles"
  builtin/merge: release outbut buffer after performing merge
  list-objects-filter-options: work around reported leak on error

 builtin/commit.c                          | 26 +++++++++++++++++------
 builtin/grep.c                            | 13 +++++++++---
 builtin/ls-remote.c                       |  1 +
 builtin/merge.c                           |  1 +
 builtin/tag.c                             |  2 +-
 combine-diff.c                            |  2 +-
 diff-lib.c                                |  1 +
 dir.c                                     | 12 +++++++++--
 grep.c                                    |  1 +
 list-objects-filter-options.c             | 17 ++++++---------
 pretty.c                                  |  1 +
 revision.c                                |  5 +++++
 sparse-index.c                            |  7 ++++--
 t/helper/test-dump-untracked-cache.c      |  2 ++
 t/helper/test-reach.c                     | 10 +++++++++
 t/helper/test-read-cache.c                |  2 --
 t/t4038-diff-combined.sh                  |  1 +
 t/t4202-log.sh                            |  1 +
 t/t4216-log-bloom.sh                      |  1 +
 t/t5702-protocol-v2.sh                    |  1 +
 t/t5801-remote-helpers.sh                 |  1 +
 t/t6112-rev-list-filters-objects.sh       |  1 +
 t/t6424-merge-unrelated-index-changes.sh  |  1 +
 t/t6600-test-reach.sh                     |  1 +
 t/t7004-tag.sh                            |  1 +
 t/t7031-verify-tag-signed-ssh.sh          |  1 +
 t/t7063-status-untracked-cache.sh         |  1 +
 t/t7500-commit-template-squash-signoff.sh |  1 +
 t/t7502-commit-porcelain.sh               |  1 +
 t/t7510-signed-commit.sh                  |  1 +
 t/t7513-interpret-trailers.sh             |  1 +
 t/t7519-status-fsmonitor.sh               |  1 +
 t/t7528-signed-commit-ssh.sh              |  1 +
 t/t7610-mergetool.sh                      |  1 +
 t/t7810-grep.sh                           |  1 +
 trailer.c                                 | 18 +++++++++++-----
 transport-helper.c                        |  2 ++
 upload-pack.c                             |  1 +
 38 files changed, 111 insertions(+), 32 deletions(-)

-- 
2.47.0.dirty

[PATCH 01/21] builtin/ls-remote: plug leaking server options

[Patrick Steinhardt]

The server options populated via `OPT_STRING_LIST()` is never cleared,
causing a memory leak. Plug it.

This leak is exposed by t5702, but plugging it alone does not make the
whole test suite pass.

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

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index f723b3bf3bb..f333821b994 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -166,6 +166,7 @@ int cmd_ls_remote(int argc,
 		status = 0; /* we found something */
 	}
 
+	string_list_clear(&server_options, 0);
 	ref_sorting_release(sorting);
 	ref_array_clear(&ref_array);
 	if (transport_disconnect(transport))
-- 
2.47.0.dirty

[PATCH 02/21] t/helper: fix leaks in "reach" test tool

[Patrick Steinhardt]

The "reach" test tool doesn't bother to clean up any of its allocated
resources, causing various leaks. Plug them.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/helper/test-reach.c | 10 ++++++++++
 t/t6600-test-reach.sh |  1 +
 2 files changed, 11 insertions(+)

diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c
index 995e382863a..84deee604ad 100644
--- a/t/helper/test-reach.c
+++ b/t/helper/test-reach.c
@@ -127,10 +127,12 @@ int cmd__reach(int ac, const char **av)
 			exit(128);
 		printf("%s(A,X):\n", av[1]);
 		print_sorted_commit_ids(list);
+		free_commit_list(list);
 	} else if (!strcmp(av[1], "reduce_heads")) {
 		struct commit_list *list = reduce_heads(X);
 		printf("%s(X):\n", av[1]);
 		print_sorted_commit_ids(list);
+		free_commit_list(list);
 	} else if (!strcmp(av[1], "can_all_from_reach")) {
 		printf("%s(X,Y):%d\n", av[1], can_all_from_reach(X, Y, 1));
 	} else if (!strcmp(av[1], "can_all_from_reach_with_flag")) {
@@ -153,6 +155,7 @@ int cmd__reach(int ac, const char **av)
 			filter.with_commit_tag_algo = 0;
 
 		printf("%s(_,A,X,_):%d\n", av[1], commit_contains(&filter, A, X, &cache));
+		clear_contains_cache(&cache);
 	} else if (!strcmp(av[1], "get_reachable_subset")) {
 		const int reachable_flag = 1;
 		int i, count = 0;
@@ -176,7 +179,14 @@ int cmd__reach(int ac, const char **av)
 			die(_("too many commits marked reachable"));
 
 		print_sorted_commit_ids(list);
+		free_commit_list(list);
 	}
 
+	object_array_clear(&X_obj);
+	strbuf_release(&buf);
+	free_commit_list(X);
+	free_commit_list(Y);
+	free(X_array);
+	free(Y_array);
 	return 0;
 }
diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
index 2591f8b8b39..307deefed2c 100755
--- a/t/t6600-test-reach.sh
+++ b/t/t6600-test-reach.sh
@@ -2,6 +2,7 @@
 
 test_description='basic commit reachability tests'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Construct a grid-like commit graph with points (x,y)
-- 
2.47.0.dirty

[PATCH 03/21] grep: fix leak in `grep_splice_or()`

[Patrick Steinhardt]

In `grep_splice_or()` we search for the next `TRUE` node in our tree of
grep exrpessions and replace it with the given new expression. But we
don't free the old node, which causes a memory leak. Plug it.

This leak is exposed by t7810, but plugging it alone isn't sufficient to
make the test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grep.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/grep.c b/grep.c
index 701e58de04e..e9337f32cbf 100644
--- a/grep.c
+++ b/grep.c
@@ -756,6 +756,7 @@ static struct grep_expr *grep_splice_or(struct grep_expr *x, struct grep_expr *y
 		assert(x->node == GREP_NODE_OR);
 		if (x->u.binary.right &&
 		    x->u.binary.right->node == GREP_NODE_TRUE) {
+			free(x->u.binary.right);
 			x->u.binary.right = y;
 			break;
 		}
-- 
2.47.0.dirty

[PATCH 04/21] builtin/grep: fix leak with `--max-count=0`

[Patrick Steinhardt]

When executing with `--max-count=0` we'll return early from git-grep(1)
without performing any cleanup, which causes memory leaks. Plug these.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/grep.c  | 13 ++++++++++---
 t/t7810-grep.sh |  1 +
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index f17d46a06e4..98b85c7fcac 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -906,6 +906,7 @@ int cmd_grep(int argc,
 	int dummy;
 	int use_index = 1;
 	int allow_revs;
+	int ret;
 
 	struct option options[] = {
 		OPT_BOOL(0, "cached", &cached,
@@ -1172,8 +1173,10 @@ int cmd_grep(int argc,
 	 * Optimize out the case where the amount of matches is limited to zero.
 	 * We do this to keep results consistent with GNU grep(1).
 	 */
-	if (opt.max_count == 0)
-		return 1;
+	if (opt.max_count == 0) {
+		ret = 1;
+		goto out;
+	}
 
 	if (show_in_pager) {
 		if (num_threads > 1)
@@ -1267,10 +1270,14 @@ int cmd_grep(int argc,
 		hit |= wait_all();
 	if (hit && show_in_pager)
 		run_pager(&opt, prefix);
+
+	ret = !hit;
+
+out:
 	clear_pathspec(&pathspec);
 	string_list_clear(&path_list, 0);
 	free_grep_patterns(&opt);
 	object_array_clear(&list);
 	free_repos();
-	return !hit;
+	return ret;
 }
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index af2cf2f78ab..9e7681f0831 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -9,6 +9,7 @@ test_description='git grep various.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_invalid_grep_expression() {
-- 
2.47.0.dirty

[PATCH 05/21] revision: fix leaking bloom filters

[Patrick Steinhardt]

The memory allocated by `prepare_to_use_bloom_filter()` is not released
by `release_revisions()`, causing a memory leak. Plug it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 revision.c           | 5 +++++
 t/t4216-log-bloom.sh | 1 +
 2 files changed, 6 insertions(+)

diff --git a/revision.c b/revision.c
index f5f5b84f2b0..8df75b82249 100644
--- a/revision.c
+++ b/revision.c
@@ -3227,6 +3227,11 @@ void release_revisions(struct rev_info *revs)
 	clear_decoration(&revs->treesame, free);
 	line_log_free(revs);
 	oidset_clear(&revs->missing_commits);
+
+	for (int i = 0; i < revs->bloom_keys_nr; i++)
+		clear_bloom_key(&revs->bloom_keys[i]);
+	FREE_AND_NULL(revs->bloom_keys);
+	revs->bloom_keys_nr = 0;
 }
 
 static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)
diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index 3f163dc3969..8d22338f6aa 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -4,6 +4,7 @@ test_description='git log for a path with Bloom filters'
 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-chunk.sh
 
-- 
2.47.0.dirty

[PATCH 06/21] diff-lib: fix leaking diffopts in `do_diff_cache()`

[Patrick Steinhardt]

In `do_diff_cache()` we initialize a new `rev_info` and then overwrite
its `diffopt` with a user-provided set of options. This can leak memory
because `repo_init_revisions()` may end up allocating memory for the
`diffopt` itself depending on the configuration. And as that field is
overwritten we won't ever free that.

Plug the memory leak by releasing the diffopts before we overwrite them.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 diff-lib.c           | 1 +
 t/t7610-mergetool.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/diff-lib.c b/diff-lib.c
index 6b14b959629..3cf353946f5 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -661,6 +661,7 @@ int do_diff_cache(const struct object_id *tree_oid, struct diff_options *opt)
 
 	repo_init_revisions(opt->repo, &revs, NULL);
 	copy_pathspec(&revs.prune_data, &opt->pathspec);
+	diff_free(&revs.diffopt);
 	revs.diffopt = *opt;
 	revs.diffopt.no_free = 1;
 
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 22b3a85b3e9..5c5e79e9905 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -10,6 +10,7 @@ Testing basic merge tool invocation'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # All the mergetool test work by checking out a temporary branch based
-- 
2.47.0.dirty

Re: [PATCH 06/21] diff-lib: fix leaking diffopts in `do_diff_cache()`

[Kristoffer Haugsbakk]

On Fri, Oct 11, 2024, at 07:32, Patrick Steinhardt wrote:
> In `do_diff_cache()` we initialize a new `rev_info` and then overwrite
> its `diffopt` with a user-provided set of options. This can leak memory
> because `repo_init_revisions()` may end up allocating memory for the
> `diffopt` itself depending on the configuration. And as that field is

s/as that/since that/

“since” communicates causality better.

> overwritten we won't ever free that.

s/free that/free it/

-- 
Kristoffer Haugsbakk

[PATCH 07/21] pretty: clear signature check

[Patrick Steinhardt]

The signature check in of the formatting context is never getting
released. Fix this to plug the resulting memory leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 pretty.c                         | 1 +
 t/t4202-log.sh                   | 1 +
 t/t7031-verify-tag-signed-ssh.sh | 1 +
 t/t7510-signed-commit.sh         | 1 +
 t/t7528-signed-commit-ssh.sh     | 1 +
 5 files changed, 5 insertions(+)

diff --git a/pretty.c b/pretty.c
index 6403e268900..098378720a4 100644
--- a/pretty.c
+++ b/pretty.c
@@ -2032,6 +2032,7 @@ void repo_format_commit_message(struct repository *r,
 
 	free(context.commit_encoding);
 	repo_unuse_commit_buffer(r, commit, context.message);
+	signature_check_clear(&context.signature_check);
 }
 
 static void pp_header(struct pretty_print_context *pp,
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 51f7beb59f8..35bec4089a3 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -5,6 +5,7 @@ test_description='git log'
 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"
 . "$TEST_DIRECTORY/lib-terminal.sh"
diff --git a/t/t7031-verify-tag-signed-ssh.sh b/t/t7031-verify-tag-signed-ssh.sh
index 20913b37134..2ee62c07293 100755
--- a/t/t7031-verify-tag-signed-ssh.sh
+++ b/t/t7031-verify-tag-signed-ssh.sh
@@ -4,6 +4,7 @@ test_description='signed tag tests'
 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"
 
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 0d2dd29fe6a..eb229082e40 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -4,6 +4,7 @@ test_description='signed commit tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 GNUPGHOME_NOT_USED=$GNUPGHOME
 . "$TEST_DIRECTORY/lib-gpg.sh"
diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh
index 065f7806362..68e18856b66 100755
--- a/t/t7528-signed-commit-ssh.sh
+++ b/t/t7528-signed-commit-ssh.sh
@@ -4,6 +4,7 @@ test_description='ssh signed commit tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 GNUPGHOME_NOT_USED=$GNUPGHOME
 . "$TEST_DIRECTORY/lib-gpg.sh"
-- 
2.47.0.dirty

Re: [PATCH 07/21] pretty: clear signature check

[Toon Claes]

Patrick Steinhardt <ps@pks.im> writes:

> The signature check in of the formatting context is never getting
> released. Fix this to plug the resulting memory leak.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  pretty.c                         | 1 +
>  t/t4202-log.sh                   | 1 +
>  t/t7031-verify-tag-signed-ssh.sh | 1 +
>  t/t7510-signed-commit.sh         | 1 +
>  t/t7528-signed-commit-ssh.sh     | 1 +
>  5 files changed, 5 insertions(+)
>
> diff --git a/pretty.c b/pretty.c
> index 6403e268900..098378720a4 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -2032,6 +2032,7 @@ void repo_format_commit_message(struct repository *r,
>  
>  	free(context.commit_encoding);
>  	repo_unuse_commit_buffer(r, commit, context.message);
> +	signature_check_clear(&context.signature_check);

I was having a very hard time finding where this gets allocated, and to
be honest, I still don't know for sure. I think in
check_commit_signature() which is called by format_commit_one().
In "[PATCH 20/21] builtin/merge: release outbut buffer after performing
merge" you mention it's not obvious to the caller they need know about
memory they need to clean up, isn't that case here as well?

--
Toon

Re: [PATCH 07/21] pretty: clear signature check

[Patrick Steinhardt]

On Fri, Oct 18, 2024 at 02:02:48PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > The signature check in of the formatting context is never getting
> > released. Fix this to plug the resulting memory leak.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  pretty.c                         | 1 +
> >  t/t4202-log.sh                   | 1 +
> >  t/t7031-verify-tag-signed-ssh.sh | 1 +
> >  t/t7510-signed-commit.sh         | 1 +
> >  t/t7528-signed-commit-ssh.sh     | 1 +
> >  5 files changed, 5 insertions(+)
> >
> > diff --git a/pretty.c b/pretty.c
> > index 6403e268900..098378720a4 100644
> > --- a/pretty.c
> > +++ b/pretty.c
> > @@ -2032,6 +2032,7 @@ void repo_format_commit_message(struct repository *r,
> >  
> >  	free(context.commit_encoding);
> >  	repo_unuse_commit_buffer(r, commit, context.message);
> > +	signature_check_clear(&context.signature_check);
> 
> I was having a very hard time finding where this gets allocated, and to
> be honest, I still don't know for sure. I think in
> check_commit_signature() which is called by format_commit_one().
> In "[PATCH 20/21] builtin/merge: release outbut buffer after performing
> merge" you mention it's not obvious to the caller they need know about
> memory they need to clean up, isn't that case here as well?

Well, I think it's clearer in this context, but hidden by the fact that
we pass around a wrapper-structure. But that's not really the problem of
the `struct signature_check` subsystem, but rather of "pretty.c".

In any case, the callchain is:

  - `format_commit_one()`
  - `check_commit_signature()`
  - `check_signature()`
  - `verify_signed_buffer()`, which is a function pointer depending on
    whether we verify a GPG, x509 or SSH signature.

From my point of view, that callchain isn't all that important in this
context. All we need to know is that we allocate the signature check
struct on our stack, and as it may get populated we have to clean it up,
as well.

Patrick

[PATCH 08/21] upload-pack: fix leaking URI protocols

[Patrick Steinhardt]

We don't clear `struct upload_pack::uri_protocols`, which causes a
memory leak. Fix this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5702-protocol-v2.sh | 1 +
 upload-pack.c          | 1 +
 2 files changed, 2 insertions(+)

diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 1ef540f73d3..ed55a2f7f95 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -7,6 +7,7 @@ TEST_NO_CREATE_REPO=1
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Test protocol v2 with 'git://' transport
diff --git a/upload-pack.c b/upload-pack.c
index 6d6e0f9f980..b4a59c3518b 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -166,6 +166,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data)
 	object_array_clear(&data->extra_edge_obj);
 	list_objects_filter_release(&data->filter_options);
 	string_list_clear(&data->allowed_filters, 0);
+	string_list_clear(&data->uri_protocols, 0);
 
 	free((char *)data->pack_objects_hook);
 }
-- 
2.47.0.dirty

[PATCH 09/21] builtin/commit: fix leaking change data contents

[Patrick Steinhardt]

While we free the worktree change data, we never free its contents. Fix
this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/commit.c                          | 9 ++++++++-
 t/t7500-commit-template-squash-signoff.sh | 1 +
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 8db4e9df0c9..18a55bd1b91 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -728,6 +728,13 @@ static void prepare_amend_commit(struct commit *commit, struct strbuf *sb,
 	repo_unuse_commit_buffer(the_repository, commit, buffer);
 }
 
+static void change_data_free(void *util, const char *str UNUSED)
+{
+	struct wt_status_change_data *d = util;
+	free(d->rename_source);
+	free(d);
+}
+
 static int prepare_to_commit(const char *index_file, const char *prefix,
 			     struct commit *current_head,
 			     struct wt_status *s,
@@ -991,7 +998,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		s->use_color = 0;
 		committable = run_status(s->fp, index_file, prefix, 1, s);
 		s->use_color = saved_color_setting;
-		string_list_clear(&s->change, 1);
+		string_list_clear_func(&s->change, change_data_free);
 	} else {
 		struct object_id oid;
 		const char *parent = "HEAD";
diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 4dca8d97a77..379d3ed3413 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -7,6 +7,7 @@ test_description='git commit
 
 Tests for template, signoff, squash and -F functions.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-rebase.sh
-- 
2.47.0.dirty

[PATCH 10/21] trailer: fix leaking trailer values

[Patrick Steinhardt]

Fix leaking trailer values when replacing the value with a command or
when the token value is empty.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t7513-interpret-trailers.sh |  1 +
 trailer.c                     | 18 +++++++++++++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 0f7d8938d98..38d6ccaa001 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -5,6 +5,7 @@
 
 test_description='git interpret-trailers'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # When we want one trailing space at the end of each line, let's use sed
diff --git a/trailer.c b/trailer.c
index 682d74505bf..5c0bfb735a9 100644
--- a/trailer.c
+++ b/trailer.c
@@ -249,17 +249,23 @@ static char *apply_command(struct conf_info *conf, const char *arg)
 static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok)
 {
 	if (arg_tok->conf.command || arg_tok->conf.cmd) {
-		const char *arg;
+		char *value_to_free = NULL;
+		char *arg;
+
 		if (arg_tok->value && arg_tok->value[0]) {
-			arg = arg_tok->value;
+			arg = (char *)arg_tok->value;
 		} else {
 			if (in_tok && in_tok->value)
 				arg = xstrdup(in_tok->value);
 			else
 				arg = xstrdup("");
+			value_to_free = arg_tok->value;
 		}
+
 		arg_tok->value = apply_command(&arg_tok->conf, arg);
-		free((char *)arg);
+
+		free(value_to_free);
+		free(arg);
 	}
 }
 
@@ -1114,6 +1120,7 @@ void format_trailers(const struct process_trailer_options *opts,
 		if (item->token) {
 			struct strbuf tok = STRBUF_INIT;
 			struct strbuf val = STRBUF_INIT;
+
 			strbuf_addstr(&tok, item->token);
 			strbuf_addstr(&val, item->value);
 
@@ -1124,7 +1131,7 @@ void format_trailers(const struct process_trailer_options *opts,
 			 * corresponding value).
 			 */
 			if (opts->trim_empty && !strlen(item->value))
-				continue;
+				goto next;
 
 			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
 				if (opts->separator && out->len != origlen)
@@ -1145,9 +1152,10 @@ void format_trailers(const struct process_trailer_options *opts,
 				if (!opts->separator)
 					strbuf_addch(out, '\n');
 			}
+
+next:
 			strbuf_release(&tok);
 			strbuf_release(&val);
-
 		} else if (!opts->only_trailers) {
 			if (opts->separator && out->len != origlen) {
 				strbuf_addbuf(out, opts->separator);
-- 
2.47.0.dirty

Re: [PATCH 10/21] trailer: fix leaking trailer values

[Toon Claes]

Patrick Steinhardt <ps@pks.im> writes:

> Fix leaking trailer values when replacing the value with a command or
> when the token value is empty.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>
> diff --git a/trailer.c b/trailer.c
> index 682d74505bf..5c0bfb735a9 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1124,7 +1131,7 @@ void format_trailers(const struct process_trailer_options *opts,
>  			 * corresponding value).
>  			 */
>  			if (opts->trim_empty && !strlen(item->value))
> -				continue;
> +				goto next;

While this is technically correct, I found it rather hard to understand
what's happening. What do you think about instead turning the `if` below
in an `else if` ?

>  
>  			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
>  				if (opts->separator && out->len != origlen)
> @@ -1145,9 +1152,10 @@ void format_trailers(const struct process_trailer_options *opts,
>  				if (!opts->separator)
>  					strbuf_addch(out, '\n');
>  			}
> +
> +next:
>  			strbuf_release(&tok);
>  			strbuf_release(&val);
> -
>  		} else if (!opts->only_trailers) {
>  			if (opts->separator && out->len != origlen) {
>  				strbuf_addbuf(out, opts->separator);
> -- 
> 2.47.0.dirty

--
Toon

Re: [PATCH 10/21] trailer: fix leaking trailer values

[Patrick Steinhardt]

On Fri, Oct 18, 2024 at 02:03:26PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Fix leaking trailer values when replacing the value with a command or
> > when the token value is empty.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >
> > diff --git a/trailer.c b/trailer.c
> > index 682d74505bf..5c0bfb735a9 100644
> > --- a/trailer.c
> > +++ b/trailer.c
> > @@ -1124,7 +1131,7 @@ void format_trailers(const struct process_trailer_options *opts,
> >  			 * corresponding value).
> >  			 */
> >  			if (opts->trim_empty && !strlen(item->value))
> > -				continue;
> > +				goto next;
> 
> While this is technically correct, I found it rather hard to understand
> what's happening. What do you think about instead turning the `if` below
> in an `else if` ?

An even better idea is to lift the buffers outside of the loop. Like this
we don't have to reallocate them on every iteration and can easily
release them once at the end of the function.

Patrick

[PATCH 11/21] builtin/commit: fix leaking cleanup config

[Patrick Steinhardt]

The cleanup string set by the config is leaking when it is being
overridden by an option. Fix this by tracking these via two separate
variables such that we can free the old value.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/commit.c            | 17 ++++++++++++-----
 t/t7502-commit-porcelain.sh |  1 +
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 18a55bd1b91..71d674138c9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -135,7 +135,7 @@ static struct strvec trailer_args = STRVEC_INIT;
  * is specified explicitly.
  */
 static enum commit_msg_cleanup_mode cleanup_mode;
-static char *cleanup_arg;
+static char *cleanup_config;
 
 static enum commit_whence whence;
 static int use_editor = 1, include_status = 1;
@@ -1387,8 +1387,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (0 <= edit_flag)
 		use_editor = edit_flag;
 
-	cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);
-
 	handle_untracked_files_arg(s);
 
 	if (all && argc > 0)
@@ -1636,8 +1634,10 @@ static int git_commit_config(const char *k, const char *v,
 		include_status = git_config_bool(k, v);
 		return 0;
 	}
-	if (!strcmp(k, "commit.cleanup"))
-		return git_config_string(&cleanup_arg, k, v);
+	if (!strcmp(k, "commit.cleanup")) {
+		FREE_AND_NULL(cleanup_config);
+		return git_config_string(&cleanup_config, k, v);
+	}
 	if (!strcmp(k, "commit.gpgsign")) {
 		sign_commit = git_config_bool(k, v) ? "" : NULL;
 		return 0;
@@ -1658,6 +1658,7 @@ int cmd_commit(int argc,
 	       struct repository *repo UNUSED)
 {
 	static struct wt_status s;
+	static const char *cleanup_arg = NULL;
 	static struct option builtin_commit_options[] = {
 		OPT__QUIET(&quiet, N_("suppress summary after successful commit")),
 		OPT__VERBOSE(&verbose, N_("show diff in commit message template")),
@@ -1757,6 +1758,12 @@ int cmd_commit(int argc,
 	if (verbose == -1)
 		verbose = (config_commit_verbose < 0) ? 0 : config_commit_verbose;
 
+	if (cleanup_arg) {
+		free(cleanup_config);
+		cleanup_config = xstrdup(cleanup_arg);
+	}
+	cleanup_mode = get_cleanup_mode(cleanup_config, use_editor);
+
 	if (dry_run)
 		return dry_run_commit(argv, prefix, current_head, &s);
 	index_file = prepare_index(argv, prefix, current_head, 0);
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index b37e2018a74..84f1ff52b67 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -5,6 +5,7 @@ test_description='git commit porcelain-ish'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 commit_msg_is () {
-- 
2.47.0.dirty

[PATCH 12/21] transport-helper: fix leaking import/export marks

[Patrick Steinhardt]

Fix leaking import and export marks for transport helpers.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5801-remote-helpers.sh | 1 +
 transport-helper.c        | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index d21877150ed..d4882288a30 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -8,6 +8,7 @@ test_description='Test remote-helper import and export commands'
 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
 
diff --git a/transport-helper.c b/transport-helper.c
index 013ec79dc9c..bc27653cdee 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -399,6 +399,8 @@ static int release_helper(struct transport *transport)
 	int res = 0;
 	struct helper_data *data = transport->data;
 	refspec_clear(&data->rs);
+	free(data->import_marks);
+	free(data->export_marks);
 	res = disconnect_helper(transport);
 	free(transport->data);
 	return res;
-- 
2.47.0.dirty

[PATCH 13/21] builtin/tag: fix leaking key ID on failure to sign

[Patrick Steinhardt]

We do not free the key ID when signing a tag fails. Do so by using
the common exit path.

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

diff --git a/builtin/tag.c b/builtin/tag.c
index 93d10d59157..c37c0a68fda 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -164,7 +164,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid,
 	int ret = -1;
 
 	if (sign_buffer(buffer, &sig, keyid))
-		return -1;
+		goto out;
 
 	if (compat) {
 		const struct git_hash_algo *algo = the_repository->hash_algo;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b1316e62f46..42b3327e69b 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -10,6 +10,7 @@ Tests for operations with tags.'
 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
 . "$TEST_DIRECTORY"/lib-terminal.sh
-- 
2.47.0.dirty

[PATCH 14/21] combine-diff: fix leaking lost lines

[Patrick Steinhardt]

The `cnt` variable tracks the number of lines in a patch diff. It can
happen though that there are no newlines, in which case we'd still end
up allocating our array of `sline`s. In fact, we always allocate it with
`cnt + 2` entries. But when we loop through the array to clear it at the
end of this function we only loop until `lno < cnt`, and thus we may not
end up releasing whatever the two extra `sline`s contain.

Plug this memory leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 combine-diff.c           | 2 +-
 t/t4038-diff-combined.sh | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/combine-diff.c b/combine-diff.c
index f6b624dc288..3c6d9507fec 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1220,7 +1220,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	}
 	free(result);
 
-	for (lno = 0; lno < cnt; lno++) {
+	for (lno = 0; lno < cnt + 2; lno++) {
 		if (sline[lno].lost) {
 			struct lline *ll = sline[lno].lost;
 			while (ll) {
diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
index 2ce26e585c9..00190802d83 100755
--- a/t/t4038-diff-combined.sh
+++ b/t/t4038-diff-combined.sh
@@ -5,6 +5,7 @@ test_description='combined diff'
 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-diff.sh
 
-- 
2.47.0.dirty

[PATCH 15/21] dir: release untracked cache data

[Patrick Steinhardt]

There are several cases where we invalidate untracked cache directory
entries where we do not free the underlying data, but reset the number
of entries. This causes us to leak memory because `free_untracked()`
will not iterate over any potential entries which we still had in the
array.

Fix this issue by freeing old entries. The leak is exposed by t7519, but
plugging it alone does not make the whole test suite pass.

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

diff --git a/dir.c b/dir.c
index e3ddd5b5296..cb9782fa11f 100644
--- a/dir.c
+++ b/dir.c
@@ -1056,6 +1056,8 @@ static void do_invalidate_gitignore(struct untracked_cache_dir *dir)
 {
 	int i;
 	dir->valid = 0;
+	for (size_t i = 0; i < dir->untracked_nr; i++)
+		free(dir->untracked[i]);
 	dir->untracked_nr = 0;
 	for (i = 0; i < dir->dirs_nr; i++)
 		do_invalidate_gitignore(dir->dirs[i]);
@@ -1083,6 +1085,8 @@ static void invalidate_directory(struct untracked_cache *uc,
 		uc->dir_invalidated++;
 
 	dir->valid = 0;
+	for (size_t i = 0; i < dir->untracked_nr; i++)
+		free(dir->untracked[i]);
 	dir->untracked_nr = 0;
 	for (i = 0; i < dir->dirs_nr; i++)
 		dir->dirs[i]->recurse = 0;
@@ -3573,6 +3577,8 @@ static void write_one_dir(struct untracked_cache_dir *untracked,
 	 * for safety..
 	 */
 	if (!untracked->valid) {
+		for (size_t i = 0; i < untracked->untracked_nr; i++)
+			free(untracked->untracked[i]);
 		untracked->untracked_nr = 0;
 		untracked->check_only = 0;
 	}
@@ -3905,6 +3911,8 @@ static void invalidate_one_directory(struct untracked_cache *uc,
 {
 	uc->dir_invalidated++;
 	ucd->valid = 0;
+	for (size_t i = 0; i < ucd->untracked_nr; i++)
+		free(ucd->untracked[i]);
 	ucd->untracked_nr = 0;
 }
 
-- 
2.47.0.dirty

[PATCH 16/21] sparse-index: correctly free EWAH contents

[Patrick Steinhardt]

While we free the `fsmonitor_dirty` member of `struct index_state`, we
do not free the contents of that EWAH. Do so by using `ewah_free()`
instead of `FREE_AND_NULL()`.

This leak is exposed by t7519, but plugging it alone does not make the
test suite pass.

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

diff --git a/sparse-index.c b/sparse-index.c
index 3d7f2164e25..2107840bfc5 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -2,6 +2,7 @@
 
 #include "git-compat-util.h"
 #include "environment.h"
+#include "ewah/ewok.h"
 #include "gettext.h"
 #include "name-hash.h"
 #include "read-cache-ll.h"
@@ -242,7 +243,8 @@ int convert_to_sparse(struct index_state *istate, int flags)
 	cache_tree_update(istate, 0);
 
 	istate->fsmonitor_has_run_once = 0;
-	FREE_AND_NULL(istate->fsmonitor_dirty);
+	ewah_free(istate->fsmonitor_dirty);
+	istate->fsmonitor_dirty = NULL;
 	FREE_AND_NULL(istate->fsmonitor_last_update);
 
 	istate->sparse_index = INDEX_COLLAPSED;
@@ -438,7 +440,8 @@ void expand_index(struct index_state *istate, struct pattern_list *pl)
 	istate->cache_nr = full->cache_nr;
 	istate->cache_alloc = full->cache_alloc;
 	istate->fsmonitor_has_run_once = 0;
-	FREE_AND_NULL(istate->fsmonitor_dirty);
+	ewah_free(istate->fsmonitor_dirty);
+	istate->fsmonitor_dirty = NULL;
 	FREE_AND_NULL(istate->fsmonitor_last_update);
 
 	strbuf_release(&base);
-- 
2.47.0.dirty

[PATCH 17/21] t/helper: stop re-initialization of `the_repository`

[Patrick Steinhardt]

While "common-main.c" already initializes `the_repository` for us, we do
so a second time in the "read-cache" test helper. This causes a memory
leak because the old repository's contents isn't released.

Stop calling `initialize_repository()` to plug this leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/helper/test-read-cache.c  | 2 --
 t/t7519-status-fsmonitor.sh | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
index d285c656bd3..e277dde8e71 100644
--- a/t/helper/test-read-cache.c
+++ b/t/helper/test-read-cache.c
@@ -11,8 +11,6 @@ int cmd__read_cache(int argc, const char **argv)
 	int i, cnt = 1;
 	const char *name = NULL;
 
-	initialize_repository(the_repository);
-
 	if (argc > 1 && skip_prefix(argv[1], "--print-and-refresh=", &name)) {
 		argc--;
 		argv++;
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 7ee69ecdd4a..0f88a58a819 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -2,6 +2,7 @@
 
 test_description='git status with file system watcher'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Note, after "git reset --hard HEAD" no extensions exist other than 'TREE'
-- 
2.47.0.dirty

[PATCH 18/21] t/helper: fix leaking buffer in "dump-untracked-cache"

[Patrick Steinhardt]

We never release the local `struct strbuf base` buffer, thus leaking
memory. Fix this leak.

This leak is exposed by t7063, but plugging it alone does not make the
whole test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/helper/test-dump-untracked-cache.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c
index 4f010d53249..b2e70837a90 100644
--- a/t/helper/test-dump-untracked-cache.c
+++ b/t/helper/test-dump-untracked-cache.c
@@ -68,5 +68,7 @@ int cmd__dump_untracked_cache(int ac UNUSED, const char **av UNUSED)
 	printf("flags %08x\n", uc->dir_flags);
 	if (uc->root)
 		dump(uc->root, &base);
+
+	strbuf_release(&base);
 	return 0;
 }
-- 
2.47.0.dirty

[PATCH 19/21] dir: fix leak when parsing "status.showUntrackedFiles"

[Patrick Steinhardt]

We use `repo_config_get_string()` to read "status.showUntrackedFiles"
from the config subsystem. This function allocates the result, but we
never free the result after parsing it.

The value never leaves the scope of the calling function, so refactor it
to instead use `repo_config_get_string_tmp()`, which does not hand over
ownership to the caller.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 dir.c                             | 4 ++--
 t/t7063-status-untracked-cache.sh | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index cb9782fa11f..7f35a3e3175 100644
--- a/dir.c
+++ b/dir.c
@@ -2872,14 +2872,14 @@ static void set_untracked_ident(struct untracked_cache *uc)
 static unsigned new_untracked_cache_flags(struct index_state *istate)
 {
 	struct repository *repo = istate->repo;
-	char *val;
+	const char *val;
 
 	/*
 	 * This logic is coordinated with the setting of these flags in
 	 * wt-status.c#wt_status_collect_untracked(), and the evaluation
 	 * of the config setting in commit.c#git_status_config()
 	 */
-	if (!repo_config_get_string(repo, "status.showuntrackedfiles", &val) &&
+	if (!repo_config_get_string_tmp(repo, "status.showuntrackedfiles", &val) &&
 	    !strcmp(val, "all"))
 		return 0;
 
diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index 8929ef481f9..13fea7931cd 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -5,6 +5,7 @@ test_description='test untracked cache'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # On some filesystems (e.g. FreeBSD's ext2 and ufs) directory mtime
-- 
2.47.0.dirty

[PATCH 20/21] builtin/merge: release outbut buffer after performing merge

[Patrick Steinhardt]

The `obuf` member of `struct merge_options` is used to buffer output in
some cases. In order to not discard its allocated memory we only release
its contents in `merge_finalize()` when we're not currently recursing
into a subtree.

This results in some situations where we seemingly do not release the
buffer reliably. We thus have calls to `strbuf_release()` for this
buffer scattered across the codebase. But we're missing one callsite in
git-merge(1), which causes a memory leak.

We should ideally refactor this interface so that callers don't have to
know about any such internals. But for now, paper over the issue by
adding one more `strbuf_release()` call.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/merge.c                          | 1 +
 t/t6424-merge-unrelated-index-changes.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index 84d0f3604bc..51038eaca84 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -754,6 +754,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 			clean = merge_recursive(&o, head, remoteheads->item,
 						reversed, &result);
 		free_commit_list(reversed);
+		strbuf_release(&o.obuf);
 
 		if (clean < 0) {
 			rollback_lock_file(&lock);
diff --git a/t/t6424-merge-unrelated-index-changes.sh b/t/t6424-merge-unrelated-index-changes.sh
index 7677c5f08d0..a7ea8acb845 100755
--- a/t/t6424-merge-unrelated-index-changes.sh
+++ b/t/t6424-merge-unrelated-index-changes.sh
@@ -2,6 +2,7 @@
 
 test_description="merges with unrelated index changes"
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Testcase for some simple merges
-- 
2.47.0.dirty

Re: [PATCH 20/21] builtin/merge: release outbut buffer after performing merge

[Toon Claes]

Patrick Steinhardt <ps@pks.im> writes:

> The `obuf` member of `struct merge_options` is used to buffer output in
> some cases. In order to not discard its allocated memory we only release
> its contents in `merge_finalize()` when we're not currently recursing
> into a subtree.
>
> This results in some situations where we seemingly do not release the
> buffer reliably. We thus have calls to `strbuf_release()` for this
> buffer scattered across the codebase. But we're missing one callsite in
> git-merge(1), which causes a memory leak.
>
> We should ideally refactor this interface so that callers don't have to
> know about any such internals. But for now, paper over the issue by
> adding one more `strbuf_release()` call.

Shall we mark this as #leftoverbits?

--
Toon

Re: [PATCH 20/21] builtin/merge: release outbut buffer after performing merge

[Patrick Steinhardt]

On Fri, Oct 18, 2024 at 02:03:48PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > The `obuf` member of `struct merge_options` is used to buffer output in
> > some cases. In order to not discard its allocated memory we only release
> > its contents in `merge_finalize()` when we're not currently recursing
> > into a subtree.
> >
> > This results in some situations where we seemingly do not release the
> > buffer reliably. We thus have calls to `strbuf_release()` for this
> > buffer scattered across the codebase. But we're missing one callsite in
> > git-merge(1), which causes a memory leak.
> >
> > We should ideally refactor this interface so that callers don't have to
> > know about any such internals. But for now, paper over the issue by
> > adding one more `strbuf_release()` call.
> 
> Shall we mark this as #leftoverbits?

I guess it is now marked as such :)

Patrick

[PATCH 21/21] list-objects-filter-options: work around reported leak on error

[Patrick Steinhardt]

This one is a little bit more curious. In t6112, we have a test that
exercises the `git rev-list --filter` option with invalid filters. We
execute git-rev-list(1) via `test_must_fail`, which means that we check
for leaks even though Git exits with an error code. This causes the
following leak:

    Direct leak of 27 byte(s) in 1 object(s) allocated from:
        #0 0x5555555e6946 in realloc.part.0 lsan_interceptors.cpp.o
        #1 0x5555558fb4b6 in xrealloc wrapper.c:137:8
        #2 0x5555558b6e06 in strbuf_grow strbuf.c:112:2
        #3 0x5555558b7550 in strbuf_add strbuf.c:311:2
        #4 0x5555557c1a88 in strbuf_addstr strbuf.h:310:2
        #5 0x5555557c1d4c in parse_list_objects_filter list-objects-filter-options.c:261:3
        #6 0x555555885ead in handle_revision_pseudo_opt revision.c:2899:3
        #7 0x555555884e20 in setup_revisions revision.c:3014:11
        #8 0x5555556c4b42 in cmd_rev_list builtin/rev-list.c:588:9
        #9 0x5555555ec5e3 in run_builtin git.c:483:11
        #10 0x5555555eb1e4 in handle_builtin git.c:749:13
        #11 0x5555555ec001 in run_argv git.c:819:4
        #12 0x5555555eaf94 in cmd_main git.c:954:19
        #13 0x5555556fd569 in main common-main.c:64:11
        #14 0x7ffff7ca714d in __libc_start_call_main (.../lib/libc.so.6+0x2a14d)
        #15 0x7ffff7ca7208 in __libc_start_main@GLIBC_2.2.5 (.../libc.so.6+0x2a208)
        #16 0x5555555ad064 in _start (git+0x59064)

This leak is valid, as we call `die()` and do not clean up the memory at
all. But what's curious is that this is the only leak reported, because
we don't clean up any other allocated memory, either, and I have no idea
why the leak sanitizer treats this buffer specially.

In any case, we can work around the leak by shuffling things around a
bit. Instead of calling `gently_parse_list_objects_filter()` and dying
after we have modified the filter spec, we simply do so beforehand. Like
this we don't allocate the buffer in the error case, which makes the
reported leak go away.

It's not pretty, but it manages to make t6112 leak free.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 list-objects-filter-options.c       | 17 +++++++----------
 t/t6112-rev-list-filters-objects.sh |  1 +
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 00611107d20..fa72e81e4ad 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -252,16 +252,14 @@ void parse_list_objects_filter(
 	const char *arg)
 {
 	struct strbuf errbuf = STRBUF_INIT;
-	int parse_error;
 
 	if (!filter_options->filter_spec.buf)
 		BUG("filter_options not properly initialized");
 
 	if (!filter_options->choice) {
+		if (gently_parse_list_objects_filter(filter_options, arg, &errbuf))
+			die("%s", errbuf.buf);
 		strbuf_addstr(&filter_options->filter_spec, arg);
-
-		parse_error = gently_parse_list_objects_filter(
-			filter_options, arg, &errbuf);
 	} else {
 		struct list_objects_filter_options *sub;
 
@@ -271,18 +269,17 @@ void parse_list_objects_filter(
 		 */
 		transform_to_combine_type(filter_options);
 
-		strbuf_addch(&filter_options->filter_spec, '+');
-		filter_spec_append_urlencode(filter_options, arg);
 		ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,
 			      filter_options->sub_alloc);
 		sub = &filter_options->sub[filter_options->sub_nr - 1];
 
 		list_objects_filter_init(sub);
-		parse_error = gently_parse_list_objects_filter(sub, arg,
-							       &errbuf);
+		if (gently_parse_list_objects_filter(sub, arg, &errbuf))
+			die("%s", errbuf.buf);
+
+		strbuf_addch(&filter_options->filter_spec, '+');
+		filter_spec_append_urlencode(filter_options, arg);
 	}
-	if (parse_error)
-		die("%s", errbuf.buf);
 }
 
 int opt_parse_list_objects_filter(const struct option *opt,
diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh
index 0387f35a326..71e38491fa8 100755
--- a/t/t6112-rev-list-filters-objects.sh
+++ b/t/t6112-rev-list-filters-objects.sh
@@ -5,6 +5,7 @@ test_description='git rev-list using object filtering'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Test the blob:none filter.
-- 
2.47.0.dirty

Re: [PATCH 21/21] list-objects-filter-options: work around reported leak on error

[Toon Claes]

Patrick Steinhardt <ps@pks.im> writes:

> This one is a little bit more curious. In t6112, we have a test that
> exercises the `git rev-list --filter` option with invalid filters. We
> execute git-rev-list(1) via `test_must_fail`, which means that we check
> for leaks even though Git exits with an error code. This causes the
> following leak:
>
>     Direct leak of 27 byte(s) in 1 object(s) allocated from:
>         #0 0x5555555e6946 in realloc.part.0 lsan_interceptors.cpp.o
>         #1 0x5555558fb4b6 in xrealloc wrapper.c:137:8
>         #2 0x5555558b6e06 in strbuf_grow strbuf.c:112:2
>         #3 0x5555558b7550 in strbuf_add strbuf.c:311:2
>         #4 0x5555557c1a88 in strbuf_addstr strbuf.h:310:2
>         #5 0x5555557c1d4c in parse_list_objects_filter list-objects-filter-options.c:261:3
>         #6 0x555555885ead in handle_revision_pseudo_opt revision.c:2899:3
>         #7 0x555555884e20 in setup_revisions revision.c:3014:11
>         #8 0x5555556c4b42 in cmd_rev_list builtin/rev-list.c:588:9
>         #9 0x5555555ec5e3 in run_builtin git.c:483:11
>         #10 0x5555555eb1e4 in handle_builtin git.c:749:13
>         #11 0x5555555ec001 in run_argv git.c:819:4
>         #12 0x5555555eaf94 in cmd_main git.c:954:19
>         #13 0x5555556fd569 in main common-main.c:64:11
>         #14 0x7ffff7ca714d in __libc_start_call_main (.../lib/libc.so.6+0x2a14d)
>         #15 0x7ffff7ca7208 in __libc_start_main@GLIBC_2.2.5 (.../libc.so.6+0x2a208)
>         #16 0x5555555ad064 in _start (git+0x59064)
>
> This leak is valid, as we call `die()` and do not clean up the memory at
> all. But what's curious is that this is the only leak reported, because
> we don't clean up any other allocated memory, either, and I have no idea
> why the leak sanitizer treats this buffer specially.
>
> In any case, we can work around the leak by shuffling things around a
> bit. Instead of calling `gently_parse_list_objects_filter()` and dying
> after we have modified the filter spec, we simply do so beforehand. Like
> this we don't allocate the buffer in the error case, which makes the
> reported leak go away.
>
> It's not pretty, but it manages to make t6112 leak free.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  list-objects-filter-options.c       | 17 +++++++----------
>  t/t6112-rev-list-filters-objects.sh |  1 +
>  2 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
> index 00611107d20..fa72e81e4ad 100644
> --- a/list-objects-filter-options.c
> +++ b/list-objects-filter-options.c
> @@ -252,16 +252,14 @@ void parse_list_objects_filter(
>  	const char *arg)
>  {
>  	struct strbuf errbuf = STRBUF_INIT;
> -	int parse_error;
>  
>  	if (!filter_options->filter_spec.buf)
>  		BUG("filter_options not properly initialized");
>  
>  	if (!filter_options->choice) {
> +		if (gently_parse_list_objects_filter(filter_options, arg, &errbuf))
> +			die("%s", errbuf.buf);
>  		strbuf_addstr(&filter_options->filter_spec, arg);
> -
> -		parse_error = gently_parse_list_objects_filter(
> -			filter_options, arg, &errbuf);
>  	} else {
>  		struct list_objects_filter_options *sub;
>  
> @@ -271,18 +269,17 @@ void parse_list_objects_filter(
>  		 */
>  		transform_to_combine_type(filter_options);
>  
> -		strbuf_addch(&filter_options->filter_spec, '+');
> -		filter_spec_append_urlencode(filter_options, arg);
>  		ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,
>  			      filter_options->sub_alloc);
>  		sub = &filter_options->sub[filter_options->sub_nr - 1];
>  
>  		list_objects_filter_init(sub);
> -		parse_error = gently_parse_list_objects_filter(sub, arg,
> -							       &errbuf);
> +		if (gently_parse_list_objects_filter(sub, arg, &errbuf))
> +			die("%s", errbuf.buf);

Do we actually have a test hitting this code path? I wanted to figure
out why `filter_options->sub` wasn't leaky (I assume that's what you're
talking about in your commit message), but I wasn't able to reproduce a
scenario where we should die here.

--
Toon

Re: [PATCH 21/21] list-objects-filter-options: work around reported leak on error

[Patrick Steinhardt]

On Fri, Oct 18, 2024 at 02:04:18PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
> > index 00611107d20..fa72e81e4ad 100644
> > --- a/list-objects-filter-options.c
> > +++ b/list-objects-filter-options.c
> > @@ -252,16 +252,14 @@ void parse_list_objects_filter(
> >  	const char *arg)
> >  {
> >  	struct strbuf errbuf = STRBUF_INIT;
> > -	int parse_error;
> >  
> >  	if (!filter_options->filter_spec.buf)
> >  		BUG("filter_options not properly initialized");
> >  
> >  	if (!filter_options->choice) {
> > +		if (gently_parse_list_objects_filter(filter_options, arg, &errbuf))
> > +			die("%s", errbuf.buf);
> >  		strbuf_addstr(&filter_options->filter_spec, arg);
> > -
> > -		parse_error = gently_parse_list_objects_filter(
> > -			filter_options, arg, &errbuf);
> >  	} else {
> >  		struct list_objects_filter_options *sub;
> >  
> > @@ -271,18 +269,17 @@ void parse_list_objects_filter(
> >  		 */
> >  		transform_to_combine_type(filter_options);
> >  
> > -		strbuf_addch(&filter_options->filter_spec, '+');
> > -		filter_spec_append_urlencode(filter_options, arg);
> >  		ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,
> >  			      filter_options->sub_alloc);
> >  		sub = &filter_options->sub[filter_options->sub_nr - 1];
> >  
> >  		list_objects_filter_init(sub);
> > -		parse_error = gently_parse_list_objects_filter(sub, arg,
> > -							       &errbuf);
> > +		if (gently_parse_list_objects_filter(sub, arg, &errbuf))
> > +			die("%s", errbuf.buf);
> 
> Do we actually have a test hitting this code path? I wanted to figure
> out why `filter_options->sub` wasn't leaky (I assume that's what you're
> talking about in your commit message), but I wasn't able to reproduce a
> scenario where we should die here.

You only refer to the second hunk, right? I couldn't find any code paths
hitting this, so this is more of a "Let's massage this code in the same
way" change. I don't want the next person to go through the same rabbit
hole as I had to go through :)

Patrick

Re: [PATCH 00/21] Memory leak fixes (pt.9)

[Taylor Blau]

On Fri, Oct 11, 2024 at 07:32:04AM +0200, Patrick Steinhardt wrote:
> Hi,
>
> here's another round of memory leak fixes. With this series we're down
> to 10 test suites which are failing with the leak sanitizer. There are
> two patch series in flight [1][2] that fix three more test suites, so in
> total we're down to 7 test suites that we'll still have to fix up. So:
> we're almost done!

I was just looking through Toon's responses lower down in the thread,
and realized that I never picked this one up.

I think that makes sense... since this came on 11 October, which is when
Junio signed off for vacation. I think that we each assumed that the
other would (or had) pick it up.

In any case, this is now in my tree as 'ps/leakfixes-part-9'. Sorry for
the wait.

Thanks,
Taylor

Re: [PATCH 00/21] Memory leak fixes (pt.9)

[Patrick Steinhardt]

On Fri, Oct 18, 2024 at 05:30:36PM -0400, Taylor Blau wrote:
> On Fri, Oct 11, 2024 at 07:32:04AM +0200, Patrick Steinhardt wrote:
> > Hi,
> >
> > here's another round of memory leak fixes. With this series we're down
> > to 10 test suites which are failing with the leak sanitizer. There are
> > two patch series in flight [1][2] that fix three more test suites, so in
> > total we're down to 7 test suites that we'll still have to fix up. So:
> > we're almost done!
> 
> I was just looking through Toon's responses lower down in the thread,
> and realized that I never picked this one up.
> 
> I think that makes sense... since this came on 11 October, which is when
> Junio signed off for vacation. I think that we each assumed that the
> other would (or had) pick it up.
> 
> In any case, this is now in my tree as 'ps/leakfixes-part-9'. Sorry for
> the wait.

No worries, I'm happy that you keep things moving in the first place.
Thanks!

Patrick

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

[Patrick Steinhardt]

Hi,

this is the second version of my 9th series of memory leak fixes.

Changes compared to v1:

  - Split up the trailer fixes into two separate patches so that we can
    explain them standalone.

  - Adapt the second trailer memory leak fix to instead pull up the
    strbufs out of the loop such that we can reuse them. This makes the
    code flow more naturally and optimizes the loop.

Thanks!

Patrick

Patrick Steinhardt (22):
  builtin/ls-remote: plug leaking server options
  t/helper: fix leaks in "reach" test tool
  grep: fix leak in `grep_splice_or()`
  builtin/grep: fix leak with `--max-count=0`
  revision: fix leaking bloom filters
  diff-lib: fix leaking diffopts in `do_diff_cache()`
  pretty: clear signature check
  upload-pack: fix leaking URI protocols
  builtin/commit: fix leaking change data contents
  trailer: fix leaking trailer values
  trailer: fix leaking strbufs when formatting trailers
  builtin/commit: fix leaking cleanup config
  transport-helper: fix leaking import/export marks
  builtin/tag: fix leaking key ID on failure to sign
  combine-diff: fix leaking lost lines
  dir: release untracked cache data
  sparse-index: correctly free EWAH contents
  t/helper: stop re-initialization of `the_repository`
  t/helper: fix leaking buffer in "dump-untracked-cache"
  dir: fix leak when parsing "status.showUntrackedFiles"
  builtin/merge: release outbut buffer after performing merge
  list-objects-filter-options: work around reported leak on error

 builtin/commit.c                          | 26 +++++++++++++++++------
 builtin/grep.c                            | 13 +++++++++---
 builtin/ls-remote.c                       |  1 +
 builtin/merge.c                           |  1 +
 builtin/tag.c                             |  2 +-
 combine-diff.c                            |  2 +-
 diff-lib.c                                |  1 +
 dir.c                                     | 12 +++++++++--
 grep.c                                    |  1 +
 list-objects-filter-options.c             | 17 ++++++---------
 pretty.c                                  |  1 +
 revision.c                                |  5 +++++
 sparse-index.c                            |  7 ++++--
 t/helper/test-dump-untracked-cache.c      |  2 ++
 t/helper/test-reach.c                     | 10 +++++++++
 t/helper/test-read-cache.c                |  2 --
 t/t4038-diff-combined.sh                  |  1 +
 t/t4202-log.sh                            |  1 +
 t/t4216-log-bloom.sh                      |  1 +
 t/t5702-protocol-v2.sh                    |  1 +
 t/t5801-remote-helpers.sh                 |  1 +
 t/t6112-rev-list-filters-objects.sh       |  1 +
 t/t6424-merge-unrelated-index-changes.sh  |  1 +
 t/t6600-test-reach.sh                     |  1 +
 t/t7004-tag.sh                            |  1 +
 t/t7031-verify-tag-signed-ssh.sh          |  1 +
 t/t7063-status-untracked-cache.sh         |  1 +
 t/t7500-commit-template-squash-signoff.sh |  1 +
 t/t7502-commit-porcelain.sh               |  1 +
 t/t7510-signed-commit.sh                  |  1 +
 t/t7513-interpret-trailers.sh             |  1 +
 t/t7519-status-fsmonitor.sh               |  1 +
 t/t7528-signed-commit-ssh.sh              |  1 +
 t/t7610-mergetool.sh                      |  1 +
 t/t7810-grep.sh                           |  1 +
 trailer.c                                 | 25 +++++++++++++++-------
 transport-helper.c                        |  2 ++
 upload-pack.c                             |  1 +
 38 files changed, 115 insertions(+), 35 deletions(-)

Range-diff against v1:
 1:  89b66411354 =  1:  89b66411354 builtin/ls-remote: plug leaking server options
 2:  1c42e194b20 =  2:  1c42e194b20 t/helper: fix leaks in "reach" test tool
 3:  cb4eee37b40 =  3:  cb4eee37b40 grep: fix leak in `grep_splice_or()`
 4:  6b2c8842ef5 =  4:  6b2c8842ef5 builtin/grep: fix leak with `--max-count=0`
 5:  7527b31a28f =  5:  7527b31a28f revision: fix leaking bloom filters
 6:  60af98cb2c7 =  6:  60af98cb2c7 diff-lib: fix leaking diffopts in `do_diff_cache()`
 7:  5d5f6867f91 =  7:  5d5f6867f91 pretty: clear signature check
 8:  0d503e40194 =  8:  0d503e40194 upload-pack: fix leaking URI protocols
 9:  9f967dfe5d5 =  9:  9f967dfe5d5 builtin/commit: fix leaking change data contents
 -:  ----------- > 10:  e3ffd59123f trailer: fix leaking trailer values
10:  ca5370d572d ! 11:  5b851453bce trailer: fix leaking trailer values
    @@ Metadata
     Author: Patrick Steinhardt <ps@pks.im>
     
      ## Commit message ##
    -    trailer: fix leaking trailer values
    +    trailer: fix leaking strbufs when formatting trailers
     
    -    Fix leaking trailer values when replacing the value with a command or
    -    when the token value is empty.
    +    We are populating, but never releasing two string buffers in
    +    `format_trailers()`, causing a memory leak. Plug this leak by lifting
    +    those buffers outside of the loop and releasing them on function return.
    +    This fixes the memory leaks, but also optimizes the loop as we don't
    +    have to reallocate the buffers on every single iteration.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ t/t7513-interpret-trailers.sh
      # When we want one trailing space at the end of each line, let's use sed
     
      ## trailer.c ##
    -@@ trailer.c: static char *apply_command(struct conf_info *conf, const char *arg)
    - static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok)
    +@@ trailer.c: void format_trailers(const struct process_trailer_options *opts,
    + 		     struct list_head *trailers,
    + 		     struct strbuf *out)
      {
    - 	if (arg_tok->conf.command || arg_tok->conf.cmd) {
    --		const char *arg;
    -+		char *value_to_free = NULL;
    -+		char *arg;
    -+
    - 		if (arg_tok->value && arg_tok->value[0]) {
    --			arg = arg_tok->value;
    -+			arg = (char *)arg_tok->value;
    - 		} else {
    - 			if (in_tok && in_tok->value)
    - 				arg = xstrdup(in_tok->value);
    - 			else
    - 				arg = xstrdup("");
    -+			value_to_free = arg_tok->value;
    - 		}
    -+
    - 		arg_tok->value = apply_command(&arg_tok->conf, arg);
    --		free((char *)arg);
    -+
    -+		free(value_to_free);
    -+		free(arg);
    - 	}
    - }
    ++	struct strbuf tok = STRBUF_INIT;
    ++	struct strbuf val = STRBUF_INIT;
    + 	size_t origlen = out->len;
    + 	struct list_head *pos;
    + 	struct trailer_item *item;
      
    -@@ trailer.c: void format_trailers(const struct process_trailer_options *opts,
    - 		if (item->token) {
    - 			struct strbuf tok = STRBUF_INIT;
    - 			struct strbuf val = STRBUF_INIT;
     +
    + 	list_for_each(pos, trailers) {
    + 		item = list_entry(pos, struct trailer_item, list);
    + 		if (item->token) {
    +-			struct strbuf tok = STRBUF_INIT;
    +-			struct strbuf val = STRBUF_INIT;
    ++			strbuf_reset(&tok);
      			strbuf_addstr(&tok, item->token);
    ++			strbuf_reset(&val);
      			strbuf_addstr(&val, item->value);
      
    -@@ trailer.c: void format_trailers(const struct process_trailer_options *opts,
    - 			 * corresponding value).
    - 			 */
    - 			if (opts->trim_empty && !strlen(item->value))
    --				continue;
    -+				goto next;
    - 
    - 			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
    - 				if (opts->separator && out->len != origlen)
    + 			/*
     @@ trailer.c: void format_trailers(const struct process_trailer_options *opts,
      				if (!opts->separator)
      					strbuf_addch(out, '\n');
      			}
    -+
    -+next:
    - 			strbuf_release(&tok);
    - 			strbuf_release(&val);
    +-			strbuf_release(&tok);
    +-			strbuf_release(&val);
     -
      		} else if (!opts->only_trailers) {
      			if (opts->separator && out->len != origlen) {
      				strbuf_addbuf(out, opts->separator);
    +@@ trailer.c: void format_trailers(const struct process_trailer_options *opts,
    + 				strbuf_addch(out, '\n');
    + 		}
    + 	}
    ++
    ++	strbuf_release(&tok);
    ++	strbuf_release(&val);
    + }
    + 
    + void format_trailers_from_commit(const struct process_trailer_options *opts,
11:  8ca4344ed86 = 12:  60c3f6146f3 builtin/commit: fix leaking cleanup config
12:  16d969ed7b1 = 13:  ea81cd8db86 transport-helper: fix leaking import/export marks
13:  9e4bc5bf73f = 14:  b700ab9079f builtin/tag: fix leaking key ID on failure to sign
14:  8d305d9b1c8 = 15:  76bbcb3fe30 combine-diff: fix leaking lost lines
15:  f977a033cf4 = 16:  d6b96a4012d dir: release untracked cache data
16:  170cc61edaa = 17:  3aa6bac5079 sparse-index: correctly free EWAH contents
17:  8e10ee844c6 = 18:  132fe750906 t/helper: stop re-initialization of `the_repository`
18:  71fd1c76b8a = 19:  b8b702eeb28 t/helper: fix leaking buffer in "dump-untracked-cache"
19:  8ccc246432d = 20:  ad309ac1b37 dir: fix leak when parsing "status.showUntrackedFiles"
20:  bc2206aa47e = 21:  5e243f9ee53 builtin/merge: release outbut buffer after performing merge
21:  6a2baf0d3e5 = 22:  b75376e3725 list-objects-filter-options: work around reported leak on error
-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH v2 01/22] builtin/ls-remote: plug leaking server options

[Patrick Steinhardt]

The server options populated via `OPT_STRING_LIST()` is never cleared,
causing a memory leak. Plug it.

This leak is exposed by t5702, but plugging it alone does not make the
whole test suite pass.

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

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index f723b3bf3bb..f333821b994 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -166,6 +166,7 @@ int cmd_ls_remote(int argc,
 		status = 0; /* we found something */
 	}
 
+	string_list_clear(&server_options, 0);
 	ref_sorting_release(sorting);
 	ref_array_clear(&ref_array);
 	if (transport_disconnect(transport))
-- 
2.47.0.72.gef8ce8f3d4.dirty

Re: [PATCH v2 01/22] builtin/ls-remote: plug leaking server options

[Justin Tobler]

On 24/10/21 11:28AM, Patrick Steinhardt wrote:
> The server options populated via `OPT_STRING_LIST()` is never cleared,

s/is/are/

> causing a memory leak. Plug it.
> 
> This leak is exposed by t5702, but plugging it alone does not make the
> whole test suite pass.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

Re: [PATCH v2 01/22] builtin/ls-remote: plug leaking server options

[Kristoffer Haugsbakk]

On Mon, Nov 4, 2024, at 23:10, Justin Tobler wrote:
> On 24/10/21 11:28AM, Patrick Steinhardt wrote:
>> The server options populated via `OPT_STRING_LIST()` is never cleared,
>
> s/is/are/

I guess “is” was chosen because “the list is”.

   The [list] populated via `OPT_STRING_LIST()` is never cleared,

-- 
Kristoffer Haugsbakk

[PATCH v2 02/22] t/helper: fix leaks in "reach" test tool

[Patrick Steinhardt]

The "reach" test tool doesn't bother to clean up any of its allocated
resources, causing various leaks. Plug them.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/helper/test-reach.c | 10 ++++++++++
 t/t6600-test-reach.sh |  1 +
 2 files changed, 11 insertions(+)

diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c
index 995e382863a..84deee604ad 100644
--- a/t/helper/test-reach.c
+++ b/t/helper/test-reach.c
@@ -127,10 +127,12 @@ int cmd__reach(int ac, const char **av)
 			exit(128);
 		printf("%s(A,X):\n", av[1]);
 		print_sorted_commit_ids(list);
+		free_commit_list(list);
 	} else if (!strcmp(av[1], "reduce_heads")) {
 		struct commit_list *list = reduce_heads(X);
 		printf("%s(X):\n", av[1]);
 		print_sorted_commit_ids(list);
+		free_commit_list(list);
 	} else if (!strcmp(av[1], "can_all_from_reach")) {
 		printf("%s(X,Y):%d\n", av[1], can_all_from_reach(X, Y, 1));
 	} else if (!strcmp(av[1], "can_all_from_reach_with_flag")) {
@@ -153,6 +155,7 @@ int cmd__reach(int ac, const char **av)
 			filter.with_commit_tag_algo = 0;
 
 		printf("%s(_,A,X,_):%d\n", av[1], commit_contains(&filter, A, X, &cache));
+		clear_contains_cache(&cache);
 	} else if (!strcmp(av[1], "get_reachable_subset")) {
 		const int reachable_flag = 1;
 		int i, count = 0;
@@ -176,7 +179,14 @@ int cmd__reach(int ac, const char **av)
 			die(_("too many commits marked reachable"));
 
 		print_sorted_commit_ids(list);
+		free_commit_list(list);
 	}
 
+	object_array_clear(&X_obj);
+	strbuf_release(&buf);
+	free_commit_list(X);
+	free_commit_list(Y);
+	free(X_array);
+	free(Y_array);
 	return 0;
 }
diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
index 2591f8b8b39..307deefed2c 100755
--- a/t/t6600-test-reach.sh
+++ b/t/t6600-test-reach.sh
@@ -2,6 +2,7 @@
 
 test_description='basic commit reachability tests'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Construct a grid-like commit graph with points (x,y)
-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH v2 03/22] grep: fix leak in `grep_splice_or()`

[Patrick Steinhardt]

In `grep_splice_or()` we search for the next `TRUE` node in our tree of
grep exrpessions and replace it with the given new expression. But we
don't free the old node, which causes a memory leak. Plug it.

This leak is exposed by t7810, but plugging it alone isn't sufficient to
make the test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grep.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/grep.c b/grep.c
index 701e58de04e..e9337f32cbf 100644
--- a/grep.c
+++ b/grep.c
@@ -756,6 +756,7 @@ static struct grep_expr *grep_splice_or(struct grep_expr *x, struct grep_expr *y
 		assert(x->node == GREP_NODE_OR);
 		if (x->u.binary.right &&
 		    x->u.binary.right->node == GREP_NODE_TRUE) {
+			free(x->u.binary.right);
 			x->u.binary.right = y;
 			break;
 		}
-- 
2.47.0.72.gef8ce8f3d4.dirty

Re: [PATCH v2 03/22] grep: fix leak in `grep_splice_or()`

[Kristoffer Haugsbakk]

On Mon, Oct 21, 2024, at 11:28, Patrick Steinhardt wrote:
> In `grep_splice_or()` we search for the next `TRUE` node in our tree of
> grep exrpessions and replace it with the given new expression. But we

s/exrpessions/expressions/

[PATCH v2 04/22] builtin/grep: fix leak with `--max-count=0`

[Patrick Steinhardt]

When executing with `--max-count=0` we'll return early from git-grep(1)
without performing any cleanup, which causes memory leaks. Plug these.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/grep.c  | 13 ++++++++++---
 t/t7810-grep.sh |  1 +
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index f17d46a06e4..98b85c7fcac 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -906,6 +906,7 @@ int cmd_grep(int argc,
 	int dummy;
 	int use_index = 1;
 	int allow_revs;
+	int ret;
 
 	struct option options[] = {
 		OPT_BOOL(0, "cached", &cached,
@@ -1172,8 +1173,10 @@ int cmd_grep(int argc,
 	 * Optimize out the case where the amount of matches is limited to zero.
 	 * We do this to keep results consistent with GNU grep(1).
 	 */
-	if (opt.max_count == 0)
-		return 1;
+	if (opt.max_count == 0) {
+		ret = 1;
+		goto out;
+	}
 
 	if (show_in_pager) {
 		if (num_threads > 1)
@@ -1267,10 +1270,14 @@ int cmd_grep(int argc,
 		hit |= wait_all();
 	if (hit && show_in_pager)
 		run_pager(&opt, prefix);
+
+	ret = !hit;
+
+out:
 	clear_pathspec(&pathspec);
 	string_list_clear(&path_list, 0);
 	free_grep_patterns(&opt);
 	object_array_clear(&list);
 	free_repos();
-	return !hit;
+	return ret;
 }
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index af2cf2f78ab..9e7681f0831 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -9,6 +9,7 @@ test_description='git grep various.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_invalid_grep_expression() {
-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH v2 05/22] revision: fix leaking bloom filters

[Patrick Steinhardt]

The memory allocated by `prepare_to_use_bloom_filter()` is not released
by `release_revisions()`, causing a memory leak. Plug it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 revision.c           | 5 +++++
 t/t4216-log-bloom.sh | 1 +
 2 files changed, 6 insertions(+)

diff --git a/revision.c b/revision.c
index f5f5b84f2b0..8df75b82249 100644
--- a/revision.c
+++ b/revision.c
@@ -3227,6 +3227,11 @@ void release_revisions(struct rev_info *revs)
 	clear_decoration(&revs->treesame, free);
 	line_log_free(revs);
 	oidset_clear(&revs->missing_commits);
+
+	for (int i = 0; i < revs->bloom_keys_nr; i++)
+		clear_bloom_key(&revs->bloom_keys[i]);
+	FREE_AND_NULL(revs->bloom_keys);
+	revs->bloom_keys_nr = 0;
 }
 
 static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)
diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index 3f163dc3969..8d22338f6aa 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -4,6 +4,7 @@ test_description='git log for a path with Bloom filters'
 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-chunk.sh
 
-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH v2 06/22] diff-lib: fix leaking diffopts in `do_diff_cache()`

[Patrick Steinhardt]

In `do_diff_cache()` we initialize a new `rev_info` and then overwrite
its `diffopt` with a user-provided set of options. This can leak memory
because `repo_init_revisions()` may end up allocating memory for the
`diffopt` itself depending on the configuration. And as that field is
overwritten we won't ever free that.

Plug the memory leak by releasing the diffopts before we overwrite them.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 diff-lib.c           | 1 +
 t/t7610-mergetool.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/diff-lib.c b/diff-lib.c
index 6b14b959629..3cf353946f5 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -661,6 +661,7 @@ int do_diff_cache(const struct object_id *tree_oid, struct diff_options *opt)
 
 	repo_init_revisions(opt->repo, &revs, NULL);
 	copy_pathspec(&revs.prune_data, &opt->pathspec);
+	diff_free(&revs.diffopt);
 	revs.diffopt = *opt;
 	revs.diffopt.no_free = 1;
 
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 22b3a85b3e9..5c5e79e9905 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -10,6 +10,7 @@ Testing basic merge tool invocation'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # All the mergetool test work by checking out a temporary branch based
-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH v2 07/22] pretty: clear signature check

[Patrick Steinhardt]

The signature check in of the formatting context is never getting
released. Fix this to plug the resulting memory leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 pretty.c                         | 1 +
 t/t4202-log.sh                   | 1 +
 t/t7031-verify-tag-signed-ssh.sh | 1 +
 t/t7510-signed-commit.sh         | 1 +
 t/t7528-signed-commit-ssh.sh     | 1 +
 5 files changed, 5 insertions(+)

diff --git a/pretty.c b/pretty.c
index 6403e268900..098378720a4 100644
--- a/pretty.c
+++ b/pretty.c
@@ -2032,6 +2032,7 @@ void repo_format_commit_message(struct repository *r,
 
 	free(context.commit_encoding);
 	repo_unuse_commit_buffer(r, commit, context.message);
+	signature_check_clear(&context.signature_check);
 }
 
 static void pp_header(struct pretty_print_context *pp,
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 51f7beb59f8..35bec4089a3 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -5,6 +5,7 @@ test_description='git log'
 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"
 . "$TEST_DIRECTORY/lib-terminal.sh"
diff --git a/t/t7031-verify-tag-signed-ssh.sh b/t/t7031-verify-tag-signed-ssh.sh
index 20913b37134..2ee62c07293 100755
--- a/t/t7031-verify-tag-signed-ssh.sh
+++ b/t/t7031-verify-tag-signed-ssh.sh
@@ -4,6 +4,7 @@ test_description='signed tag tests'
 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"
 
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 0d2dd29fe6a..eb229082e40 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -4,6 +4,7 @@ test_description='signed commit tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 GNUPGHOME_NOT_USED=$GNUPGHOME
 . "$TEST_DIRECTORY/lib-gpg.sh"
diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh
index 065f7806362..68e18856b66 100755
--- a/t/t7528-signed-commit-ssh.sh
+++ b/t/t7528-signed-commit-ssh.sh
@@ -4,6 +4,7 @@ test_description='ssh signed commit tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 GNUPGHOME_NOT_USED=$GNUPGHOME
 . "$TEST_DIRECTORY/lib-gpg.sh"
-- 
2.47.0.72.gef8ce8f3d4.dirty

Re: [PATCH v2 07/22] pretty: clear signature check

[Justin Tobler]

On 24/10/21 11:28AM, Patrick Steinhardt wrote:
> The signature check in of the formatting context is never getting

s/in of/in/

> released. Fix this to plug the resulting memory leak.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  pretty.c                         | 1 +
>  t/t4202-log.sh                   | 1 +
>  t/t7031-verify-tag-signed-ssh.sh | 1 +
>  t/t7510-signed-commit.sh         | 1 +
>  t/t7528-signed-commit-ssh.sh     | 1 +
>  5 files changed, 5 insertions(+)
> 
> diff --git a/pretty.c b/pretty.c
> index 6403e268900..098378720a4 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -2032,6 +2032,7 @@ void repo_format_commit_message(struct repository *r,
>  
>  	free(context.commit_encoding);
>  	repo_unuse_commit_buffer(r, commit, context.message);
> +	signature_check_clear(&context.signature_check);

Initially I was thinking it might be nice to see the
`format_commit_context` cleanup extracted to its own function, but being
that it only has a single use internal to "pretty.c" it probably isn't
worth it.

>  }
>  
>  static void pp_header(struct pretty_print_context *pp,

[PATCH v2 08/22] upload-pack: fix leaking URI protocols

[Patrick Steinhardt]

We don't clear `struct upload_pack::uri_protocols`, which causes a
memory leak. Fix this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5702-protocol-v2.sh | 1 +
 upload-pack.c          | 1 +
 2 files changed, 2 insertions(+)

diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 1ef540f73d3..ed55a2f7f95 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -7,6 +7,7 @@ TEST_NO_CREATE_REPO=1
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Test protocol v2 with 'git://' transport
diff --git a/upload-pack.c b/upload-pack.c
index 6d6e0f9f980..b4a59c3518b 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -166,6 +166,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data)
 	object_array_clear(&data->extra_edge_obj);
 	list_objects_filter_release(&data->filter_options);
 	string_list_clear(&data->allowed_filters, 0);
+	string_list_clear(&data->uri_protocols, 0);
 
 	free((char *)data->pack_objects_hook);
 }
-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH v2 09/22] builtin/commit: fix leaking change data contents

[Patrick Steinhardt]

While we free the worktree change data, we never free its contents. Fix
this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/commit.c                          | 9 ++++++++-
 t/t7500-commit-template-squash-signoff.sh | 1 +
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 8db4e9df0c9..18a55bd1b91 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -728,6 +728,13 @@ static void prepare_amend_commit(struct commit *commit, struct strbuf *sb,
 	repo_unuse_commit_buffer(the_repository, commit, buffer);
 }
 
+static void change_data_free(void *util, const char *str UNUSED)
+{
+	struct wt_status_change_data *d = util;
+	free(d->rename_source);
+	free(d);
+}
+
 static int prepare_to_commit(const char *index_file, const char *prefix,
 			     struct commit *current_head,
 			     struct wt_status *s,
@@ -991,7 +998,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		s->use_color = 0;
 		committable = run_status(s->fp, index_file, prefix, 1, s);
 		s->use_color = saved_color_setting;
-		string_list_clear(&s->change, 1);
+		string_list_clear_func(&s->change, change_data_free);
 	} else {
 		struct object_id oid;
 		const char *parent = "HEAD";
diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 4dca8d97a77..379d3ed3413 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -7,6 +7,7 @@ test_description='git commit
 
 Tests for template, signoff, squash and -F functions.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-rebase.sh
-- 
2.47.0.72.gef8ce8f3d4.dirty

Re: [PATCH v2 09/22] builtin/commit: fix leaking change data contents

[Justin Tobler]

On 24/10/21 11:28AM, Patrick Steinhardt wrote:
> While we free the worktree change data, we never free its contents. Fix
> this.

Ah, so this worktree change data was part of the string list and was
being freed, but it also had memory allocations itself that were not.
Makes sense :)

> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/commit.c                          | 9 ++++++++-
>  t/t7500-commit-template-squash-signoff.sh | 1 +
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 8db4e9df0c9..18a55bd1b91 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -728,6 +728,13 @@ static void prepare_amend_commit(struct commit *commit, struct strbuf *sb,
>  	repo_unuse_commit_buffer(the_repository, commit, buffer);
>  }
>  
> +static void change_data_free(void *util, const char *str UNUSED)
> +{
> +	struct wt_status_change_data *d = util;
> +	free(d->rename_source);
> +	free(d);
> +}
> +
>  static int prepare_to_commit(const char *index_file, const char *prefix,
>  			     struct commit *current_head,
>  			     struct wt_status *s,
> @@ -991,7 +998,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		s->use_color = 0;
>  		committable = run_status(s->fp, index_file, prefix, 1, s);
>  		s->use_color = saved_color_setting;
> -		string_list_clear(&s->change, 1);
> +		string_list_clear_func(&s->change, change_data_free);
>  	} else {
>  		struct object_id oid;
>  		const char *parent = "HEAD";
> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
> index 4dca8d97a77..379d3ed3413 100755
> --- a/t/t7500-commit-template-squash-signoff.sh
> +++ b/t/t7500-commit-template-squash-signoff.sh
> @@ -7,6 +7,7 @@ test_description='git commit
>  
>  Tests for template, signoff, squash and -F functions.'
>  
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>  
>  . "$TEST_DIRECTORY"/lib-rebase.sh
> -- 
> 2.47.0.72.gef8ce8f3d4.dirty
> 
> 

[PATCH v2 10/22] trailer: fix leaking trailer values

[Patrick Steinhardt]

Fix leaking trailer values when replacing the value with a command or
when the token value is empty.

This leak is exposed by t7513, but plugging it does not make the whole
test suite pass.

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

diff --git a/trailer.c b/trailer.c
index 682d74505bf..f1eca6d5d15 100644
--- a/trailer.c
+++ b/trailer.c
@@ -249,17 +249,23 @@ static char *apply_command(struct conf_info *conf, const char *arg)
 static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok)
 {
 	if (arg_tok->conf.command || arg_tok->conf.cmd) {
-		const char *arg;
+		char *value_to_free = NULL;
+		char *arg;
+
 		if (arg_tok->value && arg_tok->value[0]) {
-			arg = arg_tok->value;
+			arg = (char *)arg_tok->value;
 		} else {
 			if (in_tok && in_tok->value)
 				arg = xstrdup(in_tok->value);
 			else
 				arg = xstrdup("");
+			value_to_free = arg_tok->value;
 		}
+
 		arg_tok->value = apply_command(&arg_tok->conf, arg);
-		free((char *)arg);
+
+		free(value_to_free);
+		free(arg);
 	}
 }
 
-- 
2.47.0.72.gef8ce8f3d4.dirty

Re: [PATCH v2 10/22] trailer: fix leaking trailer values

[Justin Tobler]

On 24/10/21 11:28AM, Patrick Steinhardt wrote:
> Fix leaking trailer values when replacing the value with a command or
> when the token value is empty.
> 
> This leak is exposed by t7513, but plugging it does not make the whole
> test suite pass.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  trailer.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/trailer.c b/trailer.c
> index 682d74505bf..f1eca6d5d15 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -249,17 +249,23 @@ static char *apply_command(struct conf_info *conf, const char *arg)
>  static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok)
>  {
>  	if (arg_tok->conf.command || arg_tok->conf.cmd) {
> -		const char *arg;
> +		char *value_to_free = NULL;
> +		char *arg;
> +
>  		if (arg_tok->value && arg_tok->value[0]) {
> -			arg = arg_tok->value;
> +			arg = (char *)arg_tok->value;

Naive question, is this cast not redundant? From looking at `struct
arg_item`, `value` already looks to be this type.

>  		} else {
>  			if (in_tok && in_tok->value)
>  				arg = xstrdup(in_tok->value);
>  			else
>  				arg = xstrdup("");
> +			value_to_free = arg_tok->value;
>  		}
> +
>  		arg_tok->value = apply_command(&arg_tok->conf, arg);
> -		free((char *)arg);
> +
> +		free(value_to_free);
> +		free(arg);
>  	}
>  }
>  
> -- 
> 2.47.0.72.gef8ce8f3d4.dirty
> 
> 

Re: [PATCH v2 10/22] trailer: fix leaking trailer values

[Patrick Steinhardt]

On Mon, Nov 04, 2024 at 04:25:38PM -0600, Justin Tobler wrote:
> On 24/10/21 11:28AM, Patrick Steinhardt wrote:
> > Fix leaking trailer values when replacing the value with a command or
> > when the token value is empty.
> > 
> > This leak is exposed by t7513, but plugging it does not make the whole
> > test suite pass.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  trailer.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/trailer.c b/trailer.c
> > index 682d74505bf..f1eca6d5d15 100644
> > --- a/trailer.c
> > +++ b/trailer.c
> > @@ -249,17 +249,23 @@ static char *apply_command(struct conf_info *conf, const char *arg)
> >  static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok)
> >  {
> >  	if (arg_tok->conf.command || arg_tok->conf.cmd) {
> > -		const char *arg;
> > +		char *value_to_free = NULL;
> > +		char *arg;
> > +
> >  		if (arg_tok->value && arg_tok->value[0]) {
> > -			arg = arg_tok->value;
> > +			arg = (char *)arg_tok->value;
> 
> Naive question, is this cast not redundant? From looking at `struct
> arg_item`, `value` already looks to be this type.

Indeed it is, good catch!

Patrick

[PATCH v2 11/22] trailer: fix leaking strbufs when formatting trailers

[Patrick Steinhardt]

We are populating, but never releasing two string buffers in
`format_trailers()`, causing a memory leak. Plug this leak by lifting
those buffers outside of the loop and releasing them on function return.
This fixes the memory leaks, but also optimizes the loop as we don't
have to reallocate the buffers on every single iteration.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t7513-interpret-trailers.sh |  1 +
 trailer.c                     | 13 ++++++++-----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 0f7d8938d98..38d6ccaa001 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -5,6 +5,7 @@
 
 test_description='git interpret-trailers'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # When we want one trailing space at the end of each line, let's use sed
diff --git a/trailer.c b/trailer.c
index f1eca6d5d15..24e4e56fdf8 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1111,16 +1111,19 @@ void format_trailers(const struct process_trailer_options *opts,
 		     struct list_head *trailers,
 		     struct strbuf *out)
 {
+	struct strbuf tok = STRBUF_INIT;
+	struct strbuf val = STRBUF_INIT;
 	size_t origlen = out->len;
 	struct list_head *pos;
 	struct trailer_item *item;
 
+
 	list_for_each(pos, trailers) {
 		item = list_entry(pos, struct trailer_item, list);
 		if (item->token) {
-			struct strbuf tok = STRBUF_INIT;
-			struct strbuf val = STRBUF_INIT;
+			strbuf_reset(&tok);
 			strbuf_addstr(&tok, item->token);
+			strbuf_reset(&val);
 			strbuf_addstr(&val, item->value);
 
 			/*
@@ -1151,9 +1154,6 @@ void format_trailers(const struct process_trailer_options *opts,
 				if (!opts->separator)
 					strbuf_addch(out, '\n');
 			}
-			strbuf_release(&tok);
-			strbuf_release(&val);
-
 		} else if (!opts->only_trailers) {
 			if (opts->separator && out->len != origlen) {
 				strbuf_addbuf(out, opts->separator);
@@ -1165,6 +1165,9 @@ void format_trailers(const struct process_trailer_options *opts,
 				strbuf_addch(out, '\n');
 		}
 	}
+
+	strbuf_release(&tok);
+	strbuf_release(&val);
 }
 
 void format_trailers_from_commit(const struct process_trailer_options *opts,
-- 
2.47.0.72.gef8ce8f3d4.dirty

Re: [PATCH v2 11/22] trailer: fix leaking strbufs when formatting trailers

[Taylor Blau]

On Mon, Oct 21, 2024 at 11:28:35AM +0200, Patrick Steinhardt wrote:
> We are populating, but never releasing two string buffers in
> `format_trailers()`, causing a memory leak. Plug this leak by lifting
> those buffers outside of the loop and releasing them on function return.
> This fixes the memory leaks, but also optimizes the loop as we don't
> have to reallocate the buffers on every single iteration.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t7513-interpret-trailers.sh |  1 +
>  trailer.c                     | 13 ++++++++-----
>  2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
> index 0f7d8938d98..38d6ccaa001 100755
> --- a/t/t7513-interpret-trailers.sh
> +++ b/t/t7513-interpret-trailers.sh
> @@ -5,6 +5,7 @@
>
>  test_description='git interpret-trailers'
>
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  # When we want one trailing space at the end of each line, let's use sed
> diff --git a/trailer.c b/trailer.c
> index f1eca6d5d15..24e4e56fdf8 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1111,16 +1111,19 @@ void format_trailers(const struct process_trailer_options *opts,
>  		     struct list_head *trailers,
>  		     struct strbuf *out)
>  {
> +	struct strbuf tok = STRBUF_INIT;
> +	struct strbuf val = STRBUF_INIT;
>  	size_t origlen = out->len;
>  	struct list_head *pos;
>  	struct trailer_item *item;
>
> +
>  	list_for_each(pos, trailers) {
>  		item = list_entry(pos, struct trailer_item, list);
>  		if (item->token) {
> -			struct strbuf tok = STRBUF_INIT;
> -			struct strbuf val = STRBUF_INIT;
> +			strbuf_reset(&tok);
>  			strbuf_addstr(&tok, item->token);
> +			strbuf_reset(&val);
>  			strbuf_addstr(&val, item->value);

I have a vague preference towards writing this as:

        strbuf_reset(&tok);
        strbuf_reset(&val);
        strbuf_addstr(&tok, item->token);
        strbuf_addstr(&val, item->value);

to make clear(er) to the reader that, OK, we are resetting both of these
buffers before using them at the beginning of the loop. To me it reads a
little clearer seeing both strbuf_reset() calls one right after the
other.

I don't feel all that strongly about it, but I also see that you have a
couple of small changes you're sitting on from Kristoffer's review, so I
figured I'd throw it out there anyway as we are expecting a new round to
address those.

Thanks,
Taylor

Re: [PATCH v2 11/22] trailer: fix leaking strbufs when formatting trailers

[Justin Tobler]

On 24/10/21 11:28AM, Patrick Steinhardt wrote:
> We are populating, but never releasing two string buffers in
> `format_trailers()`, causing a memory leak. Plug this leak by lifting
> those buffers outside of the loop and releasing them on function return.
> This fixes the memory leaks, but also optimizes the loop as we don't
> have to reallocate the buffers on every single iteration.

I see that we were previously calling `strbuf_release()` inside of the
loop. In practice were these never hit? Or just sometimes skipped my the
"continue" in the loop? The commit message makes it sound like the
former.

Re: [PATCH v2 11/22] trailer: fix leaking strbufs when formatting trailers

[Patrick Steinhardt]

On Mon, Nov 04, 2024 at 04:31:34PM -0600, Justin Tobler wrote:
> On 24/10/21 11:28AM, Patrick Steinhardt wrote:
> > We are populating, but never releasing two string buffers in
> > `format_trailers()`, causing a memory leak. Plug this leak by lifting
> > those buffers outside of the loop and releasing them on function return.
> > This fixes the memory leaks, but also optimizes the loop as we don't
> > have to reallocate the buffers on every single iteration.
> 
> I see that we were previously calling `strbuf_release()` inside of the
> loop. In practice were these never hit? Or just sometimes skipped my the
> "continue" in the loop? The commit message makes it sound like the
> former.

True, "never" is definitely too strong of a wording here. Will adapt,
thanks!

Patrick

[PATCH v2 12/22] builtin/commit: fix leaking cleanup config

[Patrick Steinhardt]

The cleanup string set by the config is leaking when it is being
overridden by an option. Fix this by tracking these via two separate
variables such that we can free the old value.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/commit.c            | 17 ++++++++++++-----
 t/t7502-commit-porcelain.sh |  1 +
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 18a55bd1b91..71d674138c9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -135,7 +135,7 @@ static struct strvec trailer_args = STRVEC_INIT;
  * is specified explicitly.
  */
 static enum commit_msg_cleanup_mode cleanup_mode;
-static char *cleanup_arg;
+static char *cleanup_config;
 
 static enum commit_whence whence;
 static int use_editor = 1, include_status = 1;
@@ -1387,8 +1387,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (0 <= edit_flag)
 		use_editor = edit_flag;
 
-	cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);
-
 	handle_untracked_files_arg(s);
 
 	if (all && argc > 0)
@@ -1636,8 +1634,10 @@ static int git_commit_config(const char *k, const char *v,
 		include_status = git_config_bool(k, v);
 		return 0;
 	}
-	if (!strcmp(k, "commit.cleanup"))
-		return git_config_string(&cleanup_arg, k, v);
+	if (!strcmp(k, "commit.cleanup")) {
+		FREE_AND_NULL(cleanup_config);
+		return git_config_string(&cleanup_config, k, v);
+	}
 	if (!strcmp(k, "commit.gpgsign")) {
 		sign_commit = git_config_bool(k, v) ? "" : NULL;
 		return 0;
@@ -1658,6 +1658,7 @@ int cmd_commit(int argc,
 	       struct repository *repo UNUSED)
 {
 	static struct wt_status s;
+	static const char *cleanup_arg = NULL;
 	static struct option builtin_commit_options[] = {
 		OPT__QUIET(&quiet, N_("suppress summary after successful commit")),
 		OPT__VERBOSE(&verbose, N_("show diff in commit message template")),
@@ -1757,6 +1758,12 @@ int cmd_commit(int argc,
 	if (verbose == -1)
 		verbose = (config_commit_verbose < 0) ? 0 : config_commit_verbose;
 
+	if (cleanup_arg) {
+		free(cleanup_config);
+		cleanup_config = xstrdup(cleanup_arg);
+	}
+	cleanup_mode = get_cleanup_mode(cleanup_config, use_editor);
+
 	if (dry_run)
 		return dry_run_commit(argv, prefix, current_head, &s);
 	index_file = prepare_index(argv, prefix, current_head, 0);
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index b37e2018a74..84f1ff52b67 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -5,6 +5,7 @@ test_description='git commit porcelain-ish'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 commit_msg_is () {
-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH v2 13/22] transport-helper: fix leaking import/export marks

[Patrick Steinhardt]

Fix leaking import and export marks for transport helpers.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5801-remote-helpers.sh | 1 +
 transport-helper.c        | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index d21877150ed..d4882288a30 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -8,6 +8,7 @@ test_description='Test remote-helper import and export commands'
 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
 
diff --git a/transport-helper.c b/transport-helper.c
index 013ec79dc9c..bc27653cdee 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -399,6 +399,8 @@ static int release_helper(struct transport *transport)
 	int res = 0;
 	struct helper_data *data = transport->data;
 	refspec_clear(&data->rs);
+	free(data->import_marks);
+	free(data->export_marks);
 	res = disconnect_helper(transport);
 	free(transport->data);
 	return res;
-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH v2 14/22] builtin/tag: fix leaking key ID on failure to sign

[Patrick Steinhardt]

We do not free the key ID when signing a tag fails. Do so by using
the common exit path.

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

diff --git a/builtin/tag.c b/builtin/tag.c
index 93d10d59157..c37c0a68fda 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -164,7 +164,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid,
 	int ret = -1;
 
 	if (sign_buffer(buffer, &sig, keyid))
-		return -1;
+		goto out;
 
 	if (compat) {
 		const struct git_hash_algo *algo = the_repository->hash_algo;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b1316e62f46..42b3327e69b 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -10,6 +10,7 @@ Tests for operations with tags.'
 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
 . "$TEST_DIRECTORY"/lib-terminal.sh
-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH v2 15/22] combine-diff: fix leaking lost lines

[Patrick Steinhardt]

The `cnt` variable tracks the number of lines in a patch diff. It can
happen though that there are no newlines, in which case we'd still end
up allocating our array of `sline`s. In fact, we always allocate it with
`cnt + 2` entries. But when we loop through the array to clear it at the
end of this function we only loop until `lno < cnt`, and thus we may not
end up releasing whatever the two extra `sline`s contain.

Plug this memory leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 combine-diff.c           | 2 +-
 t/t4038-diff-combined.sh | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/combine-diff.c b/combine-diff.c
index f6b624dc288..3c6d9507fec 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1220,7 +1220,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	}
 	free(result);
 
-	for (lno = 0; lno < cnt; lno++) {
+	for (lno = 0; lno < cnt + 2; lno++) {
 		if (sline[lno].lost) {
 			struct lline *ll = sline[lno].lost;
 			while (ll) {
diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
index 2ce26e585c9..00190802d83 100755
--- a/t/t4038-diff-combined.sh
+++ b/t/t4038-diff-combined.sh
@@ -5,6 +5,7 @@ test_description='combined diff'
 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-diff.sh
 
-- 
2.47.0.72.gef8ce8f3d4.dirty

Re: [PATCH v2 15/22] combine-diff: fix leaking lost lines

[Justin Tobler]

On 24/10/21 11:28AM, Patrick Steinhardt wrote:
> The `cnt` variable tracks the number of lines in a patch diff. It can
> happen though that there are no newlines, in which case we'd still end
> up allocating our array of `sline`s. In fact, we always allocate it with
> `cnt + 2` entries. But when we loop through the array to clear it at the
> end of this function we only loop until `lno < cnt`, and thus we may not
> end up releasing whatever the two extra `sline`s contain.
> 
> Plug this memory leak.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  combine-diff.c           | 2 +-
>  t/t4038-diff-combined.sh | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/combine-diff.c b/combine-diff.c
> index f6b624dc288..3c6d9507fec 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -1220,7 +1220,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
>  	}
>  	free(result);
>  
> -	for (lno = 0; lno < cnt; lno++) {
> +	for (lno = 0; lno < cnt + 2; lno++) {

From looking only at the code, its not very obvious to me where the "+2"
comes from. Not really worth a reroll, but it might be nice to leave a
note with some context.

>  		if (sline[lno].lost) {
>  			struct lline *ll = sline[lno].lost;
>  			while (ll) {
> diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
> index 2ce26e585c9..00190802d83 100755
> --- a/t/t4038-diff-combined.sh
> +++ b/t/t4038-diff-combined.sh
> @@ -5,6 +5,7 @@ test_description='combined diff'
>  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-diff.sh
>  
> -- 
> 2.47.0.72.gef8ce8f3d4.dirty
> 
> 

Re: [PATCH v2 15/22] combine-diff: fix leaking lost lines

[Patrick Steinhardt]

On Mon, Nov 04, 2024 at 04:43:38PM -0600, Justin Tobler wrote:
> On 24/10/21 11:28AM, Patrick Steinhardt wrote:
> > The `cnt` variable tracks the number of lines in a patch diff. It can
> > happen though that there are no newlines, in which case we'd still end
> > up allocating our array of `sline`s. In fact, we always allocate it with
> > `cnt + 2` entries. But when we loop through the array to clear it at the
> > end of this function we only loop until `lno < cnt`, and thus we may not
> > end up releasing whatever the two extra `sline`s contain.
> > 
> > Plug this memory leak.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  combine-diff.c           | 2 +-
> >  t/t4038-diff-combined.sh | 1 +
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/combine-diff.c b/combine-diff.c
> > index f6b624dc288..3c6d9507fec 100644
> > --- a/combine-diff.c
> > +++ b/combine-diff.c
> > @@ -1220,7 +1220,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
> >  	}
> >  	free(result);
> >  
> > -	for (lno = 0; lno < cnt; lno++) {
> > +	for (lno = 0; lno < cnt + 2; lno++) {
> 
> From looking only at the code, its not very obvious to me where the "+2"
> comes from. Not really worth a reroll, but it might be nice to leave a
> note with some context.

The code is quite convoluted and hard to read, agreed. In any case, we
call `CALLOC_ARRAY(sline, st_add(cnt, 2))`, and as a comment further up
mentions:

    /* Even p_lno[cnt+1] is valid -- that is for the end line number for
     * deletion hunk at the end.
     */

This explains the +1. The second +1 seems to never be populated and acts
more like a sentinel value.

I don't really think it makes a ton of sense to explain why the sline
array is overallocated when freeing it, and we already do explain it.
But I'll touch up the commit message a bit and sneak in a fix of the
above comment to be properly formatted, which also gives the necessary
context when reading the diff, only.

Patrick

[PATCH v2 16/22] dir: release untracked cache data

[Patrick Steinhardt]

There are several cases where we invalidate untracked cache directory
entries where we do not free the underlying data, but reset the number
of entries. This causes us to leak memory because `free_untracked()`
will not iterate over any potential entries which we still had in the
array.

Fix this issue by freeing old entries. The leak is exposed by t7519, but
plugging it alone does not make the whole test suite pass.

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

diff --git a/dir.c b/dir.c
index e3ddd5b5296..cb9782fa11f 100644
--- a/dir.c
+++ b/dir.c
@@ -1056,6 +1056,8 @@ static void do_invalidate_gitignore(struct untracked_cache_dir *dir)
 {
 	int i;
 	dir->valid = 0;
+	for (size_t i = 0; i < dir->untracked_nr; i++)
+		free(dir->untracked[i]);
 	dir->untracked_nr = 0;
 	for (i = 0; i < dir->dirs_nr; i++)
 		do_invalidate_gitignore(dir->dirs[i]);
@@ -1083,6 +1085,8 @@ static void invalidate_directory(struct untracked_cache *uc,
 		uc->dir_invalidated++;
 
 	dir->valid = 0;
+	for (size_t i = 0; i < dir->untracked_nr; i++)
+		free(dir->untracked[i]);
 	dir->untracked_nr = 0;
 	for (i = 0; i < dir->dirs_nr; i++)
 		dir->dirs[i]->recurse = 0;
@@ -3573,6 +3577,8 @@ static void write_one_dir(struct untracked_cache_dir *untracked,
 	 * for safety..
 	 */
 	if (!untracked->valid) {
+		for (size_t i = 0; i < untracked->untracked_nr; i++)
+			free(untracked->untracked[i]);
 		untracked->untracked_nr = 0;
 		untracked->check_only = 0;
 	}
@@ -3905,6 +3911,8 @@ static void invalidate_one_directory(struct untracked_cache *uc,
 {
 	uc->dir_invalidated++;
 	ucd->valid = 0;
+	for (size_t i = 0; i < ucd->untracked_nr; i++)
+		free(ucd->untracked[i]);
 	ucd->untracked_nr = 0;
 }
 
-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH v2 17/22] sparse-index: correctly free EWAH contents

[Patrick Steinhardt]

While we free the `fsmonitor_dirty` member of `struct index_state`, we
do not free the contents of that EWAH. Do so by using `ewah_free()`
instead of `FREE_AND_NULL()`.

This leak is exposed by t7519, but plugging it alone does not make the
test suite pass.

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

diff --git a/sparse-index.c b/sparse-index.c
index 3d7f2164e25..2107840bfc5 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -2,6 +2,7 @@
 
 #include "git-compat-util.h"
 #include "environment.h"
+#include "ewah/ewok.h"
 #include "gettext.h"
 #include "name-hash.h"
 #include "read-cache-ll.h"
@@ -242,7 +243,8 @@ int convert_to_sparse(struct index_state *istate, int flags)
 	cache_tree_update(istate, 0);
 
 	istate->fsmonitor_has_run_once = 0;
-	FREE_AND_NULL(istate->fsmonitor_dirty);
+	ewah_free(istate->fsmonitor_dirty);
+	istate->fsmonitor_dirty = NULL;
 	FREE_AND_NULL(istate->fsmonitor_last_update);
 
 	istate->sparse_index = INDEX_COLLAPSED;
@@ -438,7 +440,8 @@ void expand_index(struct index_state *istate, struct pattern_list *pl)
 	istate->cache_nr = full->cache_nr;
 	istate->cache_alloc = full->cache_alloc;
 	istate->fsmonitor_has_run_once = 0;
-	FREE_AND_NULL(istate->fsmonitor_dirty);
+	ewah_free(istate->fsmonitor_dirty);
+	istate->fsmonitor_dirty = NULL;
 	FREE_AND_NULL(istate->fsmonitor_last_update);
 
 	strbuf_release(&base);
-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH v2 18/22] t/helper: stop re-initialization of `the_repository`

[Patrick Steinhardt]

While "common-main.c" already initializes `the_repository` for us, we do
so a second time in the "read-cache" test helper. This causes a memory
leak because the old repository's contents isn't released.

Stop calling `initialize_repository()` to plug this leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/helper/test-read-cache.c  | 2 --
 t/t7519-status-fsmonitor.sh | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
index d285c656bd3..e277dde8e71 100644
--- a/t/helper/test-read-cache.c
+++ b/t/helper/test-read-cache.c
@@ -11,8 +11,6 @@ int cmd__read_cache(int argc, const char **argv)
 	int i, cnt = 1;
 	const char *name = NULL;
 
-	initialize_repository(the_repository);
-
 	if (argc > 1 && skip_prefix(argv[1], "--print-and-refresh=", &name)) {
 		argc--;
 		argv++;
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 7ee69ecdd4a..0f88a58a819 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -2,6 +2,7 @@
 
 test_description='git status with file system watcher'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Note, after "git reset --hard HEAD" no extensions exist other than 'TREE'
-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH v2 19/22] t/helper: fix leaking buffer in "dump-untracked-cache"

[Patrick Steinhardt]

We never release the local `struct strbuf base` buffer, thus leaking
memory. Fix this leak.

This leak is exposed by t7063, but plugging it alone does not make the
whole test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/helper/test-dump-untracked-cache.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c
index 4f010d53249..b2e70837a90 100644
--- a/t/helper/test-dump-untracked-cache.c
+++ b/t/helper/test-dump-untracked-cache.c
@@ -68,5 +68,7 @@ int cmd__dump_untracked_cache(int ac UNUSED, const char **av UNUSED)
 	printf("flags %08x\n", uc->dir_flags);
 	if (uc->root)
 		dump(uc->root, &base);
+
+	strbuf_release(&base);
 	return 0;
 }
-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH v2 20/22] dir: fix leak when parsing "status.showUntrackedFiles"

[Patrick Steinhardt]

We use `repo_config_get_string()` to read "status.showUntrackedFiles"
from the config subsystem. This function allocates the result, but we
never free the result after parsing it.

The value never leaves the scope of the calling function, so refactor it
to instead use `repo_config_get_string_tmp()`, which does not hand over
ownership to the caller.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 dir.c                             | 4 ++--
 t/t7063-status-untracked-cache.sh | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index cb9782fa11f..7f35a3e3175 100644
--- a/dir.c
+++ b/dir.c
@@ -2872,14 +2872,14 @@ static void set_untracked_ident(struct untracked_cache *uc)
 static unsigned new_untracked_cache_flags(struct index_state *istate)
 {
 	struct repository *repo = istate->repo;
-	char *val;
+	const char *val;
 
 	/*
 	 * This logic is coordinated with the setting of these flags in
 	 * wt-status.c#wt_status_collect_untracked(), and the evaluation
 	 * of the config setting in commit.c#git_status_config()
 	 */
-	if (!repo_config_get_string(repo, "status.showuntrackedfiles", &val) &&
+	if (!repo_config_get_string_tmp(repo, "status.showuntrackedfiles", &val) &&
 	    !strcmp(val, "all"))
 		return 0;
 
diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index 8929ef481f9..13fea7931cd 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -5,6 +5,7 @@ test_description='test untracked cache'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # On some filesystems (e.g. FreeBSD's ext2 and ufs) directory mtime
-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH v2 21/22] builtin/merge: release outbut buffer after performing merge

[Patrick Steinhardt]

The `obuf` member of `struct merge_options` is used to buffer output in
some cases. In order to not discard its allocated memory we only release
its contents in `merge_finalize()` when we're not currently recursing
into a subtree.

This results in some situations where we seemingly do not release the
buffer reliably. We thus have calls to `strbuf_release()` for this
buffer scattered across the codebase. But we're missing one callsite in
git-merge(1), which causes a memory leak.

We should ideally refactor this interface so that callers don't have to
know about any such internals. But for now, paper over the issue by
adding one more `strbuf_release()` call.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/merge.c                          | 1 +
 t/t6424-merge-unrelated-index-changes.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index 84d0f3604bc..51038eaca84 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -754,6 +754,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 			clean = merge_recursive(&o, head, remoteheads->item,
 						reversed, &result);
 		free_commit_list(reversed);
+		strbuf_release(&o.obuf);
 
 		if (clean < 0) {
 			rollback_lock_file(&lock);
diff --git a/t/t6424-merge-unrelated-index-changes.sh b/t/t6424-merge-unrelated-index-changes.sh
index 7677c5f08d0..a7ea8acb845 100755
--- a/t/t6424-merge-unrelated-index-changes.sh
+++ b/t/t6424-merge-unrelated-index-changes.sh
@@ -2,6 +2,7 @@
 
 test_description="merges with unrelated index changes"
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Testcase for some simple merges
-- 
2.47.0.72.gef8ce8f3d4.dirty

Re: [PATCH v2 21/22] builtin/merge: release outbut buffer after performing merge

[Kristoffer Haugsbakk]

> Re: [PATCH v2 21/22] builtin/merge: release outbut buffer after performing merge

s/outbut/output/

[PATCH v2 22/22] list-objects-filter-options: work around reported leak on error

[Patrick Steinhardt]

This one is a little bit more curious. In t6112, we have a test that
exercises the `git rev-list --filter` option with invalid filters. We
execute git-rev-list(1) via `test_must_fail`, which means that we check
for leaks even though Git exits with an error code. This causes the
following leak:

    Direct leak of 27 byte(s) in 1 object(s) allocated from:
        #0 0x5555555e6946 in realloc.part.0 lsan_interceptors.cpp.o
        #1 0x5555558fb4b6 in xrealloc wrapper.c:137:8
        #2 0x5555558b6e06 in strbuf_grow strbuf.c:112:2
        #3 0x5555558b7550 in strbuf_add strbuf.c:311:2
        #4 0x5555557c1a88 in strbuf_addstr strbuf.h:310:2
        #5 0x5555557c1d4c in parse_list_objects_filter list-objects-filter-options.c:261:3
        #6 0x555555885ead in handle_revision_pseudo_opt revision.c:2899:3
        #7 0x555555884e20 in setup_revisions revision.c:3014:11
        #8 0x5555556c4b42 in cmd_rev_list builtin/rev-list.c:588:9
        #9 0x5555555ec5e3 in run_builtin git.c:483:11
        #10 0x5555555eb1e4 in handle_builtin git.c:749:13
        #11 0x5555555ec001 in run_argv git.c:819:4
        #12 0x5555555eaf94 in cmd_main git.c:954:19
        #13 0x5555556fd569 in main common-main.c:64:11
        #14 0x7ffff7ca714d in __libc_start_call_main (.../lib/libc.so.6+0x2a14d)
        #15 0x7ffff7ca7208 in __libc_start_main@GLIBC_2.2.5 (.../libc.so.6+0x2a208)
        #16 0x5555555ad064 in _start (git+0x59064)

This leak is valid, as we call `die()` and do not clean up the memory at
all. But what's curious is that this is the only leak reported, because
we don't clean up any other allocated memory, either, and I have no idea
why the leak sanitizer treats this buffer specially.

In any case, we can work around the leak by shuffling things around a
bit. Instead of calling `gently_parse_list_objects_filter()` and dying
after we have modified the filter spec, we simply do so beforehand. Like
this we don't allocate the buffer in the error case, which makes the
reported leak go away.

It's not pretty, but it manages to make t6112 leak free.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 list-objects-filter-options.c       | 17 +++++++----------
 t/t6112-rev-list-filters-objects.sh |  1 +
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 00611107d20..fa72e81e4ad 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -252,16 +252,14 @@ void parse_list_objects_filter(
 	const char *arg)
 {
 	struct strbuf errbuf = STRBUF_INIT;
-	int parse_error;
 
 	if (!filter_options->filter_spec.buf)
 		BUG("filter_options not properly initialized");
 
 	if (!filter_options->choice) {
+		if (gently_parse_list_objects_filter(filter_options, arg, &errbuf))
+			die("%s", errbuf.buf);
 		strbuf_addstr(&filter_options->filter_spec, arg);
-
-		parse_error = gently_parse_list_objects_filter(
-			filter_options, arg, &errbuf);
 	} else {
 		struct list_objects_filter_options *sub;
 
@@ -271,18 +269,17 @@ void parse_list_objects_filter(
 		 */
 		transform_to_combine_type(filter_options);
 
-		strbuf_addch(&filter_options->filter_spec, '+');
-		filter_spec_append_urlencode(filter_options, arg);
 		ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,
 			      filter_options->sub_alloc);
 		sub = &filter_options->sub[filter_options->sub_nr - 1];
 
 		list_objects_filter_init(sub);
-		parse_error = gently_parse_list_objects_filter(sub, arg,
-							       &errbuf);
+		if (gently_parse_list_objects_filter(sub, arg, &errbuf))
+			die("%s", errbuf.buf);
+
+		strbuf_addch(&filter_options->filter_spec, '+');
+		filter_spec_append_urlencode(filter_options, arg);
 	}
-	if (parse_error)
-		die("%s", errbuf.buf);
 }
 
 int opt_parse_list_objects_filter(const struct option *opt,
diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh
index 0387f35a326..71e38491fa8 100755
--- a/t/t6112-rev-list-filters-objects.sh
+++ b/t/t6112-rev-list-filters-objects.sh
@@ -5,6 +5,7 @@ test_description='git rev-list using object filtering'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Test the blob:none filter.
-- 
2.47.0.72.gef8ce8f3d4.dirty

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

[Kristoffer Haugsbakk]

On Mon, Oct 21, 2024, at 11:27, Patrick Steinhardt wrote:
> Hi,
>
> this is the second version of my 9th series of memory leak fixes.

I can’t even imagine the amount of effort it takes to plug all these
leaks in parallel with the stuff that everyone else is doing in the
code.  Nice work.  :)

Of course all the comments that I left just now are nitpicks.

-- 
Kristoffer Haugsbakk

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

[Patrick Steinhardt]

On Mon, Oct 21, 2024 at 11:54:52AM +0200, Kristoffer Haugsbakk wrote:
> On Mon, Oct 21, 2024, at 11:27, Patrick Steinhardt wrote:
> > Hi,
> >
> > this is the second version of my 9th series of memory leak fixes.
> 
> I can’t even imagine the amount of effort it takes to plug all these
> leaks in parallel with the stuff that everyone else is doing in the
> code.  Nice work.  :)
> 
> Of course all the comments that I left just now are nitpicks.

Thanks! I've queued all fixes locally, but will wait a bit before
sending them out so that other reviewers have a chance to chime in
first.

Patrick

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

[Toon Claes]

Patrick Steinhardt <ps@pks.im> writes:

> Thanks! I've queued all fixes locally, but will wait a bit before
> sending them out so that other reviewers have a chance to chime in
> first.

I've got nothing more to add.

--
Toon

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

[Justin Tobler]

On 24/10/21 11:27AM, Patrick Steinhardt wrote:
> Hi,
> 
> this is the second version of my 9th series of memory leak fixes.
> 
> Changes compared to v1:
> 
>   - Split up the trailer fixes into two separate patches so that we can
>     explain them standalone.
> 
>   - Adapt the second trailer memory leak fix to instead pull up the
>     strbufs out of the loop such that we can reuse them. This makes the
>     code flow more naturally and optimizes the loop.
> 
> Thanks!
> 
> Patrick

I've reviewed all the patches in this version and left a few
non-blocking thoughts. Overall, this version looks good to me though. :)

-Justin

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

[Patrick Steinhardt]

On Mon, Nov 04, 2024 at 04:46:57PM -0600, Justin Tobler wrote:
> On 24/10/21 11:27AM, Patrick Steinhardt wrote:
> > Hi,
> > 
> > this is the second version of my 9th series of memory leak fixes.
> > 
> > Changes compared to v1:
> > 
> >   - Split up the trailer fixes into two separate patches so that we can
> >     explain them standalone.
> > 
> >   - Adapt the second trailer memory leak fix to instead pull up the
> >     strbufs out of the loop such that we can reuse them. This makes the
> >     code flow more naturally and optimizes the loop.
> > 
> > Thanks!
> > 
> > Patrick
> 
> I've reviewed all the patches in this version and left a few
> non-blocking thoughts. Overall, this version looks good to me though. :)

Thanks for your review!

Patrick

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

[Patrick Steinhardt]

Hi,

this is the third revision of my 9th series of memory leak fixes.
Changes compared to v2:

  - Remove an unnecessary cast.

  - Fix a duplicate newline.

  - Polish a couple of commit messages.

Thanks!

Patrick

Patrick Steinhardt (22):
  builtin/ls-remote: plug leaking server options
  t/helper: fix leaks in "reach" test tool
  grep: fix leak in `grep_splice_or()`
  builtin/grep: fix leak with `--max-count=0`
  revision: fix leaking bloom filters
  diff-lib: fix leaking diffopts in `do_diff_cache()`
  pretty: clear signature check
  upload-pack: fix leaking URI protocols
  builtin/commit: fix leaking change data contents
  trailer: fix leaking trailer values
  trailer: fix leaking strbufs when formatting trailers
  builtin/commit: fix leaking cleanup config
  transport-helper: fix leaking import/export marks
  builtin/tag: fix leaking key ID on failure to sign
  combine-diff: fix leaking lost lines
  dir: release untracked cache data
  sparse-index: correctly free EWAH contents
  t/helper: stop re-initialization of `the_repository`
  t/helper: fix leaking buffer in "dump-untracked-cache"
  dir: fix leak when parsing "status.showUntrackedFiles"
  builtin/merge: release output buffer after performing merge
  list-objects-filter-options: work around reported leak on error

 builtin/commit.c                          | 26 +++++++++++++++++------
 builtin/grep.c                            | 13 +++++++++---
 builtin/ls-remote.c                       |  1 +
 builtin/merge.c                           |  1 +
 builtin/tag.c                             |  2 +-
 combine-diff.c                            |  5 +++--
 diff-lib.c                                |  1 +
 dir.c                                     | 12 +++++++++--
 grep.c                                    |  1 +
 list-objects-filter-options.c             | 17 ++++++---------
 pretty.c                                  |  1 +
 revision.c                                |  5 +++++
 sparse-index.c                            |  7 ++++--
 t/helper/test-dump-untracked-cache.c      |  2 ++
 t/helper/test-reach.c                     | 10 +++++++++
 t/helper/test-read-cache.c                |  2 --
 t/t4038-diff-combined.sh                  |  1 +
 t/t4202-log.sh                            |  1 +
 t/t4216-log-bloom.sh                      |  1 +
 t/t5702-protocol-v2.sh                    |  1 +
 t/t5801-remote-helpers.sh                 |  1 +
 t/t6112-rev-list-filters-objects.sh       |  1 +
 t/t6424-merge-unrelated-index-changes.sh  |  1 +
 t/t6600-test-reach.sh                     |  1 +
 t/t7004-tag.sh                            |  1 +
 t/t7031-verify-tag-signed-ssh.sh          |  1 +
 t/t7063-status-untracked-cache.sh         |  1 +
 t/t7500-commit-template-squash-signoff.sh |  1 +
 t/t7502-commit-porcelain.sh               |  1 +
 t/t7510-signed-commit.sh                  |  1 +
 t/t7513-interpret-trailers.sh             |  1 +
 t/t7519-status-fsmonitor.sh               |  1 +
 t/t7528-signed-commit-ssh.sh              |  1 +
 t/t7610-mergetool.sh                      |  1 +
 t/t7810-grep.sh                           |  1 +
 trailer.c                                 | 22 +++++++++++++------
 transport-helper.c                        |  2 ++
 upload-pack.c                             |  1 +
 38 files changed, 115 insertions(+), 35 deletions(-)

Range-diff against v2:
 1:  89b66411354 !  1:  fbb9e68e2f2 builtin/ls-remote: plug leaking server options
    @@ Metadata
      ## Commit message ##
         builtin/ls-remote: plug leaking server options
     
    -    The server options populated via `OPT_STRING_LIST()` is never cleared,
    -    causing a memory leak. Plug it.
    +    The list of server options populated via `OPT_STRING_LIST()` is never
    +    cleared, causing a memory leak. Plug it.
     
         This leak is exposed by t5702, but plugging it alone does not make the
         whole test suite pass.
 2:  1c42e194b20 =  2:  17136f4bdb3 t/helper: fix leaks in "reach" test tool
 3:  cb4eee37b40 !  3:  74b21470194 grep: fix leak in `grep_splice_or()`
    @@ Commit message
         grep: fix leak in `grep_splice_or()`
     
         In `grep_splice_or()` we search for the next `TRUE` node in our tree of
    -    grep exrpessions and replace it with the given new expression. But we
    +    grep expressions and replace it with the given new expression. But we
         don't free the old node, which causes a memory leak. Plug it.
     
         This leak is exposed by t7810, but plugging it alone isn't sufficient to
 4:  6b2c8842ef5 =  4:  d716f93169a builtin/grep: fix leak with `--max-count=0`
 5:  7527b31a28f =  5:  aeb8a19d28d revision: fix leaking bloom filters
 6:  60af98cb2c7 !  6:  24d9d9b1358 diff-lib: fix leaking diffopts in `do_diff_cache()`
    @@ Commit message
         In `do_diff_cache()` we initialize a new `rev_info` and then overwrite
         its `diffopt` with a user-provided set of options. This can leak memory
         because `repo_init_revisions()` may end up allocating memory for the
    -    `diffopt` itself depending on the configuration. And as that field is
    -    overwritten we won't ever free that.
    +    `diffopt` itself depending on the configuration. And since that field is
    +    overwritten we won't ever free it.
     
         Plug the memory leak by releasing the diffopts before we overwrite them.
     
 7:  5d5f6867f91 !  7:  58ebef7e757 pretty: clear signature check
    @@ Metadata
      ## Commit message ##
         pretty: clear signature check
     
    -    The signature check in of the formatting context is never getting
    -    released. Fix this to plug the resulting memory leak.
    +    The signature check in the formatting context is never getting released.
    +    Fix this to plug the resulting memory leak.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
 8:  0d503e40194 =  8:  c5813079d44 upload-pack: fix leaking URI protocols
 9:  9f967dfe5d5 =  9:  f66fa4e0e25 builtin/commit: fix leaking change data contents
10:  e3ffd59123f ! 10:  dac63bae39e trailer: fix leaking trailer values
    @@ trailer.c: static char *apply_command(struct conf_info *conf, const char *arg)
     +		char *arg;
     +
      		if (arg_tok->value && arg_tok->value[0]) {
    --			arg = arg_tok->value;
    -+			arg = (char *)arg_tok->value;
    + 			arg = arg_tok->value;
      		} else {
    - 			if (in_tok && in_tok->value)
    +@@ trailer.c: static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg
      				arg = xstrdup(in_tok->value);
      			else
      				arg = xstrdup("");
11:  5b851453bce ! 11:  82269e5d5be trailer: fix leaking strbufs when formatting trailers
    @@ Metadata
      ## Commit message ##
         trailer: fix leaking strbufs when formatting trailers
     
    -    We are populating, but never releasing two string buffers in
    -    `format_trailers()`, causing a memory leak. Plug this leak by lifting
    -    those buffers outside of the loop and releasing them on function return.
    -    This fixes the memory leaks, but also optimizes the loop as we don't
    -    have to reallocate the buffers on every single iteration.
    +    When formatting trailer lines we iterate through each of the trailers
    +    and munge their respective token/value pairs according to the trailer
    +    options. When formatting a trailer that has its `item->token` pointer
    +    set we perform the munging in two local buffers. In the case where we
    +    figure out that the value is empty and `trim_empty` is set we just skip
    +    over the trailer item. But the buffers are local to the loop and we
    +    don't release their contents, leading to a memory leak.
    +
    +    Plug this leak by lifting the buffers outside of the loop and releasing
    +    them on function return. This fixes the memory leaks, but also optimizes
    +    the loop as we don't have to reallocate the buffers on every single
    +    iteration.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ trailer.c: void format_trailers(const struct process_trailer_options *opts,
      	size_t origlen = out->len;
      	struct list_head *pos;
      	struct trailer_item *item;
    - 
    -+
    +@@ trailer.c: void format_trailers(const struct process_trailer_options *opts,
      	list_for_each(pos, trailers) {
      		item = list_entry(pos, struct trailer_item, list);
      		if (item->token) {
12:  60c3f6146f3 = 12:  a627e03702e builtin/commit: fix leaking cleanup config
13:  ea81cd8db86 = 13:  40e0c2a2cac transport-helper: fix leaking import/export marks
14:  b700ab9079f = 14:  ffa5d9eae7e builtin/tag: fix leaking key ID on failure to sign
15:  76bbcb3fe30 ! 15:  70dd0cb6933 combine-diff: fix leaking lost lines
    @@ Commit message
         The `cnt` variable tracks the number of lines in a patch diff. It can
         happen though that there are no newlines, in which case we'd still end
         up allocating our array of `sline`s. In fact, we always allocate it with
    -    `cnt + 2` entries. But when we loop through the array to clear it at the
    -    end of this function we only loop until `lno < cnt`, and thus we may not
    -    end up releasing whatever the two extra `sline`s contain.
    +    `cnt + 2` entries: one extra entry for the deletion hunk at the end, and
    +    another entry that we don't seem to ever populate at all but acts as a
    +    kind of sentinel value.
     
    -    Plug this memory leak.
    +    When we loop through the array to clear it at the end of this function
    +    we only loop until `lno < cnt`, and thus we may not end up releasing
    +    whatever the two extra `sline`s contain. While that shouldn't matter for
    +    the sentinel value, it does matter for the extra deletion hunk sline.
    +    Regardless of that, plug this memory leak by releasing both extra
    +    entries, which makes the logic a bit easier to reason about.
    +
    +    While at it, fix the formatting of a local comment, which incidentally
    +    also provides the necessary context for why we overallocate the `sline`
    +    array.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## combine-diff.c ##
    +@@ combine-diff.c: static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
    + 	result_file.ptr = result;
    + 	result_file.size = result_size;
    + 
    +-	/* Even p_lno[cnt+1] is valid -- that is for the end line number
    ++	/*
    ++	 * Even p_lno[cnt+1] is valid -- that is for the end line number
    + 	 * for deletion hunk at the end.
    + 	 */
    + 	CALLOC_ARRAY(sline[0].p_lno, st_mult(st_add(cnt, 2), num_parent));
     @@ combine-diff.c: static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
      	}
      	free(result);
16:  d6b96a4012d = 16:  b248f266a91 dir: release untracked cache data
17:  3aa6bac5079 = 17:  76e9a6d5792 sparse-index: correctly free EWAH contents
18:  132fe750906 = 18:  70f16486d77 t/helper: stop re-initialization of `the_repository`
19:  b8b702eeb28 = 19:  f056d31ca50 t/helper: fix leaking buffer in "dump-untracked-cache"
20:  ad309ac1b37 = 20:  714c9286e7a dir: fix leak when parsing "status.showUntrackedFiles"
21:  5e243f9ee53 ! 21:  0ff65c1213b builtin/merge: release outbut buffer after performing merge
    @@ Metadata
     Author: Patrick Steinhardt <ps@pks.im>
     
      ## Commit message ##
    -    builtin/merge: release outbut buffer after performing merge
    +    builtin/merge: release output buffer after performing merge
     
         The `obuf` member of `struct merge_options` is used to buffer output in
         some cases. In order to not discard its allocated memory we only release
22:  b75376e3725 = 22:  d9e122bb5db list-objects-filter-options: work around reported leak on error
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 01/22] builtin/ls-remote: plug leaking server options

[Patrick Steinhardt]

The list of server options populated via `OPT_STRING_LIST()` is never
cleared, causing a memory leak. Plug it.

This leak is exposed by t5702, but plugging it alone does not make the
whole test suite pass.

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

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index f723b3bf3bb..f333821b994 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -166,6 +166,7 @@ int cmd_ls_remote(int argc,
 		status = 0; /* we found something */
 	}
 
+	string_list_clear(&server_options, 0);
 	ref_sorting_release(sorting);
 	ref_array_clear(&ref_array);
 	if (transport_disconnect(transport))
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 02/22] t/helper: fix leaks in "reach" test tool

[Patrick Steinhardt]

The "reach" test tool doesn't bother to clean up any of its allocated
resources, causing various leaks. Plug them.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/helper/test-reach.c | 10 ++++++++++
 t/t6600-test-reach.sh |  1 +
 2 files changed, 11 insertions(+)

diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c
index 995e382863a..84deee604ad 100644
--- a/t/helper/test-reach.c
+++ b/t/helper/test-reach.c
@@ -127,10 +127,12 @@ int cmd__reach(int ac, const char **av)
 			exit(128);
 		printf("%s(A,X):\n", av[1]);
 		print_sorted_commit_ids(list);
+		free_commit_list(list);
 	} else if (!strcmp(av[1], "reduce_heads")) {
 		struct commit_list *list = reduce_heads(X);
 		printf("%s(X):\n", av[1]);
 		print_sorted_commit_ids(list);
+		free_commit_list(list);
 	} else if (!strcmp(av[1], "can_all_from_reach")) {
 		printf("%s(X,Y):%d\n", av[1], can_all_from_reach(X, Y, 1));
 	} else if (!strcmp(av[1], "can_all_from_reach_with_flag")) {
@@ -153,6 +155,7 @@ int cmd__reach(int ac, const char **av)
 			filter.with_commit_tag_algo = 0;
 
 		printf("%s(_,A,X,_):%d\n", av[1], commit_contains(&filter, A, X, &cache));
+		clear_contains_cache(&cache);
 	} else if (!strcmp(av[1], "get_reachable_subset")) {
 		const int reachable_flag = 1;
 		int i, count = 0;
@@ -176,7 +179,14 @@ int cmd__reach(int ac, const char **av)
 			die(_("too many commits marked reachable"));
 
 		print_sorted_commit_ids(list);
+		free_commit_list(list);
 	}
 
+	object_array_clear(&X_obj);
+	strbuf_release(&buf);
+	free_commit_list(X);
+	free_commit_list(Y);
+	free(X_array);
+	free(Y_array);
 	return 0;
 }
diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
index 2591f8b8b39..307deefed2c 100755
--- a/t/t6600-test-reach.sh
+++ b/t/t6600-test-reach.sh
@@ -2,6 +2,7 @@
 
 test_description='basic commit reachability tests'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Construct a grid-like commit graph with points (x,y)
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 03/22] grep: fix leak in `grep_splice_or()`

[Patrick Steinhardt]

In `grep_splice_or()` we search for the next `TRUE` node in our tree of
grep expressions and replace it with the given new expression. But we
don't free the old node, which causes a memory leak. Plug it.

This leak is exposed by t7810, but plugging it alone isn't sufficient to
make the test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grep.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/grep.c b/grep.c
index 701e58de04e..e9337f32cbf 100644
--- a/grep.c
+++ b/grep.c
@@ -756,6 +756,7 @@ static struct grep_expr *grep_splice_or(struct grep_expr *x, struct grep_expr *y
 		assert(x->node == GREP_NODE_OR);
 		if (x->u.binary.right &&
 		    x->u.binary.right->node == GREP_NODE_TRUE) {
+			free(x->u.binary.right);
 			x->u.binary.right = y;
 			break;
 		}
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 04/22] builtin/grep: fix leak with `--max-count=0`

[Patrick Steinhardt]

When executing with `--max-count=0` we'll return early from git-grep(1)
without performing any cleanup, which causes memory leaks. Plug these.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/grep.c  | 13 ++++++++++---
 t/t7810-grep.sh |  1 +
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index f17d46a06e4..98b85c7fcac 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -906,6 +906,7 @@ int cmd_grep(int argc,
 	int dummy;
 	int use_index = 1;
 	int allow_revs;
+	int ret;
 
 	struct option options[] = {
 		OPT_BOOL(0, "cached", &cached,
@@ -1172,8 +1173,10 @@ int cmd_grep(int argc,
 	 * Optimize out the case where the amount of matches is limited to zero.
 	 * We do this to keep results consistent with GNU grep(1).
 	 */
-	if (opt.max_count == 0)
-		return 1;
+	if (opt.max_count == 0) {
+		ret = 1;
+		goto out;
+	}
 
 	if (show_in_pager) {
 		if (num_threads > 1)
@@ -1267,10 +1270,14 @@ int cmd_grep(int argc,
 		hit |= wait_all();
 	if (hit && show_in_pager)
 		run_pager(&opt, prefix);
+
+	ret = !hit;
+
+out:
 	clear_pathspec(&pathspec);
 	string_list_clear(&path_list, 0);
 	free_grep_patterns(&opt);
 	object_array_clear(&list);
 	free_repos();
-	return !hit;
+	return ret;
 }
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index af2cf2f78ab..9e7681f0831 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -9,6 +9,7 @@ test_description='git grep various.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_invalid_grep_expression() {
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 05/22] revision: fix leaking bloom filters

[Patrick Steinhardt]

The memory allocated by `prepare_to_use_bloom_filter()` is not released
by `release_revisions()`, causing a memory leak. Plug it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 revision.c           | 5 +++++
 t/t4216-log-bloom.sh | 1 +
 2 files changed, 6 insertions(+)

diff --git a/revision.c b/revision.c
index f5f5b84f2b0..8df75b82249 100644
--- a/revision.c
+++ b/revision.c
@@ -3227,6 +3227,11 @@ void release_revisions(struct rev_info *revs)
 	clear_decoration(&revs->treesame, free);
 	line_log_free(revs);
 	oidset_clear(&revs->missing_commits);
+
+	for (int i = 0; i < revs->bloom_keys_nr; i++)
+		clear_bloom_key(&revs->bloom_keys[i]);
+	FREE_AND_NULL(revs->bloom_keys);
+	revs->bloom_keys_nr = 0;
 }
 
 static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)
diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index 3f163dc3969..8d22338f6aa 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -4,6 +4,7 @@ test_description='git log for a path with Bloom filters'
 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-chunk.sh
 
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 06/22] diff-lib: fix leaking diffopts in `do_diff_cache()`

[Patrick Steinhardt]

In `do_diff_cache()` we initialize a new `rev_info` and then overwrite
its `diffopt` with a user-provided set of options. This can leak memory
because `repo_init_revisions()` may end up allocating memory for the
`diffopt` itself depending on the configuration. And since that field is
overwritten we won't ever free it.

Plug the memory leak by releasing the diffopts before we overwrite them.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 diff-lib.c           | 1 +
 t/t7610-mergetool.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/diff-lib.c b/diff-lib.c
index 6b14b959629..3cf353946f5 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -661,6 +661,7 @@ int do_diff_cache(const struct object_id *tree_oid, struct diff_options *opt)
 
 	repo_init_revisions(opt->repo, &revs, NULL);
 	copy_pathspec(&revs.prune_data, &opt->pathspec);
+	diff_free(&revs.diffopt);
 	revs.diffopt = *opt;
 	revs.diffopt.no_free = 1;
 
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 22b3a85b3e9..5c5e79e9905 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -10,6 +10,7 @@ Testing basic merge tool invocation'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # All the mergetool test work by checking out a temporary branch based
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 07/22] pretty: clear signature check

[Patrick Steinhardt]

The signature check in the formatting context is never getting released.
Fix this to plug the resulting memory leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 pretty.c                         | 1 +
 t/t4202-log.sh                   | 1 +
 t/t7031-verify-tag-signed-ssh.sh | 1 +
 t/t7510-signed-commit.sh         | 1 +
 t/t7528-signed-commit-ssh.sh     | 1 +
 5 files changed, 5 insertions(+)

diff --git a/pretty.c b/pretty.c
index 6403e268900..098378720a4 100644
--- a/pretty.c
+++ b/pretty.c
@@ -2032,6 +2032,7 @@ void repo_format_commit_message(struct repository *r,
 
 	free(context.commit_encoding);
 	repo_unuse_commit_buffer(r, commit, context.message);
+	signature_check_clear(&context.signature_check);
 }
 
 static void pp_header(struct pretty_print_context *pp,
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 51f7beb59f8..35bec4089a3 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -5,6 +5,7 @@ test_description='git log'
 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"
 . "$TEST_DIRECTORY/lib-terminal.sh"
diff --git a/t/t7031-verify-tag-signed-ssh.sh b/t/t7031-verify-tag-signed-ssh.sh
index 20913b37134..2ee62c07293 100755
--- a/t/t7031-verify-tag-signed-ssh.sh
+++ b/t/t7031-verify-tag-signed-ssh.sh
@@ -4,6 +4,7 @@ test_description='signed tag tests'
 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"
 
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 0d2dd29fe6a..eb229082e40 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -4,6 +4,7 @@ test_description='signed commit tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 GNUPGHOME_NOT_USED=$GNUPGHOME
 . "$TEST_DIRECTORY/lib-gpg.sh"
diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh
index 065f7806362..68e18856b66 100755
--- a/t/t7528-signed-commit-ssh.sh
+++ b/t/t7528-signed-commit-ssh.sh
@@ -4,6 +4,7 @@ test_description='ssh signed commit tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 GNUPGHOME_NOT_USED=$GNUPGHOME
 . "$TEST_DIRECTORY/lib-gpg.sh"
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 08/22] upload-pack: fix leaking URI protocols

[Patrick Steinhardt]

We don't clear `struct upload_pack::uri_protocols`, which causes a
memory leak. Fix this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5702-protocol-v2.sh | 1 +
 upload-pack.c          | 1 +
 2 files changed, 2 insertions(+)

diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 1ef540f73d3..ed55a2f7f95 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -7,6 +7,7 @@ TEST_NO_CREATE_REPO=1
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Test protocol v2 with 'git://' transport
diff --git a/upload-pack.c b/upload-pack.c
index 6d6e0f9f980..b4a59c3518b 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -166,6 +166,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data)
 	object_array_clear(&data->extra_edge_obj);
 	list_objects_filter_release(&data->filter_options);
 	string_list_clear(&data->allowed_filters, 0);
+	string_list_clear(&data->uri_protocols, 0);
 
 	free((char *)data->pack_objects_hook);
 }
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 09/22] builtin/commit: fix leaking change data contents

[Patrick Steinhardt]

While we free the worktree change data, we never free its contents. Fix
this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/commit.c                          | 9 ++++++++-
 t/t7500-commit-template-squash-signoff.sh | 1 +
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 8db4e9df0c9..18a55bd1b91 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -728,6 +728,13 @@ static void prepare_amend_commit(struct commit *commit, struct strbuf *sb,
 	repo_unuse_commit_buffer(the_repository, commit, buffer);
 }
 
+static void change_data_free(void *util, const char *str UNUSED)
+{
+	struct wt_status_change_data *d = util;
+	free(d->rename_source);
+	free(d);
+}
+
 static int prepare_to_commit(const char *index_file, const char *prefix,
 			     struct commit *current_head,
 			     struct wt_status *s,
@@ -991,7 +998,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		s->use_color = 0;
 		committable = run_status(s->fp, index_file, prefix, 1, s);
 		s->use_color = saved_color_setting;
-		string_list_clear(&s->change, 1);
+		string_list_clear_func(&s->change, change_data_free);
 	} else {
 		struct object_id oid;
 		const char *parent = "HEAD";
diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 4dca8d97a77..379d3ed3413 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -7,6 +7,7 @@ test_description='git commit
 
 Tests for template, signoff, squash and -F functions.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-rebase.sh
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 10/22] trailer: fix leaking trailer values

[Patrick Steinhardt]

Fix leaking trailer values when replacing the value with a command or
when the token value is empty.

This leak is exposed by t7513, but plugging it does not make the whole
test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 trailer.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/trailer.c b/trailer.c
index 682d74505bf..6bafe92b326 100644
--- a/trailer.c
+++ b/trailer.c
@@ -249,7 +249,9 @@ static char *apply_command(struct conf_info *conf, const char *arg)
 static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok)
 {
 	if (arg_tok->conf.command || arg_tok->conf.cmd) {
-		const char *arg;
+		char *value_to_free = NULL;
+		char *arg;
+
 		if (arg_tok->value && arg_tok->value[0]) {
 			arg = arg_tok->value;
 		} else {
@@ -257,9 +259,13 @@ static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg
 				arg = xstrdup(in_tok->value);
 			else
 				arg = xstrdup("");
+			value_to_free = arg_tok->value;
 		}
+
 		arg_tok->value = apply_command(&arg_tok->conf, arg);
-		free((char *)arg);
+
+		free(value_to_free);
+		free(arg);
 	}
 }
 
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 11/22] trailer: fix leaking strbufs when formatting trailers

[Patrick Steinhardt]

When formatting trailer lines we iterate through each of the trailers
and munge their respective token/value pairs according to the trailer
options. When formatting a trailer that has its `item->token` pointer
set we perform the munging in two local buffers. In the case where we
figure out that the value is empty and `trim_empty` is set we just skip
over the trailer item. But the buffers are local to the loop and we
don't release their contents, leading to a memory leak.

Plug this leak by lifting the buffers outside of the loop and releasing
them on function return. This fixes the memory leaks, but also optimizes
the loop as we don't have to reallocate the buffers on every single
iteration.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t7513-interpret-trailers.sh |  1 +
 trailer.c                     | 12 +++++++-----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 0f7d8938d98..38d6ccaa001 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -5,6 +5,7 @@
 
 test_description='git interpret-trailers'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # When we want one trailing space at the end of each line, let's use sed
diff --git a/trailer.c b/trailer.c
index 6bafe92b326..8ba350404d4 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1111,6 +1111,8 @@ void format_trailers(const struct process_trailer_options *opts,
 		     struct list_head *trailers,
 		     struct strbuf *out)
 {
+	struct strbuf tok = STRBUF_INIT;
+	struct strbuf val = STRBUF_INIT;
 	size_t origlen = out->len;
 	struct list_head *pos;
 	struct trailer_item *item;
@@ -1118,9 +1120,9 @@ void format_trailers(const struct process_trailer_options *opts,
 	list_for_each(pos, trailers) {
 		item = list_entry(pos, struct trailer_item, list);
 		if (item->token) {
-			struct strbuf tok = STRBUF_INIT;
-			struct strbuf val = STRBUF_INIT;
+			strbuf_reset(&tok);
 			strbuf_addstr(&tok, item->token);
+			strbuf_reset(&val);
 			strbuf_addstr(&val, item->value);
 
 			/*
@@ -1151,9 +1153,6 @@ void format_trailers(const struct process_trailer_options *opts,
 				if (!opts->separator)
 					strbuf_addch(out, '\n');
 			}
-			strbuf_release(&tok);
-			strbuf_release(&val);
-
 		} else if (!opts->only_trailers) {
 			if (opts->separator && out->len != origlen) {
 				strbuf_addbuf(out, opts->separator);
@@ -1165,6 +1164,9 @@ void format_trailers(const struct process_trailer_options *opts,
 				strbuf_addch(out, '\n');
 		}
 	}
+
+	strbuf_release(&tok);
+	strbuf_release(&val);
 }
 
 void format_trailers_from_commit(const struct process_trailer_options *opts,
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 12/22] builtin/commit: fix leaking cleanup config

[Patrick Steinhardt]

The cleanup string set by the config is leaking when it is being
overridden by an option. Fix this by tracking these via two separate
variables such that we can free the old value.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/commit.c            | 17 ++++++++++++-----
 t/t7502-commit-porcelain.sh |  1 +
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 18a55bd1b91..71d674138c9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -135,7 +135,7 @@ static struct strvec trailer_args = STRVEC_INIT;
  * is specified explicitly.
  */
 static enum commit_msg_cleanup_mode cleanup_mode;
-static char *cleanup_arg;
+static char *cleanup_config;
 
 static enum commit_whence whence;
 static int use_editor = 1, include_status = 1;
@@ -1387,8 +1387,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (0 <= edit_flag)
 		use_editor = edit_flag;
 
-	cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);
-
 	handle_untracked_files_arg(s);
 
 	if (all && argc > 0)
@@ -1636,8 +1634,10 @@ static int git_commit_config(const char *k, const char *v,
 		include_status = git_config_bool(k, v);
 		return 0;
 	}
-	if (!strcmp(k, "commit.cleanup"))
-		return git_config_string(&cleanup_arg, k, v);
+	if (!strcmp(k, "commit.cleanup")) {
+		FREE_AND_NULL(cleanup_config);
+		return git_config_string(&cleanup_config, k, v);
+	}
 	if (!strcmp(k, "commit.gpgsign")) {
 		sign_commit = git_config_bool(k, v) ? "" : NULL;
 		return 0;
@@ -1658,6 +1658,7 @@ int cmd_commit(int argc,
 	       struct repository *repo UNUSED)
 {
 	static struct wt_status s;
+	static const char *cleanup_arg = NULL;
 	static struct option builtin_commit_options[] = {
 		OPT__QUIET(&quiet, N_("suppress summary after successful commit")),
 		OPT__VERBOSE(&verbose, N_("show diff in commit message template")),
@@ -1757,6 +1758,12 @@ int cmd_commit(int argc,
 	if (verbose == -1)
 		verbose = (config_commit_verbose < 0) ? 0 : config_commit_verbose;
 
+	if (cleanup_arg) {
+		free(cleanup_config);
+		cleanup_config = xstrdup(cleanup_arg);
+	}
+	cleanup_mode = get_cleanup_mode(cleanup_config, use_editor);
+
 	if (dry_run)
 		return dry_run_commit(argv, prefix, current_head, &s);
 	index_file = prepare_index(argv, prefix, current_head, 0);
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index b37e2018a74..84f1ff52b67 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -5,6 +5,7 @@ test_description='git commit porcelain-ish'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 commit_msg_is () {
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 13/22] transport-helper: fix leaking import/export marks

[Patrick Steinhardt]

Fix leaking import and export marks for transport helpers.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5801-remote-helpers.sh | 1 +
 transport-helper.c        | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index d21877150ed..d4882288a30 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -8,6 +8,7 @@ test_description='Test remote-helper import and export commands'
 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
 
diff --git a/transport-helper.c b/transport-helper.c
index 013ec79dc9c..bc27653cdee 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -399,6 +399,8 @@ static int release_helper(struct transport *transport)
 	int res = 0;
 	struct helper_data *data = transport->data;
 	refspec_clear(&data->rs);
+	free(data->import_marks);
+	free(data->export_marks);
 	res = disconnect_helper(transport);
 	free(transport->data);
 	return res;
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 14/22] builtin/tag: fix leaking key ID on failure to sign

[Patrick Steinhardt]

We do not free the key ID when signing a tag fails. Do so by using
the common exit path.

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

diff --git a/builtin/tag.c b/builtin/tag.c
index 93d10d59157..c37c0a68fda 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -164,7 +164,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid,
 	int ret = -1;
 
 	if (sign_buffer(buffer, &sig, keyid))
-		return -1;
+		goto out;
 
 	if (compat) {
 		const struct git_hash_algo *algo = the_repository->hash_algo;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b1316e62f46..42b3327e69b 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -10,6 +10,7 @@ Tests for operations with tags.'
 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
 . "$TEST_DIRECTORY"/lib-terminal.sh
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 15/22] combine-diff: fix leaking lost lines

[Patrick Steinhardt]

The `cnt` variable tracks the number of lines in a patch diff. It can
happen though that there are no newlines, in which case we'd still end
up allocating our array of `sline`s. In fact, we always allocate it with
`cnt + 2` entries: one extra entry for the deletion hunk at the end, and
another entry that we don't seem to ever populate at all but acts as a
kind of sentinel value.

When we loop through the array to clear it at the end of this function
we only loop until `lno < cnt`, and thus we may not end up releasing
whatever the two extra `sline`s contain. While that shouldn't matter for
the sentinel value, it does matter for the extra deletion hunk sline.
Regardless of that, plug this memory leak by releasing both extra
entries, which makes the logic a bit easier to reason about.

While at it, fix the formatting of a local comment, which incidentally
also provides the necessary context for why we overallocate the `sline`
array.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 combine-diff.c           | 5 +++--
 t/t4038-diff-combined.sh | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index f6b624dc288..33d0ed70975 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1185,7 +1185,8 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	result_file.ptr = result;
 	result_file.size = result_size;
 
-	/* Even p_lno[cnt+1] is valid -- that is for the end line number
+	/*
+	 * Even p_lno[cnt+1] is valid -- that is for the end line number
 	 * for deletion hunk at the end.
 	 */
 	CALLOC_ARRAY(sline[0].p_lno, st_mult(st_add(cnt, 2), num_parent));
@@ -1220,7 +1221,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	}
 	free(result);
 
-	for (lno = 0; lno < cnt; lno++) {
+	for (lno = 0; lno < cnt + 2; lno++) {
 		if (sline[lno].lost) {
 			struct lline *ll = sline[lno].lost;
 			while (ll) {
diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
index 2ce26e585c9..00190802d83 100755
--- a/t/t4038-diff-combined.sh
+++ b/t/t4038-diff-combined.sh
@@ -5,6 +5,7 @@ test_description='combined diff'
 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-diff.sh
 
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 16/22] dir: release untracked cache data

[Patrick Steinhardt]

There are several cases where we invalidate untracked cache directory
entries where we do not free the underlying data, but reset the number
of entries. This causes us to leak memory because `free_untracked()`
will not iterate over any potential entries which we still had in the
array.

Fix this issue by freeing old entries. The leak is exposed by t7519, but
plugging it alone does not make the whole test suite pass.

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

diff --git a/dir.c b/dir.c
index e3ddd5b5296..cb9782fa11f 100644
--- a/dir.c
+++ b/dir.c
@@ -1056,6 +1056,8 @@ static void do_invalidate_gitignore(struct untracked_cache_dir *dir)
 {
 	int i;
 	dir->valid = 0;
+	for (size_t i = 0; i < dir->untracked_nr; i++)
+		free(dir->untracked[i]);
 	dir->untracked_nr = 0;
 	for (i = 0; i < dir->dirs_nr; i++)
 		do_invalidate_gitignore(dir->dirs[i]);
@@ -1083,6 +1085,8 @@ static void invalidate_directory(struct untracked_cache *uc,
 		uc->dir_invalidated++;
 
 	dir->valid = 0;
+	for (size_t i = 0; i < dir->untracked_nr; i++)
+		free(dir->untracked[i]);
 	dir->untracked_nr = 0;
 	for (i = 0; i < dir->dirs_nr; i++)
 		dir->dirs[i]->recurse = 0;
@@ -3573,6 +3577,8 @@ static void write_one_dir(struct untracked_cache_dir *untracked,
 	 * for safety..
 	 */
 	if (!untracked->valid) {
+		for (size_t i = 0; i < untracked->untracked_nr; i++)
+			free(untracked->untracked[i]);
 		untracked->untracked_nr = 0;
 		untracked->check_only = 0;
 	}
@@ -3905,6 +3911,8 @@ static void invalidate_one_directory(struct untracked_cache *uc,
 {
 	uc->dir_invalidated++;
 	ucd->valid = 0;
+	for (size_t i = 0; i < ucd->untracked_nr; i++)
+		free(ucd->untracked[i]);
 	ucd->untracked_nr = 0;
 }
 
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 17/22] sparse-index: correctly free EWAH contents

[Patrick Steinhardt]

While we free the `fsmonitor_dirty` member of `struct index_state`, we
do not free the contents of that EWAH. Do so by using `ewah_free()`
instead of `FREE_AND_NULL()`.

This leak is exposed by t7519, but plugging it alone does not make the
test suite pass.

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

diff --git a/sparse-index.c b/sparse-index.c
index 3d7f2164e25..2107840bfc5 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -2,6 +2,7 @@
 
 #include "git-compat-util.h"
 #include "environment.h"
+#include "ewah/ewok.h"
 #include "gettext.h"
 #include "name-hash.h"
 #include "read-cache-ll.h"
@@ -242,7 +243,8 @@ int convert_to_sparse(struct index_state *istate, int flags)
 	cache_tree_update(istate, 0);
 
 	istate->fsmonitor_has_run_once = 0;
-	FREE_AND_NULL(istate->fsmonitor_dirty);
+	ewah_free(istate->fsmonitor_dirty);
+	istate->fsmonitor_dirty = NULL;
 	FREE_AND_NULL(istate->fsmonitor_last_update);
 
 	istate->sparse_index = INDEX_COLLAPSED;
@@ -438,7 +440,8 @@ void expand_index(struct index_state *istate, struct pattern_list *pl)
 	istate->cache_nr = full->cache_nr;
 	istate->cache_alloc = full->cache_alloc;
 	istate->fsmonitor_has_run_once = 0;
-	FREE_AND_NULL(istate->fsmonitor_dirty);
+	ewah_free(istate->fsmonitor_dirty);
+	istate->fsmonitor_dirty = NULL;
 	FREE_AND_NULL(istate->fsmonitor_last_update);
 
 	strbuf_release(&base);
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 18/22] t/helper: stop re-initialization of `the_repository`

[Patrick Steinhardt]

While "common-main.c" already initializes `the_repository` for us, we do
so a second time in the "read-cache" test helper. This causes a memory
leak because the old repository's contents isn't released.

Stop calling `initialize_repository()` to plug this leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/helper/test-read-cache.c  | 2 --
 t/t7519-status-fsmonitor.sh | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
index d285c656bd3..e277dde8e71 100644
--- a/t/helper/test-read-cache.c
+++ b/t/helper/test-read-cache.c
@@ -11,8 +11,6 @@ int cmd__read_cache(int argc, const char **argv)
 	int i, cnt = 1;
 	const char *name = NULL;
 
-	initialize_repository(the_repository);
-
 	if (argc > 1 && skip_prefix(argv[1], "--print-and-refresh=", &name)) {
 		argc--;
 		argv++;
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 7ee69ecdd4a..0f88a58a819 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -2,6 +2,7 @@
 
 test_description='git status with file system watcher'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Note, after "git reset --hard HEAD" no extensions exist other than 'TREE'
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 19/22] t/helper: fix leaking buffer in "dump-untracked-cache"

[Patrick Steinhardt]

We never release the local `struct strbuf base` buffer, thus leaking
memory. Fix this leak.

This leak is exposed by t7063, but plugging it alone does not make the
whole test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/helper/test-dump-untracked-cache.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c
index 4f010d53249..b2e70837a90 100644
--- a/t/helper/test-dump-untracked-cache.c
+++ b/t/helper/test-dump-untracked-cache.c
@@ -68,5 +68,7 @@ int cmd__dump_untracked_cache(int ac UNUSED, const char **av UNUSED)
 	printf("flags %08x\n", uc->dir_flags);
 	if (uc->root)
 		dump(uc->root, &base);
+
+	strbuf_release(&base);
 	return 0;
 }
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 20/22] dir: fix leak when parsing "status.showUntrackedFiles"

[Patrick Steinhardt]

We use `repo_config_get_string()` to read "status.showUntrackedFiles"
from the config subsystem. This function allocates the result, but we
never free the result after parsing it.

The value never leaves the scope of the calling function, so refactor it
to instead use `repo_config_get_string_tmp()`, which does not hand over
ownership to the caller.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 dir.c                             | 4 ++--
 t/t7063-status-untracked-cache.sh | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index cb9782fa11f..7f35a3e3175 100644
--- a/dir.c
+++ b/dir.c
@@ -2872,14 +2872,14 @@ static void set_untracked_ident(struct untracked_cache *uc)
 static unsigned new_untracked_cache_flags(struct index_state *istate)
 {
 	struct repository *repo = istate->repo;
-	char *val;
+	const char *val;
 
 	/*
 	 * This logic is coordinated with the setting of these flags in
 	 * wt-status.c#wt_status_collect_untracked(), and the evaluation
 	 * of the config setting in commit.c#git_status_config()
 	 */
-	if (!repo_config_get_string(repo, "status.showuntrackedfiles", &val) &&
+	if (!repo_config_get_string_tmp(repo, "status.showuntrackedfiles", &val) &&
 	    !strcmp(val, "all"))
 		return 0;
 
diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index 8929ef481f9..13fea7931cd 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -5,6 +5,7 @@ test_description='test untracked cache'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # On some filesystems (e.g. FreeBSD's ext2 and ufs) directory mtime
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 21/22] builtin/merge: release output buffer after performing merge

[Patrick Steinhardt]

The `obuf` member of `struct merge_options` is used to buffer output in
some cases. In order to not discard its allocated memory we only release
its contents in `merge_finalize()` when we're not currently recursing
into a subtree.

This results in some situations where we seemingly do not release the
buffer reliably. We thus have calls to `strbuf_release()` for this
buffer scattered across the codebase. But we're missing one callsite in
git-merge(1), which causes a memory leak.

We should ideally refactor this interface so that callers don't have to
know about any such internals. But for now, paper over the issue by
adding one more `strbuf_release()` call.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/merge.c                          | 1 +
 t/t6424-merge-unrelated-index-changes.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index 84d0f3604bc..51038eaca84 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -754,6 +754,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 			clean = merge_recursive(&o, head, remoteheads->item,
 						reversed, &result);
 		free_commit_list(reversed);
+		strbuf_release(&o.obuf);
 
 		if (clean < 0) {
 			rollback_lock_file(&lock);
diff --git a/t/t6424-merge-unrelated-index-changes.sh b/t/t6424-merge-unrelated-index-changes.sh
index 7677c5f08d0..a7ea8acb845 100755
--- a/t/t6424-merge-unrelated-index-changes.sh
+++ b/t/t6424-merge-unrelated-index-changes.sh
@@ -2,6 +2,7 @@
 
 test_description="merges with unrelated index changes"
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Testcase for some simple merges
-- 
2.47.0.229.g8f8d6eee53.dirty

[PATCH v3 22/22] list-objects-filter-options: work around reported leak on error

[Patrick Steinhardt]

This one is a little bit more curious. In t6112, we have a test that
exercises the `git rev-list --filter` option with invalid filters. We
execute git-rev-list(1) via `test_must_fail`, which means that we check
for leaks even though Git exits with an error code. This causes the
following leak:

    Direct leak of 27 byte(s) in 1 object(s) allocated from:
        #0 0x5555555e6946 in realloc.part.0 lsan_interceptors.cpp.o
        #1 0x5555558fb4b6 in xrealloc wrapper.c:137:8
        #2 0x5555558b6e06 in strbuf_grow strbuf.c:112:2
        #3 0x5555558b7550 in strbuf_add strbuf.c:311:2
        #4 0x5555557c1a88 in strbuf_addstr strbuf.h:310:2
        #5 0x5555557c1d4c in parse_list_objects_filter list-objects-filter-options.c:261:3
        #6 0x555555885ead in handle_revision_pseudo_opt revision.c:2899:3
        #7 0x555555884e20 in setup_revisions revision.c:3014:11
        #8 0x5555556c4b42 in cmd_rev_list builtin/rev-list.c:588:9
        #9 0x5555555ec5e3 in run_builtin git.c:483:11
        #10 0x5555555eb1e4 in handle_builtin git.c:749:13
        #11 0x5555555ec001 in run_argv git.c:819:4
        #12 0x5555555eaf94 in cmd_main git.c:954:19
        #13 0x5555556fd569 in main common-main.c:64:11
        #14 0x7ffff7ca714d in __libc_start_call_main (.../lib/libc.so.6+0x2a14d)
        #15 0x7ffff7ca7208 in __libc_start_main@GLIBC_2.2.5 (.../libc.so.6+0x2a208)
        #16 0x5555555ad064 in _start (git+0x59064)

This leak is valid, as we call `die()` and do not clean up the memory at
all. But what's curious is that this is the only leak reported, because
we don't clean up any other allocated memory, either, and I have no idea
why the leak sanitizer treats this buffer specially.

In any case, we can work around the leak by shuffling things around a
bit. Instead of calling `gently_parse_list_objects_filter()` and dying
after we have modified the filter spec, we simply do so beforehand. Like
this we don't allocate the buffer in the error case, which makes the
reported leak go away.

It's not pretty, but it manages to make t6112 leak free.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 list-objects-filter-options.c       | 17 +++++++----------
 t/t6112-rev-list-filters-objects.sh |  1 +
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 00611107d20..fa72e81e4ad 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -252,16 +252,14 @@ void parse_list_objects_filter(
 	const char *arg)
 {
 	struct strbuf errbuf = STRBUF_INIT;
-	int parse_error;
 
 	if (!filter_options->filter_spec.buf)
 		BUG("filter_options not properly initialized");
 
 	if (!filter_options->choice) {
+		if (gently_parse_list_objects_filter(filter_options, arg, &errbuf))
+			die("%s", errbuf.buf);
 		strbuf_addstr(&filter_options->filter_spec, arg);
-
-		parse_error = gently_parse_list_objects_filter(
-			filter_options, arg, &errbuf);
 	} else {
 		struct list_objects_filter_options *sub;
 
@@ -271,18 +269,17 @@ void parse_list_objects_filter(
 		 */
 		transform_to_combine_type(filter_options);
 
-		strbuf_addch(&filter_options->filter_spec, '+');
-		filter_spec_append_urlencode(filter_options, arg);
 		ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,
 			      filter_options->sub_alloc);
 		sub = &filter_options->sub[filter_options->sub_nr - 1];
 
 		list_objects_filter_init(sub);
-		parse_error = gently_parse_list_objects_filter(sub, arg,
-							       &errbuf);
+		if (gently_parse_list_objects_filter(sub, arg, &errbuf))
+			die("%s", errbuf.buf);
+
+		strbuf_addch(&filter_options->filter_spec, '+');
+		filter_spec_append_urlencode(filter_options, arg);
 	}
-	if (parse_error)
-		die("%s", errbuf.buf);
 }
 
 int opt_parse_list_objects_filter(const struct option *opt,
diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh
index 0387f35a326..71e38491fa8 100755
--- a/t/t6112-rev-list-filters-objects.sh
+++ b/t/t6112-rev-list-filters-objects.sh
@@ -5,6 +5,7 @@ test_description='git rev-list using object filtering'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Test the blob:none filter.
-- 
2.47.0.229.g8f8d6eee53.dirty

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Changes compared to v2:
>
>   - Remove an unnecessary cast.
>
>   - Fix a duplicate newline.
>
>   - Polish a couple of commit messages.
>
> Thanks!

I spot checked the ones that did not change from v2 and the ones I
checked at all looked sensible.  Perhaps this is now ready for
'next'?

Thanks.

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

[Patrick Steinhardt]

On Mon, Nov 04, 2024 at 10:51:10PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Changes compared to v2:
> >
> >   - Remove an unnecessary cast.
> >
> >   - Fix a duplicate newline.
> >
> >   - Polish a couple of commit messages.
> >
> > Thanks!
> 
> I spot checked the ones that did not change from v2 and the ones I
> checked at all looked sensible.  Perhaps this is now ready for
> 'next'?

From my point of view it should be ready, yes.

Patrick

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

[Justin Tobler]

On 24/11/05 07:52AM, Patrick Steinhardt wrote:
> On Mon, Nov 04, 2024 at 10:51:10PM -0800, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> > 
> > > Changes compared to v2:
> > >
> > >   - Remove an unnecessary cast.
> > >
> > >   - Fix a duplicate newline.
> > >
> > >   - Polish a couple of commit messages.
> > >
> > > Thanks!
> > 
> > I spot checked the ones that did not change from v2 and the ones I
> > checked at all looked sensible.  Perhaps this is now ready for
> > 'next'?
> 
> From my point of view it should be ready, yes.

From looking at the range-diff, this version looks good to me. :)

-Justin