[PATCH 00/20] Memory leak fixes (pt.5)

[PATCH 00/20] Memory leak fixes (pt.5)

[Patrick Steinhardt]

Hi,

another week, another set of memory leak fixes. With this series we're
down to ~128 leaking test suites, down from 359 before starting with
this effort. So we're about 2/3rds done. Naturally, part 6 is also
almost ready.

This series is built on top of bb9c16bd4f (The sixth batch, 2024-08-19)
with Junio's ps/leakfixes-part-4 at 77d4b3dd73 (builtin/diff: free
symmetric diff members, 2024-08-14) merged into it.

Thanks!

Patrick

Patrick Steinhardt (20):
  mailinfo: fix leaking header data
  convert: fix leaks when resetting attributes
  pretty: fix memory leaks when parsing pretty formats
  pretty: fix leaking key/value separator buffer
  builtin/merge-tree: fix leaking `-X` strategy options
  builtin/upload-archive: fix leaking args passed to `write_archive()`
  builtin/archive: fix leaking `OPT_FILENAME()` value
  midx-write: fix leaking hashfile on error cases
  builtin/repack: fix leaks when computing packs to repack
  t/helper: fix leaking multi-pack-indices in "read-midx"
  transport: fix leaking OID arrays in git:// transport data
  builtin/send-pack: fix leaking refspecs
  sideband: fix leaks when configuring sideband colors
  builtin/fetch-pack: fix leaking refs
  remote: fix leaking config strings
  remote: fix leaks when matching refspecs
  remote: fix leaking peer ref when expanding refmap
  builtin/fetch: fix leaking transaction with `--atomic`
  transport: fix leaking arguments when fetching from bundle
  transport: fix leaking negotiation tips

 archive.c                           | 10 ++++++
 builtin/archive.c                   |  7 ++--
 builtin/fetch-pack.c                | 20 ++++++-----
 builtin/fetch.c                     |  8 ++---
 builtin/merge-tree.c                | 13 +++++--
 builtin/repack.c                    | 36 ++++++++++++++-----
 builtin/send-pack.c                 |  1 +
 builtin/upload-archive.c            |  8 +++--
 convert.c                           |  3 ++
 mailinfo.c                          | 17 +++++++--
 midx-write.c                        | 24 ++++++-------
 pretty.c                            | 13 +++++--
 remote.c                            | 55 +++++++++++++++++++++--------
 sideband.c                          |  8 +++--
 t/helper/test-read-midx.c           |  8 ++++-
 t/t4150-am.sh                       |  1 +
 t/t4205-log-pretty-formats.sh       |  2 ++
 t/t4301-merge-tree-write-tree.sh    |  1 +
 t/t5000-tar-tree.sh                 |  1 +
 t/t5003-archive-zip.sh              |  1 +
 t/t5100-mailinfo.sh                 |  1 +
 t/t5319-multi-pack-index.sh         |  2 ++
 t/t5400-send-pack.sh                |  1 +
 t/t5401-update-hooks.sh             |  2 ++
 t/t5408-send-pack-stdin.sh          |  2 ++
 t/t5409-colorize-remote-messages.sh |  1 +
 t/t5501-fetch-push-alternates.sh    |  1 +
 t/t5505-remote.sh                   |  1 +
 t/t5510-fetch.sh                    |  1 +
 t/t5519-push-alternates.sh          |  1 +
 t/t5536-fetch-conflicts.sh          |  1 +
 t/t5548-push-porcelain.sh           |  1 +
 t/t5553-set-upstream.sh             |  1 +
 t/t5574-fetch-output.sh             |  1 +
 t/t5703-upload-pack-ref-in-want.sh  |  1 +
 t/t5812-proto-disable-http.sh       |  2 ++
 t/t6050-replace.sh                  |  1 +
 t/t7704-repack-cruft.sh             |  1 +
 transport.c                         |  8 +++++
 39 files changed, 203 insertions(+), 64 deletions(-)

-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 01/20] mailinfo: fix leaking header data

[Patrick Steinhardt]

We populate the `mailinfo` arrays `p_hdr_data` and `s_hdr_data` with
data parsed from the mail headers. These arrays may end up being only
partially populated with gaps in case some of the headers do not parse
properly. This causes memory leaks because `strbuf_list_free()` will
stop iterating once it hits the first `NULL` pointer in the backing
array.

Fix this by open-coding a variant of `strbuf_list_free()` that knows to
iterate through all headers.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 mailinfo.c          | 17 +++++++++++++++--
 t/t5100-mailinfo.sh |  1 +
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/mailinfo.c b/mailinfo.c
index 94b9b0abf22..a4fa64994ac 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -1290,8 +1290,21 @@ void clear_mailinfo(struct mailinfo *mi)
 	strbuf_release(&mi->inbody_header_accum);
 	free(mi->message_id);
 
-	strbuf_list_free(mi->p_hdr_data);
-	strbuf_list_free(mi->s_hdr_data);
+	for (size_t i = 0; header[i]; i++) {
+		if (!mi->p_hdr_data[i])
+			continue;
+		strbuf_release(mi->p_hdr_data[i]);
+		free(mi->p_hdr_data[i]);
+	}
+	free(mi->p_hdr_data);
+
+	for (size_t i = 0; header[i]; i++) {
+		if (!mi->s_hdr_data[i])
+			continue;
+		strbuf_release(mi->s_hdr_data[i]);
+		free(mi->s_hdr_data[i]);
+	}
+	free(mi->s_hdr_data);
 
 	while (mi->content < mi->content_top) {
 		free(*(mi->content_top));
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index c8d06554541..065156c1f39 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -5,6 +5,7 @@
 
 test_description='git mailinfo and git mailsplit test'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 DATA="$TEST_DIRECTORY/t5100"
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 02/20] convert: fix leaks when resetting attributes

[Patrick Steinhardt]

When resetting parsed gitattributes, we free the list of convert drivers
parsed from the config. We only free some of the drivers' fields though
and thus have memory leaks.

Fix this by freeing all allocated convert driver fields to plug these
memory leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 convert.c     | 3 +++
 t/t4150-am.sh | 1 +
 2 files changed, 4 insertions(+)

diff --git a/convert.c b/convert.c
index e6184d21f26..c9a31eb4f03 100644
--- a/convert.c
+++ b/convert.c
@@ -1371,6 +1371,9 @@ void reset_parsed_attributes(void)
 	for (drv = user_convert; drv; drv = next) {
 		next = drv->next;
 		free((void *)drv->name);
+		free((void *)drv->smudge);
+		free((void *)drv->clean);
+		free((void *)drv->process);
 		free(drv);
 	}
 	user_convert = NULL;
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 5e2b6c80eae..232e1394e8d 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -5,6 +5,7 @@ test_description='git am running'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup: messages' '
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 03/20] pretty: fix memory leaks when parsing pretty formats

[Patrick Steinhardt]

When parsing pretty formats from the config we leak the name and user
format whenever these are set multiple times. This is because we do not
free any already-set value in case there is one.

Plugging this leak for the name is trivial. For the user format we need
to be a bit more careful, because we may end up assigning a pointer into
the allocated region when the string is prefixed with either "format" or
"tformat:". In order to make it safe to unconditionally free the user
format we thus strdup the stripped string into the field instead of a
pointer into the string.

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

diff --git a/pretty.c b/pretty.c
index 44222fb83c6..af8f433cdcb 100644
--- a/pretty.c
+++ b/pretty.c
@@ -63,7 +63,7 @@ static int git_pretty_formats_config(const char *var, const char *value,
 				     void *cb UNUSED)
 {
 	struct cmt_fmt_map *commit_format = NULL;
-	const char *name;
+	const char *name, *stripped;
 	char *fmt;
 	int i;
 
@@ -90,15 +90,21 @@ static int git_pretty_formats_config(const char *var, const char *value,
 		commit_formats_len++;
 	}
 
+	free((char *) commit_format->name);
 	commit_format->name = xstrdup(name);
 	commit_format->format = CMIT_FMT_USERFORMAT;
 	if (git_config_string(&fmt, var, value))
 		return -1;
 
-	if (skip_prefix(fmt, "format:", &commit_format->user_format)) {
+	free((char *) commit_format->user_format);
+	if (skip_prefix(fmt, "format:", &stripped)) {
 		commit_format->is_tformat = 0;
-	} else if (skip_prefix(fmt, "tformat:", &commit_format->user_format)) {
+		commit_format->user_format = xstrdup(stripped);
+		free(fmt);
+	} else if (skip_prefix(fmt, "tformat:", &stripped)) {
 		commit_format->is_tformat = 1;
+		commit_format->user_format = xstrdup(stripped);
+		free(fmt);
 	} else if (strchr(fmt, '%')) {
 		commit_format->is_tformat = 1;
 		commit_format->user_format = fmt;
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 04/20] pretty: fix leaking key/value separator buffer

[Patrick Steinhardt]

The `format_set_trailers_options()` function is responsible for parsing
a custom pretty format for trailers. It puts the parsed options into a
`struct process_trailer_options` structure, while the allocated memory
required for this will be put into separate caller-provided arguments.
It is thus the caller's responsibility to free the memory not via the
options structure, but via the other parameters.

While we do this alright for the separator and filter keys, we do not
free the memory associated with the key/value separator. Fix this to
plug this memory leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 pretty.c                      | 1 +
 t/t4205-log-pretty-formats.sh | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/pretty.c b/pretty.c
index af8f433cdcb..6b684d7b828 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1776,6 +1776,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		}
 	trailer_out:
 		string_list_clear(&filter_list, 0);
+		strbuf_release(&kvsepbuf);
 		strbuf_release(&sepbuf);
 		return ret;
 	}
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 158b49d4b60..eb63ce011fa 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -5,6 +5,8 @@
 #
 
 test_description='Test pretty formats'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Tested non-UTF-8 encoding
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 05/20] builtin/merge-tree: fix leaking `-X` strategy options

[Patrick Steinhardt]

The `-X` switch for git-merge-tree(1) will push each option into a local
`xopts` vector that we then end up parsing. The vector never gets freed
though, causing a memory leak. Plug it.

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

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 9bca9b5f33c..c00469ed3db 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -533,6 +533,7 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 	int expected_remaining_argc;
 	int original_argc;
 	const char *merge_base = NULL;
+	int ret;
 
 	const char * const merge_tree_usage[] = {
 		N_("git merge-tree [--write-tree] [<options>] <branch1> <branch2>"),
@@ -625,7 +626,9 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 			strbuf_list_free(split);
 		}
 		strbuf_release(&buf);
-		return 0;
+
+		ret = 0;
+		goto out;
 	}
 
 	/* Figure out which mode to use */
@@ -664,7 +667,11 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 
 	/* Do the relevant type of merge */
 	if (o.mode == MODE_REAL)
-		return real_merge(&o, merge_base, argv[0], argv[1], prefix);
+		ret = real_merge(&o, merge_base, argv[0], argv[1], prefix);
 	else
-		return trivial_merge(argv[0], argv[1], argv[2]);
+		ret = trivial_merge(argv[0], argv[1], argv[2]);
+
+out:
+	strvec_clear(&xopts);
+	return ret;
 }
diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index eea19907b55..37f1cd7364c 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -2,6 +2,7 @@
 
 test_description='git merge-tree --write-tree'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # This test is ort-specific
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 06/20] builtin/upload-archive: fix leaking args passed to `write_archive()`

[Patrick Steinhardt]

In git-upload-archive(1), we pass an array of arguments to
`write_archive()` to tell it what exactly to do. We don't ever clear the
vector though, causing a memory leak. Furthermore though, the call to
`write_archive()` may cause contents of the array to be modified, which
would cause us to leak memory to allocated strings held by it.

Fix the issue by having `write_archive()` create a shallow copy of
`argv` before parsing the arguments. Like this, we won't modify the
caller's array and can easily `strvec_clear()` it to plug these memory
leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 archive.c                | 10 ++++++++++
 builtin/upload-archive.c |  8 ++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/archive.c b/archive.c
index 7bd60d0632a..9ba96aae4f7 100644
--- a/archive.c
+++ b/archive.c
@@ -736,6 +736,7 @@ int write_archive(int argc, const char **argv, const char *prefix,
 	struct pretty_print_describe_status describe_status = {0};
 	struct pretty_print_context ctx = {0};
 	struct archiver_args args;
+	const char **argv_copy;
 	int rc;
 
 	git_config_get_bool("uploadarchive.allowunreachable", &remote_allow_unreachable);
@@ -749,6 +750,14 @@ int write_archive(int argc, const char **argv, const char *prefix,
 	args.repo = repo;
 	args.prefix = prefix;
 	string_list_init_dup(&args.extra_files);
+
+	/*
+	 * `parse_archive_args()` modifies contents of `argv`, which is what we
+	 * want. Our callers may not want it though, so we create a copy here.
+	 */
+	DUP_ARRAY(argv_copy, argv, argc);
+	argv = argv_copy;
+
 	argc = parse_archive_args(argc, argv, &ar, &args, name_hint, remote);
 	if (!startup_info->have_repository) {
 		/*
@@ -767,6 +776,7 @@ int write_archive(int argc, const char **argv, const char *prefix,
 	string_list_clear_func(&args.extra_files, extra_file_info_clear);
 	free(args.refname);
 	clear_pathspec(&args.pathspec);
+	free(argv_copy);
 
 	return rc;
 }
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 1b09e5e1aa3..313a8dfa81c 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -22,6 +22,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
 {
 	struct strvec sent_argv = STRVEC_INIT;
 	const char *arg_cmd = "argument ";
+	int ret;
 
 	if (argc != 2 || !strcmp(argv[1], "-h"))
 		usage(upload_archive_usage);
@@ -46,8 +47,11 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
 	}
 
 	/* parse all options sent by the client */
-	return write_archive(sent_argv.nr, sent_argv.v, prefix,
-			     the_repository, NULL, 1);
+	ret = write_archive(sent_argv.nr, sent_argv.v, prefix,
+			    the_repository, NULL, 1);
+
+	strvec_clear(&sent_argv);
+	return ret;
 }
 
 __attribute__((format (printf, 1, 2)))
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 07/20] builtin/archive: fix leaking `OPT_FILENAME()` value

[Patrick Steinhardt]

The "--output" switch is an `OPT_FILENAME()` option, which allocates
memory when specified by the user. But while we free the string when
executed without the "--remote" switch, we don't otherwise because we
return via a separate exit path that doesn't know to free it.

Fix this by creating a common exit path.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/archive.c      | 7 +++++--
 t/t5000-tar-tree.sh    | 1 +
 t/t5003-archive-zip.sh | 1 +
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index b50981504f3..63f02990d11 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -100,13 +100,16 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 	if (output)
 		create_output_file(output);
 
-	if (remote)
-		return run_remote_archiver(argc, argv, remote, exec, output);
+	if (remote) {
+		ret = run_remote_archiver(argc, argv, remote, exec, output);
+		goto out;
+	}
 
 	setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
 
 	ret = write_archive(argc, argv, prefix, the_repository, output, 0);
 
+out:
 	free(output);
 	return ret;
 }
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 72b8d0ff02e..7abba8a4b20 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -25,6 +25,7 @@ commit id embedding:
 '
 
 TEST_CREATE_REPO_NO_TEMPLATE=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 SUBSTFORMAT=%H%n
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index 961c6aac256..01f591c99b9 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -3,6 +3,7 @@
 test_description='git archive --format=zip test'
 
 TEST_CREATE_REPO_NO_TEMPLATE=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 SUBSTFORMAT=%H%n
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 08/20] midx-write: fix leaking hashfile on error cases

[Patrick Steinhardt]

When writing the MIDX file we first create the `struct hashfile` used to
write the trailer hash, and then afterwards we verify whether we can
actually write the MIDX in the first place. When we decide that we
can't, this leads to a memory leak because we never free the hash file
contents.

We could fix this by freeing the hashfile on the exit path. There is a
better option though: we can simply move the checks for the error
condition earlier. As there is no early exit between creating the
hashfile and finalizing it anymore this is sufficient to fix the memory
leak.

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

diff --git a/midx-write.c b/midx-write.c
index e3fa33203fa..07d98d494aa 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -1308,6 +1308,18 @@ static int write_midx_internal(const char *object_dir,
 		pack_name_concat_len += MIDX_CHUNK_ALIGNMENT -
 					(pack_name_concat_len % MIDX_CHUNK_ALIGNMENT);
 
+	if (ctx.nr - dropped_packs == 0) {
+		error(_("no pack files to index."));
+		result = 1;
+		goto cleanup;
+	}
+
+	if (!ctx.entries_nr) {
+		if (flags & MIDX_WRITE_BITMAP)
+			warning(_("refusing to write multi-pack .bitmap without any objects"));
+		flags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP);
+	}
+
 	if (ctx.incremental) {
 		struct strbuf lock_name = STRBUF_INIT;
 
@@ -1333,18 +1345,6 @@ static int write_midx_internal(const char *object_dir,
 		f = hashfd(get_lock_file_fd(&lk), get_lock_file_path(&lk));
 	}
 
-	if (ctx.nr - dropped_packs == 0) {
-		error(_("no pack files to index."));
-		result = 1;
-		goto cleanup;
-	}
-
-	if (!ctx.entries_nr) {
-		if (flags & MIDX_WRITE_BITMAP)
-			warning(_("refusing to write multi-pack .bitmap without any objects"));
-		flags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP);
-	}
-
 	cf = init_chunkfile(f);
 
 	add_chunk(cf, MIDX_CHUNKID_PACKNAMES, pack_name_concat_len,
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 09/20] builtin/repack: fix leaks when computing packs to repack

[Patrick Steinhardt]

When writing an MIDX in git-repack(1) we first collect all the pack
names that we want to add to it in a string list. This list is marked as
`NODUP`, which indicates that it will neither duplicate nor own strings
added to it. In `write_midx_included_packs()` we then `insert()` strings
via `xstrdup()` or `strbuf_detach()`, but the resulting strings will not
be owned by anything and thus leak.

Fix this issue by marking the list as `DUP` and using a local buffer to
compute the pack names.

This leak is hit in t5319, but plugging it is not sufficient to make the
whole test suite pass.

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

diff --git a/builtin/repack.c b/builtin/repack.c
index 62cfa50c50f..8bb875532b4 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -732,14 +732,23 @@ static void midx_included_packs(struct string_list *include,
 				struct pack_geometry *geometry)
 {
 	struct string_list_item *item;
+	struct strbuf buf = STRBUF_INIT;
+
+	for_each_string_list_item(item, &existing->kept_packs) {
+		strbuf_reset(&buf);
+		strbuf_addf(&buf, "%s.idx", item->string);
+		string_list_insert(include, buf.buf);
+	}
+
+	for_each_string_list_item(item, names) {
+		strbuf_reset(&buf);
+		strbuf_addf(&buf, "pack-%s.idx", item->string);
+		string_list_insert(include, buf.buf);
+	}
 
-	for_each_string_list_item(item, &existing->kept_packs)
-		string_list_insert(include, xstrfmt("%s.idx", item->string));
-	for_each_string_list_item(item, names)
-		string_list_insert(include, xstrfmt("pack-%s.idx", item->string));
 	if (geometry->split_factor) {
-		struct strbuf buf = STRBUF_INIT;
 		uint32_t i;
+
 		for (i = geometry->split; i < geometry->pack_nr; i++) {
 			struct packed_git *p = geometry->pack[i];
 
@@ -754,17 +763,21 @@ static void midx_included_packs(struct string_list *include,
 			if (!p->pack_local)
 				continue;
 
+			strbuf_reset(&buf);
 			strbuf_addstr(&buf, pack_basename(p));
 			strbuf_strip_suffix(&buf, ".pack");
 			strbuf_addstr(&buf, ".idx");
 
-			string_list_insert(include, strbuf_detach(&buf, NULL));
+			string_list_insert(include, buf.buf);
 		}
 	} else {
 		for_each_string_list_item(item, &existing->non_kept_packs) {
 			if (pack_is_marked_for_deletion(item))
 				continue;
-			string_list_insert(include, xstrfmt("%s.idx", item->string));
+
+			strbuf_reset(&buf);
+			strbuf_addf(&buf, "%s.idx", item->string);
+			string_list_insert(include, buf.buf);
 		}
 	}
 
@@ -784,8 +797,13 @@ static void midx_included_packs(struct string_list *include,
 		 */
 		if (pack_is_marked_for_deletion(item))
 			continue;
-		string_list_insert(include, xstrfmt("%s.idx", item->string));
+
+		strbuf_reset(&buf);
+		strbuf_addf(&buf, "%s.idx", item->string);
+		string_list_insert(include, buf.buf);
 	}
+
+	strbuf_release(&buf);
 }
 
 static int write_midx_included_packs(struct string_list *include,
@@ -1476,7 +1494,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		mark_packs_for_deletion(&existing, &names);
 
 	if (write_midx) {
-		struct string_list include = STRING_LIST_INIT_NODUP;
+		struct string_list include = STRING_LIST_INIT_DUP;
 		midx_included_packs(&include, &existing, &names, &geometry);
 
 		ret = write_midx_included_packs(&include, &geometry, &names,
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 10/20] t/helper: fix leaking multi-pack-indices in "read-midx"

[Patrick Steinhardt]

Several of the subcommands of `test-helper read-midx` do not close the
MIDX that they have opened, leading to memory leaks. Fix those.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/helper/test-read-midx.c   | 8 +++++++-
 t/t5319-multi-pack-index.sh | 2 ++
 t/t7704-repack-cruft.sh     | 1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c
index 69757e94fc2..438fb9fc619 100644
--- a/t/helper/test-read-midx.c
+++ b/t/helper/test-read-midx.c
@@ -86,6 +86,8 @@ static int read_midx_checksum(const char *object_dir)
 	if (!m)
 		return 1;
 	printf("%s\n", hash_to_hex(get_midx_checksum(m)));
+
+	close_midx(m);
 	return 0;
 }
 
@@ -102,10 +104,12 @@ static int read_midx_preferred_pack(const char *object_dir)
 
 	if (midx_preferred_pack(midx, &preferred_pack) < 0) {
 		warning(_("could not determine MIDX preferred pack"));
+		close_midx(midx);
 		return 1;
 	}
 
 	printf("%s\n", midx->pack_names[preferred_pack]);
+	close_midx(midx);
 	return 0;
 }
 
@@ -122,8 +126,10 @@ static int read_midx_bitmapped_packs(const char *object_dir)
 		return 1;
 
 	for (i = 0; i < midx->num_packs + midx->num_packs_in_base; i++) {
-		if (nth_bitmapped_pack(the_repository, midx, &pack, i) < 0)
+		if (nth_bitmapped_pack(the_repository, midx, &pack, i) < 0) {
+			close_midx(midx);
 			return 1;
+		}
 
 		printf("%s\n", pack_basename(pack.p));
 		printf("  bitmap_pos: %"PRIuMAX"\n", (uintmax_t)pack.bitmap_pos);
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index ce1b58c7323..fbbc218d04a 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='multi-pack-indexes'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-chunk.sh
 . "$TEST_DIRECTORY"/lib-midx.sh
diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh
index 959e6e26488..5db9f4e10f7 100755
--- a/t/t7704-repack-cruft.sh
+++ b/t/t7704-repack-cruft.sh
@@ -2,6 +2,7 @@
 
 test_description='git repack works correctly'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 objdir=.git/objects
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 11/20] transport: fix leaking OID arrays in git:// transport data

[Patrick Steinhardt]

The transport data for the "git://" protocol contains two OID arrays
that we never free, creating a memory leak. Plug them.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5501-fetch-push-alternates.sh | 1 +
 t/t5519-push-alternates.sh       | 1 +
 transport.c                      | 2 ++
 3 files changed, 4 insertions(+)

diff --git a/t/t5501-fetch-push-alternates.sh b/t/t5501-fetch-push-alternates.sh
index 66f19a4ef2b..0c8668a1b8e 100755
--- a/t/t5501-fetch-push-alternates.sh
+++ b/t/t5501-fetch-push-alternates.sh
@@ -4,6 +4,7 @@ test_description='fetch/push involving alternates'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 count_objects () {
diff --git a/t/t5519-push-alternates.sh b/t/t5519-push-alternates.sh
index 20ba604dfde..72e97b15fab 100755
--- a/t/t5519-push-alternates.sh
+++ b/t/t5519-push-alternates.sh
@@ -5,6 +5,7 @@ test_description='push to a repository that borrows from elsewhere'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/transport.c b/transport.c
index 7c4af9f56f2..f0672fdc505 100644
--- a/transport.c
+++ b/transport.c
@@ -946,6 +946,8 @@ static int disconnect_git(struct transport *transport)
 	}
 
 	list_objects_filter_release(&data->options.filter_options);
+	oid_array_clear(&data->extra_have);
+	oid_array_clear(&data->shallow);
 	free(data);
 	return 0;
 }
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 12/20] builtin/send-pack: fix leaking refspecs

[Patrick Steinhardt]

We never free data associated with the assembled refspec in
git-send-pack(1), causing a memory leak. Fix this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/send-pack.c           | 1 +
 t/t5400-send-pack.sh          | 1 +
 t/t5401-update-hooks.sh       | 2 ++
 t/t5408-send-pack-stdin.sh    | 2 ++
 t/t5548-push-porcelain.sh     | 1 +
 t/t5812-proto-disable-http.sh | 2 ++
 6 files changed, 9 insertions(+)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 17cae6bbbdf..ef0df808249 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -338,5 +338,6 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 
 	free_refs(remote_refs);
 	free_refs(local_refs);
+	refspec_clear(&rs);
 	return ret;
 }
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 3f81f16e133..248c74d8ef2 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -9,6 +9,7 @@ test_description='See why rewinding head breaks send-pack
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 cnt=64
diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh
index d8cadeec733..3c1ea6086e7 100755
--- a/t/t5401-update-hooks.sh
+++ b/t/t5401-update-hooks.sh
@@ -4,6 +4,8 @@
 #
 
 test_description='Test the update hook infrastructure.'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t5408-send-pack-stdin.sh b/t/t5408-send-pack-stdin.sh
index e8737df6f95..c3695a4d4e3 100755
--- a/t/t5408-send-pack-stdin.sh
+++ b/t/t5408-send-pack-stdin.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='send-pack --stdin tests'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 create_ref () {
diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
index 6282728eaf3..ecb3877aa4b 100755
--- a/t/t5548-push-porcelain.sh
+++ b/t/t5548-push-porcelain.sh
@@ -4,6 +4,7 @@
 #
 test_description='Test git push porcelain output'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Create commits in <repo> and assign each commit's oid to shell variables
diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh
index 769c717e88b..f69959c64ca 100755
--- a/t/t5812-proto-disable-http.sh
+++ b/t/t5812-proto-disable-http.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test disabling of git-over-http in clone/fetch'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY/lib-proto-disable.sh"
 . "$TEST_DIRECTORY/lib-httpd.sh"
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 13/20] sideband: fix leaks when configuring sideband colors

[Patrick Steinhardt]

We read a bunch of configs in `use_sideband_colors()` to configure the
colors that Git should use. We never free the strings read from the
config though, causing memory leaks. Fix those.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 sideband.c                          | 8 +++++---
 t/t5409-colorize-remote-messages.sh | 1 +
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/sideband.c b/sideband.c
index 5d8907151fe..deb6ec0a8b7 100644
--- a/sideband.c
+++ b/sideband.c
@@ -30,7 +30,7 @@ static int use_sideband_colors(void)
 
 	const char *key = "color.remote";
 	struct strbuf sb = STRBUF_INIT;
-	char *value;
+	char *value = NULL;
 	int i;
 
 	if (use_sideband_colors_cached >= 0)
@@ -43,15 +43,17 @@ static int use_sideband_colors(void)
 	} else {
 		use_sideband_colors_cached = GIT_COLOR_AUTO;
 	}
+	FREE_AND_NULL(value);
 
 	for (i = 0; i < ARRAY_SIZE(keywords); i++) {
 		strbuf_reset(&sb);
 		strbuf_addf(&sb, "%s.%s", key, keywords[i].keyword);
 		if (git_config_get_string(sb.buf, &value))
 			continue;
-		if (color_parse(value, keywords[i].color))
-			continue;
+		color_parse(value, keywords[i].color);
+		FREE_AND_NULL(value);
 	}
+
 	strbuf_release(&sb);
 	return use_sideband_colors_cached;
 }
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
index fa5de4500a4..516b22fd963 100755
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -2,6 +2,7 @@
 
 test_description='remote messages are colorized on the client'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 14/20] builtin/fetch-pack: fix leaking refs

[Patrick Steinhardt]

We build several ref lists in git-fetch-pack(1), but never free them.
Fix those leaks.

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

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index af329e8d5cf..fe404d1305b 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -46,7 +46,7 @@ static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,
 int cmd_fetch_pack(int argc, const char **argv, const char *prefix UNUSED)
 {
 	int i, ret;
-	struct ref *ref = NULL;
+	struct ref *fetched_refs = NULL, *remote_refs = NULL;
 	const char *dest = NULL;
 	struct ref **sought = NULL;
 	int nr_sought = 0, alloc_sought = 0;
@@ -228,19 +228,20 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix UNUSED)
 	version = discover_version(&reader);
 	switch (version) {
 	case protocol_v2:
-		get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL,
+		get_remote_refs(fd[1], &reader, &remote_refs, 0, NULL, NULL,
 				args.stateless_rpc);
 		break;
 	case protocol_v1:
 	case protocol_v0:
-		get_remote_heads(&reader, &ref, 0, NULL, &shallow);
+		get_remote_heads(&reader, &remote_refs, 0, NULL, &shallow);
 		break;
 	case protocol_unknown_version:
 		BUG("unknown protocol version");
 	}
 
-	ref = fetch_pack(&args, fd, ref, sought, nr_sought,
+	fetched_refs = fetch_pack(&args, fd, remote_refs, sought, nr_sought,
 			 &shallow, pack_lockfiles_ptr, version);
+
 	if (pack_lockfiles.nr) {
 		int i;
 
@@ -260,7 +261,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix UNUSED)
 	if (finish_connect(conn))
 		return 1;
 
-	ret = !ref;
+	ret = !fetched_refs;
 
 	/*
 	 * If the heads to pull were given, we should have consumed
@@ -270,11 +271,14 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix UNUSED)
 	 */
 	ret |= report_unmatched_refs(sought, nr_sought);
 
-	while (ref) {
+	for (struct ref *ref = fetched_refs; ref; ref = ref->next)
 		printf("%s %s\n",
 		       oid_to_hex(&ref->old_oid), ref->name);
-		ref = ref->next;
-	}
 
+	for (size_t i = 0; i < nr_sought; i++)
+		free_one_ref(sought[i]);
+	free(sought);
+	free_refs(fetched_refs);
+	free_refs(remote_refs);
 	return ret;
 }
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 15/20] remote: fix leaking config strings

[Patrick Steinhardt]

We're leaking several config strings when assembling remotes, either
because we do not free preceding values in case a config was set
multiple times, or because we do not free them when releasing the remote
state. Plug those leaks.

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

diff --git a/remote.c b/remote.c
index 7d5b8f750d8..3087437bc61 100644
--- a/remote.c
+++ b/remote.c
@@ -373,8 +373,10 @@ static int handle_config(const char *key, const char *value,
 			return -1;
 		branch = make_branch(remote_state, name, namelen);
 		if (!strcmp(subkey, "remote")) {
+			FREE_AND_NULL(branch->remote_name);
 			return git_config_string(&branch->remote_name, key, value);
 		} else if (!strcmp(subkey, "pushremote")) {
+			FREE_AND_NULL(branch->pushremote_name);
 			return git_config_string(&branch->pushremote_name, key, value);
 		} else if (!strcmp(subkey, "merge")) {
 			if (!value)
@@ -406,9 +408,11 @@ static int handle_config(const char *key, const char *value,
 		return 0;
 
 	/* Handle remote.* variables */
-	if (!name && !strcmp(subkey, "pushdefault"))
+	if (!name && !strcmp(subkey, "pushdefault")) {
+		FREE_AND_NULL(remote_state->pushremote_name);
 		return git_config_string(&remote_state->pushremote_name, key,
 					 value);
+	}
 
 	if (!name)
 		return 0;
@@ -475,12 +479,15 @@ static int handle_config(const char *key, const char *value,
 		else if (!strcmp(value, "--tags"))
 			remote->fetch_tags = 2;
 	} else if (!strcmp(subkey, "proxy")) {
+		FREE_AND_NULL(remote->http_proxy);
 		return git_config_string(&remote->http_proxy,
 					 key, value);
 	} else if (!strcmp(subkey, "proxyauthmethod")) {
+		FREE_AND_NULL(remote->http_proxy_authmethod);
 		return git_config_string(&remote->http_proxy_authmethod,
 					 key, value);
 	} else if (!strcmp(subkey, "vcs")) {
+		FREE_AND_NULL(remote->foreign_vcs);
 		return git_config_string(&remote->foreign_vcs, key, value);
 	}
 	return 0;
@@ -2802,6 +2809,7 @@ void remote_state_clear(struct remote_state *remote_state)
 	for (i = 0; i < remote_state->remotes_nr; i++)
 		remote_clear(remote_state->remotes[i]);
 	FREE_AND_NULL(remote_state->remotes);
+	FREE_AND_NULL(remote_state->pushremote_name);
 	remote_state->remotes_alloc = 0;
 	remote_state->remotes_nr = 0;
 
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 16/20] remote: fix leaks when matching refspecs

[Patrick Steinhardt]

In `match_explicit()`, we try to match a source ref with a destination
ref according to a refspec item. This matching sometimes requires us to
allocate a new source spec so that it looks like we expect. And while we
in some end up assigning this allocated ref as `peer_ref`, which hands
over ownership of it to the caller, in other cases we don't. We neither
free it though, causing a memory leak.

Fix the leak by creating a common exit path where we can easily free the
source ref in case it is allocated and hasn't been handed over to the
caller.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 remote.c          | 43 +++++++++++++++++++++++++++++--------------
 t/t5505-remote.sh |  1 +
 2 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/remote.c b/remote.c
index 3087437bc61..892a47086db 100644
--- a/remote.c
+++ b/remote.c
@@ -1325,18 +1325,21 @@ static int match_explicit(struct ref *src, struct ref *dst,
 			  struct ref ***dst_tail,
 			  struct refspec_item *rs)
 {
-	struct ref *matched_src, *matched_dst;
-	int allocated_src;
+	struct ref *matched_src = NULL, *matched_dst = NULL;
+	int allocated_src = 0, ret;
 
 	const char *dst_value = rs->dst;
 	char *dst_guess;
 
-	if (rs->pattern || rs->matching || rs->negative)
-		return 0;
+	if (rs->pattern || rs->matching || rs->negative) {
+		ret = 0;
+		goto out;
+	}
 
-	matched_src = matched_dst = NULL;
-	if (match_explicit_lhs(src, rs, &matched_src, &allocated_src) < 0)
-		return -1;
+	if (match_explicit_lhs(src, rs, &matched_src, &allocated_src) < 0) {
+		ret = -1;
+		goto out;
+	}
 
 	if (!dst_value) {
 		int flag;
@@ -1375,18 +1378,30 @@ static int match_explicit(struct ref *src, struct ref *dst,
 		      dst_value);
 		break;
 	}
-	if (!matched_dst)
-		return -1;
-	if (matched_dst->peer_ref)
-		return error(_("dst ref %s receives from more than one src"),
-			     matched_dst->name);
-	else {
+
+	if (!matched_dst) {
+		ret = -1;
+		goto out;
+	}
+
+	if (matched_dst->peer_ref) {
+		ret = error(_("dst ref %s receives from more than one src"),
+			    matched_dst->name);
+		goto out;
+	} else {
 		matched_dst->peer_ref = allocated_src ?
 					matched_src :
 					copy_ref(matched_src);
 		matched_dst->force = rs->force;
+		matched_src = NULL;
 	}
-	return 0;
+
+	ret = 0;
+
+out:
+	if (allocated_src)
+		free_one_ref(matched_src);
+	return ret;
 }
 
 static int match_explicit_refs(struct ref *src, struct ref *dst,
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 08424e878e1..532035933f3 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -2,6 +2,7 @@
 
 test_description='git remote porcelain-ish'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 setup_repository () {
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 17/20] remote: fix leaking peer ref when expanding refmap

[Patrick Steinhardt]

When expanding remote refs via the refspec in `get_expanded_map()`, we
first copy the remote ref and then override its peer ref with the
expanded name. This may cause a memory leak though in case the peer ref
is already set, as this field is being copied by `copy_ref()`, as well.

Fix the leak by freeing the peer ref before we re-assign the field.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 remote.c                           | 2 ++
 t/t5536-fetch-conflicts.sh         | 1 +
 t/t5553-set-upstream.sh            | 1 +
 t/t5703-upload-pack-ref-in-want.sh | 1 +
 t/t6050-replace.sh                 | 1 +
 5 files changed, 6 insertions(+)

diff --git a/remote.c b/remote.c
index 892a47086db..6b50cafbbd3 100644
--- a/remote.c
+++ b/remote.c
@@ -2062,6 +2062,8 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,
 		    !ignore_symref_update(expn_name, &scratch)) {
 			struct ref *cpy = copy_ref(ref);
 
+			if (cpy->peer_ref)
+				free_one_ref(cpy->peer_ref);
 			cpy->peer_ref = alloc_ref(expn_name);
 			if (refspec->force)
 				cpy->peer_ref->force = 1;
diff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh
index 23bf6961700..2dcbe790523 100755
--- a/t/t5536-fetch-conflicts.sh
+++ b/t/t5536-fetch-conflicts.sh
@@ -2,6 +2,7 @@
 
 test_description='fetch handles conflicting refspecs correctly'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 D=$(pwd)
diff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh
index 70e3376d31b..33e919a17e1 100755
--- a/t/t5553-set-upstream.sh
+++ b/t/t5553-set-upstream.sh
@@ -4,6 +4,7 @@ test_description='"git fetch/pull --set-upstream" basic tests.'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 check_config () {
diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index 191097171bc..f75fae52c83 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -2,6 +2,7 @@
 
 test_description='upload-pack ref-in-want'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 get_actual_refs () {
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index c6e9b33e44e..d7702fc7562 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -7,6 +7,7 @@ test_description='Tests replace refs functionality'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY/lib-gpg.sh"
 
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 18/20] builtin/fetch: fix leaking transaction with `--atomic`

[Patrick Steinhardt]

With the `--atomic` flag, we use a single ref transaction to commit all
ref updates in git-fetch(1). The lifetime of transactions is somewhat
weird: while `ref_transaction_abort()` will free the transaction, a call
to `ref_transaction_commit()` won't. We thus have to manually free the
transaction in the successful case.

Adapt the code to free the transaction in the exit path to plug the
resulting memory leak. As `ref_transaction_abort()` already freed the
transaction for us, we have to unset the transaction when we hit that
code path to not cause a double free.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c         | 8 ++++----
 t/t5574-fetch-output.sh | 1 +
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index c297569a473..0264483c0e5 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1731,11 +1731,8 @@ static int do_fetch(struct transport *transport,
 			goto cleanup;
 
 		retcode = ref_transaction_commit(transaction, &err);
-		if (retcode) {
-			ref_transaction_free(transaction);
-			transaction = NULL;
+		if (retcode)
 			goto cleanup;
-		}
 	}
 
 	commit_fetch_head(&fetch_head);
@@ -1803,8 +1800,11 @@ static int do_fetch(struct transport *transport,
 		if (transaction && ref_transaction_abort(transaction, &err) &&
 		    err.len)
 			error("%s", err.buf);
+		transaction = NULL;
 	}
 
+	if (transaction)
+		ref_transaction_free(transaction);
 	display_state_release(&display_state);
 	close_fetch_head(&fetch_head);
 	strbuf_release(&err);
diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
index 5883839a04e..f7707326ea1 100755
--- a/t/t5574-fetch-output.sh
+++ b/t/t5574-fetch-output.sh
@@ -5,6 +5,7 @@ test_description='git fetch output format'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'fetch with invalid output format configuration' '
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 19/20] transport: fix leaking arguments when fetching from bundle

[Patrick Steinhardt]

In `fetch_refs_from_bundle()` we assemble a vector of arguments to pass
to `unbundle()`, but never free it. Fix this leak.

This memory leak gets hit in t5510, but fixing it isn't sufficient to
make the whole test suite pass.

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

diff --git a/transport.c b/transport.c
index f0672fdc505..da639d3bff0 100644
--- a/transport.c
+++ b/transport.c
@@ -189,6 +189,8 @@ static int fetch_refs_from_bundle(struct transport *transport,
 		       &extra_index_pack_args,
 		       fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0);
 	transport->hash_algo = data->header.hash_algo;
+
+	strvec_clear(&extra_index_pack_args);
 	return ret;
 }
 
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH 20/20] transport: fix leaking negotiation tips

[Patrick Steinhardt]

We do not free negotiation tips in the transport's smart options. Fix
this by freeing them on disconnect.

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

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 3b3991ab867..0890b9f61c5 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -5,6 +5,7 @@ test_description='Per branch config variables affects "git fetch".
 
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-bundle.sh
 
diff --git a/transport.c b/transport.c
index da639d3bff0..0f20fc56e40 100644
--- a/transport.c
+++ b/transport.c
@@ -947,6 +947,10 @@ static int disconnect_git(struct transport *transport)
 		finish_connect(data->conn);
 	}
 
+	if (data->options.negotiation_tips) {
+		oid_array_clear(data->options.negotiation_tips);
+		free(data->options.negotiation_tips);
+	}
 	list_objects_filter_release(&data->options.filter_options);
 	oid_array_clear(&data->extra_have);
 	oid_array_clear(&data->shallow);
-- 
2.46.0.164.g477ce5ccd6.dirty

Re: [PATCH 01/20] mailinfo: fix leaking header data

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> We populate the `mailinfo` arrays `p_hdr_data` and `s_hdr_data` with
> data parsed from the mail headers. These arrays may end up being only
> partially populated with gaps in case some of the headers do not parse
> properly. This causes memory leaks because `strbuf_list_free()` will
> stop iterating once it hits the first `NULL` pointer in the backing
> array.
>
> Fix this by open-coding a variant of `strbuf_list_free()` that knows to
> iterate through all headers.

Well spotted.  An array of strbuf is often a wrong data structure
for anything, but luckily we use it and suffer from a bug like this
only rarely.

The fix makes sense.  Will queue.



>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  mailinfo.c          | 17 +++++++++++++++--
>  t/t5100-mailinfo.sh |  1 +
>  2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/mailinfo.c b/mailinfo.c
> index 94b9b0abf22..a4fa64994ac 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -1290,8 +1290,21 @@ void clear_mailinfo(struct mailinfo *mi)
>  	strbuf_release(&mi->inbody_header_accum);
>  	free(mi->message_id);
>  
> -	strbuf_list_free(mi->p_hdr_data);
> -	strbuf_list_free(mi->s_hdr_data);
> +	for (size_t i = 0; header[i]; i++) {
> +		if (!mi->p_hdr_data[i])
> +			continue;
> +		strbuf_release(mi->p_hdr_data[i]);
> +		free(mi->p_hdr_data[i]);
> +	}
> +	free(mi->p_hdr_data);
> +
> +	for (size_t i = 0; header[i]; i++) {
> +		if (!mi->s_hdr_data[i])
> +			continue;
> +		strbuf_release(mi->s_hdr_data[i]);
> +		free(mi->s_hdr_data[i]);
> +	}
> +	free(mi->s_hdr_data);
>  
>  	while (mi->content < mi->content_top) {
>  		free(*(mi->content_top));
> diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
> index c8d06554541..065156c1f39 100755
> --- a/t/t5100-mailinfo.sh
> +++ b/t/t5100-mailinfo.sh
> @@ -5,6 +5,7 @@
>  
>  test_description='git mailinfo and git mailsplit test'
>  
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>  
>  DATA="$TEST_DIRECTORY/t5100"

Re: [PATCH 02/20] convert: fix leaks when resetting attributes

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> When resetting parsed gitattributes, we free the list of convert drivers
> parsed from the config. We only free some of the drivers' fields though
> and thus have memory leaks.
>
> Fix this by freeing all allocated convert driver fields to plug these
> memory leaks.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---

The helper has only one caller, which makes me wonder why we do not
have to call it more often.  If "git checkout" or "log -p" notice an
attribute file is updated, I wonder they should call it, for
example.  But of course this is completely out of scope of this
topic.

This has been leaking even before 9642479a (convert: fix leaking
config strings, 2024-08-01) straightened out the ownership rules of
the value obtained from git_config_string().

The fix makes sense.  Will queue.  Thanks.

Re: [PATCH 03/20] pretty: fix memory leaks when parsing pretty formats

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> When parsing pretty formats from the config we leak the name and user
> format whenever these are set multiple times. This is because we do not
> free any already-set value in case there is one.
>
> Plugging this leak for the name is trivial. For the user format we need
> to be a bit more careful, because we may end up assigning a pointer into
> the allocated region when the string is prefixed with either "format" or
> "tformat:". In order to make it safe to unconditionally free the user
> format we thus strdup the stripped string into the field instead of a
> pointer into the string.

Yup, the stripped one is trickier, but the change looks correct.

Will queue.

By the way, we tend to prefer no spaces after (cast) in our
codebase, but I just noticed that it is not spelled out in the
coding guidelines.  Comparing

    $ git grep -E -e '\((int|char \*)\) ' \*.c ':!compat/' ':!contrib/'
    $ git grep -E -e '\((int|char \*)\)[^ ]' \*.c ':!compat/' ':!contrib/'

tells me that the extra space after the (cast) is found mostly in
borrowed or imported sources and majority of culprits are found in
reftable library X-<.

Thanks.


--- >8 ---
Subject: CodingGuidelines: spaces around C operators

As we have operated with "write like how your surrounding code is
written" for too long, after a huge code drop from another project,
we'll end up being inconsistent before such an imported code is
cleaned up.  We have many uses of cast operator with a space before
its operand, mostly in the reftable code.

Spell the convention out before it spreads to other places.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/CodingGuidelines | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines
index e4bd0abdcd..ccaea39752 100644
--- c/Documentation/CodingGuidelines
+++ w/Documentation/CodingGuidelines
@@ -303,7 +303,9 @@ For C programs:
      v12.01, 2022-03-28).
 
  - Variables have to be declared at the beginning of the block, before
-   the first statement (i.e. -Wdeclaration-after-statement).
+   the first statement (i.e. -Wdeclaration-after-statement).  It is
+   encouraged to have a blank line between the end of the declarations
+   and the first statement in the block.
 
  - NULL pointers shall be written as NULL, not as 0.
 
@@ -323,6 +325,13 @@ For C programs:
         while( condition )
 		func (bar+1);
 
+ - A binary operator (other than ",") and ternary conditional "?:"
+   have a space on each side of the operator to separate it from its
+   operands.  E.g. "A + 1", not "A+1".
+
+ - A unary operator (other than "." and "->") have no space between it
+   and its operand.  E.g. "(char *)ptr", not "(char *) ptr".
+
  - Do not explicitly compare an integral value with constant 0 or '\0',
    or a pointer value with constant NULL.  For instance, to validate that
    counted array <ptr, cnt> is initialized but has no elements, write:

Re: [PATCH 05/20] builtin/merge-tree: fix leaking `-X` strategy options

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> The `-X` switch for git-merge-tree(1) will push each option into a local
> `xopts` vector that we then end up parsing. The vector never gets freed
> though, causing a memory leak. Plug it.

It is unfortunate that the ownership rules of the second parameter
to parse_merge_opt() lets the string borrowed by the merge_options
struct.  Otherwise we could have just cleared xopts immediately
after we are done with calling it.

The clean-up looks correct.  Will queue.
Thanks.


>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/merge-tree.c             | 13 ++++++++++---
>  t/t4301-merge-tree-write-tree.sh |  1 +
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index 9bca9b5f33c..c00469ed3db 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -533,6 +533,7 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
>  	int expected_remaining_argc;
>  	int original_argc;
>  	const char *merge_base = NULL;
> +	int ret;
>  
>  	const char * const merge_tree_usage[] = {
>  		N_("git merge-tree [--write-tree] [<options>] <branch1> <branch2>"),
> @@ -625,7 +626,9 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
>  			strbuf_list_free(split);
>  		}
>  		strbuf_release(&buf);
> -		return 0;
> +
> +		ret = 0;
> +		goto out;
>  	}
>  
>  	/* Figure out which mode to use */
> @@ -664,7 +667,11 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
>  
>  	/* Do the relevant type of merge */
>  	if (o.mode == MODE_REAL)
> -		return real_merge(&o, merge_base, argv[0], argv[1], prefix);
> +		ret = real_merge(&o, merge_base, argv[0], argv[1], prefix);
>  	else
> -		return trivial_merge(argv[0], argv[1], argv[2]);
> +		ret = trivial_merge(argv[0], argv[1], argv[2]);
> +
> +out:
> +	strvec_clear(&xopts);
> +	return ret;
>  }
> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
> index eea19907b55..37f1cd7364c 100755
> --- a/t/t4301-merge-tree-write-tree.sh
> +++ b/t/t4301-merge-tree-write-tree.sh
> @@ -2,6 +2,7 @@
>  
>  test_description='git merge-tree --write-tree'
>  
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>  
>  # This test is ort-specific

Re: [PATCH 08/20] midx-write: fix leaking hashfile on error cases

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> When writing the MIDX file we first create the `struct hashfile` used to
> write the trailer hash, and then afterwards we verify whether we can
> actually write the MIDX in the first place. When we decide that we
> can't, this leads to a memory leak because we never free the hash file
> contents.
>
> We could fix this by freeing the hashfile on the exit path. There is a
> better option though: we can simply move the checks for the error
> condition earlier. As there is no early exit between creating the
> hashfile and finalizing it anymore this is sufficient to fix the memory
> leak.

The above is a good explanation why "are we dropping everything"
block was moved up, but does not explain why the other "if there is
no objects" block has to move (it is however easy to see that the
latter block can be moved without any bad side effect).

In any case, the struct hashfile hashfd() gives us is now associated
with the struct chunkfile cf immediately after it is instanciated,
and there is no early exit while the chunkfile is in use, which is
great.

Re: [PATCH 13/20] sideband: fix leaks when configuring sideband colors

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> We read a bunch of configs in `use_sideband_colors()` to configure the
> colors that Git should use. We never free the strings read from the
> config though, causing memory leaks. Fix those.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  sideband.c                          | 8 +++++---
>  t/t5409-colorize-remote-messages.sh | 1 +
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/sideband.c b/sideband.c
> index 5d8907151fe..deb6ec0a8b7 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -30,7 +30,7 @@ static int use_sideband_colors(void)
>  
>  	const char *key = "color.remote";
>  	struct strbuf sb = STRBUF_INIT;
> -	char *value;
> +	char *value = NULL;

Hmph, this is a bit unfortunate.  If there is no configuration
variable, on the first call to this function, we come to the end of
if/else cascade and we need to free.

Another possibility to convey the intention better may be to assign
NULL to value after the "we already know the value, so return early"
took place.

> @@ -43,15 +43,17 @@ static int use_sideband_colors(void)
>  	} else {
>  		use_sideband_colors_cached = GIT_COLOR_AUTO;
>  	}
> +	FREE_AND_NULL(value);

This can be a simple "free(value)"; I do not see the need to clear
the variable at this point.

>  	for (i = 0; i < ARRAY_SIZE(keywords); i++) {
>  		strbuf_reset(&sb);
>  		strbuf_addf(&sb, "%s.%s", key, keywords[i].keyword);
>  		if (git_config_get_string(sb.buf, &value))
>  			continue;
> -		if (color_parse(value, keywords[i].color))
> -			continue;
> +		color_parse(value, keywords[i].color);
> +		FREE_AND_NULL(value);

Likewise.  I do not see the need to clear.  We only come here after
we got something from gti_config_get_string().  That value may or
may not fail color_parse(), but when we reach this point, value
always has something we need to free, not some leftover value from
the previous iteration.

The patch is _not_ wrong per-se, though.

Thanks.

Re: [PATCH 14/20] builtin/fetch-pack: fix leaking refs

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> We build several ref lists in git-fetch-pack(1), but never free them.
> Fix those leaks.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/fetch-pack.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)

Quite straight-forward.  In addition to plugging leaks, the
resulting code is much easier to reason about by avoiding the reuse
of the variable.

Re: [PATCH 15/20] remote: fix leaking config strings

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> @@ -2802,6 +2809,7 @@ void remote_state_clear(struct remote_state *remote_state)
>  	for (i = 0; i < remote_state->remotes_nr; i++)
>  		remote_clear(remote_state->remotes[i]);
>  	FREE_AND_NULL(remote_state->remotes);
> +	FREE_AND_NULL(remote_state->pushremote_name);
>  	remote_state->remotes_alloc = 0;
>  	remote_state->remotes_nr = 0;

As remote_state has two extra structures embedded in it, I wonder if
we should be clearing them in this function, but possibly it is
cleared elsewhere or perhaps in a later series?

As the focus of this step is about strings that we obtained from the
config API, it is totally outside the scope of this topic, even if
it turns out to be needed to clear them.

Looking good.  Thanks.

Re: [PATCH 19/20] transport: fix leaking arguments when fetching from bundle

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> In `fetch_refs_from_bundle()` we assemble a vector of arguments to pass
> to `unbundle()`, but never free it. Fix this leak.
>
> This memory leak gets hit in t5510, but fixing it isn't sufficient to
> make the whole test suite pass.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  transport.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/transport.c b/transport.c
> index f0672fdc505..da639d3bff0 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -189,6 +189,8 @@ static int fetch_refs_from_bundle(struct transport *transport,
>  		       &extra_index_pack_args,
>  		       fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0);
>  	transport->hash_algo = data->header.hash_algo;
> +
> +	strvec_clear(&extra_index_pack_args);
>  	return ret;
>  }

Hmph.  unbundle() has this:

	if (extra_index_pack_args) {
		strvec_pushv(&ip.args, extra_index_pack_args->v);
		strvec_clear(extra_index_pack_args);
	}

so while I think this patch would not hurt at all, do we need this
patch?

The other caller of unbundle() that passes the extra_index_pack_args
is cmd_bundle_unbundle() and it does not do anything after it is
done.

Re: [PATCH 03/20] pretty: fix memory leaks when parsing pretty formats

[Patrick Steinhardt]

On Tue, Aug 20, 2024 at 01:36:11PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > When parsing pretty formats from the config we leak the name and user
> > format whenever these are set multiple times. This is because we do not
> > free any already-set value in case there is one.
> >
> > Plugging this leak for the name is trivial. For the user format we need
> > to be a bit more careful, because we may end up assigning a pointer into
> > the allocated region when the string is prefixed with either "format" or
> > "tformat:". In order to make it safe to unconditionally free the user
> > format we thus strdup the stripped string into the field instead of a
> > pointer into the string.
> 
> Yup, the stripped one is trickier, but the change looks correct.
> 
> Will queue.
> 
> By the way, we tend to prefer no spaces after (cast) in our
> codebase, but I just noticed that it is not spelled out in the
> coding guidelines.  Comparing
> 
>     $ git grep -E -e '\((int|char \*)\) ' \*.c ':!compat/' ':!contrib/'
>     $ git grep -E -e '\((int|char \*)\)[^ ]' \*.c ':!compat/' ':!contrib/'
> 
> tells me that the extra space after the (cast) is found mostly in
> borrowed or imported sources and majority of culprits are found in
> reftable library X-<.

Not entirely surprising. I myself have typically favored adding a space
after the cast, and I remember wondering about the coding style several
times here. I never wondered enough to actually check though.

So thanks for clarifying and thanks for updating the coding guidelines
around this.

Patrick

Re: [PATCH 08/20] midx-write: fix leaking hashfile on error cases

[Patrick Steinhardt]

On Tue, Aug 20, 2024 at 04:19:13PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > When writing the MIDX file we first create the `struct hashfile` used to
> > write the trailer hash, and then afterwards we verify whether we can
> > actually write the MIDX in the first place. When we decide that we
> > can't, this leads to a memory leak because we never free the hash file
> > contents.
> >
> > We could fix this by freeing the hashfile on the exit path. There is a
> > better option though: we can simply move the checks for the error
> > condition earlier. As there is no early exit between creating the
> > hashfile and finalizing it anymore this is sufficient to fix the memory
> > leak.
> 
> The above is a good explanation why "are we dropping everything"
> block was moved up, but does not explain why the other "if there is
> no objects" block has to move (it is however easy to see that the
> latter block can be moved without any bad side effect).
> 
> In any case, the struct hashfile hashfd() gives us is now associated
> with the struct chunkfile cf immediately after it is instanciated,
> and there is no early exit while the chunkfile is in use, which is
> great.

There basically is no reason why the other block needs to move. It just
felt more natural to keep massaging of parameters closly together before
we go off and actually do "the thing". I'll document this accordingly.

Patrick

Re: [PATCH 15/20] remote: fix leaking config strings

[Patrick Steinhardt]

On Wed, Aug 21, 2024 at 10:58:55AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > @@ -2802,6 +2809,7 @@ void remote_state_clear(struct remote_state *remote_state)
> >  	for (i = 0; i < remote_state->remotes_nr; i++)
> >  		remote_clear(remote_state->remotes[i]);
> >  	FREE_AND_NULL(remote_state->remotes);
> > +	FREE_AND_NULL(remote_state->pushremote_name);
> >  	remote_state->remotes_alloc = 0;
> >  	remote_state->remotes_nr = 0;
> 
> As remote_state has two extra structures embedded in it, I wonder if
> we should be clearing them in this function, but possibly it is
> cleared elsewhere or perhaps in a later series?

It is not yet part of any subsequent patch series, mostly because I
didn't happen to stumble over such leaks yet. Both of the rewrites very
much are leaky though, and would be hit when we use "insteadOf" or
"pushInsteadOf" configs.

The `struct branch` also needs handling and is being populated via
"branch" configs.

> As the focus of this step is about strings that we obtained from the
> config API, it is totally outside the scope of this topic, even if
> it turns out to be needed to clear them.

Well, these are being populated via config strings. So I'd rather fix
them in this commit, as well.

Patrick

Re: [PATCH 19/20] transport: fix leaking arguments when fetching from bundle

[Patrick Steinhardt]

On Wed, Aug 21, 2024 at 11:07:39AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > In `fetch_refs_from_bundle()` we assemble a vector of arguments to pass
> > to `unbundle()`, but never free it. Fix this leak.
> >
> > This memory leak gets hit in t5510, but fixing it isn't sufficient to
> > make the whole test suite pass.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  transport.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/transport.c b/transport.c
> > index f0672fdc505..da639d3bff0 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -189,6 +189,8 @@ static int fetch_refs_from_bundle(struct transport *transport,
> >  		       &extra_index_pack_args,
> >  		       fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0);
> >  	transport->hash_algo = data->header.hash_algo;
> > +
> > +	strvec_clear(&extra_index_pack_args);
> >  	return ret;
> >  }
> 
> Hmph.  unbundle() has this:
> 
> 	if (extra_index_pack_args) {
> 		strvec_pushv(&ip.args, extra_index_pack_args->v);
> 		strvec_clear(extra_index_pack_args);
> 	}
> 
> so while I think this patch would not hurt at all, do we need this
> patch?

Yes we do, because there is an early return in case `verify_bundle()`
fails. I didn't notice that we have it in `unbundle()` though.

> The other caller of unbundle() that passes the extra_index_pack_args
> is cmd_bundle_unbundle() and it does not do anything after it is
> done.

I'd argue it's bad practice to have `unbundle()` clear the caller
provided args for us and somewhat surprising. While one way to fix this
would be to have a common exit path where we always free them. But I'd
much rather make it the responsibility of the caller itself to free
them.

I'll adapt the code in this spirit.

Patrick

Re: [PATCH 13/20] sideband: fix leaks when configuring sideband colors

[Patrick Steinhardt]

On Tue, Aug 20, 2024 at 04:52:59PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > We read a bunch of configs in `use_sideband_colors()` to configure the
> > colors that Git should use. We never free the strings read from the
> > config though, causing memory leaks. Fix those.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  sideband.c                          | 8 +++++---
> >  t/t5409-colorize-remote-messages.sh | 1 +
> >  2 files changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/sideband.c b/sideband.c
> > index 5d8907151fe..deb6ec0a8b7 100644
> > --- a/sideband.c
> > +++ b/sideband.c
> > @@ -30,7 +30,7 @@ static int use_sideband_colors(void)
> >  
> >  	const char *key = "color.remote";
> >  	struct strbuf sb = STRBUF_INIT;
> > -	char *value;
> > +	char *value = NULL;
> 
> Hmph, this is a bit unfortunate.  If there is no configuration
> variable, on the first call to this function, we come to the end of
> if/else cascade and we need to free.
> 
> Another possibility to convey the intention better may be to assign
> NULL to value after the "we already know the value, so return early"
> took place.

There is an even better option: just use `git_config_get_string_tmp()`.
Then we don't have to worry about freeing the strings at all.

Patrick

[PATCH v2 00/20] Memory leak fixes (pt.5)

[Patrick Steinhardt]

Hi,

this is version 2 of this patch series that fixes another set of memory
leaks.

Changes compared to v1:

  - Remove spaces between cast and variable.

  - Clarify why we move the `ctx.entries_nr` block.

  - Adapt sideband colors to use `git_config_get_string_tmp()` such that
    we do not have to allocate the strings in the first place.

  - Fix more memory leaks for config values in the remote state.

  - Refactor `unbundle()` to not free extra args passed by the caller
    anymore. Instead, this is now always done by the caler.

Thanks!

Patrick

Patrick Steinhardt (20):
  mailinfo: fix leaking header data
  convert: fix leaks when resetting attributes
  pretty: fix memory leaks when parsing pretty formats
  pretty: fix leaking key/value separator buffer
  builtin/merge-tree: fix leaking `-X` strategy options
  builtin/upload-archive: fix leaking args passed to `write_archive()`
  builtin/archive: fix leaking `OPT_FILENAME()` value
  midx-write: fix leaking hashfile on error cases
  builtin/repack: fix leaks when computing packs to repack
  t/helper: fix leaking multi-pack-indices in "read-midx"
  transport: fix leaking OID arrays in git:// transport data
  builtin/send-pack: fix leaking refspecs
  sideband: fix leaks when configuring sideband colors
  builtin/fetch-pack: fix leaking refs
  remote: fix leaking config strings
  remote: fix leaks when matching refspecs
  remote: fix leaking peer ref when expanding refmap
  builtin/fetch: fix leaking transaction with `--atomic`
  transport: fix leaking arguments when fetching from bundle
  transport: fix leaking negotiation tips

 archive.c                           | 10 ++++
 builtin/archive.c                   |  7 ++-
 builtin/bundle.c                    |  2 +
 builtin/fetch-pack.c                | 20 ++++---
 builtin/fetch.c                     |  8 +--
 builtin/merge-tree.c                | 13 ++++-
 builtin/repack.c                    | 36 +++++++++---
 builtin/send-pack.c                 |  1 +
 builtin/upload-archive.c            |  8 ++-
 bundle.c                            |  4 +-
 convert.c                           |  3 +
 mailinfo.c                          | 17 +++++-
 midx-write.c                        | 24 ++++----
 pretty.c                            | 13 ++++-
 remote.c                            | 85 +++++++++++++++++++++++------
 sideband.c                          | 15 +++--
 t/helper/test-read-midx.c           |  8 ++-
 t/t4150-am.sh                       |  1 +
 t/t4205-log-pretty-formats.sh       |  2 +
 t/t4301-merge-tree-write-tree.sh    |  1 +
 t/t5000-tar-tree.sh                 |  1 +
 t/t5003-archive-zip.sh              |  1 +
 t/t5100-mailinfo.sh                 |  1 +
 t/t5319-multi-pack-index.sh         |  2 +
 t/t5400-send-pack.sh                |  1 +
 t/t5401-update-hooks.sh             |  2 +
 t/t5408-send-pack-stdin.sh          |  2 +
 t/t5409-colorize-remote-messages.sh |  1 +
 t/t5501-fetch-push-alternates.sh    |  1 +
 t/t5505-remote.sh                   |  1 +
 t/t5510-fetch.sh                    |  1 +
 t/t5519-push-alternates.sh          |  1 +
 t/t5536-fetch-conflicts.sh          |  1 +
 t/t5548-push-porcelain.sh           |  1 +
 t/t5553-set-upstream.sh             |  1 +
 t/t5574-fetch-output.sh             |  1 +
 t/t5703-upload-pack-ref-in-want.sh  |  1 +
 t/t5812-proto-disable-http.sh       |  2 +
 t/t6050-replace.sh                  |  1 +
 t/t7704-repack-cruft.sh             |  1 +
 transport.c                         |  8 +++
 41 files changed, 237 insertions(+), 73 deletions(-)

Range-diff against v1:
 1:  69e30ea5179 =  1:  69e30ea5179 mailinfo: fix leaking header data
 2:  ed0f01bf92c =  2:  ed0f01bf92c convert: fix leaks when resetting attributes
 3:  82f3908f962 !  3:  8a1963685e7 pretty: fix memory leaks when parsing pretty formats
    @@ pretty.c: static int git_pretty_formats_config(const char *var, const char *valu
      		commit_formats_len++;
      	}
      
    -+	free((char *) commit_format->name);
    ++	free((char *)commit_format->name);
      	commit_format->name = xstrdup(name);
      	commit_format->format = CMIT_FMT_USERFORMAT;
      	if (git_config_string(&fmt, var, value))
      		return -1;
      
     -	if (skip_prefix(fmt, "format:", &commit_format->user_format)) {
    -+	free((char *) commit_format->user_format);
    ++	free((char *)commit_format->user_format);
     +	if (skip_prefix(fmt, "format:", &stripped)) {
      		commit_format->is_tformat = 0;
     -	} else if (skip_prefix(fmt, "tformat:", &commit_format->user_format)) {
 4:  696467780e6 =  4:  1c368a4489a pretty: fix leaking key/value separator buffer
 5:  53db2fc7206 =  5:  c62c5e9604e builtin/merge-tree: fix leaking `-X` strategy options
 6:  5b05a325218 =  6:  afdd7f90b13 builtin/upload-archive: fix leaking args passed to `write_archive()`
 7:  bad575df126 =  7:  38487f3f65b builtin/archive: fix leaking `OPT_FILENAME()` value
 8:  5f042ce5098 !  8:  693c93ddbf7 midx-write: fix leaking hashfile on error cases
    @@ Commit message
         hashfile and finalizing it anymore this is sufficient to fix the memory
         leak.
     
    +    While at it, also move around the block checking for `ctx.entries_nr`.
    +    This change is not required to fix the memory leak, but it feels natural
    +    to move together all massaging of parameters before we go with them and
    +    execute the actual logic.
    +
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## midx-write.c ##
 9:  5c820da9761 =  9:  6bca72e5c57 builtin/repack: fix leaks when computing packs to repack
10:  9caf5eeea93 = 10:  48b60279d18 t/helper: fix leaking multi-pack-indices in "read-midx"
11:  8e12c55536d = 11:  0cb440ef648 transport: fix leaking OID arrays in git:// transport data
12:  5d8e0a3d8b4 = 12:  f4300c9326b builtin/send-pack: fix leaking refspecs
13:  5d09959b642 ! 13:  28805c15a42 sideband: fix leaks when configuring sideband colors
    @@ Commit message
     
         We read a bunch of configs in `use_sideband_colors()` to configure the
         colors that Git should use. We never free the strings read from the
    -    config though, causing memory leaks. Fix those.
    +    config though, causing memory leaks.
    +
    +    Refactor the code to use `git_config_get_string_tmp()` instead, which
    +    does not allocate memory. As we throw the strings away after parsing
    +    them anyway there is no need to use allocated strings.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ sideband.c: static int use_sideband_colors(void)
      	const char *key = "color.remote";
      	struct strbuf sb = STRBUF_INIT;
     -	char *value;
    -+	char *value = NULL;
    ++	const char *value;
      	int i;
      
      	if (use_sideband_colors_cached >= 0)
    -@@ sideband.c: static int use_sideband_colors(void)
    - 	} else {
    + 		return use_sideband_colors_cached;
    + 
    +-	if (!git_config_get_string(key, &value)) {
    ++	if (!git_config_get_string_tmp(key, &value))
    + 		use_sideband_colors_cached = git_config_colorbool(key, value);
    +-	} else if (!git_config_get_string("color.ui", &value)) {
    ++	else if (!git_config_get_string_tmp("color.ui", &value))
    + 		use_sideband_colors_cached = git_config_colorbool("color.ui", value);
    +-	} else {
    ++	else
      		use_sideband_colors_cached = GIT_COLOR_AUTO;
    - 	}
    -+	FREE_AND_NULL(value);
    +-	}
      
      	for (i = 0; i < ARRAY_SIZE(keywords); i++) {
      		strbuf_reset(&sb);
      		strbuf_addf(&sb, "%s.%s", key, keywords[i].keyword);
    - 		if (git_config_get_string(sb.buf, &value))
    - 			continue;
    --		if (color_parse(value, keywords[i].color))
    +-		if (git_config_get_string(sb.buf, &value))
     -			continue;
    +-		if (color_parse(value, keywords[i].color))
    ++		if (git_config_get_string_tmp(sb.buf, &value))
    + 			continue;
     +		color_parse(value, keywords[i].color);
    -+		FREE_AND_NULL(value);
      	}
     +
      	strbuf_release(&sb);
14:  1c94195488d = 14:  aac84c5a2b7 builtin/fetch-pack: fix leaking refs
15:  97346d6f944 ! 15:  532328b7814 remote: fix leaking config strings
    @@ Commit message
         We're leaking several config strings when assembling remotes, either
         because we do not free preceding values in case a config was set
         multiple times, or because we do not free them when releasing the remote
    -    state. Plug those leaks.
    +    state. This includes config strings for "branch" sections, "insteadOf",
    +    "pushInsteadOf", and "pushDefault".
    +
    +    Plug those leaks.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## remote.c ##
    +@@ remote.c: static struct branch *make_branch(struct remote_state *remote_state,
    + 	return ret;
    + }
    + 
    ++static void branch_release(struct branch *branch)
    ++{
    ++	free((char *)branch->name);
    ++	free((char *)branch->refname);
    ++	free(branch->remote_name);
    ++	free(branch->pushremote_name);
    ++	for (int i = 0; i < branch->merge_nr; i++)
    ++		refspec_item_clear(branch->merge[i]);
    ++	free(branch->merge);
    ++}
    ++
    + static struct rewrite *make_rewrite(struct rewrites *r,
    + 				    const char *base, size_t len)
    + {
    +@@ remote.c: static struct rewrite *make_rewrite(struct rewrites *r,
    + 	return ret;
    + }
    + 
    ++static void rewrites_release(struct rewrites *r)
    ++{
    ++	for (int i = 0; i < r->rewrite_nr; i++)
    ++		free((char *)r->rewrite[i]->base);
    ++	free(r->rewrite);
    ++	memset(r, 0, sizeof(*r));
    ++}
    ++
    + static void add_instead_of(struct rewrite *rewrite, const char *instead_of)
    + {
    + 	ALLOC_GROW(rewrite->instead_of, rewrite->instead_of_nr + 1, rewrite->instead_of_alloc);
     @@ remote.c: static int handle_config(const char *key, const char *value,
      			return -1;
      		branch = make_branch(remote_state, name, namelen);
    @@ remote.c: static int handle_config(const char *key, const char *value,
      		return git_config_string(&remote->foreign_vcs, key, value);
      	}
      	return 0;
    -@@ remote.c: void remote_state_clear(struct remote_state *remote_state)
    +@@ remote.c: struct remote_state *remote_state_new(void)
    + 
    + void remote_state_clear(struct remote_state *remote_state)
    + {
    ++	struct hashmap_iter iter;
    ++	struct branch *b;
    + 	int i;
    + 
      	for (i = 0; i < remote_state->remotes_nr; i++)
      		remote_clear(remote_state->remotes[i]);
      	FREE_AND_NULL(remote_state->remotes);
    @@ remote.c: void remote_state_clear(struct remote_state *remote_state)
      	remote_state->remotes_alloc = 0;
      	remote_state->remotes_nr = 0;
      
    ++	rewrites_release(&remote_state->rewrites);
    ++	rewrites_release(&remote_state->rewrites_push);
    ++
    + 	hashmap_clear_and_free(&remote_state->remotes_hash, struct remote, ent);
    +-	hashmap_clear_and_free(&remote_state->branches_hash, struct remote, ent);
    ++	hashmap_for_each_entry(&remote_state->branches_hash, &iter, b, ent) {
    ++		branch_release(b);
    ++		free(b);
    ++	}
    ++	hashmap_clear(&remote_state->branches_hash);
    + }
    + 
    + /*
16:  e1d0be37636 = 16:  440b3d99372 remote: fix leaks when matching refspecs
17:  773fe580d75 = 17:  662ec4e6484 remote: fix leaking peer ref when expanding refmap
18:  9ede792550e = 18:  dbd8eaa2cb1 builtin/fetch: fix leaking transaction with `--atomic`
19:  b72f7d1ee39 ! 19:  4c5740afe43 transport: fix leaking arguments when fetching from bundle
    @@ Commit message
         transport: fix leaking arguments when fetching from bundle
     
         In `fetch_refs_from_bundle()` we assemble a vector of arguments to pass
    -    to `unbundle()`, but never free it. Fix this leak.
    +    to `unbundle()`, but never free it. And in theory we wouldn't have to
    +    because `unbundle()` already knows to free the vector for us. But it
    +    fails to do so when it exits early due to `verify_bundle()` failing.
    +
    +    The calling convention that the arguments are freed by the callee and
    +    not the caller feels somewhat weird. Refactor the code such that it is
    +    instead the responsibility of the caller to free the vector, adapting
    +    the only two callsites where we pass extra arguments. This also fixes
    +    the memory leak.
     
         This memory leak gets hit in t5510, but fixing it isn't sufficient to
         make the whole test suite pass.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    + ## builtin/bundle.c ##
    +@@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
    + 			 &extra_index_pack_args, 0) ||
    + 		list_bundle_refs(&header, argc, argv);
    + 	bundle_header_release(&header);
    ++
    + cleanup:
    ++	strvec_clear(&extra_index_pack_args);
    + 	free(bundle_file);
    + 	return ret;
    + }
    +
    + ## bundle.c ##
    +@@ bundle.c: int unbundle(struct repository *r, struct bundle_header *header,
    + 	if (flags & VERIFY_BUNDLE_FSCK)
    + 		strvec_push(&ip.args, "--fsck-objects");
    + 
    +-	if (extra_index_pack_args) {
    ++	if (extra_index_pack_args)
    + 		strvec_pushv(&ip.args, extra_index_pack_args->v);
    +-		strvec_clear(extra_index_pack_args);
    +-	}
    + 
    + 	ip.in = bundle_fd;
    + 	ip.no_stdout = 1;
    +
      ## transport.c ##
     @@ transport.c: static int fetch_refs_from_bundle(struct transport *transport,
      		       &extra_index_pack_args,
20:  e8f479deeb2 = 20:  c3d2b035761 transport: fix leaking negotiation tips
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 01/20] mailinfo: fix leaking header data

[Patrick Steinhardt]

We populate the `mailinfo` arrays `p_hdr_data` and `s_hdr_data` with
data parsed from the mail headers. These arrays may end up being only
partially populated with gaps in case some of the headers do not parse
properly. This causes memory leaks because `strbuf_list_free()` will
stop iterating once it hits the first `NULL` pointer in the backing
array.

Fix this by open-coding a variant of `strbuf_list_free()` that knows to
iterate through all headers.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 mailinfo.c          | 17 +++++++++++++++--
 t/t5100-mailinfo.sh |  1 +
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/mailinfo.c b/mailinfo.c
index 94b9b0abf22..a4fa64994ac 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -1290,8 +1290,21 @@ void clear_mailinfo(struct mailinfo *mi)
 	strbuf_release(&mi->inbody_header_accum);
 	free(mi->message_id);
 
-	strbuf_list_free(mi->p_hdr_data);
-	strbuf_list_free(mi->s_hdr_data);
+	for (size_t i = 0; header[i]; i++) {
+		if (!mi->p_hdr_data[i])
+			continue;
+		strbuf_release(mi->p_hdr_data[i]);
+		free(mi->p_hdr_data[i]);
+	}
+	free(mi->p_hdr_data);
+
+	for (size_t i = 0; header[i]; i++) {
+		if (!mi->s_hdr_data[i])
+			continue;
+		strbuf_release(mi->s_hdr_data[i]);
+		free(mi->s_hdr_data[i]);
+	}
+	free(mi->s_hdr_data);
 
 	while (mi->content < mi->content_top) {
 		free(*(mi->content_top));
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index c8d06554541..065156c1f39 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -5,6 +5,7 @@
 
 test_description='git mailinfo and git mailsplit test'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 DATA="$TEST_DIRECTORY/t5100"
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 02/20] convert: fix leaks when resetting attributes

[Patrick Steinhardt]

When resetting parsed gitattributes, we free the list of convert drivers
parsed from the config. We only free some of the drivers' fields though
and thus have memory leaks.

Fix this by freeing all allocated convert driver fields to plug these
memory leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 convert.c     | 3 +++
 t/t4150-am.sh | 1 +
 2 files changed, 4 insertions(+)

diff --git a/convert.c b/convert.c
index e6184d21f26..c9a31eb4f03 100644
--- a/convert.c
+++ b/convert.c
@@ -1371,6 +1371,9 @@ void reset_parsed_attributes(void)
 	for (drv = user_convert; drv; drv = next) {
 		next = drv->next;
 		free((void *)drv->name);
+		free((void *)drv->smudge);
+		free((void *)drv->clean);
+		free((void *)drv->process);
 		free(drv);
 	}
 	user_convert = NULL;
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 5e2b6c80eae..232e1394e8d 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -5,6 +5,7 @@ test_description='git am running'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup: messages' '
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 03/20] pretty: fix memory leaks when parsing pretty formats

[Patrick Steinhardt]

When parsing pretty formats from the config we leak the name and user
format whenever these are set multiple times. This is because we do not
free any already-set value in case there is one.

Plugging this leak for the name is trivial. For the user format we need
to be a bit more careful, because we may end up assigning a pointer into
the allocated region when the string is prefixed with either "format" or
"tformat:". In order to make it safe to unconditionally free the user
format we thus strdup the stripped string into the field instead of a
pointer into the string.

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

diff --git a/pretty.c b/pretty.c
index 44222fb83c6..5e162d7204d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -63,7 +63,7 @@ static int git_pretty_formats_config(const char *var, const char *value,
 				     void *cb UNUSED)
 {
 	struct cmt_fmt_map *commit_format = NULL;
-	const char *name;
+	const char *name, *stripped;
 	char *fmt;
 	int i;
 
@@ -90,15 +90,21 @@ static int git_pretty_formats_config(const char *var, const char *value,
 		commit_formats_len++;
 	}
 
+	free((char *)commit_format->name);
 	commit_format->name = xstrdup(name);
 	commit_format->format = CMIT_FMT_USERFORMAT;
 	if (git_config_string(&fmt, var, value))
 		return -1;
 
-	if (skip_prefix(fmt, "format:", &commit_format->user_format)) {
+	free((char *)commit_format->user_format);
+	if (skip_prefix(fmt, "format:", &stripped)) {
 		commit_format->is_tformat = 0;
-	} else if (skip_prefix(fmt, "tformat:", &commit_format->user_format)) {
+		commit_format->user_format = xstrdup(stripped);
+		free(fmt);
+	} else if (skip_prefix(fmt, "tformat:", &stripped)) {
 		commit_format->is_tformat = 1;
+		commit_format->user_format = xstrdup(stripped);
+		free(fmt);
 	} else if (strchr(fmt, '%')) {
 		commit_format->is_tformat = 1;
 		commit_format->user_format = fmt;
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 04/20] pretty: fix leaking key/value separator buffer

[Patrick Steinhardt]

The `format_set_trailers_options()` function is responsible for parsing
a custom pretty format for trailers. It puts the parsed options into a
`struct process_trailer_options` structure, while the allocated memory
required for this will be put into separate caller-provided arguments.
It is thus the caller's responsibility to free the memory not via the
options structure, but via the other parameters.

While we do this alright for the separator and filter keys, we do not
free the memory associated with the key/value separator. Fix this to
plug this memory leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 pretty.c                      | 1 +
 t/t4205-log-pretty-formats.sh | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/pretty.c b/pretty.c
index 5e162d7204d..5e5ae452530 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1776,6 +1776,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		}
 	trailer_out:
 		string_list_clear(&filter_list, 0);
+		strbuf_release(&kvsepbuf);
 		strbuf_release(&sepbuf);
 		return ret;
 	}
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 158b49d4b60..eb63ce011fa 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -5,6 +5,8 @@
 #
 
 test_description='Test pretty formats'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Tested non-UTF-8 encoding
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 05/20] builtin/merge-tree: fix leaking `-X` strategy options

[Patrick Steinhardt]

The `-X` switch for git-merge-tree(1) will push each option into a local
`xopts` vector that we then end up parsing. The vector never gets freed
though, causing a memory leak. Plug it.

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

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 9bca9b5f33c..c00469ed3db 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -533,6 +533,7 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 	int expected_remaining_argc;
 	int original_argc;
 	const char *merge_base = NULL;
+	int ret;
 
 	const char * const merge_tree_usage[] = {
 		N_("git merge-tree [--write-tree] [<options>] <branch1> <branch2>"),
@@ -625,7 +626,9 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 			strbuf_list_free(split);
 		}
 		strbuf_release(&buf);
-		return 0;
+
+		ret = 0;
+		goto out;
 	}
 
 	/* Figure out which mode to use */
@@ -664,7 +667,11 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 
 	/* Do the relevant type of merge */
 	if (o.mode == MODE_REAL)
-		return real_merge(&o, merge_base, argv[0], argv[1], prefix);
+		ret = real_merge(&o, merge_base, argv[0], argv[1], prefix);
 	else
-		return trivial_merge(argv[0], argv[1], argv[2]);
+		ret = trivial_merge(argv[0], argv[1], argv[2]);
+
+out:
+	strvec_clear(&xopts);
+	return ret;
 }
diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index eea19907b55..37f1cd7364c 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -2,6 +2,7 @@
 
 test_description='git merge-tree --write-tree'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # This test is ort-specific
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 06/20] builtin/upload-archive: fix leaking args passed to `write_archive()`

[Patrick Steinhardt]

In git-upload-archive(1), we pass an array of arguments to
`write_archive()` to tell it what exactly to do. We don't ever clear the
vector though, causing a memory leak. Furthermore though, the call to
`write_archive()` may cause contents of the array to be modified, which
would cause us to leak memory to allocated strings held by it.

Fix the issue by having `write_archive()` create a shallow copy of
`argv` before parsing the arguments. Like this, we won't modify the
caller's array and can easily `strvec_clear()` it to plug these memory
leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 archive.c                | 10 ++++++++++
 builtin/upload-archive.c |  8 ++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/archive.c b/archive.c
index 7bd60d0632a..9ba96aae4f7 100644
--- a/archive.c
+++ b/archive.c
@@ -736,6 +736,7 @@ int write_archive(int argc, const char **argv, const char *prefix,
 	struct pretty_print_describe_status describe_status = {0};
 	struct pretty_print_context ctx = {0};
 	struct archiver_args args;
+	const char **argv_copy;
 	int rc;
 
 	git_config_get_bool("uploadarchive.allowunreachable", &remote_allow_unreachable);
@@ -749,6 +750,14 @@ int write_archive(int argc, const char **argv, const char *prefix,
 	args.repo = repo;
 	args.prefix = prefix;
 	string_list_init_dup(&args.extra_files);
+
+	/*
+	 * `parse_archive_args()` modifies contents of `argv`, which is what we
+	 * want. Our callers may not want it though, so we create a copy here.
+	 */
+	DUP_ARRAY(argv_copy, argv, argc);
+	argv = argv_copy;
+
 	argc = parse_archive_args(argc, argv, &ar, &args, name_hint, remote);
 	if (!startup_info->have_repository) {
 		/*
@@ -767,6 +776,7 @@ int write_archive(int argc, const char **argv, const char *prefix,
 	string_list_clear_func(&args.extra_files, extra_file_info_clear);
 	free(args.refname);
 	clear_pathspec(&args.pathspec);
+	free(argv_copy);
 
 	return rc;
 }
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 1b09e5e1aa3..313a8dfa81c 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -22,6 +22,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
 {
 	struct strvec sent_argv = STRVEC_INIT;
 	const char *arg_cmd = "argument ";
+	int ret;
 
 	if (argc != 2 || !strcmp(argv[1], "-h"))
 		usage(upload_archive_usage);
@@ -46,8 +47,11 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
 	}
 
 	/* parse all options sent by the client */
-	return write_archive(sent_argv.nr, sent_argv.v, prefix,
-			     the_repository, NULL, 1);
+	ret = write_archive(sent_argv.nr, sent_argv.v, prefix,
+			    the_repository, NULL, 1);
+
+	strvec_clear(&sent_argv);
+	return ret;
 }
 
 __attribute__((format (printf, 1, 2)))
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 07/20] builtin/archive: fix leaking `OPT_FILENAME()` value

[Patrick Steinhardt]

The "--output" switch is an `OPT_FILENAME()` option, which allocates
memory when specified by the user. But while we free the string when
executed without the "--remote" switch, we don't otherwise because we
return via a separate exit path that doesn't know to free it.

Fix this by creating a common exit path.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/archive.c      | 7 +++++--
 t/t5000-tar-tree.sh    | 1 +
 t/t5003-archive-zip.sh | 1 +
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index b50981504f3..63f02990d11 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -100,13 +100,16 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 	if (output)
 		create_output_file(output);
 
-	if (remote)
-		return run_remote_archiver(argc, argv, remote, exec, output);
+	if (remote) {
+		ret = run_remote_archiver(argc, argv, remote, exec, output);
+		goto out;
+	}
 
 	setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
 
 	ret = write_archive(argc, argv, prefix, the_repository, output, 0);
 
+out:
 	free(output);
 	return ret;
 }
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 72b8d0ff02e..7abba8a4b20 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -25,6 +25,7 @@ commit id embedding:
 '
 
 TEST_CREATE_REPO_NO_TEMPLATE=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 SUBSTFORMAT=%H%n
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index 961c6aac256..01f591c99b9 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -3,6 +3,7 @@
 test_description='git archive --format=zip test'
 
 TEST_CREATE_REPO_NO_TEMPLATE=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 SUBSTFORMAT=%H%n
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 08/20] midx-write: fix leaking hashfile on error cases

[Patrick Steinhardt]

When writing the MIDX file we first create the `struct hashfile` used to
write the trailer hash, and then afterwards we verify whether we can
actually write the MIDX in the first place. When we decide that we
can't, this leads to a memory leak because we never free the hash file
contents.

We could fix this by freeing the hashfile on the exit path. There is a
better option though: we can simply move the checks for the error
condition earlier. As there is no early exit between creating the
hashfile and finalizing it anymore this is sufficient to fix the memory
leak.

While at it, also move around the block checking for `ctx.entries_nr`.
This change is not required to fix the memory leak, but it feels natural
to move together all massaging of parameters before we go with them and
execute the actual logic.

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

diff --git a/midx-write.c b/midx-write.c
index e3fa33203fa..07d98d494aa 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -1308,6 +1308,18 @@ static int write_midx_internal(const char *object_dir,
 		pack_name_concat_len += MIDX_CHUNK_ALIGNMENT -
 					(pack_name_concat_len % MIDX_CHUNK_ALIGNMENT);
 
+	if (ctx.nr - dropped_packs == 0) {
+		error(_("no pack files to index."));
+		result = 1;
+		goto cleanup;
+	}
+
+	if (!ctx.entries_nr) {
+		if (flags & MIDX_WRITE_BITMAP)
+			warning(_("refusing to write multi-pack .bitmap without any objects"));
+		flags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP);
+	}
+
 	if (ctx.incremental) {
 		struct strbuf lock_name = STRBUF_INIT;
 
@@ -1333,18 +1345,6 @@ static int write_midx_internal(const char *object_dir,
 		f = hashfd(get_lock_file_fd(&lk), get_lock_file_path(&lk));
 	}
 
-	if (ctx.nr - dropped_packs == 0) {
-		error(_("no pack files to index."));
-		result = 1;
-		goto cleanup;
-	}
-
-	if (!ctx.entries_nr) {
-		if (flags & MIDX_WRITE_BITMAP)
-			warning(_("refusing to write multi-pack .bitmap without any objects"));
-		flags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP);
-	}
-
 	cf = init_chunkfile(f);
 
 	add_chunk(cf, MIDX_CHUNKID_PACKNAMES, pack_name_concat_len,
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 09/20] builtin/repack: fix leaks when computing packs to repack

[Patrick Steinhardt]

When writing an MIDX in git-repack(1) we first collect all the pack
names that we want to add to it in a string list. This list is marked as
`NODUP`, which indicates that it will neither duplicate nor own strings
added to it. In `write_midx_included_packs()` we then `insert()` strings
via `xstrdup()` or `strbuf_detach()`, but the resulting strings will not
be owned by anything and thus leak.

Fix this issue by marking the list as `DUP` and using a local buffer to
compute the pack names.

This leak is hit in t5319, but plugging it is not sufficient to make the
whole test suite pass.

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

diff --git a/builtin/repack.c b/builtin/repack.c
index 62cfa50c50f..8bb875532b4 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -732,14 +732,23 @@ static void midx_included_packs(struct string_list *include,
 				struct pack_geometry *geometry)
 {
 	struct string_list_item *item;
+	struct strbuf buf = STRBUF_INIT;
+
+	for_each_string_list_item(item, &existing->kept_packs) {
+		strbuf_reset(&buf);
+		strbuf_addf(&buf, "%s.idx", item->string);
+		string_list_insert(include, buf.buf);
+	}
+
+	for_each_string_list_item(item, names) {
+		strbuf_reset(&buf);
+		strbuf_addf(&buf, "pack-%s.idx", item->string);
+		string_list_insert(include, buf.buf);
+	}
 
-	for_each_string_list_item(item, &existing->kept_packs)
-		string_list_insert(include, xstrfmt("%s.idx", item->string));
-	for_each_string_list_item(item, names)
-		string_list_insert(include, xstrfmt("pack-%s.idx", item->string));
 	if (geometry->split_factor) {
-		struct strbuf buf = STRBUF_INIT;
 		uint32_t i;
+
 		for (i = geometry->split; i < geometry->pack_nr; i++) {
 			struct packed_git *p = geometry->pack[i];
 
@@ -754,17 +763,21 @@ static void midx_included_packs(struct string_list *include,
 			if (!p->pack_local)
 				continue;
 
+			strbuf_reset(&buf);
 			strbuf_addstr(&buf, pack_basename(p));
 			strbuf_strip_suffix(&buf, ".pack");
 			strbuf_addstr(&buf, ".idx");
 
-			string_list_insert(include, strbuf_detach(&buf, NULL));
+			string_list_insert(include, buf.buf);
 		}
 	} else {
 		for_each_string_list_item(item, &existing->non_kept_packs) {
 			if (pack_is_marked_for_deletion(item))
 				continue;
-			string_list_insert(include, xstrfmt("%s.idx", item->string));
+
+			strbuf_reset(&buf);
+			strbuf_addf(&buf, "%s.idx", item->string);
+			string_list_insert(include, buf.buf);
 		}
 	}
 
@@ -784,8 +797,13 @@ static void midx_included_packs(struct string_list *include,
 		 */
 		if (pack_is_marked_for_deletion(item))
 			continue;
-		string_list_insert(include, xstrfmt("%s.idx", item->string));
+
+		strbuf_reset(&buf);
+		strbuf_addf(&buf, "%s.idx", item->string);
+		string_list_insert(include, buf.buf);
 	}
+
+	strbuf_release(&buf);
 }
 
 static int write_midx_included_packs(struct string_list *include,
@@ -1476,7 +1494,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		mark_packs_for_deletion(&existing, &names);
 
 	if (write_midx) {
-		struct string_list include = STRING_LIST_INIT_NODUP;
+		struct string_list include = STRING_LIST_INIT_DUP;
 		midx_included_packs(&include, &existing, &names, &geometry);
 
 		ret = write_midx_included_packs(&include, &geometry, &names,
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 10/20] t/helper: fix leaking multi-pack-indices in "read-midx"

[Patrick Steinhardt]

Several of the subcommands of `test-helper read-midx` do not close the
MIDX that they have opened, leading to memory leaks. Fix those.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/helper/test-read-midx.c   | 8 +++++++-
 t/t5319-multi-pack-index.sh | 2 ++
 t/t7704-repack-cruft.sh     | 1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c
index 69757e94fc2..438fb9fc619 100644
--- a/t/helper/test-read-midx.c
+++ b/t/helper/test-read-midx.c
@@ -86,6 +86,8 @@ static int read_midx_checksum(const char *object_dir)
 	if (!m)
 		return 1;
 	printf("%s\n", hash_to_hex(get_midx_checksum(m)));
+
+	close_midx(m);
 	return 0;
 }
 
@@ -102,10 +104,12 @@ static int read_midx_preferred_pack(const char *object_dir)
 
 	if (midx_preferred_pack(midx, &preferred_pack) < 0) {
 		warning(_("could not determine MIDX preferred pack"));
+		close_midx(midx);
 		return 1;
 	}
 
 	printf("%s\n", midx->pack_names[preferred_pack]);
+	close_midx(midx);
 	return 0;
 }
 
@@ -122,8 +126,10 @@ static int read_midx_bitmapped_packs(const char *object_dir)
 		return 1;
 
 	for (i = 0; i < midx->num_packs + midx->num_packs_in_base; i++) {
-		if (nth_bitmapped_pack(the_repository, midx, &pack, i) < 0)
+		if (nth_bitmapped_pack(the_repository, midx, &pack, i) < 0) {
+			close_midx(midx);
 			return 1;
+		}
 
 		printf("%s\n", pack_basename(pack.p));
 		printf("  bitmap_pos: %"PRIuMAX"\n", (uintmax_t)pack.bitmap_pos);
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index ce1b58c7323..fbbc218d04a 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='multi-pack-indexes'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-chunk.sh
 . "$TEST_DIRECTORY"/lib-midx.sh
diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh
index 959e6e26488..5db9f4e10f7 100755
--- a/t/t7704-repack-cruft.sh
+++ b/t/t7704-repack-cruft.sh
@@ -2,6 +2,7 @@
 
 test_description='git repack works correctly'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 objdir=.git/objects
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 11/20] transport: fix leaking OID arrays in git:// transport data

[Patrick Steinhardt]

The transport data for the "git://" protocol contains two OID arrays
that we never free, creating a memory leak. Plug them.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5501-fetch-push-alternates.sh | 1 +
 t/t5519-push-alternates.sh       | 1 +
 transport.c                      | 2 ++
 3 files changed, 4 insertions(+)

diff --git a/t/t5501-fetch-push-alternates.sh b/t/t5501-fetch-push-alternates.sh
index 66f19a4ef2b..0c8668a1b8e 100755
--- a/t/t5501-fetch-push-alternates.sh
+++ b/t/t5501-fetch-push-alternates.sh
@@ -4,6 +4,7 @@ test_description='fetch/push involving alternates'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 count_objects () {
diff --git a/t/t5519-push-alternates.sh b/t/t5519-push-alternates.sh
index 20ba604dfde..72e97b15fab 100755
--- a/t/t5519-push-alternates.sh
+++ b/t/t5519-push-alternates.sh
@@ -5,6 +5,7 @@ test_description='push to a repository that borrows from elsewhere'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/transport.c b/transport.c
index 7c4af9f56f2..f0672fdc505 100644
--- a/transport.c
+++ b/transport.c
@@ -946,6 +946,8 @@ static int disconnect_git(struct transport *transport)
 	}
 
 	list_objects_filter_release(&data->options.filter_options);
+	oid_array_clear(&data->extra_have);
+	oid_array_clear(&data->shallow);
 	free(data);
 	return 0;
 }
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 12/20] builtin/send-pack: fix leaking refspecs

[Patrick Steinhardt]

We never free data associated with the assembled refspec in
git-send-pack(1), causing a memory leak. Fix this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/send-pack.c           | 1 +
 t/t5400-send-pack.sh          | 1 +
 t/t5401-update-hooks.sh       | 2 ++
 t/t5408-send-pack-stdin.sh    | 2 ++
 t/t5548-push-porcelain.sh     | 1 +
 t/t5812-proto-disable-http.sh | 2 ++
 6 files changed, 9 insertions(+)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 17cae6bbbdf..ef0df808249 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -338,5 +338,6 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 
 	free_refs(remote_refs);
 	free_refs(local_refs);
+	refspec_clear(&rs);
 	return ret;
 }
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 3f81f16e133..248c74d8ef2 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -9,6 +9,7 @@ test_description='See why rewinding head breaks send-pack
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 cnt=64
diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh
index d8cadeec733..3c1ea6086e7 100755
--- a/t/t5401-update-hooks.sh
+++ b/t/t5401-update-hooks.sh
@@ -4,6 +4,8 @@
 #
 
 test_description='Test the update hook infrastructure.'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t5408-send-pack-stdin.sh b/t/t5408-send-pack-stdin.sh
index e8737df6f95..c3695a4d4e3 100755
--- a/t/t5408-send-pack-stdin.sh
+++ b/t/t5408-send-pack-stdin.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='send-pack --stdin tests'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 create_ref () {
diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
index 6282728eaf3..ecb3877aa4b 100755
--- a/t/t5548-push-porcelain.sh
+++ b/t/t5548-push-porcelain.sh
@@ -4,6 +4,7 @@
 #
 test_description='Test git push porcelain output'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Create commits in <repo> and assign each commit's oid to shell variables
diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh
index 769c717e88b..f69959c64ca 100755
--- a/t/t5812-proto-disable-http.sh
+++ b/t/t5812-proto-disable-http.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test disabling of git-over-http in clone/fetch'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY/lib-proto-disable.sh"
 . "$TEST_DIRECTORY/lib-httpd.sh"
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 13/20] sideband: fix leaks when configuring sideband colors

[Patrick Steinhardt]

We read a bunch of configs in `use_sideband_colors()` to configure the
colors that Git should use. We never free the strings read from the
config though, causing memory leaks.

Refactor the code to use `git_config_get_string_tmp()` instead, which
does not allocate memory. As we throw the strings away after parsing
them anyway there is no need to use allocated strings.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 sideband.c                          | 15 +++++++--------
 t/t5409-colorize-remote-messages.sh |  1 +
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/sideband.c b/sideband.c
index 5d8907151fe..27853736901 100644
--- a/sideband.c
+++ b/sideband.c
@@ -30,28 +30,27 @@ static int use_sideband_colors(void)
 
 	const char *key = "color.remote";
 	struct strbuf sb = STRBUF_INIT;
-	char *value;
+	const char *value;
 	int i;
 
 	if (use_sideband_colors_cached >= 0)
 		return use_sideband_colors_cached;
 
-	if (!git_config_get_string(key, &value)) {
+	if (!git_config_get_string_tmp(key, &value))
 		use_sideband_colors_cached = git_config_colorbool(key, value);
-	} else if (!git_config_get_string("color.ui", &value)) {
+	else if (!git_config_get_string_tmp("color.ui", &value))
 		use_sideband_colors_cached = git_config_colorbool("color.ui", value);
-	} else {
+	else
 		use_sideband_colors_cached = GIT_COLOR_AUTO;
-	}
 
 	for (i = 0; i < ARRAY_SIZE(keywords); i++) {
 		strbuf_reset(&sb);
 		strbuf_addf(&sb, "%s.%s", key, keywords[i].keyword);
-		if (git_config_get_string(sb.buf, &value))
-			continue;
-		if (color_parse(value, keywords[i].color))
+		if (git_config_get_string_tmp(sb.buf, &value))
 			continue;
+		color_parse(value, keywords[i].color);
 	}
+
 	strbuf_release(&sb);
 	return use_sideband_colors_cached;
 }
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
index fa5de4500a4..516b22fd963 100755
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -2,6 +2,7 @@
 
 test_description='remote messages are colorized on the client'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 14/20] builtin/fetch-pack: fix leaking refs

[Patrick Steinhardt]

We build several ref lists in git-fetch-pack(1), but never free them.
Fix those leaks.

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

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index af329e8d5cf..fe404d1305b 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -46,7 +46,7 @@ static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,
 int cmd_fetch_pack(int argc, const char **argv, const char *prefix UNUSED)
 {
 	int i, ret;
-	struct ref *ref = NULL;
+	struct ref *fetched_refs = NULL, *remote_refs = NULL;
 	const char *dest = NULL;
 	struct ref **sought = NULL;
 	int nr_sought = 0, alloc_sought = 0;
@@ -228,19 +228,20 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix UNUSED)
 	version = discover_version(&reader);
 	switch (version) {
 	case protocol_v2:
-		get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL,
+		get_remote_refs(fd[1], &reader, &remote_refs, 0, NULL, NULL,
 				args.stateless_rpc);
 		break;
 	case protocol_v1:
 	case protocol_v0:
-		get_remote_heads(&reader, &ref, 0, NULL, &shallow);
+		get_remote_heads(&reader, &remote_refs, 0, NULL, &shallow);
 		break;
 	case protocol_unknown_version:
 		BUG("unknown protocol version");
 	}
 
-	ref = fetch_pack(&args, fd, ref, sought, nr_sought,
+	fetched_refs = fetch_pack(&args, fd, remote_refs, sought, nr_sought,
 			 &shallow, pack_lockfiles_ptr, version);
+
 	if (pack_lockfiles.nr) {
 		int i;
 
@@ -260,7 +261,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix UNUSED)
 	if (finish_connect(conn))
 		return 1;
 
-	ret = !ref;
+	ret = !fetched_refs;
 
 	/*
 	 * If the heads to pull were given, we should have consumed
@@ -270,11 +271,14 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix UNUSED)
 	 */
 	ret |= report_unmatched_refs(sought, nr_sought);
 
-	while (ref) {
+	for (struct ref *ref = fetched_refs; ref; ref = ref->next)
 		printf("%s %s\n",
 		       oid_to_hex(&ref->old_oid), ref->name);
-		ref = ref->next;
-	}
 
+	for (size_t i = 0; i < nr_sought; i++)
+		free_one_ref(sought[i]);
+	free(sought);
+	free_refs(fetched_refs);
+	free_refs(remote_refs);
 	return ret;
 }
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 15/20] remote: fix leaking config strings

[Patrick Steinhardt]

We're leaking several config strings when assembling remotes, either
because we do not free preceding values in case a config was set
multiple times, or because we do not free them when releasing the remote
state. This includes config strings for "branch" sections, "insteadOf",
"pushInsteadOf", and "pushDefault".

Plug those leaks.

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

diff --git a/remote.c b/remote.c
index 7d5b8f750d8..2c52119bbb2 100644
--- a/remote.c
+++ b/remote.c
@@ -243,6 +243,17 @@ static struct branch *make_branch(struct remote_state *remote_state,
 	return ret;
 }
 
+static void branch_release(struct branch *branch)
+{
+	free((char *)branch->name);
+	free((char *)branch->refname);
+	free(branch->remote_name);
+	free(branch->pushremote_name);
+	for (int i = 0; i < branch->merge_nr; i++)
+		refspec_item_clear(branch->merge[i]);
+	free(branch->merge);
+}
+
 static struct rewrite *make_rewrite(struct rewrites *r,
 				    const char *base, size_t len)
 {
@@ -263,6 +274,14 @@ static struct rewrite *make_rewrite(struct rewrites *r,
 	return ret;
 }
 
+static void rewrites_release(struct rewrites *r)
+{
+	for (int i = 0; i < r->rewrite_nr; i++)
+		free((char *)r->rewrite[i]->base);
+	free(r->rewrite);
+	memset(r, 0, sizeof(*r));
+}
+
 static void add_instead_of(struct rewrite *rewrite, const char *instead_of)
 {
 	ALLOC_GROW(rewrite->instead_of, rewrite->instead_of_nr + 1, rewrite->instead_of_alloc);
@@ -373,8 +392,10 @@ static int handle_config(const char *key, const char *value,
 			return -1;
 		branch = make_branch(remote_state, name, namelen);
 		if (!strcmp(subkey, "remote")) {
+			FREE_AND_NULL(branch->remote_name);
 			return git_config_string(&branch->remote_name, key, value);
 		} else if (!strcmp(subkey, "pushremote")) {
+			FREE_AND_NULL(branch->pushremote_name);
 			return git_config_string(&branch->pushremote_name, key, value);
 		} else if (!strcmp(subkey, "merge")) {
 			if (!value)
@@ -406,9 +427,11 @@ static int handle_config(const char *key, const char *value,
 		return 0;
 
 	/* Handle remote.* variables */
-	if (!name && !strcmp(subkey, "pushdefault"))
+	if (!name && !strcmp(subkey, "pushdefault")) {
+		FREE_AND_NULL(remote_state->pushremote_name);
 		return git_config_string(&remote_state->pushremote_name, key,
 					 value);
+	}
 
 	if (!name)
 		return 0;
@@ -475,12 +498,15 @@ static int handle_config(const char *key, const char *value,
 		else if (!strcmp(value, "--tags"))
 			remote->fetch_tags = 2;
 	} else if (!strcmp(subkey, "proxy")) {
+		FREE_AND_NULL(remote->http_proxy);
 		return git_config_string(&remote->http_proxy,
 					 key, value);
 	} else if (!strcmp(subkey, "proxyauthmethod")) {
+		FREE_AND_NULL(remote->http_proxy_authmethod);
 		return git_config_string(&remote->http_proxy_authmethod,
 					 key, value);
 	} else if (!strcmp(subkey, "vcs")) {
+		FREE_AND_NULL(remote->foreign_vcs);
 		return git_config_string(&remote->foreign_vcs, key, value);
 	}
 	return 0;
@@ -2797,16 +2823,26 @@ struct remote_state *remote_state_new(void)
 
 void remote_state_clear(struct remote_state *remote_state)
 {
+	struct hashmap_iter iter;
+	struct branch *b;
 	int i;
 
 	for (i = 0; i < remote_state->remotes_nr; i++)
 		remote_clear(remote_state->remotes[i]);
 	FREE_AND_NULL(remote_state->remotes);
+	FREE_AND_NULL(remote_state->pushremote_name);
 	remote_state->remotes_alloc = 0;
 	remote_state->remotes_nr = 0;
 
+	rewrites_release(&remote_state->rewrites);
+	rewrites_release(&remote_state->rewrites_push);
+
 	hashmap_clear_and_free(&remote_state->remotes_hash, struct remote, ent);
-	hashmap_clear_and_free(&remote_state->branches_hash, struct remote, ent);
+	hashmap_for_each_entry(&remote_state->branches_hash, &iter, b, ent) {
+		branch_release(b);
+		free(b);
+	}
+	hashmap_clear(&remote_state->branches_hash);
 }
 
 /*
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 16/20] remote: fix leaks when matching refspecs

[Patrick Steinhardt]

In `match_explicit()`, we try to match a source ref with a destination
ref according to a refspec item. This matching sometimes requires us to
allocate a new source spec so that it looks like we expect. And while we
in some end up assigning this allocated ref as `peer_ref`, which hands
over ownership of it to the caller, in other cases we don't. We neither
free it though, causing a memory leak.

Fix the leak by creating a common exit path where we can easily free the
source ref in case it is allocated and hasn't been handed over to the
caller.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 remote.c          | 43 +++++++++++++++++++++++++++++--------------
 t/t5505-remote.sh |  1 +
 2 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/remote.c b/remote.c
index 2c52119bbb2..6ea81f9665b 100644
--- a/remote.c
+++ b/remote.c
@@ -1344,18 +1344,21 @@ static int match_explicit(struct ref *src, struct ref *dst,
 			  struct ref ***dst_tail,
 			  struct refspec_item *rs)
 {
-	struct ref *matched_src, *matched_dst;
-	int allocated_src;
+	struct ref *matched_src = NULL, *matched_dst = NULL;
+	int allocated_src = 0, ret;
 
 	const char *dst_value = rs->dst;
 	char *dst_guess;
 
-	if (rs->pattern || rs->matching || rs->negative)
-		return 0;
+	if (rs->pattern || rs->matching || rs->negative) {
+		ret = 0;
+		goto out;
+	}
 
-	matched_src = matched_dst = NULL;
-	if (match_explicit_lhs(src, rs, &matched_src, &allocated_src) < 0)
-		return -1;
+	if (match_explicit_lhs(src, rs, &matched_src, &allocated_src) < 0) {
+		ret = -1;
+		goto out;
+	}
 
 	if (!dst_value) {
 		int flag;
@@ -1394,18 +1397,30 @@ static int match_explicit(struct ref *src, struct ref *dst,
 		      dst_value);
 		break;
 	}
-	if (!matched_dst)
-		return -1;
-	if (matched_dst->peer_ref)
-		return error(_("dst ref %s receives from more than one src"),
-			     matched_dst->name);
-	else {
+
+	if (!matched_dst) {
+		ret = -1;
+		goto out;
+	}
+
+	if (matched_dst->peer_ref) {
+		ret = error(_("dst ref %s receives from more than one src"),
+			    matched_dst->name);
+		goto out;
+	} else {
 		matched_dst->peer_ref = allocated_src ?
 					matched_src :
 					copy_ref(matched_src);
 		matched_dst->force = rs->force;
+		matched_src = NULL;
 	}
-	return 0;
+
+	ret = 0;
+
+out:
+	if (allocated_src)
+		free_one_ref(matched_src);
+	return ret;
 }
 
 static int match_explicit_refs(struct ref *src, struct ref *dst,
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 08424e878e1..532035933f3 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -2,6 +2,7 @@
 
 test_description='git remote porcelain-ish'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 setup_repository () {
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 17/20] remote: fix leaking peer ref when expanding refmap

[Patrick Steinhardt]

When expanding remote refs via the refspec in `get_expanded_map()`, we
first copy the remote ref and then override its peer ref with the
expanded name. This may cause a memory leak though in case the peer ref
is already set, as this field is being copied by `copy_ref()`, as well.

Fix the leak by freeing the peer ref before we re-assign the field.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 remote.c                           | 2 ++
 t/t5536-fetch-conflicts.sh         | 1 +
 t/t5553-set-upstream.sh            | 1 +
 t/t5703-upload-pack-ref-in-want.sh | 1 +
 t/t6050-replace.sh                 | 1 +
 5 files changed, 6 insertions(+)

diff --git a/remote.c b/remote.c
index 6ea81f9665b..8f3dee13186 100644
--- a/remote.c
+++ b/remote.c
@@ -2081,6 +2081,8 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,
 		    !ignore_symref_update(expn_name, &scratch)) {
 			struct ref *cpy = copy_ref(ref);
 
+			if (cpy->peer_ref)
+				free_one_ref(cpy->peer_ref);
 			cpy->peer_ref = alloc_ref(expn_name);
 			if (refspec->force)
 				cpy->peer_ref->force = 1;
diff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh
index 23bf6961700..2dcbe790523 100755
--- a/t/t5536-fetch-conflicts.sh
+++ b/t/t5536-fetch-conflicts.sh
@@ -2,6 +2,7 @@
 
 test_description='fetch handles conflicting refspecs correctly'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 D=$(pwd)
diff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh
index 70e3376d31b..33e919a17e1 100755
--- a/t/t5553-set-upstream.sh
+++ b/t/t5553-set-upstream.sh
@@ -4,6 +4,7 @@ test_description='"git fetch/pull --set-upstream" basic tests.'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 check_config () {
diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index 191097171bc..f75fae52c83 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -2,6 +2,7 @@
 
 test_description='upload-pack ref-in-want'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 get_actual_refs () {
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index c6e9b33e44e..d7702fc7562 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -7,6 +7,7 @@ test_description='Tests replace refs functionality'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY/lib-gpg.sh"
 
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 18/20] builtin/fetch: fix leaking transaction with `--atomic`

[Patrick Steinhardt]

With the `--atomic` flag, we use a single ref transaction to commit all
ref updates in git-fetch(1). The lifetime of transactions is somewhat
weird: while `ref_transaction_abort()` will free the transaction, a call
to `ref_transaction_commit()` won't. We thus have to manually free the
transaction in the successful case.

Adapt the code to free the transaction in the exit path to plug the
resulting memory leak. As `ref_transaction_abort()` already freed the
transaction for us, we have to unset the transaction when we hit that
code path to not cause a double free.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c         | 8 ++++----
 t/t5574-fetch-output.sh | 1 +
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index c297569a473..0264483c0e5 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1731,11 +1731,8 @@ static int do_fetch(struct transport *transport,
 			goto cleanup;
 
 		retcode = ref_transaction_commit(transaction, &err);
-		if (retcode) {
-			ref_transaction_free(transaction);
-			transaction = NULL;
+		if (retcode)
 			goto cleanup;
-		}
 	}
 
 	commit_fetch_head(&fetch_head);
@@ -1803,8 +1800,11 @@ static int do_fetch(struct transport *transport,
 		if (transaction && ref_transaction_abort(transaction, &err) &&
 		    err.len)
 			error("%s", err.buf);
+		transaction = NULL;
 	}
 
+	if (transaction)
+		ref_transaction_free(transaction);
 	display_state_release(&display_state);
 	close_fetch_head(&fetch_head);
 	strbuf_release(&err);
diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
index 5883839a04e..f7707326ea1 100755
--- a/t/t5574-fetch-output.sh
+++ b/t/t5574-fetch-output.sh
@@ -5,6 +5,7 @@ test_description='git fetch output format'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'fetch with invalid output format configuration' '
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 19/20] transport: fix leaking arguments when fetching from bundle

[Patrick Steinhardt]

In `fetch_refs_from_bundle()` we assemble a vector of arguments to pass
to `unbundle()`, but never free it. And in theory we wouldn't have to
because `unbundle()` already knows to free the vector for us. But it
fails to do so when it exits early due to `verify_bundle()` failing.

The calling convention that the arguments are freed by the callee and
not the caller feels somewhat weird. Refactor the code such that it is
instead the responsibility of the caller to free the vector, adapting
the only two callsites where we pass extra arguments. This also fixes
the memory leak.

This memory leak gets hit in t5510, but fixing it isn't sufficient to
make the whole test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/bundle.c | 2 ++
 bundle.c         | 4 +---
 transport.c      | 2 ++
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index d5d41a8f672..df97f399019 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -220,7 +220,9 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 			 &extra_index_pack_args, 0) ||
 		list_bundle_refs(&header, argc, argv);
 	bundle_header_release(&header);
+
 cleanup:
+	strvec_clear(&extra_index_pack_args);
 	free(bundle_file);
 	return ret;
 }
diff --git a/bundle.c b/bundle.c
index ce164c37bc8..0f6c7a71ef1 100644
--- a/bundle.c
+++ b/bundle.c
@@ -639,10 +639,8 @@ int unbundle(struct repository *r, struct bundle_header *header,
 	if (flags & VERIFY_BUNDLE_FSCK)
 		strvec_push(&ip.args, "--fsck-objects");
 
-	if (extra_index_pack_args) {
+	if (extra_index_pack_args)
 		strvec_pushv(&ip.args, extra_index_pack_args->v);
-		strvec_clear(extra_index_pack_args);
-	}
 
 	ip.in = bundle_fd;
 	ip.no_stdout = 1;
diff --git a/transport.c b/transport.c
index f0672fdc505..da639d3bff0 100644
--- a/transport.c
+++ b/transport.c
@@ -189,6 +189,8 @@ static int fetch_refs_from_bundle(struct transport *transport,
 		       &extra_index_pack_args,
 		       fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0);
 	transport->hash_algo = data->header.hash_algo;
+
+	strvec_clear(&extra_index_pack_args);
 	return ret;
 }
 
-- 
2.46.0.164.g477ce5ccd6.dirty

[PATCH v2 20/20] transport: fix leaking negotiation tips

[Patrick Steinhardt]

We do not free negotiation tips in the transport's smart options. Fix
this by freeing them on disconnect.

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

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 3b3991ab867..0890b9f61c5 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -5,6 +5,7 @@ test_description='Per branch config variables affects "git fetch".
 
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-bundle.sh
 
diff --git a/transport.c b/transport.c
index da639d3bff0..0f20fc56e40 100644
--- a/transport.c
+++ b/transport.c
@@ -947,6 +947,10 @@ static int disconnect_git(struct transport *transport)
 		finish_connect(data->conn);
 	}
 
+	if (data->options.negotiation_tips) {
+		oid_array_clear(data->options.negotiation_tips);
+		free(data->options.negotiation_tips);
+	}
 	list_objects_filter_release(&data->options.filter_options);
 	oid_array_clear(&data->extra_have);
 	oid_array_clear(&data->shallow);
-- 
2.46.0.164.g477ce5ccd6.dirty

Re: [PATCH 15/20] remote: fix leaking config strings

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Wed, Aug 21, 2024 at 10:58:55AM -0700, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> 
>> > @@ -2802,6 +2809,7 @@ void remote_state_clear(struct remote_state *remote_state)
>> >  	for (i = 0; i < remote_state->remotes_nr; i++)
>> >  		remote_clear(remote_state->remotes[i]);
>> >  	FREE_AND_NULL(remote_state->remotes);
>> > +	FREE_AND_NULL(remote_state->pushremote_name);
>> >  	remote_state->remotes_alloc = 0;
>> >  	remote_state->remotes_nr = 0;
>> 
>> As remote_state has two extra structures embedded in it, I wonder if
>> we should be clearing them in this function, but possibly it is
>> cleared elsewhere or perhaps in a later series?
>
> It is not yet part of any subsequent patch series, mostly because I
> didn't happen to stumble over such leaks yet. Both of the rewrites very
> much are leaky though, and would be hit when we use "insteadOf" or
> "pushInsteadOf" configs.

Yes, I was wondering if our test coverage for the feature is
lacking.  In a sense, leaks from these insteadOf configuration
variables are tiny and uninteresting, compared to how much it
bothers me to find us not testing these often used features.

Re: [PATCH 19/20] transport: fix leaking arguments when fetching from bundle

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Yes we do, because there is an early return in case `verify_bundle()`
> fails. I didn't notice that we have it in `unbundle()` though.

Ah, I missed that one.

> I'd argue it's bad practice to have `unbundle()` clear the caller
> provided args for us and somewhat surprising.

I was surprised so it makes it at least two of us.  In some call
patterns, having the callee _consume_ what the caller fed it may
be natural, but not in this code path.

Thanks.

Re: [PATCH v2 19/20] transport: fix leaking arguments when fetching from bundle

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> In `fetch_refs_from_bundle()` we assemble a vector of arguments to pass
> to `unbundle()`, but never free it. And in theory we wouldn't have to
> because `unbundle()` already knows to free the vector for us. But it
> fails to do so when it exits early due to `verify_bundle()` failing.
>
> The calling convention that the arguments are freed by the callee and
> not the caller feels somewhat weird. Refactor the code such that it is
> instead the responsibility of the caller to free the vector, adapting
> the only two callsites where we pass extra arguments. This also fixes
> the memory leak.
>
> This memory leak gets hit in t5510, but fixing it isn't sufficient to
> make the whole test suite pass.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/bundle.c | 2 ++
>  bundle.c         | 4 +---
>  transport.c      | 2 ++
>  3 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/bundle.c b/builtin/bundle.c
> index d5d41a8f672..df97f399019 100644
> --- a/builtin/bundle.c
> +++ b/builtin/bundle.c
> @@ -220,7 +220,9 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
>  			 &extra_index_pack_args, 0) ||
>  		list_bundle_refs(&header, argc, argv);
>  	bundle_header_release(&header);
> +
>  cleanup:
> +	strvec_clear(&extra_index_pack_args);
>  	free(bundle_file);
>  	return ret;
>  }
> diff --git a/bundle.c b/bundle.c
> index ce164c37bc8..0f6c7a71ef1 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -639,10 +639,8 @@ int unbundle(struct repository *r, struct bundle_header *header,
>  	if (flags & VERIFY_BUNDLE_FSCK)
>  		strvec_push(&ip.args, "--fsck-objects");
>  
> -	if (extra_index_pack_args) {
> +	if (extra_index_pack_args)
>  		strvec_pushv(&ip.args, extra_index_pack_args->v);
> -		strvec_clear(extra_index_pack_args);
> -	}
>  
>  	ip.in = bundle_fd;
>  	ip.no_stdout = 1;
> diff --git a/transport.c b/transport.c
> index f0672fdc505..da639d3bff0 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -189,6 +189,8 @@ static int fetch_refs_from_bundle(struct transport *transport,
>  		       &extra_index_pack_args,
>  		       fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0);
>  	transport->hash_algo = data->header.hash_algo;
> +
> +	strvec_clear(&extra_index_pack_args);
>  	return ret;
>  }

Looks much better.  Will queue.  Thanks.

Re: [PATCH v2 00/20] Memory leak fixes (pt.5)

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> this is version 2 of this patch series that fixes another set of memory
> leaks.
>
> Changes compared to v1:
>
>   - Remove spaces between cast and variable.
>
>   - Clarify why we move the `ctx.entries_nr` block.
>
>   - Adapt sideband colors to use `git_config_get_string_tmp()` such that
>     we do not have to allocate the strings in the first place.
>
>   - Fix more memory leaks for config values in the remote state.
>
>   - Refactor `unbundle()` to not free extra args passed by the caller
>     anymore. Instead, this is now always done by the caler.

All these changes look sensible to me.

Let me wait for a few days for others' comments, and then mark the
topic for 'next'.

Thanks.