[PATCH 00/23] Memory leak fixes (pt.3)

[PATCH 00/23] Memory leak fixes (pt.3)

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4923 bytes --]

Hi,

I originally wanted to hold off with sending out this series until v2.46
was out. But I saw that Junio sent out some patches which are plugging
the same leaks as I did, so I dedcided to send it out now to avoid some
duplicated work.

There isn't really any structure to this series, I just happened to pick
some random test suites that fail with the leak checker enabled and then
fixed those. Naturally, I've also got part 4 of this series of patch
series in the pipeline already :) As mentioned elsewhere, I hope to get
the number of failing test suites to zero this year. Let's see whether
this is realistic.

Patrick

Patrick Steinhardt (23):
  builtin/replay: plug leaking `advance_name` variable
  builtin/log: fix leaking branch name when creating cover letters
  builtin/describe: fix memory leak with `--contains=`
  builtin/describe: fix leaking array when running diff-index
  builtin/describe: fix trivial memory leak when describing blob
  builtin/name-rev: fix various trivial memory leaks
  builtin/submodule--helper: fix various trivial memory leaks
  builtin/ls-remote: fix leaking `pattern` strings
  builtin/remote: fix leaking strings in `branch_list`
  builtin/remote: fix various trivial memory leaks
  builtin/stash: fix various trivial memory leaks
  builtin/rev-parse: fix memory leak with `--parseopt`
  builtin/show-branch: fix several memory leaks
  builtin/credential-store: fix leaking credential
  builtin/rerere: fix various trivial memory leaks
  builtin/shortlog: fix various trivial memory leaks
  builtin/worktree: fix leaking derived branch names
  builtin/credential-cache: fix trivial leaks
  t/test-repository: fix leaking repository
  object-name: fix leaking commit list items
  entry: fix leaking pathnames during delayed checkout
  convert: fix leaking config strings
  commit-reach: fix trivial memory leak when computing reachability

 builtin/credential-cache.c              |  9 ++++-
 builtin/credential-store.c              |  1 +
 builtin/describe.c                      | 25 ++++++++++--
 builtin/log.c                           |  4 +-
 builtin/ls-remote.c                     | 11 ++++--
 builtin/name-rev.c                      |  6 ++-
 builtin/remote.c                        | 44 ++++++++++++++++-----
 builtin/replay.c                        | 20 +++++++---
 builtin/rerere.c                        |  8 +++-
 builtin/rev-parse.c                     |  5 ++-
 builtin/shortlog.c                      |  1 +
 builtin/show-branch.c                   | 52 +++++++++++++++++--------
 builtin/stash.c                         | 18 ++++++++-
 builtin/submodule--helper.c             | 13 +++++--
 builtin/worktree.c                      |  7 ++--
 commit-reach.c                          |  1 +
 convert.c                               | 14 +++++--
 entry.c                                 |  4 +-
 object-name.c                           | 26 ++++++++-----
 rerere.c                                |  9 ++++-
 t/helper/test-repository.c              |  4 +-
 t/t0021-conversion.sh                   |  1 +
 t/t0301-credential-cache.sh             |  2 +
 t/t0302-credential-store.sh             |  2 +
 t/t0303-credential-external.sh          |  1 +
 t/t1502-rev-parse-parseopt.sh           |  2 +
 t/t1511-rev-parse-caret.sh              |  1 +
 t/t2030-unresolve-info.sh               |  1 +
 t/t2080-parallel-checkout-basics.sh     |  1 +
 t/t2082-parallel-checkout-attributes.sh |  1 +
 t/t2400-worktree-add.sh                 |  1 +
 t/t2501-cwd-empty.sh                    |  1 +
 t/t3201-branch-contains.sh              |  1 +
 t/t3202-show-branch.sh                  |  1 +
 t/t3206-range-diff.sh                   |  1 +
 t/t3650-replay-basics.sh                |  1 +
 t/t3903-stash.sh                        |  1 +
 t/t3904-stash-patch.sh                  |  2 +
 t/t3905-stash-include-untracked.sh      |  1 +
 t/t4200-rerere.sh                       |  1 +
 t/t4201-shortlog.sh                     |  1 +
 t/t5318-commit-graph.sh                 |  2 +
 t/t5512-ls-remote.sh                    |  1 +
 t/t5514-fetch-multiple.sh               |  1 +
 t/t5520-pull.sh                         |  1 +
 t/t5528-push-default.sh                 |  1 +
 t/t5535-fetch-push-symref.sh            |  1 +
 t/t5543-atomic-push.sh                  |  1 +
 t/t5570-git-daemon.sh                   |  1 +
 t/t6007-rev-list-cherry-pick-file.sh    |  1 +
 t/t6010-merge-base.sh                   |  1 +
 t/t6120-describe.sh                     |  1 +
 t/t6133-pathspec-rev-dwim.sh            |  2 +
 t/t7064-wtstatus-pv2.sh                 |  1 +
 t/t7400-submodule-basic.sh              |  1 +
 t/t9902-completion.sh                   |  1 +
 t/t9903-bash-prompt.sh                  |  1 +
 57 files changed, 251 insertions(+), 73 deletions(-)

-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 01/23] builtin/replay: plug leaking `advance_name` variable

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4357 bytes --]

The `advance_name` variable can either contain a static string when
parsed via the `--advance` command line option or it may be an allocated
string when set via `determine_replay_mode()`. Because we cannot be sure
whether it is allocated or not we just didn't free it at all, resulting
in a memory leak.

Split up the variables such that we can track the static and allocated
strings separately and then free the allocated one to fix the memory
leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/replay.c         | 20 ++++++++++++++------
 t/t3650-replay-basics.sh |  1 +
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/builtin/replay.c b/builtin/replay.c
index 0448326636..27b40eda98 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -151,7 +151,7 @@ static void get_ref_information(struct rev_cmdline_info *cmd_info,
 
 static void determine_replay_mode(struct rev_cmdline_info *cmd_info,
 				  const char *onto_name,
-				  const char **advance_name,
+				  char **advance_name,
 				  struct commit **onto,
 				  struct strset **update_refs)
 {
@@ -174,6 +174,7 @@ static void determine_replay_mode(struct rev_cmdline_info *cmd_info,
 		*onto = peel_committish(*advance_name);
 		if (repo_dwim_ref(the_repository, *advance_name, strlen(*advance_name),
 			     &oid, &fullname, 0) == 1) {
+			free(*advance_name);
 			*advance_name = fullname;
 		} else {
 			die(_("argument to --advance must be a reference"));
@@ -197,6 +198,7 @@ static void determine_replay_mode(struct rev_cmdline_info *cmd_info,
 		if (negative_refs_complete) {
 			struct hashmap_iter iter;
 			struct strmap_entry *entry;
+			const char *last_key = NULL;
 
 			if (rinfo.negative_refexprs == 0)
 				die(_("all positive revisions given must be references"));
@@ -208,8 +210,11 @@ static void determine_replay_mode(struct rev_cmdline_info *cmd_info,
 			/* Only one entry, but we have to loop to get it */
 			strset_for_each_entry(&rinfo.negative_refs,
 					      &iter, entry) {
-				*advance_name = entry->key;
+				last_key = entry->key;
 			}
+
+			free(*advance_name);
+			*advance_name = xstrdup_or_null(last_key);
 		} else { /* positive_refs_complete */
 			if (rinfo.negative_refexprs > 1)
 				die(_("cannot implicitly determine correct base for --onto"));
@@ -271,7 +276,8 @@ static struct commit *pick_regular_commit(struct commit *pickme,
 
 int cmd_replay(int argc, const char **argv, const char *prefix)
 {
-	const char *advance_name = NULL;
+	const char *advance_name_opt = NULL;
+	char *advance_name = NULL;
 	struct commit *onto = NULL;
 	const char *onto_name = NULL;
 	int contained = 0;
@@ -292,7 +298,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
 		NULL
 	};
 	struct option replay_options[] = {
-		OPT_STRING(0, "advance", &advance_name,
+		OPT_STRING(0, "advance", &advance_name_opt,
 			   N_("branch"),
 			   N_("make replay advance given branch")),
 		OPT_STRING(0, "onto", &onto_name,
@@ -306,14 +312,15 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, replay_options, replay_usage,
 			     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN_OPT);
 
-	if (!onto_name && !advance_name) {
+	if (!onto_name && !advance_name_opt) {
 		error(_("option --onto or --advance is mandatory"));
 		usage_with_options(replay_usage, replay_options);
 	}
 
-	if (advance_name && contained)
+	if (advance_name_opt && contained)
 		die(_("options '%s' and '%s' cannot be used together"),
 		    "--advance", "--contained");
+	advance_name = xstrdup_or_null(advance_name_opt);
 
 	repo_init_revisions(the_repository, &revs, prefix);
 
@@ -441,6 +448,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
 
 cleanup:
 	release_revisions(&revs);
+	free(advance_name);
 
 	/* Return */
 	if (ret < 0)
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index 389670262e..12bd3db4cb 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -5,6 +5,7 @@ test_description='basic git replay tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 GIT_AUTHOR_NAME=author@name
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 01/23] builtin/replay: plug leaking `advance_name` variable

[Taylor Blau]

On Fri, Jul 26, 2024 at 02:13:59PM +0200, Patrick Steinhardt wrote:
> The `advance_name` variable can either contain a static string when
> parsed via the `--advance` command line option or it may be an allocated
> string when set via `determine_replay_mode()`. Because we cannot be sure
> whether it is allocated or not we just didn't free it at all, resulting
> in a memory leak.
>
> Split up the variables such that we can track the static and allocated
> strings separately and then free the allocated one to fix the memory
> leak.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/replay.c         | 20 ++++++++++++++------
>  t/t3650-replay-basics.sh |  1 +
>  2 files changed, 15 insertions(+), 6 deletions(-)

I had to read this patch a couple of times to make sure that I
understood the flow into and out of determine_replay_mode(). But after
reading it a couple of times, I agree with the change that you made
here.

Thanks,
Taylor

[PATCH 02/23] builtin/log: fix leaking branch name when creating cover letters

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1864 bytes --]

When calling `make_cover_letter()` without a branch name, then we try to
derive the branch name by calling `find_branch_name()`. But while this
function returns an allocated string, we never free the result and thus
have a memory leak. Fix this.

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

diff --git a/builtin/log.c b/builtin/log.c
index 4d4b60caa7..a73a767606 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1434,6 +1434,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
 	int need_8bit_cte = 0;
 	struct pretty_print_context pp = {0};
 	struct commit *head = list[0];
+	char *to_free = NULL;
 
 	if (!cmit_fmt_is_mail(rev->commit_format))
 		die(_("cover letter needs email format"));
@@ -1455,7 +1456,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
 	}
 
 	if (!branch_name)
-		branch_name = find_branch_name(rev);
+		branch_name = to_free = find_branch_name(rev);
 
 	pp.fmt = CMIT_FMT_EMAIL;
 	pp.date_mode.type = DATE_RFC2822;
@@ -1466,6 +1467,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
 			   encoding, need_8bit_cte, cfg);
 	fprintf(rev->diffopt.file, "%s\n", sb.buf);
 
+	free(to_free);
 	free(pp.after_subject);
 	strbuf_release(&sb);
 
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index a767c3520e..973e20254b 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -5,6 +5,7 @@ test_description='range-diff tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Note that because of the range-diff's heuristics, test_commit does more
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 02/23] builtin/log: fix leaking branch name when creating cover letters

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 318 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> When calling `make_cover_letter()` without a branch name, then we try to

Nit: s/then//

> derive the branch name by calling `find_branch_name()`. But while this
> function returns an allocated string, we never free the result and thus
> have a memory leak. Fix this.

[snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 02/23] builtin/log: fix leaking branch name when creating cover letters

[Taylor Blau]

On Tue, Jul 30, 2024 at 02:14:00AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > When calling `make_cover_letter()` without a branch name, then we try to
>
> Nit: s/then//

Good spotting :-).

Thanks,
Taylor

[PATCH 03/23] builtin/describe: fix memory leak with `--contains=`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1975 bytes --]

When calling `git describe --contains=`, we end up invoking
`cmd_name_rev()` with some munged argv array. This array may contain
allocated strings and furthermore will likely be modified by the called
function. This results in two memory leaks:

  - First, we leak the array that we use to assemble the arguments.

  - Second, we leak the allocated strings that we may have put into the
    array.

Fix those leaks by creating a separate copy of the array that we can
hand over to `cmd_name_rev()`. This allows us to free all strings
contained in the `strvec`, as the original vector will not be modified
anymore.

Furthermore, free both the `strvec` and the copied array to fix the
first memory leak.

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

diff --git a/builtin/describe.c b/builtin/describe.c
index cf8edc4222..4c0980c675 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -619,6 +619,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 	if (contains) {
 		struct string_list_item *item;
 		struct strvec args;
+		const char **argv_copy;
+		int ret;
 
 		strvec_init(&args);
 		strvec_pushl(&args, "name-rev",
@@ -637,7 +639,21 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 			strvec_pushv(&args, argv);
 		else
 			strvec_push(&args, "HEAD");
-		return cmd_name_rev(args.nr, args.v, prefix);
+
+		/*
+		 * `cmd_name_rev()` modifies the array, so we'd link its
+		 * contained strings if we didn't do a copy here.
+		 */
+		ALLOC_ARRAY(argv_copy, args.nr + 1);
+		for (size_t i = 0; i < args.nr; i++)
+			argv_copy[i] = args.v[i];
+		argv_copy[args.nr] = NULL;
+
+		ret = cmd_name_rev(args.nr, argv_copy, prefix);
+
+		strvec_clear(&args);
+		free(argv_copy);
+		return ret;
 	}
 
 	hashmap_init(&names, commit_name_neq, NULL, 0);
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 03/23] builtin/describe: fix memory leak with `--contains=`

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 893 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

[snip]

> @@ -637,7 +639,21 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
>  			strvec_pushv(&args, argv);
>  		else
>  			strvec_push(&args, "HEAD");
> -		return cmd_name_rev(args.nr, args.v, prefix);
> +
> +		/*
> +		 * `cmd_name_rev()` modifies the array, so we'd link its
> +		 * contained strings if we didn't do a copy here.
> +		 */
> +		ALLOC_ARRAY(argv_copy, args.nr + 1);
> +		for (size_t i = 0; i < args.nr; i++)
> +			argv_copy[i] = args.v[i];
> +		argv_copy[args.nr] = NULL;

Eventhough we pass `args.nr`, we still set the last element to NULL.
This replicates what strvec does and makes it more robust. Nice.

> +		ret = cmd_name_rev(args.nr, argv_copy, prefix);
> +
> +		strvec_clear(&args);
> +		free(argv_copy);
> +		return ret;
>  	}
>
>  	hashmap_init(&names, commit_name_neq, NULL, 0);
> --
> 2.46.0.rc1.dirty

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 03/23] builtin/describe: fix memory leak with `--contains=`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> When calling `git describe --contains=`, we end up invoking
> `cmd_name_rev()` with some munged argv array. This array may contain
> allocated strings and furthermore will likely be modified by the called
> function. This results in two memory leaks:
>
>   - First, we leak the array that we use to assemble the arguments.
>
>   - Second, we leak the allocated strings that we may have put into the
>     array.
>
> Fix those leaks by creating a separate copy of the array that we can
> hand over to `cmd_name_rev()`. This allows us to free all strings
> contained in the `strvec`, as the original vector will not be modified
> anymore.

OK, the separate copy has to be a shallow copy, as its purpose is
not to lose pointers to the contained strings.

> Furthermore, free both the `strvec` and the copied array to fix the
> first memory leak.
> ...
> +		strvec_clear(&args);
> +		free(argv_copy);

So, calling cmd_name_rev() may shuffle the argv_copy[] array but at
least it will not free any element in it (as expected---it is
typically the (argc, argv[]) the process receives from getting
exec'ed) [*].  Freeing the argv_copy shell itself is sufficient to
discard what we used to call cmd_name_rev().  And we discard args
both its content strings and the array.  OK.

> +		return ret;
>  	}
>  
>  	hashmap_init(&names, commit_name_neq, NULL, 0);


[Footnote]

 * The fact that cmd_foo() is called is not a hygiene thing to do in
   the first place, and in the longer term #leftoverbits we may need
   to refactor the thing further, into a proper library-ish reusable
   helper function that can be used to compute name_rev() any number
   of times, plus cmd_name_rev() and this caller that call it.

Re: [PATCH 03/23] builtin/describe: fix memory leak with `--contains=`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 732 bytes --]

On Tue, Jul 30, 2024 at 08:27:59AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> [Footnote]
> 
>  * The fact that cmd_foo() is called is not a hygiene thing to do in
>    the first place, and in the longer term #leftoverbits we may need
>    to refactor the thing further, into a proper library-ish reusable
>    helper function that can be used to compute name_rev() any number
>    of times, plus cmd_name_rev() and this caller that call it.

Agreed. There have been several instances of this scattered across the
codebase. The fix is quite ugly in my opinion, but it would be a bigger
topic to refactor those cases properly, so I refrained from doing so as
part of this series.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 03/23] builtin/describe: fix memory leak with `--contains=`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, Jul 30, 2024 at 08:27:59AM -0700, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> [Footnote]
>> 
>>  * The fact that cmd_foo() is called is not a hygiene thing to do in
>>    the first place, and in the longer term #leftoverbits we may need
>>    to refactor the thing further, into a proper library-ish reusable
>>    helper function that can be used to compute name_rev() any number
>>    of times, plus cmd_name_rev() and this caller that call it.
>
> Agreed. There have been several instances of this scattered across the
> codebase. The fix is quite ugly in my opinion, but it would be a bigger
> topic to refactor those cases properly, so I refrained from doing so as
> part of this series.

Oh, I agree that it would be a "after all the dust settles" kind of
clean-up.

Thanks.

Re: [PATCH 03/23] builtin/describe: fix memory leak with `--contains=`

[Taylor Blau]

On Fri, Jul 26, 2024 at 02:14:18PM +0200, Patrick Steinhardt wrote:
> @@ -637,7 +639,21 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
>  			strvec_pushv(&args, argv);
>  		else
>  			strvec_push(&args, "HEAD");
> -		return cmd_name_rev(args.nr, args.v, prefix);
> +
> +		/*
> +		 * `cmd_name_rev()` modifies the array, so we'd link its

s/link/leak/ ?

Thanks,
Taylor

[PATCH 04/23] builtin/describe: fix leaking array when running diff-index

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1684 bytes --]

When running git-describe(1) with `--dirty`, we will set up a `struct
rev_info` with arguments for git-diff-index(1). The way we assmble the
arguments it causes two memory leaks though:

  - We never release the `struct strvec`.

  - `setup_revisions()` may end up removing some entries from the
    `strvec`, which we wouldn't free even if we released the struct.

While we could plug those leaks, this is ultimately unnecessary as the
arguments we pass are part of a static array anyway. So instead,
refactor the code to drop the `struct strvec` and just pass this static
array directly.

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

diff --git a/builtin/describe.c b/builtin/describe.c
index 4c0980c675..3b61aa1baa 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -695,7 +695,6 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 		} else if (dirty) {
 			struct lock_file index_lock = LOCK_INIT;
 			struct rev_info revs;
-			struct strvec args = STRVEC_INIT;
 			int fd;
 
 			setup_work_tree();
@@ -710,8 +709,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 				repo_update_index_if_able(the_repository, &index_lock);
 
 			repo_init_revisions(the_repository, &revs, prefix);
-			strvec_pushv(&args, diff_index_args);
-			if (setup_revisions(args.nr, args.v, &revs, NULL) != 1)
+
+			if (setup_revisions(ARRAY_SIZE(diff_index_args) - 1,
+					    diff_index_args, &revs, NULL) != 1)
 				BUG("malformed internal diff-index command line");
 			run_diff_index(&revs, 0);
 
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 04/23] builtin/describe: fix leaking array when running diff-index

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 213 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> When running git-describe(1) with `--dirty`, we will set up a `struct
> rev_info` with arguments for git-diff-index(1). The way we assmble the

s/assmble/assemble

[snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH 05/23] builtin/describe: fix trivial memory leak when describing blob

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1171 bytes --]

We never free the `struct strvec args` variable in `describe_blob()`,
which thus causes a memory leak. Fix this.

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

diff --git a/builtin/describe.c b/builtin/describe.c
index 3b61aa1baa..aa5ec8d68a 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -529,6 +529,7 @@ static void describe_blob(struct object_id oid, struct strbuf *dst)
 	traverse_commit_list(&revs, process_commit, process_object, &pcd);
 	reset_revision_walk();
 	release_revisions(&revs);
+	strvec_clear(&args);
 }
 
 static void describe(const char *arg, int last_one)
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index d667dda654..95e9955bca 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -8,6 +8,7 @@ test_description='test git-specific bash prompt functions'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-bash.sh
 
 . "$GIT_BUILD_DIR/contrib/completion/git-prompt.sh"
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 06/23] builtin/name-rev: fix various trivial memory leaks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1705 bytes --]

There are several structures that we don't release after
`cmd_name_rev()` is done. Plug those leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/name-rev.c                   | 6 ++++--
 t/t6007-rev-list-cherry-pick-file.sh | 1 +
 t/t6120-describe.sh                  | 1 +
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 70e9ec4e47..f62c0a36cb 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -677,7 +677,9 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 				  always, allow_undefined, data.name_only);
 	}
 
-	UNLEAK(string_pool);
-	UNLEAK(revs);
+	string_list_clear(&data.ref_filters, 0);
+	string_list_clear(&data.exclude_filters, 0);
+	mem_pool_discard(&string_pool, 0);
+	object_array_clear(&revs);
 	return 0;
 }
diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index 6f3e543977..2d337d7287 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -5,6 +5,7 @@ test_description='test git rev-list --cherry-pick -- file'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # A---B---D---F
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 79e0f19deb..05ed2510d9 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -14,6 +14,7 @@ test_description='test describe'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 check_describe () {
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 06/23] builtin/name-rev: fix various trivial memory leaks

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> There are several structures that we don't release after
> `cmd_name_rev()` is done. Plug those leaks.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/name-rev.c                   | 6 ++++--
>  t/t6007-rev-list-cherry-pick-file.sh | 1 +
>  t/t6120-describe.sh                  | 1 +
>  3 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index 70e9ec4e47..f62c0a36cb 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -677,7 +677,9 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
>  				  always, allow_undefined, data.name_only);
>  	}
>  
> -	UNLEAK(string_pool);
> -	UNLEAK(revs);
> +	string_list_clear(&data.ref_filters, 0);
> +	string_list_clear(&data.exclude_filters, 0);
> +	mem_pool_discard(&string_pool, 0);
> +	object_array_clear(&revs);
>  	return 0;
>  }

You, originally these "we know we are at the very end of the
process, so _exit() will take care of releasing the resources" was a
very much deliberate, and in a sense, cmd_describe() calling to this
function as the last thing to do was still in the same spirit, but
it was not a hygiene thing to do.

In the longer term, we would want to make a major part of the body
of this "main" function into a reusable library-ish function, which
will be called the desribe and name-rev command implementations, and
when that happens, these fixes would move together to that
library-ish function.

Looking good.

[PATCH 07/23] builtin/submodule--helper: fix various trivial memory leaks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2587 bytes --]

There are multiple trivial memory leaks in the submodule helper. Fix
those.

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f1218a1995..5ae06c3e0b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2269,6 +2269,7 @@ static int is_tip_reachable(const char *path, const struct object_id *oid)
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf rev = STRBUF_INIT;
 	char *hex = oid_to_hex(oid);
+	int reachable;
 
 	cp.git_cmd = 1;
 	cp.dir = path;
@@ -2278,9 +2279,12 @@ static int is_tip_reachable(const char *path, const struct object_id *oid)
 	prepare_submodule_repo_env(&cp.env);
 
 	if (capture_command(&cp, &rev, GIT_MAX_HEXSZ + 1) || rev.len)
-		return 0;
+		reachable = 0;
+	else
+		reachable = 1;
 
-	return 1;
+	strbuf_release(&rev);
+	return reachable;
 }
 
 static int fetch_in_submodule(const char *module_path, int depth, int quiet,
@@ -3135,6 +3139,7 @@ static void append_fetch_remotes(struct strbuf *msg, const char *git_dir_path)
 static int add_submodule(const struct add_data *add_data)
 {
 	char *submod_gitdir_path;
+	char *depth_to_free = NULL;
 	struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;
 	struct string_list reference = STRING_LIST_INIT_NODUP;
 	int ret = -1;
@@ -3200,7 +3205,7 @@ static int add_submodule(const struct add_data *add_data)
 		}
 		clone_data.dissociate = add_data->dissociate;
 		if (add_data->depth >= 0)
-			clone_data.depth = xstrfmt("%d", add_data->depth);
+			clone_data.depth = depth_to_free = xstrfmt("%d", add_data->depth);
 
 		if (clone_submodule(&clone_data, &reference))
 			goto cleanup;
@@ -3223,7 +3228,9 @@ static int add_submodule(const struct add_data *add_data)
 			die(_("unable to checkout submodule '%s'"), add_data->sm_path);
 	}
 	ret = 0;
+
 cleanup:
+	free(depth_to_free);
 	string_list_clear(&reference, 1);
 	return ret;
 }
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 981488885f..098d8833b6 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -12,6 +12,7 @@ subcommands of git submodule.
 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 - enable local submodules' '
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 07/23] builtin/submodule--helper: fix various trivial memory leaks

[Rubén Justo]

On Fri, Jul 26, 2024 at 02:15:09PM +0200, Patrick Steinhardt wrote:

> There are multiple trivial memory leaks in the submodule helper. Fix
> those.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

Hi Patrick,

While working on another series, I fixed some leaks.  But, I haven't
sent any patches.

The series this message belongs to fixes all the leaks that my unsent
series addresses.  So, I won't bother sending my patches.

However, I think we can do better for the second leak solved in this
patch.  I leave my patch at the end of this message in case you want
to take a look.

In any case, thanks for working on this.  I have the habit of having
"SANITIZE=leak" in my config.mak, so this series will make me have
fewer distractions :)

> ---
>  builtin/submodule--helper.c | 13 ++++++++++---
>  t/t7400-submodule-basic.sh  |  1 +
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f1218a1995..5ae06c3e0b 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2269,6 +2269,7 @@ static int is_tip_reachable(const char *path, const struct object_id *oid)
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  	struct strbuf rev = STRBUF_INIT;
>  	char *hex = oid_to_hex(oid);
> +	int reachable;
>  
>  	cp.git_cmd = 1;
>  	cp.dir = path;
> @@ -2278,9 +2279,12 @@ static int is_tip_reachable(const char *path, const struct object_id *oid)
>  	prepare_submodule_repo_env(&cp.env);
>  
>  	if (capture_command(&cp, &rev, GIT_MAX_HEXSZ + 1) || rev.len)
> -		return 0;
> +		reachable = 0;
> +	else
> +		reachable = 1;
>  
> -	return 1;
> +	strbuf_release(&rev);
> +	return reachable;
>  }
>  
>  static int fetch_in_submodule(const char *module_path, int depth, int quiet,
> @@ -3135,6 +3139,7 @@ static void append_fetch_remotes(struct strbuf *msg, const char *git_dir_path)
>  static int add_submodule(const struct add_data *add_data)
>  {
>  	char *submod_gitdir_path;
> +	char *depth_to_free = NULL;
>  	struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;
>  	struct string_list reference = STRING_LIST_INIT_NODUP;
>  	int ret = -1;
> @@ -3200,7 +3205,7 @@ static int add_submodule(const struct add_data *add_data)
>  		}
>  		clone_data.dissociate = add_data->dissociate;
>  		if (add_data->depth >= 0)
> -			clone_data.depth = xstrfmt("%d", add_data->depth);
> +			clone_data.depth = depth_to_free = xstrfmt("%d", add_data->depth);
>  
>  		if (clone_submodule(&clone_data, &reference))
>  			goto cleanup;
> @@ -3223,7 +3228,9 @@ static int add_submodule(const struct add_data *add_data)
>  			die(_("unable to checkout submodule '%s'"), add_data->sm_path);
>  	}
>  	ret = 0;
> +
>  cleanup:
> +	free(depth_to_free);
>  	string_list_clear(&reference, 1);
>  	return ret;
>  }
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 981488885f..098d8833b6 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -12,6 +12,7 @@ subcommands of git submodule.
>  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 - enable local submodules' '
> -- 
> 2.46.0.rc1.dirty
> 

----- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH] submodule--helper: depth clone parameter

The `clone` subcommand has an OPT_STRING parameter: `--depth`, that
eventually is used when invoking `git-clone`.

As `git-clone` only support positive integer values for `--depth`
since 5594bcad21 (clone,fetch: catch non positive `--depth` option
value, 2013-12-05), seems safe to change the `--depth` parameter to
OPT_INTEGER in `module_clone`.

Doing so we avoid a leak in `add_submodule`.

Do it.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/submodule--helper.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f1218a1995..e7d52f38d9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1530,7 +1530,7 @@ struct module_clone_data {
 	const char *path;
 	const char *name;
 	const char *url;
-	const char *depth;
+	int depth;
 	struct list_objects_filter_options *filter_options;
 	unsigned int quiet: 1;
 	unsigned int progress: 1;
@@ -1729,8 +1729,8 @@ static int clone_submodule(const struct module_clone_data *clone_data,
 			strvec_push(&cp.args, "--quiet");
 		if (clone_data->progress)
 			strvec_push(&cp.args, "--progress");
-		if (clone_data->depth && *(clone_data->depth))
-			strvec_pushl(&cp.args, "--depth", clone_data->depth, NULL);
+		if (clone_data->depth > 0)
+			strvec_pushf(&cp.args, "--depth=%d", clone_data->depth);
 		if (reference->nr) {
 			struct string_list_item *item;
 
@@ -1851,8 +1851,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 			   N_("reference repository")),
 		OPT_BOOL(0, "dissociate", &dissociate,
 			   N_("use --reference only while cloning")),
-		OPT_STRING(0, "depth", &clone_data.depth,
-			   N_("string"),
+		OPT_INTEGER(0, "depth", &clone_data.depth,
 			   N_("depth for shallow clones")),
 		OPT__QUIET(&quiet, "suppress output for cloning a submodule"),
 		OPT_BOOL(0, "progress", &progress,
@@ -3199,8 +3198,7 @@ static int add_submodule(const struct add_data *add_data)
 			string_list_append(&reference, p)->util = p;
 		}
 		clone_data.dissociate = add_data->dissociate;
-		if (add_data->depth >= 0)
-			clone_data.depth = xstrfmt("%d", add_data->depth);
+		clone_data.depth = add_data->depth;
 
 		if (clone_submodule(&clone_data, &reference))
 			goto cleanup;
-- 
2.45.1

Re: [PATCH 07/23] builtin/submodule--helper: fix various trivial memory leaks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 850 bytes --]

On Wed, Jul 31, 2024 at 11:52:26PM +0200, Rubén Justo wrote:
> On Fri, Jul 26, 2024 at 02:15:09PM +0200, Patrick Steinhardt wrote:
> 
> > There are multiple trivial memory leaks in the submodule helper. Fix
> > those.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> 
> Hi Patrick,
> 
> While working on another series, I fixed some leaks.  But, I haven't
> sent any patches.
> 
> The series this message belongs to fixes all the leaks that my unsent
> series addresses.  So, I won't bother sending my patches.
> 
> However, I think we can do better for the second leak solved in this
> patch.  I leave my patch at the end of this message in case you want
> to take a look.

Agreed, your patch looks way neater. I'll massage the patch a bit and
then add it to the series with an "Original-patch-by" trailer.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 08/23] builtin/ls-remote: fix leaking `pattern` strings

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2317 bytes --]

Users can pass patterns to git-ls-remote(1), which allows them to filter
the list of printed references. We assemble those patterns into an array
and prefix them with "*/", but never free either the array nor the
allocated strings.

Fix those leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/ls-remote.c          | 11 +++++++----
 t/t5535-fetch-push-symref.sh |  1 +
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index debf2d4f88..500f76fe4c 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -47,7 +47,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	int status = 0;
 	int show_symref_target = 0;
 	const char *uploadpack = NULL;
-	const char **pattern = NULL;
+	char **pattern = NULL;
 	struct transport_ls_refs_options transport_options =
 		TRANSPORT_LS_REFS_OPTIONS_INIT;
 	int i;
@@ -96,9 +96,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	if (argc > 1) {
 		int i;
 		CALLOC_ARRAY(pattern, argc);
-		for (i = 1; i < argc; i++) {
+		for (i = 1; i < argc; i++)
 			pattern[i - 1] = xstrfmt("*/%s", argv[i]);
-		}
 	}
 
 	if (flags & REF_TAGS)
@@ -136,7 +135,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		struct ref_array_item *item;
 		if (!check_ref_type(ref, flags))
 			continue;
-		if (!tail_match(pattern, ref->name))
+		if (!tail_match((const char **) pattern, ref->name))
 			continue;
 		item = ref_array_push(&ref_array, ref->name, &ref->old_oid);
 		item->symref = xstrdup_or_null(ref->symref);
@@ -158,5 +157,9 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	if (transport_disconnect(transport))
 		status = 1;
 	transport_ls_refs_options_release(&transport_options);
+
+	for (i = 1; i < argc; i++)
+		free(pattern[i - 1]);
+	free(pattern);
 	return status;
 }
diff --git a/t/t5535-fetch-push-symref.sh b/t/t5535-fetch-push-symref.sh
index e8f6d233ff..7122af7fdb 100755
--- a/t/t5535-fetch-push-symref.sh
+++ b/t/t5535-fetch-push-symref.sh
@@ -2,6 +2,7 @@
 
 test_description='avoiding conflicting update through symref aliasing'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 08/23] builtin/ls-remote: fix leaking `pattern` strings

[Taylor Blau]

On Fri, Jul 26, 2024 at 02:15:24PM +0200, Patrick Steinhardt wrote:
> Users can pass patterns to git-ls-remote(1), which allows them to filter
> the list of printed references. We assemble those patterns into an array
> and prefix them with "*/", but never free either the array nor the
> allocated strings.
>
> Fix those leaks.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/ls-remote.c          | 11 +++++++----
>  t/t5535-fetch-push-symref.sh |  1 +
>  2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
> index debf2d4f88..500f76fe4c 100644
> --- a/builtin/ls-remote.c
> +++ b/builtin/ls-remote.c
> @@ -47,7 +47,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>  	int status = 0;
>  	int show_symref_target = 0;
>  	const char *uploadpack = NULL;
> -	const char **pattern = NULL;
> +	char **pattern = NULL;
>  	struct transport_ls_refs_options transport_options =
>  		TRANSPORT_LS_REFS_OPTIONS_INIT;
>  	int i;
> @@ -96,9 +96,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>  	if (argc > 1) {
>  		int i;
>  		CALLOC_ARRAY(pattern, argc);
> -		for (i = 1; i < argc; i++) {
> +		for (i = 1; i < argc; i++)
>  			pattern[i - 1] = xstrfmt("*/%s", argv[i]);
> -		}

Instead of xstrfmt()-ing these strings directly, it may be more
ergonomic to use a strvec here and call strvec_pushf(). That would save
you explicitly CALLOC'ing the array, and would also save you the
explicit free() on each element of pattern below.

>  	}
>
>  	if (flags & REF_TAGS)
> @@ -136,7 +135,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>  		struct ref_array_item *item;
>  		if (!check_ref_type(ref, flags))
>  			continue;
> -		if (!tail_match(pattern, ref->name))
> +		if (!tail_match((const char **) pattern, ref->name))

You could also drop the const cast here as well. The resulting patch on
top of this one simplifies things a bit:

--- 8< ---
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 500f76fe4c..b59ac98d81 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -47,7 +47,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	int status = 0;
 	int show_symref_target = 0;
 	const char *uploadpack = NULL;
-	char **pattern = NULL;
+	struct strvec pattern = STRVEC_INIT;
 	struct transport_ls_refs_options transport_options =
 		TRANSPORT_LS_REFS_OPTIONS_INIT;
 	int i;
@@ -93,12 +93,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)

 	packet_trace_identity("ls-remote");

-	if (argc > 1) {
-		int i;
-		CALLOC_ARRAY(pattern, argc);
-		for (i = 1; i < argc; i++)
-			pattern[i - 1] = xstrfmt("*/%s", argv[i]);
-	}
+	for (int i = 1; i < argc; i++)
+		strvec_pushf(&pattern, "*/%s", argv[i]);

 	if (flags & REF_TAGS)
 		strvec_push(&transport_options.ref_prefixes, "refs/tags/");
@@ -135,7 +131,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		struct ref_array_item *item;
 		if (!check_ref_type(ref, flags))
 			continue;
-		if (!tail_match((const char **) pattern, ref->name))
+		if (!tail_match(pattern.v, ref->name))
 			continue;
 		item = ref_array_push(&ref_array, ref->name, &ref->old_oid);
 		item->symref = xstrdup_or_null(ref->symref);
@@ -158,8 +154,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		status = 1;
 	transport_ls_refs_options_release(&transport_options);

-	for (i = 1; i < argc; i++)
-		free(pattern[i - 1]);
-	free(pattern);
+	strvec_clear(&pattern);
 	return status;
 }
--- >8 ---

Thanks,
Taylor

Re: [PATCH 08/23] builtin/ls-remote: fix leaking `pattern` strings

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

On Wed, Jul 31, 2024 at 12:35:29PM -0400, Taylor Blau wrote:
> On Fri, Jul 26, 2024 at 02:15:24PM +0200, Patrick Steinhardt wrote:
> > Users can pass patterns to git-ls-remote(1), which allows them to filter
> > the list of printed references. We assemble those patterns into an array
> > and prefix them with "*/", but never free either the array nor the
> > allocated strings.
> >
> > Fix those leaks.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  builtin/ls-remote.c          | 11 +++++++----
> >  t/t5535-fetch-push-symref.sh |  1 +
> >  2 files changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
> > index debf2d4f88..500f76fe4c 100644
> > --- a/builtin/ls-remote.c
> > +++ b/builtin/ls-remote.c
> > @@ -47,7 +47,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
> >  	int status = 0;
> >  	int show_symref_target = 0;
> >  	const char *uploadpack = NULL;
> > -	const char **pattern = NULL;
> > +	char **pattern = NULL;
> >  	struct transport_ls_refs_options transport_options =
> >  		TRANSPORT_LS_REFS_OPTIONS_INIT;
> >  	int i;
> > @@ -96,9 +96,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
> >  	if (argc > 1) {
> >  		int i;
> >  		CALLOC_ARRAY(pattern, argc);
> > -		for (i = 1; i < argc; i++) {
> > +		for (i = 1; i < argc; i++)
> >  			pattern[i - 1] = xstrfmt("*/%s", argv[i]);
> > -		}
> 
> Instead of xstrfmt()-ing these strings directly, it may be more
> ergonomic to use a strvec here and call strvec_pushf(). That would save
> you explicitly CALLOC'ing the array, and would also save you the
> explicit free() on each element of pattern below.

Agreed, that reads way better. Thanks!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 09/23] builtin/remote: fix leaking strings in `branch_list`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1394 bytes --]

The `struct string_list branch_list` is declared as `NODUP`, which makes
it not copy strings inserted into it. This causes memory leaks though,
as this means it also won't be responsible for _freeing_ inserted
strings. Thus, every branch we add to this will leak.

Fix this by marking the list as `DUP` instead and free the local copy we
have of the variable.

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

diff --git a/builtin/remote.c b/builtin/remote.c
index 08292498bd..303da7f73f 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -258,7 +258,7 @@ struct branch_info {
 	char *push_remote_name;
 };
 
-static struct string_list branch_list = STRING_LIST_INIT_NODUP;
+static struct string_list branch_list = STRING_LIST_INIT_DUP;
 
 static const char *abbrev_ref(const char *name, const char *prefix)
 {
@@ -292,8 +292,8 @@ static int config_read_branches(const char *key, const char *value,
 		type = PUSH_REMOTE;
 	else
 		return 0;
-	name = xmemdupz(key, key_len);
 
+	name = xmemdupz(key, key_len);
 	item = string_list_insert(&branch_list, name);
 
 	if (!item->util)
@@ -337,6 +337,7 @@ static int config_read_branches(const char *key, const char *value,
 		BUG("unexpected type=%d", type);
 	}
 
+	free(name);
 	return 0;
 }
 
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 09/23] builtin/remote: fix leaking strings in `branch_list`

[Taylor Blau]

On Fri, Jul 26, 2024 at 02:15:36PM +0200, Patrick Steinhardt wrote:
> @@ -292,8 +292,8 @@ static int config_read_branches(const char *key, const char *value,
>  		type = PUSH_REMOTE;
>  	else
>  		return 0;
> -	name = xmemdupz(key, key_len);
>
> +	name = xmemdupz(key, key_len);

I stared at this for longer than I'd like to admit wondering if there
was a whitespace error in the pre-image or something. But I see you just
moved initializing 'name' next to the initialization of 'item'.

I'd argue that might be a stray diff, but I don't think it matters much.

>  	item = string_list_insert(&branch_list, name);
>
>  	if (!item->util)
> @@ -337,6 +337,7 @@ static int config_read_branches(const char *key, const char *value,
>  		BUG("unexpected type=%d", type);
>  	}
>
> +	free(name);
>  	return 0;
>  }

The patch looks good to me otherwise.

Thanks,
Taylor

[PATCH 10/23] builtin/remote: fix various trivial memory leaks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 6099 bytes --]

There are multiple trivial memory leaks in git-remote(1). Fix those.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/remote.c          | 39 ++++++++++++++++++++++++++++++++-------
 t/t5512-ls-remote.sh      |  1 +
 t/t5514-fetch-multiple.sh |  1 +
 t/t5520-pull.sh           |  1 +
 t/t5528-push-default.sh   |  1 +
 t/t5543-atomic-push.sh    |  1 +
 t/t5570-git-daemon.sh     |  1 +
 7 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 303da7f73f..9d54fddf8c 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -555,13 +555,16 @@ static int add_branch_for_removal(const char *refname,
 	refspec.dst = (char *)refname;
 	if (remote_find_tracking(branches->remote, &refspec))
 		return 0;
+	free(refspec.src);
 
 	/* don't delete a branch if another remote also uses it */
 	for (kr = branches->keep->list; kr; kr = kr->next) {
 		memset(&refspec, 0, sizeof(refspec));
 		refspec.dst = (char *)refname;
-		if (!remote_find_tracking(kr->remote, &refspec))
+		if (!remote_find_tracking(kr->remote, &refspec)) {
+			free(refspec.src);
 			return 0;
+		}
 	}
 
 	/* don't delete non-remote-tracking refs */
@@ -668,7 +671,11 @@ static int config_read_push_default(const char *key, const char *value,
 static void handle_push_default(const char* old_name, const char* new_name)
 {
 	struct push_default_info push_default = {
-		old_name, CONFIG_SCOPE_UNKNOWN, STRBUF_INIT, -1 };
+		.old_name = old_name,
+		.scope = CONFIG_SCOPE_UNKNOWN,
+		.origin = STRBUF_INIT,
+		.linenr = -1,
+	};
 	git_config(config_read_push_default, &push_default);
 	if (push_default.scope >= CONFIG_SCOPE_COMMAND)
 		; /* pass */
@@ -688,6 +695,8 @@ static void handle_push_default(const char* old_name, const char* new_name)
 			push_default.origin.buf, push_default.linenr,
 			old_name);
 	}
+
+	strbuf_release(&push_default.origin);
 }
 
 
@@ -785,7 +794,7 @@ static int mv(int argc, const char **argv, const char *prefix)
 	}
 
 	if (!refspec_updated)
-		return 0;
+		goto out;
 
 	/*
 	 * First remove symrefs, then rename the rest, finally create
@@ -851,10 +860,15 @@ static int mv(int argc, const char **argv, const char *prefix)
 		display_progress(progress, ++refs_renamed_nr);
 	}
 	stop_progress(&progress);
-	string_list_clear(&remote_branches, 1);
 
 	handle_push_default(rename.old_name, rename.new_name);
 
+out:
+	string_list_clear(&remote_branches, 1);
+	strbuf_release(&old_remote_context);
+	strbuf_release(&buf);
+	strbuf_release(&buf2);
+	strbuf_release(&buf3);
 	return 0;
 }
 
@@ -945,12 +959,21 @@ static int rm(int argc, const char **argv, const char *prefix)
 
 	if (!result) {
 		strbuf_addf(&buf, "remote.%s", remote->name);
-		if (git_config_rename_section(buf.buf, NULL) < 1)
-			return error(_("Could not remove config section '%s'"), buf.buf);
+		if (git_config_rename_section(buf.buf, NULL) < 1) {
+			result = error(_("Could not remove config section '%s'"), buf.buf);
+			goto out;
+		}
 
 		handle_push_default(remote->name, NULL);
 	}
 
+out:
+	for (struct known_remote *r = known_remotes.list; r;) {
+		struct known_remote *next = r->next;
+		free(r);
+		r = next;
+	}
+	strbuf_release(&buf);
 	return result;
 }
 
@@ -983,8 +1006,10 @@ static int append_ref_to_tracked_list(const char *refname,
 
 	memset(&refspec, 0, sizeof(refspec));
 	refspec.dst = (char *)refname;
-	if (!remote_find_tracking(states->remote, &refspec))
+	if (!remote_find_tracking(states->remote, &refspec)) {
 		string_list_append(&states->tracked, abbrev_branch(refspec.src));
+		free(refspec.src);
+	}
 
 	return 0;
 }
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 42e77eb5a9..d687d824d1 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -5,6 +5,7 @@ test_description='git ls-remote'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 generate_references () {
diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
index 25772c85c5..579872c258 100755
--- a/t/t5514-fetch-multiple.sh
+++ b/t/t5514-fetch-multiple.sh
@@ -5,6 +5,7 @@ test_description='fetch --all works correctly'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 setup_repository () {
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 47534f1062..1098cbd0a1 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -5,6 +5,7 @@ test_description='pulling into void'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 modify () {
diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
index 14f7eced9a..bc2bada34c 100755
--- a/t/t5528-push-default.sh
+++ b/t/t5528-push-default.sh
@@ -4,6 +4,7 @@ test_description='check various push.default settings'
 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 bare remotes' '
diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
index 04b47ad84a..479d103469 100755
--- a/t/t5543-atomic-push.sh
+++ b/t/t5543-atomic-push.sh
@@ -5,6 +5,7 @@ test_description='pushing to a repository using the atomic push option'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 mk_repo_pair () {
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index f9a9bf9503..c5f08b6799 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -4,6 +4,7 @@ test_description='test fetching over git protocol'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-git-daemon.sh
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 11/23] builtin/stash: fix various trivial memory leaks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4181 bytes --]

There are multiple trivial memory leaks in git-stash(1). Fix those.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/stash.c                    | 18 ++++++++++++++++--
 t/t2501-cwd-empty.sh               |  1 +
 t/t3903-stash.sh                   |  1 +
 t/t3904-stash-patch.sh             |  2 ++
 t/t3905-stash-include-untracked.sh |  1 +
 t/t7064-wtstatus-pv2.sh            |  1 +
 6 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 46b981c4dd..7f2f989b69 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1521,6 +1521,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 	struct strbuf patch = STRBUF_INIT;
 	struct strbuf stash_msg_buf = STRBUF_INIT;
 	struct strbuf untracked_files = STRBUF_INIT;
+	struct strbuf out = STRBUF_INIT;
 
 	if (patch_mode && keep_index == -1)
 		keep_index = 1;
@@ -1626,7 +1627,6 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 			struct child_process cp_add = CHILD_PROCESS_INIT;
 			struct child_process cp_diff = CHILD_PROCESS_INIT;
 			struct child_process cp_apply = CHILD_PROCESS_INIT;
-			struct strbuf out = STRBUF_INIT;
 
 			cp_add.git_cmd = 1;
 			strvec_push(&cp_add.args, "add");
@@ -1718,6 +1718,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 
 done:
 	strbuf_release(&patch);
+	strbuf_release(&out);
 	free_stash_info(&info);
 	strbuf_release(&stash_msg_buf);
 	strbuf_release(&untracked_files);
@@ -1869,6 +1870,8 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
 		OPT_SUBCOMMAND_F("save", &fn, save_stash, PARSE_OPT_NOCOMPLETE),
 		OPT_END()
 	};
+	const char **args_copy;
+	int ret;
 
 	git_config(git_stash_config, NULL);
 
@@ -1892,5 +1895,16 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
 	/* Assume 'stash push' */
 	strvec_push(&args, "push");
 	strvec_pushv(&args, argv);
-	return !!push_stash(args.nr, args.v, prefix, 1);
+
+	/*
+	 * `push_stash()` ends up modifying the array, which causes memory
+	 * leaks if we didn't copy the array here.
+	 */
+	DUP_ARRAY(args_copy, args.v, args.nr);
+
+	ret = !!push_stash(args.nr, args_copy, prefix, 1);
+
+	strvec_clear(&args);
+	free(args_copy);
+	return ret;
 }
diff --git a/t/t2501-cwd-empty.sh b/t/t2501-cwd-empty.sh
index f6d8d7d03d..8af4e8cfe3 100755
--- a/t/t2501-cwd-empty.sh
+++ b/t/t2501-cwd-empty.sh
@@ -2,6 +2,7 @@
 
 test_description='Test handling of the current working directory becoming empty'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index a7f71f8126..e4c0937f61 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -8,6 +8,7 @@ test_description='Test git stash'
 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-unique-files.sh
 
diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
index 368fc2a6cc..aa5019fd6c 100755
--- a/t/t3904-stash-patch.sh
+++ b/t/t3904-stash-patch.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='stash -p'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-patch-mode.sh
 
 test_expect_success 'setup' '
diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
index 1289ae3e07..a1733f45c3 100755
--- a/t/t3905-stash-include-untracked.sh
+++ b/t/t3905-stash-include-untracked.sh
@@ -5,6 +5,7 @@
 
 test_description='Test git stash --include-untracked'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'stash save --include-untracked some dirty working directory' '
diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
index 11884d2fc3..06c1301222 100755
--- a/t/t7064-wtstatus-pv2.sh
+++ b/t/t7064-wtstatus-pv2.sh
@@ -4,6 +4,7 @@ test_description='git status --porcelain=v2
 
 This test exercises porcelain V2 output for git status.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 11/23] builtin/stash: fix various trivial memory leaks

[Taylor Blau]

On Fri, Jul 26, 2024 at 02:16:12PM +0200, Patrick Steinhardt wrote:
> @@ -1892,5 +1895,16 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
>  	/* Assume 'stash push' */
>  	strvec_push(&args, "push");
>  	strvec_pushv(&args, argv);
> -	return !!push_stash(args.nr, args.v, prefix, 1);
> +
> +	/*
> +	 * `push_stash()` ends up modifying the array, which causes memory
> +	 * leaks if we didn't copy the array here.
> +	 */
> +	DUP_ARRAY(args_copy, args.v, args.nr);
> +
> +	ret = !!push_stash(args.nr, args_copy, prefix, 1);
> +
> +	strvec_clear(&args);
> +	free(args_copy);
> +	return ret;
>  }

OK, so this is the same pattern as we saw in the third patch of this
series. I agree with Junio's comments in that sub-thread, but also that
they are out-of-scope for the present series ;-).

Looking good.

Thanks,
Taylor

[PATCH 12/23] builtin/rev-parse: fix memory leak with `--parseopt`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1606 bytes --]

The `--parseopt` mode allows shell scripts to have the same option
parsing mode as we have in C builtins. It soaks up a set of option
descriptions via stdin and massages them into proper `struct option`s
that we can then use to parse a set of arguments.

We only partially free those options when done though, creating a memory
leak. Interestingly, we only end up free'ing the first option's help,
which is of course wrong.

Fix this by freeing all option's help fields as well as their `argh`
fielids to plug this memory leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/rev-parse.c           | 5 ++++-
 t/t1502-rev-parse-parseopt.sh | 2 ++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 2e64f5bda7..5845d3f59b 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -553,7 +553,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 	strbuf_release(&sb);
 	strvec_clear(&longnames);
 	strvec_clear(&usage);
-	free((char *) opts->help);
+	for (size_t i = 0; i < opts_nr; i++) {
+		free((char *) opts[i].help);
+		free((char *) opts[i].argh);
+	}
 	free(opts);
 	return 0;
 }
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index b754b9fd74..5eaa6428c4 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test git rev-parse --parseopt'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 check_invalid_long_option () {
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 12/23] builtin/rev-parse: fix memory leak with `--parseopt`

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 616 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> The `--parseopt` mode allows shell scripts to have the same option
> parsing mode as we have in C builtins. It soaks up a set of option
> descriptions via stdin and massages them into proper `struct option`s
> that we can then use to parse a set of arguments.
>
> We only partially free those options when done though, creating a memory
> leak. Interestingly, we only end up free'ing the first option's help,
> which is of course wrong.
>
> Fix this by freeing all option's help fields as well as their `argh`
> fielids to plug this memory leak.
>

s/fielids/fields

[snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH 13/23] builtin/show-branch: fix several memory leaks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5251 bytes --]

There are several memory leaks in git-show-branch(1). Fix them.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/show-branch.c  | 52 +++++++++++++++++++++++++++++-------------
 t/t3202-show-branch.sh |  1 +
 t/t6010-merge-base.sh  |  1 +
 3 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index d72f4cb98d..7d797a880c 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -502,14 +502,14 @@ static int rev_is_head(const char *head, const char *name)
 	return !strcmp(head, name);
 }
 
-static int show_merge_base(struct commit_list *seen, int num_rev)
+static int show_merge_base(const struct commit_list *seen, int num_rev)
 {
 	int all_mask = ((1u << (REV_SHIFT + num_rev)) - 1);
 	int all_revs = all_mask & ~((1u << REV_SHIFT) - 1);
 	int exit_status = 1;
 
-	while (seen) {
-		struct commit *commit = pop_commit(&seen);
+	for (const struct commit_list *s = seen; s; s = s->next) {
+		struct commit *commit = s->item;
 		int flags = commit->object.flags & all_mask;
 		if (!(flags & UNINTERESTING) &&
 		    ((flags & all_revs) == all_revs)) {
@@ -635,7 +635,7 @@ static int parse_reflog_param(const struct option *opt, const char *arg,
 int cmd_show_branch(int ac, const char **av, const char *prefix)
 {
 	struct commit *rev[MAX_REVS], *commit;
-	char *reflog_msg[MAX_REVS];
+	char *reflog_msg[MAX_REVS] = {0};
 	struct commit_list *list = NULL, *seen = NULL;
 	unsigned int rev_mask[MAX_REVS];
 	int num_rev, i, extra = 0;
@@ -692,6 +692,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			    parse_reflog_param),
 		OPT_END()
 	};
+	const char **args_copy = NULL;
+	int ret;
 
 	init_commit_name_slab(&name_slab);
 
@@ -699,8 +701,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 
 	/* If nothing is specified, try the default first */
 	if (ac == 1 && default_args.nr) {
+		DUP_ARRAY(args_copy, default_args.v, default_args.nr);
 		ac = default_args.nr;
-		av = default_args.v;
+		av = args_copy;
 	}
 
 	ac = parse_options(ac, av, prefix, builtin_show_branch_options,
@@ -780,7 +783,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 		}
 
 		for (i = 0; i < reflog; i++) {
-			char *logmsg;
+			char *logmsg = NULL;
 			char *nth_desc;
 			const char *msg;
 			char *end;
@@ -790,6 +793,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			if (read_ref_at(get_main_ref_store(the_repository),
 					ref, flags, 0, base + i, &oid, &logmsg,
 					&timestamp, &tz, NULL)) {
+				free(logmsg);
 				reflog = i;
 				break;
 			}
@@ -842,7 +846,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 
 	if (!ref_name_cnt) {
 		fprintf(stderr, "No revs to be shown.\n");
-		exit(0);
+		ret = 0;
+		goto out;
 	}
 
 	for (num_rev = 0; ref_name[num_rev]; num_rev++) {
@@ -879,11 +884,15 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 
 	commit_list_sort_by_date(&seen);
 
-	if (merge_base)
-		return show_merge_base(seen, num_rev);
+	if (merge_base) {
+		ret = show_merge_base(seen, num_rev);
+		goto out;
+	}
 
-	if (independent)
-		return show_independent(rev, num_rev, rev_mask);
+	if (independent) {
+		ret = show_independent(rev, num_rev, rev_mask);
+		goto out;
+	}
 
 	/* Show list; --more=-1 means list-only */
 	if (1 < num_rev || extra < 0) {
@@ -919,8 +928,10 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			putchar('\n');
 		}
 	}
-	if (extra < 0)
-		exit(0);
+	if (extra < 0) {
+		ret = 0;
+		goto out;
+	}
 
 	/* Sort topologically */
 	sort_in_topological_order(&seen, sort_order);
@@ -932,8 +943,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 	all_mask = ((1u << (REV_SHIFT + num_rev)) - 1);
 	all_revs = all_mask & ~((1u << REV_SHIFT) - 1);
 
-	while (seen) {
-		struct commit *commit = pop_commit(&seen);
+	for (struct commit_list *l = seen; l; l = l->next) {
+		struct commit *commit = l->item;
 		int this_flag = commit->object.flags;
 		int is_merge_point = ((this_flag & all_revs) == all_revs);
 
@@ -973,6 +984,15 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 		if (shown_merge_point && --extra < 0)
 			break;
 	}
+
+	ret = 0;
+
+out:
+	for (size_t i = 0; i < ARRAY_SIZE(reflog_msg); i++)
+		free(reflog_msg[i]);
+	free_commit_list(seen);
+	free_commit_list(list);
+	free(args_copy);
 	free(head);
-	return 0;
+	return ret;
 }
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index a1139f79e2..3b6dad0c46 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -2,6 +2,7 @@
 
 test_description='test show-branch'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'error descriptions on empty repository' '
diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh
index 44c726ea39..f96ea82e78 100755
--- a/t/t6010-merge-base.sh
+++ b/t/t6010-merge-base.sh
@@ -6,6 +6,7 @@
 test_description='Merge base and parent list computation.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 M=1130000000
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 14/23] builtin/credential-store: fix leaking credential

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1454 bytes --]

We never free credentials read by the credential store, leading to a
memory leak. Plug it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/credential-store.c     | 1 +
 t/t0302-credential-store.sh    | 2 ++
 t/t0303-credential-external.sh | 1 +
 3 files changed, 4 insertions(+)

diff --git a/builtin/credential-store.c b/builtin/credential-store.c
index 494c809332..97968bfa1c 100644
--- a/builtin/credential-store.c
+++ b/builtin/credential-store.c
@@ -218,5 +218,6 @@ int cmd_credential_store(int argc, const char **argv, const char *prefix)
 		; /* Ignore unknown operation. */
 
 	string_list_clear(&fns, 0);
+	credential_clear(&c);
 	return 0;
 }
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index 716bf1af9f..f83db659e2 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='credential-store tests'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-credential.sh
 
diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
index 72ae405c3e..8aadbe86c4 100755
--- a/t/t0303-credential-external.sh
+++ b/t/t0303-credential-external.sh
@@ -29,6 +29,7 @@ you can set GIT_TEST_CREDENTIAL_HELPER_SETUP to a sequence of shell
 commands.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-credential.sh
 
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 15/23] builtin/rerere: fix various trivial memory leaks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2554 bytes --]

There are multiple trivial memory leaks in git-rerere(1). Fix those.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/rerere.c          | 8 +++++++-
 rerere.c                  | 9 +++++++--
 t/t2030-unresolve-info.sh | 1 +
 t/t4200-rerere.sh         | 1 +
 4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/builtin/rerere.c b/builtin/rerere.c
index b2efc6f640..81b65ffa39 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -73,11 +73,17 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
 
 	if (!strcmp(argv[0], "forget")) {
 		struct pathspec pathspec;
+		int ret;
+
 		if (argc < 2)
 			warning(_("'git rerere forget' without paths is deprecated"));
 		parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD,
 			       prefix, argv + 1);
-		return rerere_forget(the_repository, &pathspec);
+
+		ret = rerere_forget(the_repository, &pathspec);
+
+		clear_pathspec(&pathspec);
+		return ret;
 	}
 
 	if (!strcmp(argv[0], "clear")) {
diff --git a/rerere.c b/rerere.c
index 3a3888cce2..525ed6cc1e 100644
--- a/rerere.c
+++ b/rerere.c
@@ -1107,7 +1107,7 @@ static int rerere_forget_one_path(struct index_state *istate,
 
 int rerere_forget(struct repository *r, struct pathspec *pathspec)
 {
-	int i, fd;
+	int i, fd, ret;
 	struct string_list conflict = STRING_LIST_INIT_DUP;
 	struct string_list merge_rr = STRING_LIST_INIT_DUP;
 
@@ -1132,7 +1132,12 @@ int rerere_forget(struct repository *r, struct pathspec *pathspec)
 			continue;
 		rerere_forget_one_path(r->index, it->string, &merge_rr);
 	}
-	return write_rr(&merge_rr, fd);
+
+	ret = write_rr(&merge_rr, fd);
+
+	string_list_clear(&conflict, 0);
+	string_list_clear(&merge_rr, 1);
+	return ret;
 }
 
 /*
diff --git a/t/t2030-unresolve-info.sh b/t/t2030-unresolve-info.sh
index be3fcdde07..b3f6bc97b5 100755
--- a/t/t2030-unresolve-info.sh
+++ b/t/t2030-unresolve-info.sh
@@ -5,6 +5,7 @@ test_description='undoing resolution'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 check_resolve_undo () {
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index b0a3e84984..213b36fb96 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -25,6 +25,7 @@ test_description='git rerere
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 16/23] builtin/shortlog: fix various trivial memory leaks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 967 bytes --]

There is a trivial memory leak in git-shortlog(1). Fix it.

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

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 5bde7c68c2..b529608c92 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -514,4 +514,5 @@ void shortlog_output(struct shortlog *log)
 	string_list_clear(&log->list, 1);
 	clear_mailmap(&log->mailmap);
 	string_list_clear(&log->format, 0);
+	string_list_clear(&log->trailers, 0);
 }
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index f698d0c9ad..c20c885724 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -9,6 +9,7 @@ test_description='git shortlog
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 17/23] builtin/worktree: fix leaking derived branch names

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2719 bytes --]

There are several heuristics that git-worktree(1) uses to derive the
name of the newly created branch when not given explicitly. These
heuristics all allocate a new string, but we only end up freeing that
string in a subset of cases.

Fix the remaining cases where we didn't yet free the derived branch
names. While at it, also free `opt_track`, which is being populated via
an `OPT_PASSTHRU()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/worktree.c      | 7 ++++---
 t/t2400-worktree-add.sh | 1 +
 t/t9902-completion.sh   | 1 +
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 1d51e54fcd..cec3ada6b0 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -769,7 +769,7 @@ static int add(int ac, const char **av, const char *prefix)
 	char *branch_to_free = NULL;
 	char *new_branch_to_free = NULL;
 	const char *new_branch = NULL;
-	const char *opt_track = NULL;
+	char *opt_track = NULL;
 	const char *lock_reason = NULL;
 	int keep_locked = 0;
 	int used_new_branch_options;
@@ -846,7 +846,7 @@ static int add(int ac, const char **av, const char *prefix)
 	if (opts.orphan && !new_branch) {
 		int n;
 		const char *s = worktree_basename(path, &n);
-		new_branch = xstrndup(s, n);
+		new_branch = new_branch_to_free = xstrndup(s, n);
 	} else if (opts.orphan) {
 		; /* no-op */
 	} else if (opts.detach) {
@@ -875,7 +875,7 @@ static int add(int ac, const char **av, const char *prefix)
 			remote = unique_tracking_name(branch, &oid, NULL);
 			if (remote) {
 				new_branch = branch;
-				branch = remote;
+				branch = new_branch_to_free = remote;
 			}
 		}
 
@@ -923,6 +923,7 @@ static int add(int ac, const char **av, const char *prefix)
 
 	ret = add_worktree(path, branch, &opts);
 	free(path);
+	free(opt_track);
 	free(branch_to_free);
 	free(new_branch_to_free);
 	return ret;
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index ba320dc417..cfc4aeb179 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -6,6 +6,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 TEST_CREATE_REPO_NO_TEMPLATE=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-rebase.sh
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 932d5ad759..cc6aa9f0cd 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -16,6 +16,7 @@ test_untraceable=UnfortunatelyYes
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-bash.sh
 
 complete ()
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 18/23] builtin/credential-cache: fix trivial leaks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2600 bytes --]

There are two trivial leaks in git-credential-cache(1):

  - We leak the child process in `spawn_daemon()`. As we do not call
    `finish_command()` and instead let the created process daemonize, we
    have to clear the process manually.

  - We do not free the computed socket path in case it wasn't given via
    `--socket=`.

Plug both of these memory leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/credential-cache.c  | 9 +++++++--
 t/t0301-credential-cache.sh | 2 ++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c
index 3db8df70a9..aaf2f8438b 100644
--- a/builtin/credential-cache.c
+++ b/builtin/credential-cache.c
@@ -88,6 +88,8 @@ static void spawn_daemon(const char *socket)
 		die_errno("unable to read result code from cache daemon");
 	if (r != 3 || memcmp(buf, "ok\n", 3))
 		die("cache daemon did not start: %.*s", r, buf);
+
+	child_process_clear(&daemon);
 	close(daemon.out);
 }
 
@@ -137,7 +139,8 @@ static void announce_capabilities(void)
 
 int cmd_credential_cache(int argc, const char **argv, const char *prefix)
 {
-	char *socket_path = NULL;
+	const char *socket_path_arg = NULL;
+	char *socket_path;
 	int timeout = 900;
 	const char *op;
 	const char * const usage[] = {
@@ -147,7 +150,7 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_INTEGER(0, "timeout", &timeout,
 			    "number of seconds to cache credentials"),
-		OPT_STRING(0, "socket", &socket_path, "path",
+		OPT_STRING(0, "socket", &socket_path_arg, "path",
 			   "path of cache-daemon socket"),
 		OPT_END()
 	};
@@ -160,6 +163,7 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix)
 	if (!have_unix_sockets())
 		die(_("credential-cache unavailable; no unix socket support"));
 
+	socket_path = xstrdup_or_null(socket_path_arg);
 	if (!socket_path)
 		socket_path = get_socket_path();
 	if (!socket_path)
@@ -176,6 +180,7 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix)
 	else
 		; /* ignore unknown operation */
 
+	free(socket_path);
 	return 0;
 }
 
diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index c10e35905e..5d5b64205f 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='credential-cache tests'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-credential.sh
 
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 19/23] t/test-repository: fix leaking repository

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]

The test-repository test helper zeroes out `the_repository` such that it
can be sure that our codebase only ends up using the supplied repository
that we initialize in the respective helper functions. This does cause
memory leaks though as the data that `the_repository` has been holding
onto is not referenced anymore.

Fix this by calling `repo_clear()` instead.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/helper/test-repository.c | 4 ++--
 t/t5318-commit-graph.sh    | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/helper/test-repository.c b/t/helper/test-repository.c
index c6a074df3d..63c37de33d 100644
--- a/t/helper/test-repository.c
+++ b/t/helper/test-repository.c
@@ -19,7 +19,7 @@ static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,
 
 	setup_git_env(gitdir);
 
-	memset(the_repository, 0, sizeof(*the_repository));
+	repo_clear(the_repository);
 
 	if (repo_init(&r, gitdir, worktree))
 		die("Couldn't init repo");
@@ -49,7 +49,7 @@ static void test_get_commit_tree_in_graph(const char *gitdir,
 
 	setup_git_env(gitdir);
 
-	memset(the_repository, 0, sizeof(*the_repository));
+	repo_clear(the_repository);
 
 	if (repo_init(&r, gitdir, worktree))
 		die("Couldn't init repo");
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index a2b4442660..2916c07e3c 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='commit graph'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-chunk.sh
 
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 20/23] object-name: fix leaking commit list items

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4410 bytes --]

When calling `get_oid_oneline()`, we pass in a `struct commit_list` that
gets modified by the function. This creates a weird situation where the
commit list may sometimes be empty after returning, but sometimes it
will continue to carry additional commits. In those cases the remainder
of the list leaks.

Ultimately, the design where we only pass partial ownership to
`get_oid_oneline()` feels shoddy. Refactor the code such that we only
pass a constant pointer to the list, creating a local copy as needed.
Callers are thus always responsible for freeing the commit list, which
then allows us to plug a bunch of memory leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 object-name.c                | 26 ++++++++++++++++----------
 t/t1511-rev-parse-caret.sh   |  1 +
 t/t6133-pathspec-rev-dwim.sh |  2 ++
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/object-name.c b/object-name.c
index 527b853ac4..240a93e7ce 100644
--- a/object-name.c
+++ b/object-name.c
@@ -27,7 +27,8 @@
 #include "date.h"
 #include "object-file-convert.h"
 
-static int get_oid_oneline(struct repository *r, const char *, struct object_id *, struct commit_list *);
+static int get_oid_oneline(struct repository *r, const char *, struct object_id *,
+			   const struct commit_list *);
 
 typedef int (*disambiguate_hint_fn)(struct repository *, const struct object_id *, void *);
 
@@ -1254,6 +1255,8 @@ static int peel_onion(struct repository *r, const char *name, int len,
 		prefix = xstrndup(sp + 1, name + len - 1 - (sp + 1));
 		commit_list_insert((struct commit *)o, &list);
 		ret = get_oid_oneline(r, prefix, oid, list);
+
+		free_commit_list(list);
 		free(prefix);
 		return ret;
 	}
@@ -1388,9 +1391,10 @@ static int handle_one_ref(const char *path, const struct object_id *oid,
 
 static int get_oid_oneline(struct repository *r,
 			   const char *prefix, struct object_id *oid,
-			   struct commit_list *list)
+			   const struct commit_list *list)
 {
-	struct commit_list *backup = NULL, *l;
+	struct commit_list *copy = NULL;
+	const struct commit_list *l;
 	int found = 0;
 	int negative = 0;
 	regex_t regex;
@@ -1411,14 +1415,14 @@ static int get_oid_oneline(struct repository *r,
 
 	for (l = list; l; l = l->next) {
 		l->item->object.flags |= ONELINE_SEEN;
-		commit_list_insert(l->item, &backup);
+		commit_list_insert(l->item, &copy);
 	}
-	while (list) {
+	while (copy) {
 		const char *p, *buf;
 		struct commit *commit;
 		int matches;
 
-		commit = pop_most_recent_commit(&list, ONELINE_SEEN);
+		commit = pop_most_recent_commit(&copy, ONELINE_SEEN);
 		if (!parse_object(r, &commit->object.oid))
 			continue;
 		buf = repo_get_commit_buffer(r, commit, NULL);
@@ -1433,10 +1437,9 @@ static int get_oid_oneline(struct repository *r,
 		}
 	}
 	regfree(&regex);
-	free_commit_list(list);
-	for (l = backup; l; l = l->next)
+	for (l = list; l; l = l->next)
 		clear_commit_marks(l->item, ONELINE_SEEN);
-	free_commit_list(backup);
+	free_commit_list(copy);
 	return found ? 0 : -1;
 }
 
@@ -2024,7 +2027,10 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,
 			refs_for_each_ref(get_main_ref_store(repo), handle_one_ref, &cb);
 			refs_head_ref(get_main_ref_store(repo), handle_one_ref, &cb);
 			commit_list_sort_by_date(&list);
-			return get_oid_oneline(repo, name + 2, oid, list);
+			ret = get_oid_oneline(repo, name + 2, oid, list);
+
+			free_commit_list(list);
+			return ret;
 		}
 		if (namelen < 3 ||
 		    name[2] != ':' ||
diff --git a/t/t1511-rev-parse-caret.sh b/t/t1511-rev-parse-caret.sh
index 6ecfed86bc..e7e78a4028 100755
--- a/t/t1511-rev-parse-caret.sh
+++ b/t/t1511-rev-parse-caret.sh
@@ -5,6 +5,7 @@ test_description='tests for ref^{stuff}'
 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/t/t6133-pathspec-rev-dwim.sh b/t/t6133-pathspec-rev-dwim.sh
index a290ffca0d..6dd4bbbf9f 100755
--- a/t/t6133-pathspec-rev-dwim.sh
+++ b/t/t6133-pathspec-rev-dwim.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test dwim of revs versus pathspecs in revision parser'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 21/23] entry: fix leaking pathnames during delayed checkout

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3176 bytes --]

When filtering files during delayed checkout, we pass a string list to
`async_query_available_blobs()`. This list is initialized with NODUP,
and thus inserted strings will not be owned by the list. In the latter
function we then try to hand over ownership by passing an `xstrup()`'d
value to `string_list_insert()`. But this is not how this works: a NODUP
list does not take ownership of allocated strings and will never free
them for the caller.

Fix this issue by initializing the list as `DUP` instead and dropping
the explicit call to `xstrdup()`. This is okay to do given that this is
the single callsite of `async_query_available_blobs()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 convert.c                               | 2 +-
 entry.c                                 | 4 +++-
 t/t2080-parallel-checkout-basics.sh     | 1 +
 t/t2082-parallel-checkout-attributes.sh | 1 +
 4 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/convert.c b/convert.c
index d8737fe0f2..61a540e212 100644
--- a/convert.c
+++ b/convert.c
@@ -960,7 +960,7 @@ int async_query_available_blobs(const char *cmd, struct string_list *available_p
 	while ((line = packet_read_line(process->out, NULL))) {
 		const char *path;
 		if (skip_prefix(line, "pathname=", &path))
-			string_list_insert(available_paths, xstrdup(path));
+			string_list_insert(available_paths, path);
 		else
 			; /* ignore unknown keys */
 	}
diff --git a/entry.c b/entry.c
index e7ed440ce2..3143b9996b 100644
--- a/entry.c
+++ b/entry.c
@@ -191,7 +191,7 @@ int finish_delayed_checkout(struct checkout *state, int show_progress)
 		progress = start_delayed_progress(_("Filtering content"), dco->paths.nr);
 	while (dco->filters.nr > 0) {
 		for_each_string_list_item(filter, &dco->filters) {
-			struct string_list available_paths = STRING_LIST_INIT_NODUP;
+			struct string_list available_paths = STRING_LIST_INIT_DUP;
 
 			if (!async_query_available_blobs(filter->string, &available_paths)) {
 				/* Filter reported an error */
@@ -245,6 +245,8 @@ int finish_delayed_checkout(struct checkout *state, int show_progress)
 				} else
 					errs = 1;
 			}
+
+			string_list_clear(&available_paths, 0);
 		}
 
 		filter_string_list(&dco->filters, 0, string_is_not_null, NULL);
diff --git a/t/t2080-parallel-checkout-basics.sh b/t/t2080-parallel-checkout-basics.sh
index 5ffe1a41e2..59e5570cb2 100755
--- a/t/t2080-parallel-checkout-basics.sh
+++ b/t/t2080-parallel-checkout-basics.sh
@@ -8,6 +8,7 @@ working tree.
 '
 
 TEST_NO_CREATE_REPO=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY/lib-parallel-checkout.sh"
 
diff --git a/t/t2082-parallel-checkout-attributes.sh b/t/t2082-parallel-checkout-attributes.sh
index f3511cd43a..aec55496eb 100755
--- a/t/t2082-parallel-checkout-attributes.sh
+++ b/t/t2082-parallel-checkout-attributes.sh
@@ -10,6 +10,7 @@ properly (without access to the index or attribute stack).
 '
 
 TEST_NO_CREATE_REPO=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY/lib-parallel-checkout.sh"
 . "$TEST_DIRECTORY/lib-encoding.sh"
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 22/23] convert: fix leaking config strings

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1701 bytes --]

In `read_convert_config()`, we end up reading some string values into
variables. We don't free any potentially-existing old values though,
which will result in a memory leak in case the same key has been defined
multiple times.

Fix those leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 convert.c             | 12 +++++++++---
 t/t0021-conversion.sh |  1 +
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/convert.c b/convert.c
index 61a540e212..92ce04c406 100644
--- a/convert.c
+++ b/convert.c
@@ -1050,14 +1050,20 @@ static int read_convert_config(const char *var, const char *value,
 	 * The command-line will not be interpolated in any way.
 	 */
 
-	if (!strcmp("smudge", key))
+	if (!strcmp("smudge", key)) {
+		FREE_AND_NULL(drv->smudge);
 		return git_config_string(&drv->smudge, var, value);
+	}
 
-	if (!strcmp("clean", key))
+	if (!strcmp("clean", key)) {
+		FREE_AND_NULL(drv->clean);
 		return git_config_string(&drv->clean, var, value);
+	}
 
-	if (!strcmp("process", key))
+	if (!strcmp("process", key)) {
+		FREE_AND_NULL(drv->process);
 		return git_config_string(&drv->process, var, value);
+	}
 
 	if (!strcmp("required", key)) {
 		drv->required = git_config_bool(var, value);
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 0b4997022b..eeb2714d9d 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -5,6 +5,7 @@ test_description='blob conversion via gitattributes'
 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-terminal.sh
 
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 23/23] commit-reach: fix trivial memory leak when computing reachability

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 982 bytes --]

We don't free the local `stack` commit list that we use to compute
reachability of multiple commits at once. Do so.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 commit-reach.c             | 1 +
 t/t3201-branch-contains.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/commit-reach.c b/commit-reach.c
index dabc2972e4..02f8218b8e 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -1227,4 +1227,5 @@ void tips_reachable_from_bases(struct repository *r,
 done:
 	free(commits);
 	repo_clear_commit_marks(r, SEEN);
+	free_commit_list(stack);
 }
diff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh
index 800fc33165..6e587d27d7 100755
--- a/t/t3201-branch-contains.sh
+++ b/t/t3201-branch-contains.sh
@@ -2,6 +2,7 @@
 
 test_description='branch --contains <commit>, --no-contains <commit> --merged, and --no-merged'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 00/23] Memory leak fixes (pt.3)

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 870 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> Hi,
>
> I originally wanted to hold off with sending out this series until v2.46
> was out. But I saw that Junio sent out some patches which are plugging
> the same leaks as I did, so I dedcided to send it out now to avoid some
> duplicated work.
>
> There isn't really any structure to this series, I just happened to pick
> some random test suites that fail with the leak checker enabled and then
> fixed those. Naturally, I've also got part 4 of this series of patch
> series in the pipeline already :) As mentioned elsewhere, I hope to get
> the number of failing test suites to zero this year. Let's see whether
> this is realistic.
>
> Patrick
>

This was quite easy to review since there wasn't much to add and it was
clearly split into small commits. Apart from some nits, the series looks
great to me.

Thanks,
Karthik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 00/23] Memory leak fixes (pt.3)

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1252 bytes --]

On Tue, Jul 30, 2024 at 07:09:57AM -0400, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Hi,
> >
> > I originally wanted to hold off with sending out this series until v2.46
> > was out. But I saw that Junio sent out some patches which are plugging
> > the same leaks as I did, so I dedcided to send it out now to avoid some
> > duplicated work.
> >
> > There isn't really any structure to this series, I just happened to pick
> > some random test suites that fail with the leak checker enabled and then
> > fixed those. Naturally, I've also got part 4 of this series of patch
> > series in the pipeline already :) As mentioned elsewhere, I hope to get
> > the number of failing test suites to zero this year. Let's see whether
> > this is realistic.
> >
> > Patrick
> >
> 
> This was quite easy to review since there wasn't much to add and it was
> clearly split into small commits. Apart from some nits, the series looks
> great to me.

Thanks for your review! Given that until now the only thing that needs
fixing is smallish typoes I'll refrain from sending the whole thing out
just to fix those. I've fixed them locally though and will send them out
in case anything else needs fixing.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 00/23] Memory leak fixes (pt.3)

[Taylor Blau]

On Fri, Jul 26, 2024 at 02:13:43PM +0200, Patrick Steinhardt wrote:
>  57 files changed, 251 insertions(+), 73 deletions(-)

I took a careful read through these patches, and found most of them easy
to review. I was admittedly a little lost with the "fix various leak"
patches, and having slightly more context in the commit descriptions
there would have been helpful.

I think most of these patches look all good to me, but there is one
where I think using a strvec would be a better fit than allocating our
own array.

Otherwise, these are looking in good shape. Thanks for working on this!

Thanks,
Taylor

Re: [PATCH 00/23] Memory leak fixes (pt.3)

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 869 bytes --]

On Wed, Jul 31, 2024 at 01:01:07PM -0400, Taylor Blau wrote:
> On Fri, Jul 26, 2024 at 02:13:43PM +0200, Patrick Steinhardt wrote:
> >  57 files changed, 251 insertions(+), 73 deletions(-)
> 
> I took a careful read through these patches, and found most of them easy
> to review. I was admittedly a little lost with the "fix various leak"
> patches, and having slightly more context in the commit descriptions
> there would have been helpful.

Yeah, I was a bit torn here on whether to expand the commit messages or
not. I just didn't think it's all that useful to always say "Variable x
is allocated, but never freed" for trivial cases where allocation and
freeing need to happen in the same function, much less so if we repeat
such commits for every single such variable in the same subsystem. So I
just threw those fixes into a single bag.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 00/23] Memory leak fixes (pt.3)

[Taylor Blau]

On Thu, Aug 01, 2024 at 10:19:45AM +0200, Patrick Steinhardt wrote:
> On Wed, Jul 31, 2024 at 01:01:07PM -0400, Taylor Blau wrote:
> > On Fri, Jul 26, 2024 at 02:13:43PM +0200, Patrick Steinhardt wrote:
> > >  57 files changed, 251 insertions(+), 73 deletions(-)
> >
> > I took a careful read through these patches, and found most of them easy
> > to review. I was admittedly a little lost with the "fix various leak"
> > patches, and having slightly more context in the commit descriptions
> > there would have been helpful.
>
> Yeah, I was a bit torn here on whether to expand the commit messages or
> not. I just didn't think it's all that useful to always say "Variable x
> is allocated, but never freed" for trivial cases where allocation and
> freeing need to happen in the same function, much less so if we repeat
> such commits for every single such variable in the same subsystem. So I
> just threw those fixes into a single bag.

I think sometimes that level of detail is useful, e.g., for tracking
where the leak came from, if it's always existed, etc.

But I agree enumerating each leak in an otherwise straightforward commit
is not a good use of author or reviewer time. I think more useful would
be enumerating the kinds of leaks that you clean up.

Thanks,
Taylor

[PATCH v2 00/24] Memory leak fixes (pt.3)

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 19369 bytes --]

Hi,

this is the second version of my patch series that plugs another round
of memory leaks in Git. Changes compared to v2:

  - Various typo fixes.

  - Adopted a patch by Rubén that plugs a git-submodule--helper(1) leak
    in a neater way by using an integer to track the clone depth instead
    of using a (possibly allocated) string.

  - Refactored git-ls-remote(1) to use a `struct strvec` to track
    patterns instead of manually populating an array, which makes the
    code easier to reason about while still plugging the memory leak.
    This was suggested by Taylor.

Thanks for all the reviews so far!

Patrick

Patrick Steinhardt (23):
  builtin/replay: plug leaking `advance_name` variable
  builtin/log: fix leaking branch name when creating cover letters
  builtin/describe: fix memory leak with `--contains=`
  builtin/describe: fix leaking array when running diff-index
  builtin/describe: fix trivial memory leak when describing blob
  builtin/name-rev: fix various trivial memory leaks
  builtin/submodule--helper: fix leaking buffer in `is_tip_reachable`
  builtin/ls-remote: fix leaking `pattern` strings
  builtin/remote: fix leaking strings in `branch_list`
  builtin/remote: fix various trivial memory leaks
  builtin/stash: fix various trivial memory leaks
  builtin/rev-parse: fix memory leak with `--parseopt`
  builtin/show-branch: fix several memory leaks
  builtin/credential-store: fix leaking credential
  builtin/rerere: fix various trivial memory leaks
  builtin/shortlog: fix various trivial memory leaks
  builtin/worktree: fix leaking derived branch names
  builtin/credential-cache: fix trivial leaks
  t/test-repository: fix leaking repository
  object-name: fix leaking commit list items
  entry: fix leaking pathnames during delayed checkout
  convert: fix leaking config strings
  commit-reach: fix trivial memory leak when computing reachability

Rubén Justico (1):
  builtin/submodule--helper: fix leaking clone depth parameter

 builtin/credential-cache.c              |  9 ++++-
 builtin/credential-store.c              |  1 +
 builtin/describe.c                      | 25 ++++++++++--
 builtin/log.c                           |  4 +-
 builtin/ls-remote.c                     | 24 +++++-------
 builtin/name-rev.c                      |  6 ++-
 builtin/remote.c                        | 44 ++++++++++++++++-----
 builtin/replay.c                        | 20 +++++++---
 builtin/rerere.c                        |  8 +++-
 builtin/rev-parse.c                     |  5 ++-
 builtin/shortlog.c                      |  1 +
 builtin/show-branch.c                   | 52 +++++++++++++++++--------
 builtin/stash.c                         | 18 ++++++++-
 builtin/submodule--helper.c             | 20 ++++++----
 builtin/worktree.c                      |  7 ++--
 commit-reach.c                          |  1 +
 convert.c                               | 14 +++++--
 entry.c                                 |  4 +-
 object-name.c                           | 26 ++++++++-----
 rerere.c                                |  9 ++++-
 t/helper/test-repository.c              |  4 +-
 t/t0021-conversion.sh                   |  1 +
 t/t0301-credential-cache.sh             |  2 +
 t/t0302-credential-store.sh             |  2 +
 t/t0303-credential-external.sh          |  1 +
 t/t1502-rev-parse-parseopt.sh           |  2 +
 t/t1511-rev-parse-caret.sh              |  1 +
 t/t2030-unresolve-info.sh               |  1 +
 t/t2080-parallel-checkout-basics.sh     |  1 +
 t/t2082-parallel-checkout-attributes.sh |  1 +
 t/t2400-worktree-add.sh                 |  1 +
 t/t2501-cwd-empty.sh                    |  1 +
 t/t3201-branch-contains.sh              |  1 +
 t/t3202-show-branch.sh                  |  1 +
 t/t3206-range-diff.sh                   |  1 +
 t/t3650-replay-basics.sh                |  1 +
 t/t3903-stash.sh                        |  1 +
 t/t3904-stash-patch.sh                  |  2 +
 t/t3905-stash-include-untracked.sh      |  1 +
 t/t4200-rerere.sh                       |  1 +
 t/t4201-shortlog.sh                     |  1 +
 t/t5318-commit-graph.sh                 |  2 +
 t/t5512-ls-remote.sh                    |  1 +
 t/t5514-fetch-multiple.sh               |  1 +
 t/t5520-pull.sh                         |  1 +
 t/t5528-push-default.sh                 |  1 +
 t/t5535-fetch-push-symref.sh            |  1 +
 t/t5543-atomic-push.sh                  |  1 +
 t/t5570-git-daemon.sh                   |  1 +
 t/t6007-rev-list-cherry-pick-file.sh    |  1 +
 t/t6010-merge-base.sh                   |  1 +
 t/t6120-describe.sh                     |  1 +
 t/t6133-pathspec-rev-dwim.sh            |  2 +
 t/t7064-wtstatus-pv2.sh                 |  1 +
 t/t7400-submodule-basic.sh              |  1 +
 t/t9902-completion.sh                   |  1 +
 t/t9903-bash-prompt.sh                  |  1 +
 57 files changed, 256 insertions(+), 88 deletions(-)

Range-diff against v1:
 1:  dd044eacc2 =  1:  b5c8a9495a builtin/replay: plug leaking `advance_name` variable
 2:  4daae88877 !  2:  73a16fff43 builtin/log: fix leaking branch name when creating cover letters
    @@ Metadata
      ## Commit message ##
         builtin/log: fix leaking branch name when creating cover letters
     
    -    When calling `make_cover_letter()` without a branch name, then we try to
    +    When calling `make_cover_letter()` without a branch name, we try to
         derive the branch name by calling `find_branch_name()`. But while this
         function returns an allocated string, we never free the result and thus
         have a memory leak. Fix this.
 -:  ---------- >  3:  65027d13b7 builtin/describe: fix memory leak with `--contains=`
 4:  e8d86c01cf !  4:  19ca97e33a builtin/describe: fix leaking array when running diff-index
    @@ Commit message
         builtin/describe: fix leaking array when running diff-index
     
         When running git-describe(1) with `--dirty`, we will set up a `struct
    -    rev_info` with arguments for git-diff-index(1). The way we assmble the
    +    rev_info` with arguments for git-diff-index(1). The way we assemble the
         arguments it causes two memory leaks though:
     
           - We never release the `struct strvec`.
 5:  8aba7434bd =  5:  6deacb8667 builtin/describe: fix trivial memory leak when describing blob
 6:  088f730572 =  6:  e38e9d1b62 builtin/name-rev: fix various trivial memory leaks
 3:  08a12be13c !  7:  d3eb0372bd builtin/describe: fix memory leak with `--contains=`
    @@
      ## Metadata ##
    -Author: Patrick Steinhardt <ps@pks.im>
    +Author: Rubén Justico <rjusto@gmail.com>
     
      ## Commit message ##
    -    builtin/describe: fix memory leak with `--contains=`
    +    builtin/submodule--helper: fix leaking clone depth parameter
     
    -    When calling `git describe --contains=`, we end up invoking
    -    `cmd_name_rev()` with some munged argv array. This array may contain
    -    allocated strings and furthermore will likely be modified by the called
    -    function. This results in two memory leaks:
    +    The submodule helper supports a `--depth` parameter for both its "add"
    +    and "clone" subcommands, which in both cases end up being forwarded to
    +    git-clone(1). But while the former subcommand uses an `OPT_INTEGER()` to
    +    parse the depth, the latter uses `OPT_STRING()`. Consequently, it is
    +    possible to pass non-integer input to "--depth" when calling the "clone"
    +    subcommand, where the value will then ultimately cause git-clone(1) to
    +    bail out.
     
    -      - First, we leak the array that we use to assemble the arguments.
    +    Besides the fact that the parameter verification should happen earlier,
    +    the submodule helper infrastructure also internally tracks the depth via
    +    a string. This requires us to convert the integer in the "add"
    +    subcommand into an allocated string, and this string ultimately leaks.
     
    -      - Second, we leak the allocated strings that we may have put into the
    -        array.
    -
    -    Fix those leaks by creating a separate copy of the array that we can
    -    hand over to `cmd_name_rev()`. This allows us to free all strings
    -    contained in the `strvec`, as the original vector will not be modified
    -    anymore.
    -
    -    Furthermore, free both the `strvec` and the copied array to fix the
    -    first memory leak.
    +    Refactor the code to consistently track the clone depth as an integer.
    +    This plugs the memory leak, simplifies the code and allows us to use
    +    `OPT_INTEGER()` instead of `OPT_STRING()`, validating the input before
    +    we shell out to git--clone(1).
     
    +    Original-patch-by: Rubén Justo <rjusto@gmail.com>
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    - ## builtin/describe.c ##
    -@@ builtin/describe.c: int cmd_describe(int argc, const char **argv, const char *prefix)
    - 	if (contains) {
    - 		struct string_list_item *item;
    - 		struct strvec args;
    -+		const char **argv_copy;
    -+		int ret;
    + ## builtin/submodule--helper.c ##
    +@@ builtin/submodule--helper.c: struct module_clone_data {
    + 	const char *path;
    + 	const char *name;
    + 	const char *url;
    +-	const char *depth;
    ++	int depth;
    + 	struct list_objects_filter_options *filter_options;
    + 	unsigned int quiet: 1;
    + 	unsigned int progress: 1;
    +@@ builtin/submodule--helper.c: static int clone_submodule(const struct module_clone_data *clone_data,
    + 			strvec_push(&cp.args, "--quiet");
    + 		if (clone_data->progress)
    + 			strvec_push(&cp.args, "--progress");
    +-		if (clone_data->depth && *(clone_data->depth))
    +-			strvec_pushl(&cp.args, "--depth", clone_data->depth, NULL);
    ++		if (clone_data->depth > 0)
    ++			strvec_pushf(&cp.args, "--depth=%d", clone_data->depth);
    + 		if (reference->nr) {
    + 			struct string_list_item *item;
      
    - 		strvec_init(&args);
    - 		strvec_pushl(&args, "name-rev",
    -@@ builtin/describe.c: int cmd_describe(int argc, const char **argv, const char *prefix)
    - 			strvec_pushv(&args, argv);
    - 		else
    - 			strvec_push(&args, "HEAD");
    --		return cmd_name_rev(args.nr, args.v, prefix);
    -+
    -+		/*
    -+		 * `cmd_name_rev()` modifies the array, so we'd link its
    -+		 * contained strings if we didn't do a copy here.
    -+		 */
    -+		ALLOC_ARRAY(argv_copy, args.nr + 1);
    -+		for (size_t i = 0; i < args.nr; i++)
    -+			argv_copy[i] = args.v[i];
    -+		argv_copy[args.nr] = NULL;
    -+
    -+		ret = cmd_name_rev(args.nr, argv_copy, prefix);
    -+
    -+		strvec_clear(&args);
    -+		free(argv_copy);
    -+		return ret;
    - 	}
    +@@ builtin/submodule--helper.c: static int module_clone(int argc, const char **argv, const char *prefix)
    + 			   N_("reference repository")),
    + 		OPT_BOOL(0, "dissociate", &dissociate,
    + 			   N_("use --reference only while cloning")),
    +-		OPT_STRING(0, "depth", &clone_data.depth,
    +-			   N_("string"),
    ++		OPT_INTEGER(0, "depth", &clone_data.depth,
    + 			   N_("depth for shallow clones")),
    + 		OPT__QUIET(&quiet, "suppress output for cloning a submodule"),
    + 		OPT_BOOL(0, "progress", &progress,
    +@@ builtin/submodule--helper.c: static int add_submodule(const struct add_data *add_data)
    + 		}
    + 		clone_data.dissociate = add_data->dissociate;
    + 		if (add_data->depth >= 0)
    +-			clone_data.depth = xstrfmt("%d", add_data->depth);
    ++			clone_data.depth = add_data->depth;
      
    - 	hashmap_init(&names, commit_name_neq, NULL, 0);
    + 		if (clone_submodule(&clone_data, &reference))
    + 			goto cleanup;
 7:  5220c91bda !  8:  49c156cebb builtin/submodule--helper: fix various trivial memory leaks
    @@ Metadata
     Author: Patrick Steinhardt <ps@pks.im>
     
      ## Commit message ##
    -    builtin/submodule--helper: fix various trivial memory leaks
    +    builtin/submodule--helper: fix leaking buffer in `is_tip_reachable`
     
    -    There are multiple trivial memory leaks in the submodule helper. Fix
    -    those.
    +    The `rev` buffer in `is_tip_reachable()` is being populated with the
    +    output of git-rev-list(1) -- if either the command fails or the buffer
    +    contains any data, then the input commit is not reachable.
    +
    +    The buffer isn't used for anything else, but neither do we free it,
    +    causing a memory leak. Fix this.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ builtin/submodule--helper.c: static int is_tip_reachable(const char *path, const
      }
      
      static int fetch_in_submodule(const char *module_path, int depth, int quiet,
    -@@ builtin/submodule--helper.c: static void append_fetch_remotes(struct strbuf *msg, const char *git_dir_path)
    - static int add_submodule(const struct add_data *add_data)
    - {
    - 	char *submod_gitdir_path;
    -+	char *depth_to_free = NULL;
    - 	struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;
    - 	struct string_list reference = STRING_LIST_INIT_NODUP;
    - 	int ret = -1;
    -@@ builtin/submodule--helper.c: static int add_submodule(const struct add_data *add_data)
    - 		}
    - 		clone_data.dissociate = add_data->dissociate;
    - 		if (add_data->depth >= 0)
    --			clone_data.depth = xstrfmt("%d", add_data->depth);
    -+			clone_data.depth = depth_to_free = xstrfmt("%d", add_data->depth);
    - 
    - 		if (clone_submodule(&clone_data, &reference))
    - 			goto cleanup;
     @@ builtin/submodule--helper.c: static int add_submodule(const struct add_data *add_data)
      			die(_("unable to checkout submodule '%s'"), add_data->sm_path);
      	}
      	ret = 0;
     +
      cleanup:
    -+	free(depth_to_free);
      	string_list_clear(&reference, 1);
      	return ret;
    - }
     
      ## t/t7400-submodule-basic.sh ##
     @@ t/t7400-submodule-basic.sh: subcommands of git submodule.
 8:  d42152654b !  9:  4330c80905 builtin/ls-remote: fix leaking `pattern` strings
    @@ Commit message
         and prefix them with "*/", but never free either the array nor the
         allocated strings.
     
    -    Fix those leaks.
    +    Refactor the code to use a `struct strvec` instead of manually tracking
    +    the strings in an array. Like this, we can easily use `strvec_clear()`
    +    to release both the vector and the contained string for us, plugging the
    +    leak.
     
    +    Helped-by: Taylor Blau <me@ttaylorr.com>
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## builtin/ls-remote.c ##
    +@@ builtin/ls-remote.c: static const char * const ls_remote_usage[] = {
    +  * Is there one among the list of patterns that match the tail part
    +  * of the path?
    +  */
    +-static int tail_match(const char **pattern, const char *path)
    ++static int tail_match(const struct strvec *pattern, const char *path)
    + {
    +-	const char *p;
    + 	char *pathbuf;
    + 
    +-	if (!pattern)
    ++	if (!pattern->nr)
    + 		return 1; /* no restriction */
    + 
    + 	pathbuf = xstrfmt("/%s", path);
    +-	while ((p = *(pattern++)) != NULL) {
    +-		if (!wildmatch(p, pathbuf, 0)) {
    ++	for (size_t i = 0; i < pattern->nr; i++) {
    ++		if (!wildmatch(pattern->v[i], pathbuf, 0)) {
    + 			free(pathbuf);
    + 			return 1;
    + 		}
     @@ builtin/ls-remote.c: int cmd_ls_remote(int argc, const char **argv, const char *prefix)
      	int status = 0;
      	int show_symref_target = 0;
      	const char *uploadpack = NULL;
     -	const char **pattern = NULL;
    -+	char **pattern = NULL;
    ++	struct strvec pattern = STRVEC_INIT;
      	struct transport_ls_refs_options transport_options =
      		TRANSPORT_LS_REFS_OPTIONS_INIT;
      	int i;
     @@ builtin/ls-remote.c: int cmd_ls_remote(int argc, const char **argv, const char *prefix)
    - 	if (argc > 1) {
    - 		int i;
    - 		CALLOC_ARRAY(pattern, argc);
    + 
    + 	packet_trace_identity("ls-remote");
    + 
    +-	if (argc > 1) {
    +-		int i;
    +-		CALLOC_ARRAY(pattern, argc);
     -		for (i = 1; i < argc; i++) {
    -+		for (i = 1; i < argc; i++)
    - 			pattern[i - 1] = xstrfmt("*/%s", argv[i]);
    +-			pattern[i - 1] = xstrfmt("*/%s", argv[i]);
     -		}
    - 	}
    +-	}
    ++	for (int i = 1; i < argc; i++)
    ++		strvec_pushf(&pattern, "*/%s", argv[i]);
      
      	if (flags & REF_TAGS)
    + 		strvec_push(&transport_options.ref_prefixes, "refs/tags/");
     @@ builtin/ls-remote.c: int cmd_ls_remote(int argc, const char **argv, const char *prefix)
      		struct ref_array_item *item;
      		if (!check_ref_type(ref, flags))
      			continue;
     -		if (!tail_match(pattern, ref->name))
    -+		if (!tail_match((const char **) pattern, ref->name))
    ++		if (!tail_match(&pattern, ref->name))
      			continue;
      		item = ref_array_push(&ref_array, ref->name, &ref->old_oid);
      		item->symref = xstrdup_or_null(ref->symref);
    @@ builtin/ls-remote.c: int cmd_ls_remote(int argc, const char **argv, const char *
      		status = 1;
      	transport_ls_refs_options_release(&transport_options);
     +
    -+	for (i = 1; i < argc; i++)
    -+		free(pattern[i - 1]);
    -+	free(pattern);
    ++	strvec_clear(&pattern);
      	return status;
      }
     
 9:  6952fb2ff2 = 10:  2e3f76b428 builtin/remote: fix leaking strings in `branch_list`
10:  8bedcfdad8 = 11:  384203c34b builtin/remote: fix various trivial memory leaks
11:  a4b3ca49c9 = 12:  b7c0671797 builtin/stash: fix various trivial memory leaks
12:  e277222bd2 ! 13:  034c416d46 builtin/rev-parse: fix memory leak with `--parseopt`
    @@ Commit message
         which is of course wrong.
     
         Fix this by freeing all option's help fields as well as their `argh`
    -    fielids to plug this memory leak.
    +    fields to plug this memory leak.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
13:  fd857efa9f = 14:  20a40e2fd4 builtin/show-branch: fix several memory leaks
14:  ba4a883ae1 = 15:  b553f1ffb9 builtin/credential-store: fix leaking credential
15:  6d49645c0f = 16:  76d81c8ae1 builtin/rerere: fix various trivial memory leaks
16:  778e87221a = 17:  d52ac1a550 builtin/shortlog: fix various trivial memory leaks
17:  8a649bf7ed = 18:  dccaf695a9 builtin/worktree: fix leaking derived branch names
18:  2c7a369490 = 19:  788f0bee7b builtin/credential-cache: fix trivial leaks
19:  7564911d2a = 20:  eb58826b14 t/test-repository: fix leaking repository
20:  e3dbf9116b = 21:  fa95a67ccd object-name: fix leaking commit list items
21:  e426fd7ec5 = 22:  8cae4c44f4 entry: fix leaking pathnames during delayed checkout
22:  8c2a19994f = 23:  5d2bf0f0f3 convert: fix leaking config strings
23:  60aef16aef = 24:  6db1829c79 commit-reach: fix trivial memory leak when computing reachability

base-commit: d19b6cd2dd72dc811f19df4b32c7ed223256c3ee
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 01/24] builtin/replay: plug leaking `advance_name` variable

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4353 bytes --]

The `advance_name` variable can either contain a static string when
parsed via the `--advance` command line option or it may be an allocated
string when set via `determine_replay_mode()`. Because we cannot be sure
whether it is allocated or not we just didn't free it at all, resulting
in a memory leak.

Split up the variables such that we can track the static and allocated
strings separately and then free the allocated one to fix the memory
leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/replay.c         | 20 ++++++++++++++------
 t/t3650-replay-basics.sh |  1 +
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/builtin/replay.c b/builtin/replay.c
index 0448326636..27b40eda98 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -151,7 +151,7 @@ static void get_ref_information(struct rev_cmdline_info *cmd_info,
 
 static void determine_replay_mode(struct rev_cmdline_info *cmd_info,
 				  const char *onto_name,
-				  const char **advance_name,
+				  char **advance_name,
 				  struct commit **onto,
 				  struct strset **update_refs)
 {
@@ -174,6 +174,7 @@ static void determine_replay_mode(struct rev_cmdline_info *cmd_info,
 		*onto = peel_committish(*advance_name);
 		if (repo_dwim_ref(the_repository, *advance_name, strlen(*advance_name),
 			     &oid, &fullname, 0) == 1) {
+			free(*advance_name);
 			*advance_name = fullname;
 		} else {
 			die(_("argument to --advance must be a reference"));
@@ -197,6 +198,7 @@ static void determine_replay_mode(struct rev_cmdline_info *cmd_info,
 		if (negative_refs_complete) {
 			struct hashmap_iter iter;
 			struct strmap_entry *entry;
+			const char *last_key = NULL;
 
 			if (rinfo.negative_refexprs == 0)
 				die(_("all positive revisions given must be references"));
@@ -208,8 +210,11 @@ static void determine_replay_mode(struct rev_cmdline_info *cmd_info,
 			/* Only one entry, but we have to loop to get it */
 			strset_for_each_entry(&rinfo.negative_refs,
 					      &iter, entry) {
-				*advance_name = entry->key;
+				last_key = entry->key;
 			}
+
+			free(*advance_name);
+			*advance_name = xstrdup_or_null(last_key);
 		} else { /* positive_refs_complete */
 			if (rinfo.negative_refexprs > 1)
 				die(_("cannot implicitly determine correct base for --onto"));
@@ -271,7 +276,8 @@ static struct commit *pick_regular_commit(struct commit *pickme,
 
 int cmd_replay(int argc, const char **argv, const char *prefix)
 {
-	const char *advance_name = NULL;
+	const char *advance_name_opt = NULL;
+	char *advance_name = NULL;
 	struct commit *onto = NULL;
 	const char *onto_name = NULL;
 	int contained = 0;
@@ -292,7 +298,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
 		NULL
 	};
 	struct option replay_options[] = {
-		OPT_STRING(0, "advance", &advance_name,
+		OPT_STRING(0, "advance", &advance_name_opt,
 			   N_("branch"),
 			   N_("make replay advance given branch")),
 		OPT_STRING(0, "onto", &onto_name,
@@ -306,14 +312,15 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, replay_options, replay_usage,
 			     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN_OPT);
 
-	if (!onto_name && !advance_name) {
+	if (!onto_name && !advance_name_opt) {
 		error(_("option --onto or --advance is mandatory"));
 		usage_with_options(replay_usage, replay_options);
 	}
 
-	if (advance_name && contained)
+	if (advance_name_opt && contained)
 		die(_("options '%s' and '%s' cannot be used together"),
 		    "--advance", "--contained");
+	advance_name = xstrdup_or_null(advance_name_opt);
 
 	repo_init_revisions(the_repository, &revs, prefix);
 
@@ -441,6 +448,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
 
 cleanup:
 	release_revisions(&revs);
+	free(advance_name);
 
 	/* Return */
 	if (ret < 0)
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index 389670262e..12bd3db4cb 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -5,6 +5,7 @@ test_description='basic git replay tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 GIT_AUTHOR_NAME=author@name
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 02/24] builtin/log: fix leaking branch name when creating cover letters

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1855 bytes --]

When calling `make_cover_letter()` without a branch name, we try to
derive the branch name by calling `find_branch_name()`. But while this
function returns an allocated string, we never free the result and thus
have a memory leak. Fix this.

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

diff --git a/builtin/log.c b/builtin/log.c
index 4d4b60caa7..a73a767606 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1434,6 +1434,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
 	int need_8bit_cte = 0;
 	struct pretty_print_context pp = {0};
 	struct commit *head = list[0];
+	char *to_free = NULL;
 
 	if (!cmit_fmt_is_mail(rev->commit_format))
 		die(_("cover letter needs email format"));
@@ -1455,7 +1456,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
 	}
 
 	if (!branch_name)
-		branch_name = find_branch_name(rev);
+		branch_name = to_free = find_branch_name(rev);
 
 	pp.fmt = CMIT_FMT_EMAIL;
 	pp.date_mode.type = DATE_RFC2822;
@@ -1466,6 +1467,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
 			   encoding, need_8bit_cte, cfg);
 	fprintf(rev->diffopt.file, "%s\n", sb.buf);
 
+	free(to_free);
 	free(pp.after_subject);
 	strbuf_release(&sb);
 
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index a767c3520e..973e20254b 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -5,6 +5,7 @@ test_description='range-diff tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Note that because of the range-diff's heuristics, test_commit does more
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 03/24] builtin/describe: fix memory leak with `--contains=`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1971 bytes --]

When calling `git describe --contains=`, we end up invoking
`cmd_name_rev()` with some munged argv array. This array may contain
allocated strings and furthermore will likely be modified by the called
function. This results in two memory leaks:

  - First, we leak the array that we use to assemble the arguments.

  - Second, we leak the allocated strings that we may have put into the
    array.

Fix those leaks by creating a separate copy of the array that we can
hand over to `cmd_name_rev()`. This allows us to free all strings
contained in the `strvec`, as the original vector will not be modified
anymore.

Furthermore, free both the `strvec` and the copied array to fix the
first memory leak.

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

diff --git a/builtin/describe.c b/builtin/describe.c
index cf8edc4222..739878db85 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -619,6 +619,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 	if (contains) {
 		struct string_list_item *item;
 		struct strvec args;
+		const char **argv_copy;
+		int ret;
 
 		strvec_init(&args);
 		strvec_pushl(&args, "name-rev",
@@ -637,7 +639,21 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 			strvec_pushv(&args, argv);
 		else
 			strvec_push(&args, "HEAD");
-		return cmd_name_rev(args.nr, args.v, prefix);
+
+		/*
+		 * `cmd_name_rev()` modifies the array, so we'd leak its
+		 * contained strings if we didn't do a copy here.
+		 */
+		ALLOC_ARRAY(argv_copy, args.nr + 1);
+		for (size_t i = 0; i < args.nr; i++)
+			argv_copy[i] = args.v[i];
+		argv_copy[args.nr] = NULL;
+
+		ret = cmd_name_rev(args.nr, argv_copy, prefix);
+
+		strvec_clear(&args);
+		free(argv_copy);
+		return ret;
 	}
 
 	hashmap_init(&names, commit_name_neq, NULL, 0);
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 04/24] builtin/describe: fix leaking array when running diff-index

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1681 bytes --]

When running git-describe(1) with `--dirty`, we will set up a `struct
rev_info` with arguments for git-diff-index(1). The way we assemble the
arguments it causes two memory leaks though:

  - We never release the `struct strvec`.

  - `setup_revisions()` may end up removing some entries from the
    `strvec`, which we wouldn't free even if we released the struct.

While we could plug those leaks, this is ultimately unnecessary as the
arguments we pass are part of a static array anyway. So instead,
refactor the code to drop the `struct strvec` and just pass this static
array directly.

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

diff --git a/builtin/describe.c b/builtin/describe.c
index 739878db85..2957ff7031 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -695,7 +695,6 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 		} else if (dirty) {
 			struct lock_file index_lock = LOCK_INIT;
 			struct rev_info revs;
-			struct strvec args = STRVEC_INIT;
 			int fd;
 
 			setup_work_tree();
@@ -710,8 +709,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 				repo_update_index_if_able(the_repository, &index_lock);
 
 			repo_init_revisions(the_repository, &revs, prefix);
-			strvec_pushv(&args, diff_index_args);
-			if (setup_revisions(args.nr, args.v, &revs, NULL) != 1)
+
+			if (setup_revisions(ARRAY_SIZE(diff_index_args) - 1,
+					    diff_index_args, &revs, NULL) != 1)
 				BUG("malformed internal diff-index command line");
 			run_diff_index(&revs, 0);
 
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 05/24] builtin/describe: fix trivial memory leak when describing blob

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

We never free the `struct strvec args` variable in `describe_blob()`,
which thus causes a memory leak. Fix this.

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

diff --git a/builtin/describe.c b/builtin/describe.c
index 2957ff7031..954929c514 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -529,6 +529,7 @@ static void describe_blob(struct object_id oid, struct strbuf *dst)
 	traverse_commit_list(&revs, process_commit, process_object, &pcd);
 	reset_revision_walk();
 	release_revisions(&revs);
+	strvec_clear(&args);
 }
 
 static void describe(const char *arg, int last_one)
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index d667dda654..95e9955bca 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -8,6 +8,7 @@ test_description='test git-specific bash prompt functions'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-bash.sh
 
 . "$GIT_BUILD_DIR/contrib/completion/git-prompt.sh"
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 06/24] builtin/name-rev: fix various trivial memory leaks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1701 bytes --]

There are several structures that we don't release after
`cmd_name_rev()` is done. Plug those leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/name-rev.c                   | 6 ++++--
 t/t6007-rev-list-cherry-pick-file.sh | 1 +
 t/t6120-describe.sh                  | 1 +
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 70e9ec4e47..f62c0a36cb 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -677,7 +677,9 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 				  always, allow_undefined, data.name_only);
 	}
 
-	UNLEAK(string_pool);
-	UNLEAK(revs);
+	string_list_clear(&data.ref_filters, 0);
+	string_list_clear(&data.exclude_filters, 0);
+	mem_pool_discard(&string_pool, 0);
+	object_array_clear(&revs);
 	return 0;
 }
diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index 6f3e543977..2d337d7287 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -5,6 +5,7 @@ test_description='test git rev-list --cherry-pick -- file'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # A---B---D---F
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 79e0f19deb..05ed2510d9 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -14,6 +14,7 @@ test_description='test describe'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 check_describe () {
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 08/24] builtin/submodule--helper: fix leaking buffer in `is_tip_reachable`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2044 bytes --]

The `rev` buffer in `is_tip_reachable()` is being populated with the
output of git-rev-list(1) -- if either the command fails or the buffer
contains any data, then the input commit is not reachable.

The buffer isn't used for anything else, but neither do we free it,
causing a memory leak. Fix this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/submodule--helper.c | 9 +++++++--
 t/t7400-submodule-basic.sh  | 1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 863b01627c..673810d2c0 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2268,6 +2268,7 @@ static int is_tip_reachable(const char *path, const struct object_id *oid)
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf rev = STRBUF_INIT;
 	char *hex = oid_to_hex(oid);
+	int reachable;
 
 	cp.git_cmd = 1;
 	cp.dir = path;
@@ -2277,9 +2278,12 @@ static int is_tip_reachable(const char *path, const struct object_id *oid)
 	prepare_submodule_repo_env(&cp.env);
 
 	if (capture_command(&cp, &rev, GIT_MAX_HEXSZ + 1) || rev.len)
-		return 0;
+		reachable = 0;
+	else
+		reachable = 1;
 
-	return 1;
+	strbuf_release(&rev);
+	return reachable;
 }
 
 static int fetch_in_submodule(const char *module_path, int depth, int quiet,
@@ -3222,6 +3226,7 @@ static int add_submodule(const struct add_data *add_data)
 			die(_("unable to checkout submodule '%s'"), add_data->sm_path);
 	}
 	ret = 0;
+
 cleanup:
 	string_list_clear(&reference, 1);
 	return ret;
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 981488885f..098d8833b6 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -12,6 +12,7 @@ subcommands of git submodule.
 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 - enable local submodules' '
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 09/24] builtin/ls-remote: fix leaking `pattern` strings

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3354 bytes --]

Users can pass patterns to git-ls-remote(1), which allows them to filter
the list of printed references. We assemble those patterns into an array
and prefix them with "*/", but never free either the array nor the
allocated strings.

Refactor the code to use a `struct strvec` instead of manually tracking
the strings in an array. Like this, we can easily use `strvec_clear()`
to release both the vector and the contained string for us, plugging the
leak.

Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/ls-remote.c          | 24 ++++++++++--------------
 t/t5535-fetch-push-symref.sh |  1 +
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index debf2d4f88..5b61af5d78 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -19,17 +19,16 @@ static const char * const ls_remote_usage[] = {
  * Is there one among the list of patterns that match the tail part
  * of the path?
  */
-static int tail_match(const char **pattern, const char *path)
+static int tail_match(const struct strvec *pattern, const char *path)
 {
-	const char *p;
 	char *pathbuf;
 
-	if (!pattern)
+	if (!pattern->nr)
 		return 1; /* no restriction */
 
 	pathbuf = xstrfmt("/%s", path);
-	while ((p = *(pattern++)) != NULL) {
-		if (!wildmatch(p, pathbuf, 0)) {
+	for (size_t i = 0; i < pattern->nr; i++) {
+		if (!wildmatch(pattern->v[i], pathbuf, 0)) {
 			free(pathbuf);
 			return 1;
 		}
@@ -47,7 +46,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	int status = 0;
 	int show_symref_target = 0;
 	const char *uploadpack = NULL;
-	const char **pattern = NULL;
+	struct strvec pattern = STRVEC_INIT;
 	struct transport_ls_refs_options transport_options =
 		TRANSPORT_LS_REFS_OPTIONS_INIT;
 	int i;
@@ -93,13 +92,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 
 	packet_trace_identity("ls-remote");
 
-	if (argc > 1) {
-		int i;
-		CALLOC_ARRAY(pattern, argc);
-		for (i = 1; i < argc; i++) {
-			pattern[i - 1] = xstrfmt("*/%s", argv[i]);
-		}
-	}
+	for (int i = 1; i < argc; i++)
+		strvec_pushf(&pattern, "*/%s", argv[i]);
 
 	if (flags & REF_TAGS)
 		strvec_push(&transport_options.ref_prefixes, "refs/tags/");
@@ -136,7 +130,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		struct ref_array_item *item;
 		if (!check_ref_type(ref, flags))
 			continue;
-		if (!tail_match(pattern, ref->name))
+		if (!tail_match(&pattern, ref->name))
 			continue;
 		item = ref_array_push(&ref_array, ref->name, &ref->old_oid);
 		item->symref = xstrdup_or_null(ref->symref);
@@ -158,5 +152,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	if (transport_disconnect(transport))
 		status = 1;
 	transport_ls_refs_options_release(&transport_options);
+
+	strvec_clear(&pattern);
 	return status;
 }
diff --git a/t/t5535-fetch-push-symref.sh b/t/t5535-fetch-push-symref.sh
index e8f6d233ff..7122af7fdb 100755
--- a/t/t5535-fetch-push-symref.sh
+++ b/t/t5535-fetch-push-symref.sh
@@ -2,6 +2,7 @@
 
 test_description='avoiding conflicting update through symref aliasing'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 10/24] builtin/remote: fix leaking strings in `branch_list`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1390 bytes --]

The `struct string_list branch_list` is declared as `NODUP`, which makes
it not copy strings inserted into it. This causes memory leaks though,
as this means it also won't be responsible for _freeing_ inserted
strings. Thus, every branch we add to this will leak.

Fix this by marking the list as `DUP` instead and free the local copy we
have of the variable.

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

diff --git a/builtin/remote.c b/builtin/remote.c
index 08292498bd..303da7f73f 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -258,7 +258,7 @@ struct branch_info {
 	char *push_remote_name;
 };
 
-static struct string_list branch_list = STRING_LIST_INIT_NODUP;
+static struct string_list branch_list = STRING_LIST_INIT_DUP;
 
 static const char *abbrev_ref(const char *name, const char *prefix)
 {
@@ -292,8 +292,8 @@ static int config_read_branches(const char *key, const char *value,
 		type = PUSH_REMOTE;
 	else
 		return 0;
-	name = xmemdupz(key, key_len);
 
+	name = xmemdupz(key, key_len);
 	item = string_list_insert(&branch_list, name);
 
 	if (!item->util)
@@ -337,6 +337,7 @@ static int config_read_branches(const char *key, const char *value,
 		BUG("unexpected type=%d", type);
 	}
 
+	free(name);
 	return 0;
 }
 
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 11/24] builtin/remote: fix various trivial memory leaks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 6095 bytes --]

There are multiple trivial memory leaks in git-remote(1). Fix those.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/remote.c          | 39 ++++++++++++++++++++++++++++++++-------
 t/t5512-ls-remote.sh      |  1 +
 t/t5514-fetch-multiple.sh |  1 +
 t/t5520-pull.sh           |  1 +
 t/t5528-push-default.sh   |  1 +
 t/t5543-atomic-push.sh    |  1 +
 t/t5570-git-daemon.sh     |  1 +
 7 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 303da7f73f..9d54fddf8c 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -555,13 +555,16 @@ static int add_branch_for_removal(const char *refname,
 	refspec.dst = (char *)refname;
 	if (remote_find_tracking(branches->remote, &refspec))
 		return 0;
+	free(refspec.src);
 
 	/* don't delete a branch if another remote also uses it */
 	for (kr = branches->keep->list; kr; kr = kr->next) {
 		memset(&refspec, 0, sizeof(refspec));
 		refspec.dst = (char *)refname;
-		if (!remote_find_tracking(kr->remote, &refspec))
+		if (!remote_find_tracking(kr->remote, &refspec)) {
+			free(refspec.src);
 			return 0;
+		}
 	}
 
 	/* don't delete non-remote-tracking refs */
@@ -668,7 +671,11 @@ static int config_read_push_default(const char *key, const char *value,
 static void handle_push_default(const char* old_name, const char* new_name)
 {
 	struct push_default_info push_default = {
-		old_name, CONFIG_SCOPE_UNKNOWN, STRBUF_INIT, -1 };
+		.old_name = old_name,
+		.scope = CONFIG_SCOPE_UNKNOWN,
+		.origin = STRBUF_INIT,
+		.linenr = -1,
+	};
 	git_config(config_read_push_default, &push_default);
 	if (push_default.scope >= CONFIG_SCOPE_COMMAND)
 		; /* pass */
@@ -688,6 +695,8 @@ static void handle_push_default(const char* old_name, const char* new_name)
 			push_default.origin.buf, push_default.linenr,
 			old_name);
 	}
+
+	strbuf_release(&push_default.origin);
 }
 
 
@@ -785,7 +794,7 @@ static int mv(int argc, const char **argv, const char *prefix)
 	}
 
 	if (!refspec_updated)
-		return 0;
+		goto out;
 
 	/*
 	 * First remove symrefs, then rename the rest, finally create
@@ -851,10 +860,15 @@ static int mv(int argc, const char **argv, const char *prefix)
 		display_progress(progress, ++refs_renamed_nr);
 	}
 	stop_progress(&progress);
-	string_list_clear(&remote_branches, 1);
 
 	handle_push_default(rename.old_name, rename.new_name);
 
+out:
+	string_list_clear(&remote_branches, 1);
+	strbuf_release(&old_remote_context);
+	strbuf_release(&buf);
+	strbuf_release(&buf2);
+	strbuf_release(&buf3);
 	return 0;
 }
 
@@ -945,12 +959,21 @@ static int rm(int argc, const char **argv, const char *prefix)
 
 	if (!result) {
 		strbuf_addf(&buf, "remote.%s", remote->name);
-		if (git_config_rename_section(buf.buf, NULL) < 1)
-			return error(_("Could not remove config section '%s'"), buf.buf);
+		if (git_config_rename_section(buf.buf, NULL) < 1) {
+			result = error(_("Could not remove config section '%s'"), buf.buf);
+			goto out;
+		}
 
 		handle_push_default(remote->name, NULL);
 	}
 
+out:
+	for (struct known_remote *r = known_remotes.list; r;) {
+		struct known_remote *next = r->next;
+		free(r);
+		r = next;
+	}
+	strbuf_release(&buf);
 	return result;
 }
 
@@ -983,8 +1006,10 @@ static int append_ref_to_tracked_list(const char *refname,
 
 	memset(&refspec, 0, sizeof(refspec));
 	refspec.dst = (char *)refname;
-	if (!remote_find_tracking(states->remote, &refspec))
+	if (!remote_find_tracking(states->remote, &refspec)) {
 		string_list_append(&states->tracked, abbrev_branch(refspec.src));
+		free(refspec.src);
+	}
 
 	return 0;
 }
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 42e77eb5a9..d687d824d1 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -5,6 +5,7 @@ test_description='git ls-remote'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 generate_references () {
diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
index 25772c85c5..579872c258 100755
--- a/t/t5514-fetch-multiple.sh
+++ b/t/t5514-fetch-multiple.sh
@@ -5,6 +5,7 @@ test_description='fetch --all works correctly'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 setup_repository () {
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 47534f1062..1098cbd0a1 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -5,6 +5,7 @@ test_description='pulling into void'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 modify () {
diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
index 14f7eced9a..bc2bada34c 100755
--- a/t/t5528-push-default.sh
+++ b/t/t5528-push-default.sh
@@ -4,6 +4,7 @@ test_description='check various push.default settings'
 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 bare remotes' '
diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
index 04b47ad84a..479d103469 100755
--- a/t/t5543-atomic-push.sh
+++ b/t/t5543-atomic-push.sh
@@ -5,6 +5,7 @@ test_description='pushing to a repository using the atomic push option'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 mk_repo_pair () {
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index f9a9bf9503..c5f08b6799 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -4,6 +4,7 @@ test_description='test fetching over git protocol'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-git-daemon.sh
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 12/24] builtin/stash: fix various trivial memory leaks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4177 bytes --]

There are multiple trivial memory leaks in git-stash(1). Fix those.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/stash.c                    | 18 ++++++++++++++++--
 t/t2501-cwd-empty.sh               |  1 +
 t/t3903-stash.sh                   |  1 +
 t/t3904-stash-patch.sh             |  2 ++
 t/t3905-stash-include-untracked.sh |  1 +
 t/t7064-wtstatus-pv2.sh            |  1 +
 6 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 46b981c4dd..7f2f989b69 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1521,6 +1521,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 	struct strbuf patch = STRBUF_INIT;
 	struct strbuf stash_msg_buf = STRBUF_INIT;
 	struct strbuf untracked_files = STRBUF_INIT;
+	struct strbuf out = STRBUF_INIT;
 
 	if (patch_mode && keep_index == -1)
 		keep_index = 1;
@@ -1626,7 +1627,6 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 			struct child_process cp_add = CHILD_PROCESS_INIT;
 			struct child_process cp_diff = CHILD_PROCESS_INIT;
 			struct child_process cp_apply = CHILD_PROCESS_INIT;
-			struct strbuf out = STRBUF_INIT;
 
 			cp_add.git_cmd = 1;
 			strvec_push(&cp_add.args, "add");
@@ -1718,6 +1718,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 
 done:
 	strbuf_release(&patch);
+	strbuf_release(&out);
 	free_stash_info(&info);
 	strbuf_release(&stash_msg_buf);
 	strbuf_release(&untracked_files);
@@ -1869,6 +1870,8 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
 		OPT_SUBCOMMAND_F("save", &fn, save_stash, PARSE_OPT_NOCOMPLETE),
 		OPT_END()
 	};
+	const char **args_copy;
+	int ret;
 
 	git_config(git_stash_config, NULL);
 
@@ -1892,5 +1895,16 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
 	/* Assume 'stash push' */
 	strvec_push(&args, "push");
 	strvec_pushv(&args, argv);
-	return !!push_stash(args.nr, args.v, prefix, 1);
+
+	/*
+	 * `push_stash()` ends up modifying the array, which causes memory
+	 * leaks if we didn't copy the array here.
+	 */
+	DUP_ARRAY(args_copy, args.v, args.nr);
+
+	ret = !!push_stash(args.nr, args_copy, prefix, 1);
+
+	strvec_clear(&args);
+	free(args_copy);
+	return ret;
 }
diff --git a/t/t2501-cwd-empty.sh b/t/t2501-cwd-empty.sh
index f6d8d7d03d..8af4e8cfe3 100755
--- a/t/t2501-cwd-empty.sh
+++ b/t/t2501-cwd-empty.sh
@@ -2,6 +2,7 @@
 
 test_description='Test handling of the current working directory becoming empty'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index a7f71f8126..e4c0937f61 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -8,6 +8,7 @@ test_description='Test git stash'
 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-unique-files.sh
 
diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
index 368fc2a6cc..aa5019fd6c 100755
--- a/t/t3904-stash-patch.sh
+++ b/t/t3904-stash-patch.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='stash -p'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-patch-mode.sh
 
 test_expect_success 'setup' '
diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
index 1289ae3e07..a1733f45c3 100755
--- a/t/t3905-stash-include-untracked.sh
+++ b/t/t3905-stash-include-untracked.sh
@@ -5,6 +5,7 @@
 
 test_description='Test git stash --include-untracked'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'stash save --include-untracked some dirty working directory' '
diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
index 11884d2fc3..06c1301222 100755
--- a/t/t7064-wtstatus-pv2.sh
+++ b/t/t7064-wtstatus-pv2.sh
@@ -4,6 +4,7 @@ test_description='git status --porcelain=v2
 
 This test exercises porcelain V2 output for git status.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 13/24] builtin/rev-parse: fix memory leak with `--parseopt`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1601 bytes --]

The `--parseopt` mode allows shell scripts to have the same option
parsing mode as we have in C builtins. It soaks up a set of option
descriptions via stdin and massages them into proper `struct option`s
that we can then use to parse a set of arguments.

We only partially free those options when done though, creating a memory
leak. Interestingly, we only end up free'ing the first option's help,
which is of course wrong.

Fix this by freeing all option's help fields as well as their `argh`
fields to plug this memory leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/rev-parse.c           | 5 ++++-
 t/t1502-rev-parse-parseopt.sh | 2 ++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 2e64f5bda7..5845d3f59b 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -553,7 +553,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 	strbuf_release(&sb);
 	strvec_clear(&longnames);
 	strvec_clear(&usage);
-	free((char *) opts->help);
+	for (size_t i = 0; i < opts_nr; i++) {
+		free((char *) opts[i].help);
+		free((char *) opts[i].argh);
+	}
 	free(opts);
 	return 0;
 }
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index b754b9fd74..5eaa6428c4 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test git rev-parse --parseopt'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 check_invalid_long_option () {
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 14/24] builtin/show-branch: fix several memory leaks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5247 bytes --]

There are several memory leaks in git-show-branch(1). Fix them.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/show-branch.c  | 52 +++++++++++++++++++++++++++++-------------
 t/t3202-show-branch.sh |  1 +
 t/t6010-merge-base.sh  |  1 +
 3 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index d72f4cb98d..7d797a880c 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -502,14 +502,14 @@ static int rev_is_head(const char *head, const char *name)
 	return !strcmp(head, name);
 }
 
-static int show_merge_base(struct commit_list *seen, int num_rev)
+static int show_merge_base(const struct commit_list *seen, int num_rev)
 {
 	int all_mask = ((1u << (REV_SHIFT + num_rev)) - 1);
 	int all_revs = all_mask & ~((1u << REV_SHIFT) - 1);
 	int exit_status = 1;
 
-	while (seen) {
-		struct commit *commit = pop_commit(&seen);
+	for (const struct commit_list *s = seen; s; s = s->next) {
+		struct commit *commit = s->item;
 		int flags = commit->object.flags & all_mask;
 		if (!(flags & UNINTERESTING) &&
 		    ((flags & all_revs) == all_revs)) {
@@ -635,7 +635,7 @@ static int parse_reflog_param(const struct option *opt, const char *arg,
 int cmd_show_branch(int ac, const char **av, const char *prefix)
 {
 	struct commit *rev[MAX_REVS], *commit;
-	char *reflog_msg[MAX_REVS];
+	char *reflog_msg[MAX_REVS] = {0};
 	struct commit_list *list = NULL, *seen = NULL;
 	unsigned int rev_mask[MAX_REVS];
 	int num_rev, i, extra = 0;
@@ -692,6 +692,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			    parse_reflog_param),
 		OPT_END()
 	};
+	const char **args_copy = NULL;
+	int ret;
 
 	init_commit_name_slab(&name_slab);
 
@@ -699,8 +701,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 
 	/* If nothing is specified, try the default first */
 	if (ac == 1 && default_args.nr) {
+		DUP_ARRAY(args_copy, default_args.v, default_args.nr);
 		ac = default_args.nr;
-		av = default_args.v;
+		av = args_copy;
 	}
 
 	ac = parse_options(ac, av, prefix, builtin_show_branch_options,
@@ -780,7 +783,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 		}
 
 		for (i = 0; i < reflog; i++) {
-			char *logmsg;
+			char *logmsg = NULL;
 			char *nth_desc;
 			const char *msg;
 			char *end;
@@ -790,6 +793,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			if (read_ref_at(get_main_ref_store(the_repository),
 					ref, flags, 0, base + i, &oid, &logmsg,
 					&timestamp, &tz, NULL)) {
+				free(logmsg);
 				reflog = i;
 				break;
 			}
@@ -842,7 +846,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 
 	if (!ref_name_cnt) {
 		fprintf(stderr, "No revs to be shown.\n");
-		exit(0);
+		ret = 0;
+		goto out;
 	}
 
 	for (num_rev = 0; ref_name[num_rev]; num_rev++) {
@@ -879,11 +884,15 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 
 	commit_list_sort_by_date(&seen);
 
-	if (merge_base)
-		return show_merge_base(seen, num_rev);
+	if (merge_base) {
+		ret = show_merge_base(seen, num_rev);
+		goto out;
+	}
 
-	if (independent)
-		return show_independent(rev, num_rev, rev_mask);
+	if (independent) {
+		ret = show_independent(rev, num_rev, rev_mask);
+		goto out;
+	}
 
 	/* Show list; --more=-1 means list-only */
 	if (1 < num_rev || extra < 0) {
@@ -919,8 +928,10 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			putchar('\n');
 		}
 	}
-	if (extra < 0)
-		exit(0);
+	if (extra < 0) {
+		ret = 0;
+		goto out;
+	}
 
 	/* Sort topologically */
 	sort_in_topological_order(&seen, sort_order);
@@ -932,8 +943,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 	all_mask = ((1u << (REV_SHIFT + num_rev)) - 1);
 	all_revs = all_mask & ~((1u << REV_SHIFT) - 1);
 
-	while (seen) {
-		struct commit *commit = pop_commit(&seen);
+	for (struct commit_list *l = seen; l; l = l->next) {
+		struct commit *commit = l->item;
 		int this_flag = commit->object.flags;
 		int is_merge_point = ((this_flag & all_revs) == all_revs);
 
@@ -973,6 +984,15 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 		if (shown_merge_point && --extra < 0)
 			break;
 	}
+
+	ret = 0;
+
+out:
+	for (size_t i = 0; i < ARRAY_SIZE(reflog_msg); i++)
+		free(reflog_msg[i]);
+	free_commit_list(seen);
+	free_commit_list(list);
+	free(args_copy);
 	free(head);
-	return 0;
+	return ret;
 }
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index a1139f79e2..3b6dad0c46 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -2,6 +2,7 @@
 
 test_description='test show-branch'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'error descriptions on empty repository' '
diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh
index 44c726ea39..f96ea82e78 100755
--- a/t/t6010-merge-base.sh
+++ b/t/t6010-merge-base.sh
@@ -6,6 +6,7 @@
 test_description='Merge base and parent list computation.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 M=1130000000
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 15/24] builtin/credential-store: fix leaking credential

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]

We never free credentials read by the credential store, leading to a
memory leak. Plug it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/credential-store.c     | 1 +
 t/t0302-credential-store.sh    | 2 ++
 t/t0303-credential-external.sh | 1 +
 3 files changed, 4 insertions(+)

diff --git a/builtin/credential-store.c b/builtin/credential-store.c
index 494c809332..97968bfa1c 100644
--- a/builtin/credential-store.c
+++ b/builtin/credential-store.c
@@ -218,5 +218,6 @@ int cmd_credential_store(int argc, const char **argv, const char *prefix)
 		; /* Ignore unknown operation. */
 
 	string_list_clear(&fns, 0);
+	credential_clear(&c);
 	return 0;
 }
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index 716bf1af9f..f83db659e2 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='credential-store tests'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-credential.sh
 
diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
index 72ae405c3e..8aadbe86c4 100755
--- a/t/t0303-credential-external.sh
+++ b/t/t0303-credential-external.sh
@@ -29,6 +29,7 @@ you can set GIT_TEST_CREDENTIAL_HELPER_SETUP to a sequence of shell
 commands.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-credential.sh
 
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 16/24] builtin/rerere: fix various trivial memory leaks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2550 bytes --]

There are multiple trivial memory leaks in git-rerere(1). Fix those.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/rerere.c          | 8 +++++++-
 rerere.c                  | 9 +++++++--
 t/t2030-unresolve-info.sh | 1 +
 t/t4200-rerere.sh         | 1 +
 4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/builtin/rerere.c b/builtin/rerere.c
index b2efc6f640..81b65ffa39 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -73,11 +73,17 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
 
 	if (!strcmp(argv[0], "forget")) {
 		struct pathspec pathspec;
+		int ret;
+
 		if (argc < 2)
 			warning(_("'git rerere forget' without paths is deprecated"));
 		parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD,
 			       prefix, argv + 1);
-		return rerere_forget(the_repository, &pathspec);
+
+		ret = rerere_forget(the_repository, &pathspec);
+
+		clear_pathspec(&pathspec);
+		return ret;
 	}
 
 	if (!strcmp(argv[0], "clear")) {
diff --git a/rerere.c b/rerere.c
index 3a3888cce2..525ed6cc1e 100644
--- a/rerere.c
+++ b/rerere.c
@@ -1107,7 +1107,7 @@ static int rerere_forget_one_path(struct index_state *istate,
 
 int rerere_forget(struct repository *r, struct pathspec *pathspec)
 {
-	int i, fd;
+	int i, fd, ret;
 	struct string_list conflict = STRING_LIST_INIT_DUP;
 	struct string_list merge_rr = STRING_LIST_INIT_DUP;
 
@@ -1132,7 +1132,12 @@ int rerere_forget(struct repository *r, struct pathspec *pathspec)
 			continue;
 		rerere_forget_one_path(r->index, it->string, &merge_rr);
 	}
-	return write_rr(&merge_rr, fd);
+
+	ret = write_rr(&merge_rr, fd);
+
+	string_list_clear(&conflict, 0);
+	string_list_clear(&merge_rr, 1);
+	return ret;
 }
 
 /*
diff --git a/t/t2030-unresolve-info.sh b/t/t2030-unresolve-info.sh
index be3fcdde07..b3f6bc97b5 100755
--- a/t/t2030-unresolve-info.sh
+++ b/t/t2030-unresolve-info.sh
@@ -5,6 +5,7 @@ test_description='undoing resolution'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 check_resolve_undo () {
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index b0a3e84984..213b36fb96 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -25,6 +25,7 @@ test_description='git rerere
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 17/24] builtin/shortlog: fix various trivial memory leaks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 963 bytes --]

There is a trivial memory leak in git-shortlog(1). Fix it.

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

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 5bde7c68c2..b529608c92 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -514,4 +514,5 @@ void shortlog_output(struct shortlog *log)
 	string_list_clear(&log->list, 1);
 	clear_mailmap(&log->mailmap);
 	string_list_clear(&log->format, 0);
+	string_list_clear(&log->trailers, 0);
 }
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index f698d0c9ad..c20c885724 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -9,6 +9,7 @@ test_description='git shortlog
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 18/24] builtin/worktree: fix leaking derived branch names

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2715 bytes --]

There are several heuristics that git-worktree(1) uses to derive the
name of the newly created branch when not given explicitly. These
heuristics all allocate a new string, but we only end up freeing that
string in a subset of cases.

Fix the remaining cases where we didn't yet free the derived branch
names. While at it, also free `opt_track`, which is being populated via
an `OPT_PASSTHRU()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/worktree.c      | 7 ++++---
 t/t2400-worktree-add.sh | 1 +
 t/t9902-completion.sh   | 1 +
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 1d51e54fcd..cec3ada6b0 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -769,7 +769,7 @@ static int add(int ac, const char **av, const char *prefix)
 	char *branch_to_free = NULL;
 	char *new_branch_to_free = NULL;
 	const char *new_branch = NULL;
-	const char *opt_track = NULL;
+	char *opt_track = NULL;
 	const char *lock_reason = NULL;
 	int keep_locked = 0;
 	int used_new_branch_options;
@@ -846,7 +846,7 @@ static int add(int ac, const char **av, const char *prefix)
 	if (opts.orphan && !new_branch) {
 		int n;
 		const char *s = worktree_basename(path, &n);
-		new_branch = xstrndup(s, n);
+		new_branch = new_branch_to_free = xstrndup(s, n);
 	} else if (opts.orphan) {
 		; /* no-op */
 	} else if (opts.detach) {
@@ -875,7 +875,7 @@ static int add(int ac, const char **av, const char *prefix)
 			remote = unique_tracking_name(branch, &oid, NULL);
 			if (remote) {
 				new_branch = branch;
-				branch = remote;
+				branch = new_branch_to_free = remote;
 			}
 		}
 
@@ -923,6 +923,7 @@ static int add(int ac, const char **av, const char *prefix)
 
 	ret = add_worktree(path, branch, &opts);
 	free(path);
+	free(opt_track);
 	free(branch_to_free);
 	free(new_branch_to_free);
 	return ret;
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index ba320dc417..cfc4aeb179 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -6,6 +6,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 TEST_CREATE_REPO_NO_TEMPLATE=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-rebase.sh
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 932d5ad759..cc6aa9f0cd 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -16,6 +16,7 @@ test_untraceable=UnfortunatelyYes
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-bash.sh
 
 complete ()
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 19/24] builtin/credential-cache: fix trivial leaks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2596 bytes --]

There are two trivial leaks in git-credential-cache(1):

  - We leak the child process in `spawn_daemon()`. As we do not call
    `finish_command()` and instead let the created process daemonize, we
    have to clear the process manually.

  - We do not free the computed socket path in case it wasn't given via
    `--socket=`.

Plug both of these memory leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/credential-cache.c  | 9 +++++++--
 t/t0301-credential-cache.sh | 2 ++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c
index 3db8df70a9..aaf2f8438b 100644
--- a/builtin/credential-cache.c
+++ b/builtin/credential-cache.c
@@ -88,6 +88,8 @@ static void spawn_daemon(const char *socket)
 		die_errno("unable to read result code from cache daemon");
 	if (r != 3 || memcmp(buf, "ok\n", 3))
 		die("cache daemon did not start: %.*s", r, buf);
+
+	child_process_clear(&daemon);
 	close(daemon.out);
 }
 
@@ -137,7 +139,8 @@ static void announce_capabilities(void)
 
 int cmd_credential_cache(int argc, const char **argv, const char *prefix)
 {
-	char *socket_path = NULL;
+	const char *socket_path_arg = NULL;
+	char *socket_path;
 	int timeout = 900;
 	const char *op;
 	const char * const usage[] = {
@@ -147,7 +150,7 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_INTEGER(0, "timeout", &timeout,
 			    "number of seconds to cache credentials"),
-		OPT_STRING(0, "socket", &socket_path, "path",
+		OPT_STRING(0, "socket", &socket_path_arg, "path",
 			   "path of cache-daemon socket"),
 		OPT_END()
 	};
@@ -160,6 +163,7 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix)
 	if (!have_unix_sockets())
 		die(_("credential-cache unavailable; no unix socket support"));
 
+	socket_path = xstrdup_or_null(socket_path_arg);
 	if (!socket_path)
 		socket_path = get_socket_path();
 	if (!socket_path)
@@ -176,6 +180,7 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix)
 	else
 		; /* ignore unknown operation */
 
+	free(socket_path);
 	return 0;
 }
 
diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index c10e35905e..5d5b64205f 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='credential-cache tests'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-credential.sh
 
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 20/24] t/test-repository: fix leaking repository

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1635 bytes --]

The test-repository test helper zeroes out `the_repository` such that it
can be sure that our codebase only ends up using the supplied repository
that we initialize in the respective helper functions. This does cause
memory leaks though as the data that `the_repository` has been holding
onto is not referenced anymore.

Fix this by calling `repo_clear()` instead.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/helper/test-repository.c | 4 ++--
 t/t5318-commit-graph.sh    | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/helper/test-repository.c b/t/helper/test-repository.c
index c6a074df3d..63c37de33d 100644
--- a/t/helper/test-repository.c
+++ b/t/helper/test-repository.c
@@ -19,7 +19,7 @@ static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,
 
 	setup_git_env(gitdir);
 
-	memset(the_repository, 0, sizeof(*the_repository));
+	repo_clear(the_repository);
 
 	if (repo_init(&r, gitdir, worktree))
 		die("Couldn't init repo");
@@ -49,7 +49,7 @@ static void test_get_commit_tree_in_graph(const char *gitdir,
 
 	setup_git_env(gitdir);
 
-	memset(the_repository, 0, sizeof(*the_repository));
+	repo_clear(the_repository);
 
 	if (repo_init(&r, gitdir, worktree))
 		die("Couldn't init repo");
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index a2b4442660..2916c07e3c 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='commit graph'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-chunk.sh
 
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 21/24] object-name: fix leaking commit list items

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4406 bytes --]

When calling `get_oid_oneline()`, we pass in a `struct commit_list` that
gets modified by the function. This creates a weird situation where the
commit list may sometimes be empty after returning, but sometimes it
will continue to carry additional commits. In those cases the remainder
of the list leaks.

Ultimately, the design where we only pass partial ownership to
`get_oid_oneline()` feels shoddy. Refactor the code such that we only
pass a constant pointer to the list, creating a local copy as needed.
Callers are thus always responsible for freeing the commit list, which
then allows us to plug a bunch of memory leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 object-name.c                | 26 ++++++++++++++++----------
 t/t1511-rev-parse-caret.sh   |  1 +
 t/t6133-pathspec-rev-dwim.sh |  2 ++
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/object-name.c b/object-name.c
index 527b853ac4..240a93e7ce 100644
--- a/object-name.c
+++ b/object-name.c
@@ -27,7 +27,8 @@
 #include "date.h"
 #include "object-file-convert.h"
 
-static int get_oid_oneline(struct repository *r, const char *, struct object_id *, struct commit_list *);
+static int get_oid_oneline(struct repository *r, const char *, struct object_id *,
+			   const struct commit_list *);
 
 typedef int (*disambiguate_hint_fn)(struct repository *, const struct object_id *, void *);
 
@@ -1254,6 +1255,8 @@ static int peel_onion(struct repository *r, const char *name, int len,
 		prefix = xstrndup(sp + 1, name + len - 1 - (sp + 1));
 		commit_list_insert((struct commit *)o, &list);
 		ret = get_oid_oneline(r, prefix, oid, list);
+
+		free_commit_list(list);
 		free(prefix);
 		return ret;
 	}
@@ -1388,9 +1391,10 @@ static int handle_one_ref(const char *path, const struct object_id *oid,
 
 static int get_oid_oneline(struct repository *r,
 			   const char *prefix, struct object_id *oid,
-			   struct commit_list *list)
+			   const struct commit_list *list)
 {
-	struct commit_list *backup = NULL, *l;
+	struct commit_list *copy = NULL;
+	const struct commit_list *l;
 	int found = 0;
 	int negative = 0;
 	regex_t regex;
@@ -1411,14 +1415,14 @@ static int get_oid_oneline(struct repository *r,
 
 	for (l = list; l; l = l->next) {
 		l->item->object.flags |= ONELINE_SEEN;
-		commit_list_insert(l->item, &backup);
+		commit_list_insert(l->item, &copy);
 	}
-	while (list) {
+	while (copy) {
 		const char *p, *buf;
 		struct commit *commit;
 		int matches;
 
-		commit = pop_most_recent_commit(&list, ONELINE_SEEN);
+		commit = pop_most_recent_commit(&copy, ONELINE_SEEN);
 		if (!parse_object(r, &commit->object.oid))
 			continue;
 		buf = repo_get_commit_buffer(r, commit, NULL);
@@ -1433,10 +1437,9 @@ static int get_oid_oneline(struct repository *r,
 		}
 	}
 	regfree(&regex);
-	free_commit_list(list);
-	for (l = backup; l; l = l->next)
+	for (l = list; l; l = l->next)
 		clear_commit_marks(l->item, ONELINE_SEEN);
-	free_commit_list(backup);
+	free_commit_list(copy);
 	return found ? 0 : -1;
 }
 
@@ -2024,7 +2027,10 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,
 			refs_for_each_ref(get_main_ref_store(repo), handle_one_ref, &cb);
 			refs_head_ref(get_main_ref_store(repo), handle_one_ref, &cb);
 			commit_list_sort_by_date(&list);
-			return get_oid_oneline(repo, name + 2, oid, list);
+			ret = get_oid_oneline(repo, name + 2, oid, list);
+
+			free_commit_list(list);
+			return ret;
 		}
 		if (namelen < 3 ||
 		    name[2] != ':' ||
diff --git a/t/t1511-rev-parse-caret.sh b/t/t1511-rev-parse-caret.sh
index 6ecfed86bc..e7e78a4028 100755
--- a/t/t1511-rev-parse-caret.sh
+++ b/t/t1511-rev-parse-caret.sh
@@ -5,6 +5,7 @@ test_description='tests for ref^{stuff}'
 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/t/t6133-pathspec-rev-dwim.sh b/t/t6133-pathspec-rev-dwim.sh
index a290ffca0d..6dd4bbbf9f 100755
--- a/t/t6133-pathspec-rev-dwim.sh
+++ b/t/t6133-pathspec-rev-dwim.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test dwim of revs versus pathspecs in revision parser'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 22/24] entry: fix leaking pathnames during delayed checkout

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3172 bytes --]

When filtering files during delayed checkout, we pass a string list to
`async_query_available_blobs()`. This list is initialized with NODUP,
and thus inserted strings will not be owned by the list. In the latter
function we then try to hand over ownership by passing an `xstrup()`'d
value to `string_list_insert()`. But this is not how this works: a NODUP
list does not take ownership of allocated strings and will never free
them for the caller.

Fix this issue by initializing the list as `DUP` instead and dropping
the explicit call to `xstrdup()`. This is okay to do given that this is
the single callsite of `async_query_available_blobs()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 convert.c                               | 2 +-
 entry.c                                 | 4 +++-
 t/t2080-parallel-checkout-basics.sh     | 1 +
 t/t2082-parallel-checkout-attributes.sh | 1 +
 4 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/convert.c b/convert.c
index d8737fe0f2..61a540e212 100644
--- a/convert.c
+++ b/convert.c
@@ -960,7 +960,7 @@ int async_query_available_blobs(const char *cmd, struct string_list *available_p
 	while ((line = packet_read_line(process->out, NULL))) {
 		const char *path;
 		if (skip_prefix(line, "pathname=", &path))
-			string_list_insert(available_paths, xstrdup(path));
+			string_list_insert(available_paths, path);
 		else
 			; /* ignore unknown keys */
 	}
diff --git a/entry.c b/entry.c
index e7ed440ce2..3143b9996b 100644
--- a/entry.c
+++ b/entry.c
@@ -191,7 +191,7 @@ int finish_delayed_checkout(struct checkout *state, int show_progress)
 		progress = start_delayed_progress(_("Filtering content"), dco->paths.nr);
 	while (dco->filters.nr > 0) {
 		for_each_string_list_item(filter, &dco->filters) {
-			struct string_list available_paths = STRING_LIST_INIT_NODUP;
+			struct string_list available_paths = STRING_LIST_INIT_DUP;
 
 			if (!async_query_available_blobs(filter->string, &available_paths)) {
 				/* Filter reported an error */
@@ -245,6 +245,8 @@ int finish_delayed_checkout(struct checkout *state, int show_progress)
 				} else
 					errs = 1;
 			}
+
+			string_list_clear(&available_paths, 0);
 		}
 
 		filter_string_list(&dco->filters, 0, string_is_not_null, NULL);
diff --git a/t/t2080-parallel-checkout-basics.sh b/t/t2080-parallel-checkout-basics.sh
index 5ffe1a41e2..59e5570cb2 100755
--- a/t/t2080-parallel-checkout-basics.sh
+++ b/t/t2080-parallel-checkout-basics.sh
@@ -8,6 +8,7 @@ working tree.
 '
 
 TEST_NO_CREATE_REPO=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY/lib-parallel-checkout.sh"
 
diff --git a/t/t2082-parallel-checkout-attributes.sh b/t/t2082-parallel-checkout-attributes.sh
index f3511cd43a..aec55496eb 100755
--- a/t/t2082-parallel-checkout-attributes.sh
+++ b/t/t2082-parallel-checkout-attributes.sh
@@ -10,6 +10,7 @@ properly (without access to the index or attribute stack).
 '
 
 TEST_NO_CREATE_REPO=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY/lib-parallel-checkout.sh"
 . "$TEST_DIRECTORY/lib-encoding.sh"
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 23/24] convert: fix leaking config strings

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1697 bytes --]

In `read_convert_config()`, we end up reading some string values into
variables. We don't free any potentially-existing old values though,
which will result in a memory leak in case the same key has been defined
multiple times.

Fix those leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 convert.c             | 12 +++++++++---
 t/t0021-conversion.sh |  1 +
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/convert.c b/convert.c
index 61a540e212..92ce04c406 100644
--- a/convert.c
+++ b/convert.c
@@ -1050,14 +1050,20 @@ static int read_convert_config(const char *var, const char *value,
 	 * The command-line will not be interpolated in any way.
 	 */
 
-	if (!strcmp("smudge", key))
+	if (!strcmp("smudge", key)) {
+		FREE_AND_NULL(drv->smudge);
 		return git_config_string(&drv->smudge, var, value);
+	}
 
-	if (!strcmp("clean", key))
+	if (!strcmp("clean", key)) {
+		FREE_AND_NULL(drv->clean);
 		return git_config_string(&drv->clean, var, value);
+	}
 
-	if (!strcmp("process", key))
+	if (!strcmp("process", key)) {
+		FREE_AND_NULL(drv->process);
 		return git_config_string(&drv->process, var, value);
+	}
 
 	if (!strcmp("required", key)) {
 		drv->required = git_config_bool(var, value);
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 0b4997022b..eeb2714d9d 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -5,6 +5,7 @@ test_description='blob conversion via gitattributes'
 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-terminal.sh
 
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 24/24] commit-reach: fix trivial memory leak when computing reachability

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 978 bytes --]

We don't free the local `stack` commit list that we use to compute
reachability of multiple commits at once. Do so.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 commit-reach.c             | 1 +
 t/t3201-branch-contains.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/commit-reach.c b/commit-reach.c
index dabc2972e4..02f8218b8e 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -1227,4 +1227,5 @@ void tips_reachable_from_bases(struct repository *r,
 done:
 	free(commits);
 	repo_clear_commit_marks(r, SEEN);
+	free_commit_list(stack);
 }
diff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh
index 800fc33165..6e587d27d7 100755
--- a/t/t3201-branch-contains.sh
+++ b/t/t3201-branch-contains.sh
@@ -2,6 +2,7 @@
 
 test_description='branch --contains <commit>, --no-contains <commit> --merged, and --no-merged'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 07/24] builtin/submodule--helper: fix leaking clone depth parameter

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2944 bytes --]

The submodule helper supports a `--depth` parameter for both its "add"
and "clone" subcommands, which in both cases end up being forwarded to
git-clone(1). But while the former subcommand uses an `OPT_INTEGER()` to
parse the depth, the latter uses `OPT_STRING()`. Consequently, it is
possible to pass non-integer input to "--depth" when calling the "clone"
subcommand, where the value will then ultimately cause git-clone(1) to
bail out.

Besides the fact that the parameter verification should happen earlier,
the submodule helper infrastructure also internally tracks the depth via
a string. This requires us to convert the integer in the "add"
subcommand into an allocated string, and this string ultimately leaks.

Refactor the code to consistently track the clone depth as an integer.
This plugs the memory leak, simplifies the code and allows us to use
`OPT_INTEGER()` instead of `OPT_STRING()`, validating the input before
we shell out to git--clone(1).

Original-patch-by: Rubén Justo <rjusto@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/submodule--helper.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f1218a1995..863b01627c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1530,7 +1530,7 @@ struct module_clone_data {
 	const char *path;
 	const char *name;
 	const char *url;
-	const char *depth;
+	int depth;
 	struct list_objects_filter_options *filter_options;
 	unsigned int quiet: 1;
 	unsigned int progress: 1;
@@ -1729,8 +1729,8 @@ static int clone_submodule(const struct module_clone_data *clone_data,
 			strvec_push(&cp.args, "--quiet");
 		if (clone_data->progress)
 			strvec_push(&cp.args, "--progress");
-		if (clone_data->depth && *(clone_data->depth))
-			strvec_pushl(&cp.args, "--depth", clone_data->depth, NULL);
+		if (clone_data->depth > 0)
+			strvec_pushf(&cp.args, "--depth=%d", clone_data->depth);
 		if (reference->nr) {
 			struct string_list_item *item;
 
@@ -1851,8 +1851,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 			   N_("reference repository")),
 		OPT_BOOL(0, "dissociate", &dissociate,
 			   N_("use --reference only while cloning")),
-		OPT_STRING(0, "depth", &clone_data.depth,
-			   N_("string"),
+		OPT_INTEGER(0, "depth", &clone_data.depth,
 			   N_("depth for shallow clones")),
 		OPT__QUIET(&quiet, "suppress output for cloning a submodule"),
 		OPT_BOOL(0, "progress", &progress,
@@ -3200,7 +3199,7 @@ static int add_submodule(const struct add_data *add_data)
 		}
 		clone_data.dissociate = add_data->dissociate;
 		if (add_data->depth >= 0)
-			clone_data.depth = xstrfmt("%d", add_data->depth);
+			clone_data.depth = add_data->depth;
 
 		if (clone_submodule(&clone_data, &reference))
 			goto cleanup;
-- 
2.46.0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 00/24] Memory leak fixes (pt.3)

[Taylor Blau]

On Thu, Aug 01, 2024 at 12:38:06PM +0200, Patrick Steinhardt wrote:
> Range-diff against v1:

All looks reasonable to me. Thanks for incorporating mine and others'
suggestions, and again for working on this!

Thanks,
Taylor