Memory leak fixes (pt.9)

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]: <[email protected]>
[2]: <[email protected]>

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

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.

---
 builtin/ls-remote.c | 1 +
 1 file changed, 1 insertion(+)

index f723b3bf3bb..f333821b994 100644
@@ -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

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

---
 t/helper/test-reach.c | 10 ++++++++++
 t/t6600-test-reach.sh |  1 +
 2 files changed, 11 insertions(+)

index 995e382863a..84deee604ad 100644
@@ -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;
 }

index 2591f8b8b39..307deefed2c 100755
@@ -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

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.

---
 grep.c | 1 +
 1 file changed, 1 insertion(+)

index 701e58de04e..e9337f32cbf 100644
@@ -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

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

---
 builtin/grep.c  | 13 ++++++++++---
 t/t7810-grep.sh |  1 +
 2 files changed, 11 insertions(+), 3 deletions(-)

index f17d46a06e4..98b85c7fcac 100644
@@ -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;
 }

index af2cf2f78ab..9e7681f0831 100755
@@ -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

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

---
 revision.c           | 5 +++++
 t/t4216-log-bloom.sh | 1 +
 2 files changed, 6 insertions(+)

index f5f5b84f2b0..8df75b82249 100644
@@ -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)

index 3f163dc3969..8d22338f6aa 100755
@@ -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

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.

---
 diff-lib.c           | 1 +
 t/t7610-mergetool.sh | 1 +
 2 files changed, 2 insertions(+)

index 6b14b959629..3cf353946f5 100644
@@ -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;
 

index 22b3a85b3e9..5c5e79e9905 100755
@@ -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

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

---
 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(+)

index 6403e268900..098378720a4 100644
@@ -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,

index 51f7beb59f8..35bec4089a3 100755
@@ -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"

index 20913b37134..2ee62c07293 100755
@@ -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"
 

index 0d2dd29fe6a..eb229082e40 100755
@@ -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"

index 065f7806362..68e18856b66 100755
@@ -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

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

---
 t/t5702-protocol-v2.sh | 1 +
 upload-pack.c          | 1 +
 2 files changed, 2 insertions(+)

index 1ef540f73d3..ed55a2f7f95 100755
@@ -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

index 6d6e0f9f980..b4a59c3518b 100644
@@ -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

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

---
 builtin/commit.c                          | 9 ++++++++-
 t/t7500-commit-template-squash-signoff.sh | 1 +
 2 files changed, 9 insertions(+), 1 deletion(-)

index 8db4e9df0c9..18a55bd1b91 100644
@@ -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";

index 4dca8d97a77..379d3ed3413 100755
@@ -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

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

---
 t/t7513-interpret-trailers.sh |  1 +
 trailer.c                     | 18 +++++++++++++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

index 0f7d8938d98..38d6ccaa001 100755
@@ -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

index 682d74505bf..5c0bfb735a9 100644
@@ -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

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.

---
 builtin/commit.c            | 17 ++++++++++++-----
 t/t7502-commit-porcelain.sh |  1 +
 2 files changed, 13 insertions(+), 5 deletions(-)

index 18a55bd1b91..71d674138c9 100644
@@ -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);

index b37e2018a74..84f1ff52b67 100755
@@ -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

Fix leaking import and export marks for transport helpers.

---
 t/t5801-remote-helpers.sh | 1 +
 transport-helper.c        | 2 ++
 2 files changed, 3 insertions(+)

index d21877150ed..d4882288a30 100755
@@ -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
 

index 013ec79dc9c..bc27653cdee 100644
@@ -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

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

---
 builtin/tag.c  | 2 +-
 t/t7004-tag.sh | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

index 93d10d59157..c37c0a68fda 100644
@@ -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;

index b1316e62f46..42b3327e69b 100755
@@ -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

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 slines. 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 slines contain.

Plug this memory leak.

---
 combine-diff.c           | 2 +-
 t/t4038-diff-combined.sh | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

index f6b624dc288..3c6d9507fec 100644
@@ -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) {

index 2ce26e585c9..00190802d83 100755
@@ -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

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.

---
 dir.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index e3ddd5b5296..cb9782fa11f 100644
@@ -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

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.

---
 sparse-index.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index 3d7f2164e25..2107840bfc5 100644
@@ -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

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.

---
 t/helper/test-read-cache.c  | 2 --
 t/t7519-status-fsmonitor.sh | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

index d285c656bd3..e277dde8e71 100644
@@ -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++;

index 7ee69ecdd4a..0f88a58a819 100755
@@ -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

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.

---
 t/helper/test-dump-untracked-cache.c | 2 ++
 1 file changed, 2 insertions(+)

index 4f010d53249..b2e70837a90 100644
@@ -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

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.

---
 dir.c                             | 4 ++--
 t/t7063-status-untracked-cache.sh | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

index cb9782fa11f..7f35a3e3175 100644
@@ -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;
 

index 8929ef481f9..13fea7931cd 100755
@@ -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

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.

---
 builtin/merge.c                          | 1 +
 t/t6424-merge-unrelated-index-changes.sh | 1 +
 2 files changed, 2 insertions(+)

index 84d0f3604bc..51038eaca84 100644
@@ -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);

index 7677c5f08d0..a7ea8acb845 100755
@@ -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

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.

---
 list-objects-filter-options.c       | 17 +++++++----------
 t/t6112-rev-list-filters-objects.sh |  1 +
 2 files changed, 8 insertions(+), 10 deletions(-)

index 00611107d20..fa72e81e4ad 100644
@@ -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,

index 0387f35a326..71e38491fa8 100755
@@ -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

--
Toon

Shall we mark this as #leftoverbits?

--
Toon

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

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

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

I guess it is now marked as such :)

Patrick

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

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

Patrick

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 <[email protected]>

## 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.

@@ 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

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.

---
 builtin/ls-remote.c | 1 +
 1 file changed, 1 insertion(+)

index f723b3bf3bb..f333821b994 100644
@@ -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

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

---
 t/helper/test-reach.c | 10 ++++++++++
 t/t6600-test-reach.sh |  1 +
 2 files changed, 11 insertions(+)

index 995e382863a..84deee604ad 100644
@@ -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;
 }

index 2591f8b8b39..307deefed2c 100755
@@ -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

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.

---
 grep.c | 1 +
 1 file changed, 1 insertion(+)

index 701e58de04e..e9337f32cbf 100644
@@ -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

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

---
 builtin/grep.c  | 13 ++++++++++---
 t/t7810-grep.sh |  1 +
 2 files changed, 11 insertions(+), 3 deletions(-)

index f17d46a06e4..98b85c7fcac 100644
@@ -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;
 }

index af2cf2f78ab..9e7681f0831 100755
@@ -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

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

---
 revision.c           | 5 +++++
 t/t4216-log-bloom.sh | 1 +
 2 files changed, 6 insertions(+)

index f5f5b84f2b0..8df75b82249 100644
@@ -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)

index 3f163dc3969..8d22338f6aa 100755
@@ -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

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.

---
 diff-lib.c           | 1 +
 t/t7610-mergetool.sh | 1 +
 2 files changed, 2 insertions(+)

index 6b14b959629..3cf353946f5 100644
@@ -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;
 

index 22b3a85b3e9..5c5e79e9905 100755
@@ -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

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

---
 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(+)

index 6403e268900..098378720a4 100644
@@ -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,

index 51f7beb59f8..35bec4089a3 100755
@@ -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"

index 20913b37134..2ee62c07293 100755
@@ -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"
 

index 0d2dd29fe6a..eb229082e40 100755
@@ -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"

index 065f7806362..68e18856b66 100755
@@ -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

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

---
 t/t5702-protocol-v2.sh | 1 +
 upload-pack.c          | 1 +
 2 files changed, 2 insertions(+)

index 1ef540f73d3..ed55a2f7f95 100755
@@ -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

index 6d6e0f9f980..b4a59c3518b 100644
@@ -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

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

---
 builtin/commit.c                          | 9 ++++++++-
 t/t7500-commit-template-squash-signoff.sh | 1 +
 2 files changed, 9 insertions(+), 1 deletion(-)

index 8db4e9df0c9..18a55bd1b91 100644
@@ -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";

index 4dca8d97a77..379d3ed3413 100755
@@ -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

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.

---
 trailer.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

index 682d74505bf..f1eca6d5d15 100644
@@ -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

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.

---
 t/t7513-interpret-trailers.sh |  1 +
 trailer.c                     | 13 ++++++++-----
 2 files changed, 9 insertions(+), 5 deletions(-)

index 0f7d8938d98..38d6ccaa001 100755
@@ -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

index f1eca6d5d15..24e4e56fdf8 100644
@@ -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

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.

---
 builtin/commit.c            | 17 ++++++++++++-----
 t/t7502-commit-porcelain.sh |  1 +
 2 files changed, 13 insertions(+), 5 deletions(-)

index 18a55bd1b91..71d674138c9 100644
@@ -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);

index b37e2018a74..84f1ff52b67 100755
@@ -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

Fix leaking import and export marks for transport helpers.

---
 t/t5801-remote-helpers.sh | 1 +
 transport-helper.c        | 2 ++
 2 files changed, 3 insertions(+)

index d21877150ed..d4882288a30 100755
@@ -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
 

index 013ec79dc9c..bc27653cdee 100644
@@ -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

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

---
 builtin/tag.c  | 2 +-
 t/t7004-tag.sh | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

index 93d10d59157..c37c0a68fda 100644
@@ -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;

index b1316e62f46..42b3327e69b 100755
@@ -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

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 slines. 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 slines contain.

Plug this memory leak.

---
 combine-diff.c           | 2 +-
 t/t4038-diff-combined.sh | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

index f6b624dc288..3c6d9507fec 100644
@@ -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) {

index 2ce26e585c9..00190802d83 100755
@@ -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

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.

---
 dir.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index e3ddd5b5296..cb9782fa11f 100644
@@ -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

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.

---
 sparse-index.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index 3d7f2164e25..2107840bfc5 100644
@@ -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

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.

---
 t/helper/test-read-cache.c  | 2 --
 t/t7519-status-fsmonitor.sh | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

index d285c656bd3..e277dde8e71 100644
@@ -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++;

index 7ee69ecdd4a..0f88a58a819 100755
@@ -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

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.

---
 t/helper/test-dump-untracked-cache.c | 2 ++
 1 file changed, 2 insertions(+)

index 4f010d53249..b2e70837a90 100644
@@ -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

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.

---
 dir.c                             | 4 ++--
 t/t7063-status-untracked-cache.sh | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

index cb9782fa11f..7f35a3e3175 100644
@@ -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;
 

index 8929ef481f9..13fea7931cd 100755
@@ -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

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.

---
 builtin/merge.c                          | 1 +
 t/t6424-merge-unrelated-index-changes.sh | 1 +
 2 files changed, 2 insertions(+)

index 84d0f3604bc..51038eaca84 100644
@@ -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);

index 7677c5f08d0..a7ea8acb845 100755
@@ -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

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.

---
 list-objects-filter-options.c       | 17 +++++++----------
 t/t6112-rev-list-filters-objects.sh |  1 +
 2 files changed, 8 insertions(+), 10 deletions(-)

index 00611107d20..fa72e81e4ad 100644
@@ -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,

index 0387f35a326..71e38491fa8 100755
@@ -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

s/outbut/output/

s/exrpessions/expressions/

s/as that/since that/

“since” communicates causality better.

s/free that/free it/

Kristoffer Haugsbakk

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

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

I've got nothing more to add.

--
Toon

s/is/are/

s/in of/in/

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

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

Kristoffer Haugsbakk

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 :)

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.

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

Indeed it is, good catch!

Patrick

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

Patrick

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

Thanks for your review!

Patrick

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.

 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.

@@ 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 slines. 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 slines 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 slines 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.

## 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 <[email protected]>

## 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

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.

---
 builtin/ls-remote.c | 1 +
 1 file changed, 1 insertion(+)

index f723b3bf3bb..f333821b994 100644
@@ -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

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

---
 t/helper/test-reach.c | 10 ++++++++++
 t/t6600-test-reach.sh |  1 +
 2 files changed, 11 insertions(+)

index 995e382863a..84deee604ad 100644
@@ -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;
 }

index 2591f8b8b39..307deefed2c 100755
@@ -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

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.

---
 grep.c | 1 +
 1 file changed, 1 insertion(+)

index 701e58de04e..e9337f32cbf 100644
@@ -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

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

---
 builtin/grep.c  | 13 ++++++++++---
 t/t7810-grep.sh |  1 +
 2 files changed, 11 insertions(+), 3 deletions(-)

index f17d46a06e4..98b85c7fcac 100644
@@ -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;
 }

index af2cf2f78ab..9e7681f0831 100755
@@ -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

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

---
 revision.c           | 5 +++++
 t/t4216-log-bloom.sh | 1 +
 2 files changed, 6 insertions(+)

index f5f5b84f2b0..8df75b82249 100644
@@ -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)

index 3f163dc3969..8d22338f6aa 100755
@@ -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

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.

---
 diff-lib.c           | 1 +
 t/t7610-mergetool.sh | 1 +
 2 files changed, 2 insertions(+)

index 6b14b959629..3cf353946f5 100644
@@ -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;
 

index 22b3a85b3e9..5c5e79e9905 100755
@@ -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

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

---
 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(+)

index 6403e268900..098378720a4 100644
@@ -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,

index 51f7beb59f8..35bec4089a3 100755
@@ -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"

index 20913b37134..2ee62c07293 100755
@@ -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"
 

index 0d2dd29fe6a..eb229082e40 100755
@@ -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"

index 065f7806362..68e18856b66 100755
@@ -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

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

---
 t/t5702-protocol-v2.sh | 1 +
 upload-pack.c          | 1 +
 2 files changed, 2 insertions(+)

index 1ef540f73d3..ed55a2f7f95 100755
@@ -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

index 6d6e0f9f980..b4a59c3518b 100644
@@ -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

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

---
 builtin/commit.c                          | 9 ++++++++-
 t/t7500-commit-template-squash-signoff.sh | 1 +
 2 files changed, 9 insertions(+), 1 deletion(-)

index 8db4e9df0c9..18a55bd1b91 100644
@@ -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";

index 4dca8d97a77..379d3ed3413 100755
@@ -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

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.

---
 trailer.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

index 682d74505bf..6bafe92b326 100644
@@ -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

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.

---
 t/t7513-interpret-trailers.sh |  1 +
 trailer.c                     | 12 +++++++-----
 2 files changed, 8 insertions(+), 5 deletions(-)

index 0f7d8938d98..38d6ccaa001 100755
@@ -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

index 6bafe92b326..8ba350404d4 100644
@@ -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

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.

---
 builtin/commit.c            | 17 ++++++++++++-----
 t/t7502-commit-porcelain.sh |  1 +
 2 files changed, 13 insertions(+), 5 deletions(-)

index 18a55bd1b91..71d674138c9 100644
@@ -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);

index b37e2018a74..84f1ff52b67 100755
@@ -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

Fix leaking import and export marks for transport helpers.

---
 t/t5801-remote-helpers.sh | 1 +
 transport-helper.c        | 2 ++
 2 files changed, 3 insertions(+)

index d21877150ed..d4882288a30 100755
@@ -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
 

index 013ec79dc9c..bc27653cdee 100644
@@ -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

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

---
 builtin/tag.c  | 2 +-
 t/t7004-tag.sh | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

index 93d10d59157..c37c0a68fda 100644
@@ -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;

index b1316e62f46..42b3327e69b 100755
@@ -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

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 slines. 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 slines 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.

---
 combine-diff.c           | 5 +++--
 t/t4038-diff-combined.sh | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

index f6b624dc288..33d0ed70975 100644
@@ -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) {