[PATCH 00/11] built-ins: fix common memory leaks

[PATCH 00/11] built-ins: fix common memory leaks

[Ævar Arnfjörð Bjarmason]

This is a series of trivial leak-fixes which allows us to mark a
significant number of new tests as entirely passing under
SANITIZE=leak.

Now that 2da81d1efb0 (Merge branch 'ab/plug-leak-in-revisions',
2022-06-07) has landed we're going to have more & more cases where
fixing just one leak will allow us to mark N tests as passing in their
entirety. This is the first stepping stone on the way to that end.

Ævar Arnfjörð Bjarmason (11):
  check-ref-format: fix trivial memory leak
  clone: fix memory leak in copy_ref() call
  submodule.c: free() memory from xgetcwd()
  revert: free "struct replay_opts" members
  cat-file: fix a memory leak in --batch-command mode
  merge-file: refactor for subsequent memory leak fix
  merge-file: fix memory leaks on error path
  checkout: add a missing clear_unpack_trees_porcelain()
  gc: fix a memory leak
  cat-file: fix a common "struct object_context" memory leak
  pull: fix a "struct oid_array" memory leak

 builtin/cat-file.c                   | 33 +++++++++++++++++++---------
 builtin/check-ref-format.c           | 11 +++++++---
 builtin/checkout.c                   |  1 +
 builtin/clone.c                      |  1 +
 builtin/gc.c                         |  8 ++++++-
 builtin/merge-file.c                 | 32 +++++++++++++++------------
 builtin/pull.c                       | 16 +++++++++-----
 builtin/revert.c                     |  3 +++
 submodule.c                          |  3 ++-
 t/t0028-working-tree-encoding.sh     |  1 +
 t/t1051-large-conversion.sh          |  2 ++
 t/t1402-check-ref-format.sh          |  1 +
 t/t3304-notes-mixed.sh               |  1 +
 t/t4044-diff-index-unique-abbrev.sh  |  2 ++
 t/t4140-apply-ita.sh                 |  1 +
 t/t5314-pack-cycle-detection.sh      |  4 ++--
 t/t5524-pull-msg.sh                  |  1 +
 t/t6403-merge-file.sh                |  2 ++
 t/t6417-merge-ours-theirs.sh         |  1 +
 t/t6422-merge-rename-corner-cases.sh |  1 +
 t/t8007-cat-file-textconv.sh         |  2 ++
 t/t8010-cat-file-filters.sh          |  2 ++
 t/t9101-git-svn-props.sh             |  1 -
 t/t9104-git-svn-follow-parent.sh     |  1 -
 t/t9132-git-svn-broken-symlink.sh    |  1 -
 t/t9301-fast-import-notes.sh         |  1 +
 26 files changed, 93 insertions(+), 40 deletions(-)

-- 
2.37.0.874.g7d3439f13c4

[PATCH 01/11] check-ref-format: fix trivial memory leak

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in "git check-ref-format" that's been present in the
code in one form or another since 38eedc634bc (git check-ref-format
--print, 2009-10-12), the code got substantially refactored in
cfbe22f03f9 (check-ref-format: handle subcommands in separate
functions, 2010-08-05).

As a result we can mark a test as passing with SANITIZE=leak using
"TEST_PASSES_SANITIZE_LEAK=true".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/check-ref-format.c  | 11 ++++++++---
 t/t1402-check-ref-format.sh |  1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index bc67d3f0a83..fd0e5f86832 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -57,6 +57,8 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
 	int normalize = 0;
 	int flags = 0;
 	const char *refname;
+	char *to_free = NULL;
+	int ret = 1;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage(builtin_check_ref_format_usage);
@@ -81,11 +83,14 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
 
 	refname = argv[i];
 	if (normalize)
-		refname = collapse_slashes(refname);
+		refname = to_free = collapse_slashes(refname);
 	if (check_refname_format(refname, flags))
-		return 1;
+		goto cleanup;
 	if (normalize)
 		printf("%s\n", refname);
 
-	return 0;
+	ret = 0;
+cleanup:
+	free(to_free);
+	return ret;
 }
diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
index cabc516ae9a..5ed9d7318e0 100755
--- a/t/t1402-check-ref-format.sh
+++ b/t/t1402-check-ref-format.sh
@@ -2,6 +2,7 @@
 
 test_description='Test git check-ref-format'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 valid_ref() {
-- 
2.37.0.874.g7d3439f13c4

Re: [PATCH 01/11] check-ref-format: fix trivial memory leak

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
> index bc67d3f0a83..fd0e5f86832 100644
> --- a/builtin/check-ref-format.c
> +++ b/builtin/check-ref-format.c
> @@ -57,6 +57,8 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
>  	int normalize = 0;
>  	int flags = 0;
>  	const char *refname;
> +	char *to_free = NULL;
> +	int ret = 1;
>  
>  	if (argc == 2 && !strcmp(argv[1], "-h"))
>  		usage(builtin_check_ref_format_usage);
> @@ -81,11 +83,14 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
>  
>  	refname = argv[i];
>  	if (normalize)
> -		refname = collapse_slashes(refname);
> +		refname = to_free = collapse_slashes(refname);
>  	if (check_refname_format(refname, flags))
> -		return 1;
> +		goto cleanup;
>  	if (normalize)
>  		printf("%s\n", refname);
>  
> -	return 0;
> +	ret = 0;
> +cleanup:
> +	free(to_free);
> +	return ret;
>  }

Nice.

> diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
> index cabc516ae9a..5ed9d7318e0 100755
> --- a/t/t1402-check-ref-format.sh
> +++ b/t/t1402-check-ref-format.sh
> @@ -2,6 +2,7 @@
>  
>  test_description='Test git check-ref-format'
>  
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>  
>  valid_ref() {

[PATCH 02/11] clone: fix memory leak in copy_ref() call

[Ævar Arnfjörð Bjarmason]

Fix a memory leak added in 0ec4b1650cc (clone: fix ref selection in
--single-branch --branch=xxx, 2012-06-22), we need to free_refs() the
result of copy_ref().

We can't mark any tests passing passing with SANITIZE=leak using
"TEST_PASSES_SANITIZE_LEAK=true" as a result of this change, but
e.g. "t/t1500-rev-parse.sh" now gets closer to passing.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/clone.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index 89a91b00177..c43c85dad07 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -494,6 +494,7 @@ static struct ref *wanted_peer_refs(const struct ref *refs,
 			/* if --branch=tag, pull the requested tag explicitly */
 			get_fetch_map(remote_head, tag_refspec, &tail, 0);
 		}
+		free_refs(remote_head);
 	} else {
 		int i;
 		for (i = 0; i < refspec->nr; i++)
-- 
2.37.0.874.g7d3439f13c4

Re: [PATCH 02/11] clone: fix memory leak in copy_ref() call

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Fix a memory leak added in 0ec4b1650cc (clone: fix ref selection in
> --single-branch --branch=xxx, 2012-06-22), we need to free_refs() the
> result of copy_ref().

remote_head could be the result of guess_remote_head(), instead of
copy_ref(), and in either case, once we are done with it, we need to
and can safely free it.

> diff --git a/builtin/clone.c b/builtin/clone.c
> index 89a91b00177..c43c85dad07 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -494,6 +494,7 @@ static struct ref *wanted_peer_refs(const struct ref *refs,
>  			/* if --branch=tag, pull the requested tag explicitly */
>  			get_fetch_map(remote_head, tag_refspec, &tail, 0);
>  		}
> +		free_refs(remote_head);

Looks good.

>  	} else {
>  		int i;
>  		for (i = 0; i < refspec->nr; i++)

[PATCH 03/11] submodule.c: free() memory from xgetcwd()

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in code added in bf0231c6614 (rev-parse: add
--show-superproject-working-tree, 2017-03-08), we should never have
made the result of xgetcwd() a "const char *", as we return a
strbuf_detach()'d value. Let's fix that and free() it when we're done
with it.

We can't mark any tests passing passing with SANITIZE=leak using
"TEST_PASSES_SANITIZE_LEAK=true" as a result of this change, but
e.g. "t/t1500-rev-parse.sh" now gets closer to passing.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 submodule.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index 4e299f578f9..06073b2e7be 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2388,7 +2388,7 @@ int get_superproject_working_tree(struct strbuf *buf)
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf one_up = STRBUF_INIT;
-	const char *cwd = xgetcwd();
+	char *cwd = xgetcwd();
 	int ret = 0;
 	const char *subpath;
 	int code;
@@ -2451,6 +2451,7 @@ int get_superproject_working_tree(struct strbuf *buf)
 		ret = 1;
 		free(super_wt);
 	}
+	free(cwd);
 	strbuf_release(&sb);
 
 	code = finish_command(&cp);
-- 
2.37.0.874.g7d3439f13c4

[PATCH 04/11] revert: free "struct replay_opts" members

[Ævar Arnfjörð Bjarmason]

Call the release_revisions() function added in
1878b5edc03 (revision.[ch]: provide and start using a
release_revisions(), 2022-04-13) in cmd_revert(), as well as freeing
the xmalloc()'d "revs" member itself.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/revert.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/revert.c b/builtin/revert.c
index f84c253f4c6..2554f9099cc 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -246,6 +246,9 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	res = run_sequencer(argc, argv, &opts);
 	if (res < 0)
 		die(_("revert failed"));
+	if (opts.revs)
+		release_revisions(opts.revs);
+	free(opts.revs);
 	return res;
 }
 
-- 
2.37.0.874.g7d3439f13c4

[PATCH 05/11] cat-file: fix a memory leak in --batch-command mode

[Ævar Arnfjörð Bjarmason]

Fix a memory leak introduced in 440c705ea63 (cat-file: add
--batch-command mode, 2022-02-18). The free_cmds() function was only
called on "queued_nr" if we had a "flush" command. As the "without
flush for blob info" test added in the same commit shows we can't rely
on that, so let's call free_cmds() again at the end.

Since "nr" follows the usual pattern of being set to 0 if we've
free()'d the memory already it's OK to call it twice, even in cases
where we are doing a "flush".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/cat-file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 50cf38999d1..ac0459f96be 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -655,6 +655,7 @@ static void batch_objects_command(struct batch_options *opt,
 		free_cmds(queued_cmd, &nr);
 	}
 
+	free_cmds(queued_cmd, &nr);
 	free(queued_cmd);
 	strbuf_release(&input);
 }
-- 
2.37.0.874.g7d3439f13c4

[PATCH 06/11] merge-file: refactor for subsequent memory leak fix

[Ævar Arnfjörð Bjarmason]

Refactor the code in builtin/merge-file.c to:

 * Use the initializer to zero out "mmfs", and use modern C syntax for
   the rest.

 * Refactor the the inner loop to use a variable and "if/else if"
   pattern followed by "return". This will make a change to change it to
   a "goto cleanup" pattern smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/merge-file.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index e695867ee54..793817f3cb9 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -25,10 +25,10 @@ static int label_cb(const struct option *opt, const char *arg, int unset)
 
 int cmd_merge_file(int argc, const char **argv, const char *prefix)
 {
-	const char *names[3] = { NULL, NULL, NULL };
-	mmfile_t mmfs[3];
-	mmbuffer_t result = {NULL, 0};
-	xmparam_t xmp = {{0}};
+	const char *names[3] = { 0 };
+	mmfile_t mmfs[3] = { 0 };
+	mmbuffer_t result = { 0 };
+	xmparam_t xmp = { 0 };
 	int ret = 0, i = 0, to_stdout = 0;
 	int quiet = 0;
 	struct option options[] = {
@@ -71,21 +71,23 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 
 	for (i = 0; i < 3; i++) {
 		char *fname;
-		int ret;
+		mmfile_t *mmf = mmfs + i;
 
 		if (!names[i])
 			names[i] = argv[i];
 
 		fname = prefix_filename(prefix, argv[i]);
-		ret = read_mmfile(mmfs + i, fname);
+
+		if (read_mmfile(mmf, fname))
+			ret = -1;
+		else if (mmf->size > MAX_XDIFF_SIZE ||
+			 buffer_is_binary(mmf->ptr, mmf->size))
+			ret = error("Cannot merge binary files: %s",
+				    argv[i]);
+
 		free(fname);
 		if (ret)
-			return -1;
-
-		if (mmfs[i].size > MAX_XDIFF_SIZE ||
-		    buffer_is_binary(mmfs[i].ptr, mmfs[i].size))
-			return error("Cannot merge binary files: %s",
-					argv[i]);
+			return ret;
 	}
 
 	xmp.ancestor = names[1];
-- 
2.37.0.874.g7d3439f13c4

[PATCH 07/11] merge-file: fix memory leaks on error path

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in "merge-file", we need to loop over the "mmfs"
array and free() what we've got so far when we error out. As a result
we can mark a test as passing with SANITIZE=leak using
"TEST_PASSES_SANITIZE_LEAK=true".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/merge-file.c  | 10 ++++++----
 t/t6403-merge-file.sh |  2 ++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index 793817f3cb9..c923bbf2abb 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -87,7 +87,8 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 
 		free(fname);
 		if (ret)
-			return ret;
+			goto cleanup;
+
 	}
 
 	xmp.ancestor = names[1];
@@ -95,9 +96,6 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	xmp.file2 = names[2];
 	ret = xdl_merge(mmfs + 1, mmfs + 0, mmfs + 2, &xmp, &result);
 
-	for (i = 0; i < 3; i++)
-		free(mmfs[i].ptr);
-
 	if (ret >= 0) {
 		const char *filename = argv[0];
 		char *fpath = prefix_filename(prefix, argv[0]);
@@ -118,5 +116,9 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	if (ret > 127)
 		ret = 127;
 
+cleanup:
+	for (i = 0; i < 3; i++)
+		free(mmfs[i].ptr);
+
 	return ret;
 }
diff --git a/t/t6403-merge-file.sh b/t/t6403-merge-file.sh
index 2f421d967ab..1a7082323dd 100755
--- a/t/t6403-merge-file.sh
+++ b/t/t6403-merge-file.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='RCS merge replacement: merge-file'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.37.0.874.g7d3439f13c4

[PATCH 08/11] checkout: add a missing clear_unpack_trees_porcelain()

[Ævar Arnfjörð Bjarmason]

In 1c41d2805e4 (unpack_trees_options: free messages when done,
2018-05-21) we started calling clear_unpack_trees_porcelain() on this
codepath, but missed this error path, let's also clear what we've
allocated in that case.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/checkout.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2eefda81d8c..3d6762106e8 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -750,6 +750,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 		refresh_cache(REFRESH_QUIET);
 
 		if (unmerged_cache()) {
+			clear_unpack_trees_porcelain(&topts);
 			error(_("you need to resolve your current index first"));
 			return 1;
 		}
-- 
2.37.0.874.g7d3439f13c4

Re: [PATCH 08/11] checkout: add a missing clear_unpack_trees_porcelain()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> In 1c41d2805e4 (unpack_trees_options: free messages when done,
> 2018-05-21) we started calling clear_unpack_trees_porcelain() on this
> codepath, but missed this error path, let's also clear what we've
> allocated in that case.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/checkout.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 2eefda81d8c..3d6762106e8 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -750,6 +750,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
>  		refresh_cache(REFRESH_QUIET);
>  
>  		if (unmerged_cache()) {
> +			clear_unpack_trees_porcelain(&topts);
>  			error(_("you need to resolve your current index first"));
>  			return 1;
>  		}

Does refresh_cache(REFRESH_QUIET) depend on the porcelain error
messages already set?  If not another way to fix it may be to delay
the call to setup_unpack_trees_porcelain() until it becomes needed.

But the patch as posted is certainly an improvement.

Thanks.

Re: [PATCH 08/11] checkout: add a missing clear_unpack_trees_porcelain()

[Ævar Arnfjörð Bjarmason]

On Thu, Jun 30 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> In 1c41d2805e4 (unpack_trees_options: free messages when done,
>> 2018-05-21) we started calling clear_unpack_trees_porcelain() on this
>> codepath, but missed this error path, let's also clear what we've
>> allocated in that case.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  builtin/checkout.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/builtin/checkout.c b/builtin/checkout.c
>> index 2eefda81d8c..3d6762106e8 100644
>> --- a/builtin/checkout.c
>> +++ b/builtin/checkout.c
>> @@ -750,6 +750,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
>>  		refresh_cache(REFRESH_QUIET);
>>  
>>  		if (unmerged_cache()) {
>> +			clear_unpack_trees_porcelain(&topts);
>>  			error(_("you need to resolve your current index first"));
>>  			return 1;
>>  		}
>
> Does refresh_cache(REFRESH_QUIET) depend on the porcelain error
> messages already set?  If not another way to fix it may be to delay
> the call to setup_unpack_trees_porcelain() until it becomes needed.
>
> But the patch as posted is certainly an improvement.

Yes, that would work too, I can do it that way if you'd like.

I was trying to keep these fixes as narrow as possible, and resist any
temptations to re-arrange code so as to avoid allocations, which we can
often do, but then instead of a 1-line diff it's 10, 50, 100... :)

In this case it'll be somewhere around 10, and just a code-move, so
maybe that's fine...

Re: [PATCH 08/11] checkout: add a missing clear_unpack_trees_porcelain()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> Does refresh_cache(REFRESH_QUIET) depend on the porcelain error
>> messages already set?  If not another way to fix it may be to delay
>> the call to setup_unpack_trees_porcelain() until it becomes needed.
>>
>> But the patch as posted is certainly an improvement.
>
> Yes, that would work too, I can do it that way if you'd like.
>
> I was trying to keep these fixes as narrow as possible, and resist any
> temptations to re-arrange code so as to avoid allocations, which we can
> often do, but then instead of a 1-line diff it's 10, 50, 100... :)
>
> In this case it'll be somewhere around 10, and just a code-move, so
> maybe that's fine...

The ideal answer I was hoping to hear was

    The current implementation of the refresh_cache() function might
    not, but there is no guarantee that it will stay so.  We may add
    more calls that may (or may not) need to access the error
    message table before this early return.  Setting the porcelain
    messages before making any calls to any functions in the API and
    unsetting after we are done is a much better code structure.

;-)

[PATCH 09/11] gc: fix a memory leak

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in code added in 41abfe15d95 (maintenance: add
pack-refs task, 2021-02-09), we need to call strvec_clear() on the
"struct strvec" that we initialized.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 021e9256ae2..eeff2b760e0 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -168,9 +168,15 @@ struct maintenance_run_opts;
 static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts)
 {
 	struct strvec pack_refs_cmd = STRVEC_INIT;
+	int ret;
+
 	strvec_pushl(&pack_refs_cmd, "pack-refs", "--all", "--prune", NULL);
 
-	return run_command_v_opt(pack_refs_cmd.v, RUN_GIT_CMD);
+	ret = run_command_v_opt(pack_refs_cmd.v, RUN_GIT_CMD);
+
+	strvec_clear(&pack_refs_cmd);
+
+	return ret;
 }
 
 static int too_many_loose_objects(void)
-- 
2.37.0.874.g7d3439f13c4

[PATCH 10/11] cat-file: fix a common "struct object_context" memory leak

[Ævar Arnfjörð Bjarmason]

Fix a memory leak where "cat-file" will leak the "path" member. See
e5fba602e59 (textconv: support for cat_file, 2010-06-15) for the code
that introduced the offending get_oid_with_context() call (called
get_sha1_with_context() at the time).

As a result we can mark several tests as passing with SANITIZE=leak
using "TEST_PASSES_SANITIZE_LEAK=true".

As noted in dc944b65f1d (get_sha1_with_context: dynamically allocate
oc->path, 2017-05-19) callers must free the "path" member. That same
commit added the relevant free() to this function, but we weren't
catching cases where we'd return early.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/cat-file.c                   | 32 +++++++++++++++++++---------
 t/t0028-working-tree-encoding.sh     |  1 +
 t/t1051-large-conversion.sh          |  2 ++
 t/t3304-notes-mixed.sh               |  1 +
 t/t4044-diff-index-unique-abbrev.sh  |  2 ++
 t/t4140-apply-ita.sh                 |  1 +
 t/t5314-pack-cycle-detection.sh      |  4 ++--
 t/t6422-merge-rename-corner-cases.sh |  1 +
 t/t8007-cat-file-textconv.sh         |  2 ++
 t/t8010-cat-file-filters.sh          |  2 ++
 t/t9104-git-svn-follow-parent.sh     |  1 -
 t/t9132-git-svn-broken-symlink.sh    |  1 -
 t/t9301-fast-import-notes.sh         |  1 +
 13 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index ac0459f96be..f42782e955f 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -71,6 +71,7 @@ static int stream_blob(const struct object_id *oid)
 static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 			int unknown_type)
 {
+	int ret;
 	struct object_id oid;
 	enum object_type type;
 	char *buf;
@@ -106,7 +107,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 		if (sb.len) {
 			printf("%s\n", sb.buf);
 			strbuf_release(&sb);
-			return 0;
+			ret = 0;
+			goto cleanup;
 		}
 		break;
 
@@ -115,7 +117,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 		if (oid_object_info_extended(the_repository, &oid, &oi, flags) < 0)
 			die("git cat-file: could not get object info");
 		printf("%"PRIuMAX"\n", (uintmax_t)size);
-		return 0;
+		ret = 0;
+		goto cleanup;
 
 	case 'e':
 		return !has_object_file(&oid);
@@ -123,8 +126,10 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 	case 'w':
 
 		if (filter_object(path, obj_context.mode,
-				  &oid, &buf, &size))
-			return -1;
+				  &oid, &buf, &size)) {
+			ret = -1;
+			goto cleanup;
+		}
 		break;
 
 	case 'c':
@@ -143,11 +148,14 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 			const char *ls_args[3] = { NULL };
 			ls_args[0] =  "ls-tree";
 			ls_args[1] =  obj_name;
-			return cmd_ls_tree(2, ls_args, NULL);
+			ret = cmd_ls_tree(2, ls_args, NULL);
+			goto cleanup;
 		}
 
-		if (type == OBJ_BLOB)
-			return stream_blob(&oid);
+		if (type == OBJ_BLOB) {
+			ret = stream_blob(&oid);
+			goto cleanup;
+		}
 		buf = read_object_file(&oid, &type, &size);
 		if (!buf)
 			die("Cannot read object %s", obj_name);
@@ -172,8 +180,10 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 			} else
 				oidcpy(&blob_oid, &oid);
 
-			if (oid_object_info(the_repository, &blob_oid, NULL) == OBJ_BLOB)
-				return stream_blob(&blob_oid);
+			if (oid_object_info(the_repository, &blob_oid, NULL) == OBJ_BLOB) {
+				ret = stream_blob(&blob_oid);
+				goto cleanup;
+			}
 			/*
 			 * we attempted to dereference a tag to a blob
 			 * and failed; there may be new dereference
@@ -193,9 +203,11 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 		die("git cat-file %s: bad file", obj_name);
 
 	write_or_die(1, buf, size);
+	ret = 0;
+cleanup:
 	free(buf);
 	free(obj_context.path);
-	return 0;
+	return ret;
 }
 
 struct expand_data {
diff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh
index 82905a2156f..416eeabdb94 100755
--- a/t/t0028-working-tree-encoding.sh
+++ b/t/t0028-working-tree-encoding.sh
@@ -5,6 +5,7 @@ test_description='working-tree-encoding 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-encoding.sh"
 
diff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh
index 042b0e44292..f6709c9f569 100755
--- a/t/t1051-large-conversion.sh
+++ b/t/t1051-large-conversion.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test conversion filters on large files'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 set_attr() {
diff --git a/t/t3304-notes-mixed.sh b/t/t3304-notes-mixed.sh
index 03dfcd3954c..2c3a2452668 100755
--- a/t/t3304-notes-mixed.sh
+++ b/t/t3304-notes-mixed.sh
@@ -5,6 +5,7 @@ test_description='Test notes trees that also contain non-notes'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 number_of_commits=100
diff --git a/t/t4044-diff-index-unique-abbrev.sh b/t/t4044-diff-index-unique-abbrev.sh
index 4701796d10e..29e49d22902 100755
--- a/t/t4044-diff-index-unique-abbrev.sh
+++ b/t/t4044-diff-index-unique-abbrev.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test unique sha1 abbreviation on "index from..to" line'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t4140-apply-ita.sh b/t/t4140-apply-ita.sh
index c614eaf04cc..b375aca0d74 100755
--- a/t/t4140-apply-ita.sh
+++ b/t/t4140-apply-ita.sh
@@ -2,6 +2,7 @@
 
 test_description='git apply of i-t-a file'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t5314-pack-cycle-detection.sh b/t/t5314-pack-cycle-detection.sh
index 0aec8619e22..73a241743aa 100755
--- a/t/t5314-pack-cycle-detection.sh
+++ b/t/t5314-pack-cycle-detection.sh
@@ -49,9 +49,9 @@ Then no matter which order we start looking at the packs in, we know that we
 will always find a delta for "file", because its lookup will always come
 immediately after the lookup for "dummy".
 '
-. ./test-lib.sh
-
 
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
 
 # Create a pack containing the tree $1 and blob $1:file, with
 # the latter stored as a delta against $2:file.
diff --git a/t/t6422-merge-rename-corner-cases.sh b/t/t6422-merge-rename-corner-cases.sh
index bf4ce3c63d4..9b65768aed6 100755
--- a/t/t6422-merge-rename-corner-cases.sh
+++ b/t/t6422-merge-rename-corner-cases.sh
@@ -6,6 +6,7 @@ test_description="recursive merge corner cases w/ renames but not criss-crosses"
 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-merge.sh
 
diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh
index b067983ba1c..c8266f17f14 100755
--- a/t/t8007-cat-file-textconv.sh
+++ b/t/t8007-cat-file-textconv.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='git cat-file textconv support'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 cat >helper <<'EOF'
diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
index 31de4b64dc0..ca04242ca01 100755
--- a/t/t8010-cat-file-filters.sh
+++ b/t/t8010-cat-file-filters.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='git cat-file filters support'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup ' '
diff --git a/t/t9104-git-svn-follow-parent.sh b/t/t9104-git-svn-follow-parent.sh
index 5cf2ef4b8b0..85d735861fc 100755
--- a/t/t9104-git-svn-follow-parent.sh
+++ b/t/t9104-git-svn-follow-parent.sh
@@ -5,7 +5,6 @@
 
 test_description='git svn fetching'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 test_expect_success 'initialize repo' '
diff --git a/t/t9132-git-svn-broken-symlink.sh b/t/t9132-git-svn-broken-symlink.sh
index 4d8d0584b79..aeceffaf7b0 100755
--- a/t/t9132-git-svn-broken-symlink.sh
+++ b/t/t9132-git-svn-broken-symlink.sh
@@ -2,7 +2,6 @@
 
 test_description='test that git handles an svn repository with empty symlinks'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 test_expect_success 'load svn dumpfile' '
 	svnadmin load "$rawsvnrepo" <<EOF
diff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh
index 1ae4d7c0d37..58413221e6a 100755
--- a/t/t9301-fast-import-notes.sh
+++ b/t/t9301-fast-import-notes.sh
@@ -7,6 +7,7 @@ test_description='test git fast-import of notes objects'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 
-- 
2.37.0.874.g7d3439f13c4

[PATCH 11/11] pull: fix a "struct oid_array" memory leak

[Ævar Arnfjörð Bjarmason]

Fix a memory leak introduced in 44c175c7a46 (pull: error on no merge
candidates, 2015-06-18). As a result we can mark several tests as
passing with SANITIZE=leak using "TEST_PASSES_SANITIZE_LEAK=true".

Removing the "int ret = 0" assignment added here in a6d7eb2c7a6 (pull:
optionally rebase submodules (remote submodule changes only),
2017-06-23) is not a logic error, it could always have been left
uninitialized (as "int ret"), now that we'll use the "ret" from the
upper scope we can drop the assignment in the "opt_rebase" branch.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/pull.c               | 16 ++++++++++------
 t/t5524-pull-msg.sh          |  1 +
 t/t6417-merge-ours-theirs.sh |  1 +
 t/t9101-git-svn-props.sh     |  1 -
 4 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 01155ba67b2..403a24d7ca6 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -990,6 +990,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	int rebase_unspecified = 0;
 	int can_ff;
 	int divergent;
+	int ret;
 
 	if (!getenv("GIT_REFLOG_ACTION"))
 		set_reflog_message(argc, argv);
@@ -1100,7 +1101,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (is_null_oid(&orig_head)) {
 		if (merge_heads.nr > 1)
 			die(_("Cannot merge multiple branches into empty head."));
-		return pull_into_void(merge_heads.oid, &curr_head);
+		ret = pull_into_void(merge_heads.oid, &curr_head);
+		goto cleanup;
 	}
 	if (merge_heads.nr > 1) {
 		if (opt_rebase)
@@ -1125,8 +1127,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	}
 
 	if (opt_rebase) {
-		int ret = 0;
-
 		struct object_id newbase;
 		struct object_id upstream;
 		get_rebase_newbase_and_upstream(&newbase, &upstream, &curr_head,
@@ -1149,12 +1149,16 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
 			ret = rebase_submodules();
 
-		return ret;
+		goto cleanup;
 	} else {
-		int ret = run_merge();
+		ret = run_merge();
 		if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
 			     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
 			ret = update_submodules();
-		return ret;
+		goto cleanup;
 	}
+
+cleanup:
+	oid_array_clear(&merge_heads);
+	return ret;
 }
diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
index b2be3605f5a..56716e29ddf 100755
--- a/t/t5524-pull-msg.sh
+++ b/t/t5524-pull-msg.sh
@@ -2,6 +2,7 @@
 
 test_description='git pull message generation'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 dollar='$Dollar'
diff --git a/t/t6417-merge-ours-theirs.sh b/t/t6417-merge-ours-theirs.sh
index 62d1406119e..482b73a998f 100755
--- a/t/t6417-merge-ours-theirs.sh
+++ b/t/t6417-merge-ours-theirs.sh
@@ -4,6 +4,7 @@ test_description='Merge-recursive ours and theirs variants'
 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/t9101-git-svn-props.sh b/t/t9101-git-svn-props.sh
index d043e80fc34..52046e60d51 100755
--- a/t/t9101-git-svn-props.sh
+++ b/t/t9101-git-svn-props.sh
@@ -5,7 +5,6 @@
 
 test_description='git svn property tests'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 mkdir import
-- 
2.37.0.874.g7d3439f13c4

[PATCH v2 00/11] built-ins: fix common memory leaks

[Ævar Arnfjörð Bjarmason]

Fix common memory leaks in built-ins, for a general overview see the
v1 CL:
https://lore.kernel.org/git/cover-00.11-00000000000-20220630T175714Z-avarab@gmail.com/

Changes since v1:

 * Amend the commit message of 2/11, we always get copy_ref() data,
   but note that we do so via an indirection in clone.c
 * Replace 8/11, maybe that solution is going overboard, we could also
   just drop it from this series...

Ævar Arnfjörð Bjarmason (11):
  check-ref-format: fix trivial memory leak
  clone: fix memory leak in wanted_peer_refs()
  submodule.c: free() memory from xgetcwd()
  revert: free "struct replay_opts" members
  cat-file: fix a memory leak in --batch-command mode
  merge-file: refactor for subsequent memory leak fix
  merge-file: fix memory leaks on error path
  checkout: avoid "struct unpack_trees_options" leak
  gc: fix a memory leak
  cat-file: fix a common "struct object_context" memory leak
  pull: fix a "struct oid_array" memory leak

 builtin/cat-file.c                   | 33 +++++++++++++++++--------
 builtin/check-ref-format.c           | 11 ++++++---
 builtin/checkout.c                   | 36 +++++++++++++++++-----------
 builtin/clone.c                      |  1 +
 builtin/gc.c                         |  8 ++++++-
 builtin/merge-file.c                 | 32 ++++++++++++++-----------
 builtin/pull.c                       | 16 ++++++++-----
 builtin/revert.c                     |  3 +++
 submodule.c                          |  3 ++-
 t/t0028-working-tree-encoding.sh     |  1 +
 t/t1051-large-conversion.sh          |  2 ++
 t/t1402-check-ref-format.sh          |  1 +
 t/t3304-notes-mixed.sh               |  1 +
 t/t4044-diff-index-unique-abbrev.sh  |  2 ++
 t/t4140-apply-ita.sh                 |  1 +
 t/t5314-pack-cycle-detection.sh      |  4 ++--
 t/t5524-pull-msg.sh                  |  1 +
 t/t6403-merge-file.sh                |  2 ++
 t/t6417-merge-ours-theirs.sh         |  1 +
 t/t6422-merge-rename-corner-cases.sh |  1 +
 t/t8007-cat-file-textconv.sh         |  2 ++
 t/t8010-cat-file-filters.sh          |  2 ++
 t/t9101-git-svn-props.sh             |  1 -
 t/t9104-git-svn-follow-parent.sh     |  1 -
 t/t9132-git-svn-broken-symlink.sh    |  1 -
 t/t9301-fast-import-notes.sh         |  1 +
 26 files changed, 114 insertions(+), 54 deletions(-)

Range-diff against v1:
 1:  f35cf7c6ee9 =  1:  f35cf7c6ee9 check-ref-format: fix trivial memory leak
 2:  8e424238071 !  2:  24a1eaa51a3 clone: fix memory leak in copy_ref() call
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    clone: fix memory leak in copy_ref() call
    +    clone: fix memory leak in wanted_peer_refs()
     
         Fix a memory leak added in 0ec4b1650cc (clone: fix ref selection in
    -    --single-branch --branch=xxx, 2012-06-22), we need to free_refs() the
    -    result of copy_ref().
    +    --single-branch --branch=xxx, 2012-06-22).
    +
    +    Whether we get our "remote_head" from copy_ref() directly, or with a
    +    call to guess_remote_head() it'll be the result of a copy_ref() in
    +    either case, as guess_remote_head() is a wrapper for copy_ref() (or it
    +    returns NULL).
     
         We can't mark any tests passing passing with SANITIZE=leak using
         "TEST_PASSES_SANITIZE_LEAK=true" as a result of this change, but
 3:  a687d1281f8 =  3:  691b178aaf0 submodule.c: free() memory from xgetcwd()
 4:  c9c2380be34 =  4:  6fc895676f4 revert: free "struct replay_opts" members
 5:  9ba267377ee =  5:  692d6e0e3d8 cat-file: fix a memory leak in --batch-command mode
 6:  17e66130b94 =  6:  d894e97b5ae merge-file: refactor for subsequent memory leak fix
 7:  8803a0df799 =  7:  8658f3ad020 merge-file: fix memory leaks on error path
 8:  94e28aa7ab3 !  8:  e21d7e4e9df checkout: add a missing clear_unpack_trees_porcelain()
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    checkout: add a missing clear_unpack_trees_porcelain()
    +    checkout: avoid "struct unpack_trees_options" leak
     
         In 1c41d2805e4 (unpack_trees_options: free messages when done,
         2018-05-21) we started calling clear_unpack_trees_porcelain() on this
    -    codepath, but missed this error path, let's also clear what we've
    -    allocated in that case.
    +    codepath, but missed this error path.
    +
    +    We could call clear_unpack_trees_porcelain() just before we error()
    +    and return when unmerged_cache() fails, but the more correct fix is to
    +    not have the unmerged_cache() check happen in the middle of our
    +    "topts" setup.
    +
    +    Before 23cbf11b5c0 (merge-recursive: porcelain messages for checkout,
    +    2010-08-11) we would not malloc() to setup our "topts", which is when
    +    this started to leak on the error path.
    +
    +    Before that this code wasn't conflating the setup of "topts" and the
    +    unmerged_cache() call in any meaningful way. The initial version in
    +    782c2d65c24 (Build in checkout, 2008-02-07) just does a "memset" of
    +    it, and initializes a single struct member.
    +
    +    Then in 8ccba008ee3 (unpack-trees: allow Porcelain to give different
    +    error messages, 2008-05-17) we added the initialization of the error
    +    message, which as noted above finally started leaking in 23cbf11b5c0.
    +
    +    Let's fix the memory leak, and avoid future issues by initializing the
    +    "topts" with a helper function. There are no functional changes here.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/checkout.c ##
    +@@ builtin/checkout.c: static void setup_branch_path(struct branch_info *branch)
    + 	branch->path = strbuf_detach(&buf, NULL);
    + }
    + 
    ++static void init_topts(struct unpack_trees_options *topts, int merge,
    ++		       int show_progress, int overwrite_ignore,
    ++		       struct commit *old_commit)
    ++{
    ++	memset(topts, 0, sizeof(*topts));
    ++	topts->head_idx = -1;
    ++	topts->src_index = &the_index;
    ++	topts->dst_index = &the_index;
    ++
    ++	setup_unpack_trees_porcelain(topts, "checkout");
    ++
    ++	topts->initial_checkout = is_cache_unborn();
    ++	topts->update = 1;
    ++	topts->merge = 1;
    ++	topts->quiet = merge && old_commit;
    ++	topts->verbose_update = show_progress;
    ++	topts->fn = twoway_merge;
    ++	topts->preserve_ignored = !overwrite_ignore;
    ++}
    ++
    + static int merge_working_tree(const struct checkout_opts *opts,
    + 			      struct branch_info *old_branch_info,
    + 			      struct branch_info *new_branch_info,
     @@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *opts,
    + 		struct unpack_trees_options topts;
    + 		const struct object_id *old_commit_oid;
    + 
    +-		memset(&topts, 0, sizeof(topts));
    +-		topts.head_idx = -1;
    +-		topts.src_index = &the_index;
    +-		topts.dst_index = &the_index;
    +-
    +-		setup_unpack_trees_porcelain(&topts, "checkout");
    +-
      		refresh_cache(REFRESH_QUIET);
      
      		if (unmerged_cache()) {
    -+			clear_unpack_trees_porcelain(&topts);
    - 			error(_("you need to resolve your current index first"));
    - 			return 1;
    +@@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *opts,
      		}
    + 
    + 		/* 2-way merge to the new branch */
    +-		topts.initial_checkout = is_cache_unborn();
    +-		topts.update = 1;
    +-		topts.merge = 1;
    +-		topts.quiet = opts->merge && old_branch_info->commit;
    +-		topts.verbose_update = opts->show_progress;
    +-		topts.fn = twoway_merge;
    ++		init_topts(&topts, opts->merge, opts->show_progress,
    ++			   opts->overwrite_ignore, old_branch_info->commit);
    + 		init_checkout_metadata(&topts.meta, new_branch_info->refname,
    + 				       new_branch_info->commit ?
    + 				       &new_branch_info->commit->object.oid :
    + 				       &new_branch_info->oid, NULL);
    +-		topts.preserve_ignored = !opts->overwrite_ignore;
    + 
    + 		old_commit_oid = old_branch_info->commit ?
    + 			&old_branch_info->commit->object.oid :
 9:  b51a275b6b1 =  9:  10a97a9cac4 gc: fix a memory leak
10:  9474e2bcf93 = 10:  481d006378b cat-file: fix a common "struct object_context" memory leak
11:  022399ad652 = 11:  5666104943f pull: fix a "struct oid_array" memory leak
-- 
2.37.0.900.g4d0de1cceb2

[PATCH v2 01/11] check-ref-format: fix trivial memory leak

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in "git check-ref-format" that's been present in the
code in one form or another since 38eedc634bc (git check-ref-format
--print, 2009-10-12), the code got substantially refactored in
cfbe22f03f9 (check-ref-format: handle subcommands in separate
functions, 2010-08-05).

As a result we can mark a test as passing with SANITIZE=leak using
"TEST_PASSES_SANITIZE_LEAK=true".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/check-ref-format.c  | 11 ++++++++---
 t/t1402-check-ref-format.sh |  1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index bc67d3f0a83..fd0e5f86832 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -57,6 +57,8 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
 	int normalize = 0;
 	int flags = 0;
 	const char *refname;
+	char *to_free = NULL;
+	int ret = 1;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage(builtin_check_ref_format_usage);
@@ -81,11 +83,14 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
 
 	refname = argv[i];
 	if (normalize)
-		refname = collapse_slashes(refname);
+		refname = to_free = collapse_slashes(refname);
 	if (check_refname_format(refname, flags))
-		return 1;
+		goto cleanup;
 	if (normalize)
 		printf("%s\n", refname);
 
-	return 0;
+	ret = 0;
+cleanup:
+	free(to_free);
+	return ret;
 }
diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
index cabc516ae9a..5ed9d7318e0 100755
--- a/t/t1402-check-ref-format.sh
+++ b/t/t1402-check-ref-format.sh
@@ -2,6 +2,7 @@
 
 test_description='Test git check-ref-format'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 valid_ref() {
-- 
2.37.0.900.g4d0de1cceb2

[PATCH v2 02/11] clone: fix memory leak in wanted_peer_refs()

[Ævar Arnfjörð Bjarmason]

Fix a memory leak added in 0ec4b1650cc (clone: fix ref selection in
--single-branch --branch=xxx, 2012-06-22).

Whether we get our "remote_head" from copy_ref() directly, or with a
call to guess_remote_head() it'll be the result of a copy_ref() in
either case, as guess_remote_head() is a wrapper for copy_ref() (or it
returns NULL).

We can't mark any tests passing passing with SANITIZE=leak using
"TEST_PASSES_SANITIZE_LEAK=true" as a result of this change, but
e.g. "t/t1500-rev-parse.sh" now gets closer to passing.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/clone.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index 89a91b00177..c43c85dad07 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -494,6 +494,7 @@ static struct ref *wanted_peer_refs(const struct ref *refs,
 			/* if --branch=tag, pull the requested tag explicitly */
 			get_fetch_map(remote_head, tag_refspec, &tail, 0);
 		}
+		free_refs(remote_head);
 	} else {
 		int i;
 		for (i = 0; i < refspec->nr; i++)
-- 
2.37.0.900.g4d0de1cceb2

[PATCH v2 03/11] submodule.c: free() memory from xgetcwd()

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in code added in bf0231c6614 (rev-parse: add
--show-superproject-working-tree, 2017-03-08), we should never have
made the result of xgetcwd() a "const char *", as we return a
strbuf_detach()'d value. Let's fix that and free() it when we're done
with it.

We can't mark any tests passing passing with SANITIZE=leak using
"TEST_PASSES_SANITIZE_LEAK=true" as a result of this change, but
e.g. "t/t1500-rev-parse.sh" now gets closer to passing.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 submodule.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index 4e299f578f9..06073b2e7be 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2388,7 +2388,7 @@ int get_superproject_working_tree(struct strbuf *buf)
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf one_up = STRBUF_INIT;
-	const char *cwd = xgetcwd();
+	char *cwd = xgetcwd();
 	int ret = 0;
 	const char *subpath;
 	int code;
@@ -2451,6 +2451,7 @@ int get_superproject_working_tree(struct strbuf *buf)
 		ret = 1;
 		free(super_wt);
 	}
+	free(cwd);
 	strbuf_release(&sb);
 
 	code = finish_command(&cp);
-- 
2.37.0.900.g4d0de1cceb2

[PATCH v2 04/11] revert: free "struct replay_opts" members

[Ævar Arnfjörð Bjarmason]

Call the release_revisions() function added in
1878b5edc03 (revision.[ch]: provide and start using a
release_revisions(), 2022-04-13) in cmd_revert(), as well as freeing
the xmalloc()'d "revs" member itself.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/revert.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/revert.c b/builtin/revert.c
index f84c253f4c6..2554f9099cc 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -246,6 +246,9 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	res = run_sequencer(argc, argv, &opts);
 	if (res < 0)
 		die(_("revert failed"));
+	if (opts.revs)
+		release_revisions(opts.revs);
+	free(opts.revs);
 	return res;
 }
 
-- 
2.37.0.900.g4d0de1cceb2

[PATCH v2 05/11] cat-file: fix a memory leak in --batch-command mode

[Ævar Arnfjörð Bjarmason]

Fix a memory leak introduced in 440c705ea63 (cat-file: add
--batch-command mode, 2022-02-18). The free_cmds() function was only
called on "queued_nr" if we had a "flush" command. As the "without
flush for blob info" test added in the same commit shows we can't rely
on that, so let's call free_cmds() again at the end.

Since "nr" follows the usual pattern of being set to 0 if we've
free()'d the memory already it's OK to call it twice, even in cases
where we are doing a "flush".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/cat-file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 50cf38999d1..ac0459f96be 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -655,6 +655,7 @@ static void batch_objects_command(struct batch_options *opt,
 		free_cmds(queued_cmd, &nr);
 	}
 
+	free_cmds(queued_cmd, &nr);
 	free(queued_cmd);
 	strbuf_release(&input);
 }
-- 
2.37.0.900.g4d0de1cceb2

[PATCH v2 06/11] merge-file: refactor for subsequent memory leak fix

[Ævar Arnfjörð Bjarmason]

Refactor the code in builtin/merge-file.c to:

 * Use the initializer to zero out "mmfs", and use modern C syntax for
   the rest.

 * Refactor the the inner loop to use a variable and "if/else if"
   pattern followed by "return". This will make a change to change it to
   a "goto cleanup" pattern smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/merge-file.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index e695867ee54..793817f3cb9 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -25,10 +25,10 @@ static int label_cb(const struct option *opt, const char *arg, int unset)
 
 int cmd_merge_file(int argc, const char **argv, const char *prefix)
 {
-	const char *names[3] = { NULL, NULL, NULL };
-	mmfile_t mmfs[3];
-	mmbuffer_t result = {NULL, 0};
-	xmparam_t xmp = {{0}};
+	const char *names[3] = { 0 };
+	mmfile_t mmfs[3] = { 0 };
+	mmbuffer_t result = { 0 };
+	xmparam_t xmp = { 0 };
 	int ret = 0, i = 0, to_stdout = 0;
 	int quiet = 0;
 	struct option options[] = {
@@ -71,21 +71,23 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 
 	for (i = 0; i < 3; i++) {
 		char *fname;
-		int ret;
+		mmfile_t *mmf = mmfs + i;
 
 		if (!names[i])
 			names[i] = argv[i];
 
 		fname = prefix_filename(prefix, argv[i]);
-		ret = read_mmfile(mmfs + i, fname);
+
+		if (read_mmfile(mmf, fname))
+			ret = -1;
+		else if (mmf->size > MAX_XDIFF_SIZE ||
+			 buffer_is_binary(mmf->ptr, mmf->size))
+			ret = error("Cannot merge binary files: %s",
+				    argv[i]);
+
 		free(fname);
 		if (ret)
-			return -1;
-
-		if (mmfs[i].size > MAX_XDIFF_SIZE ||
-		    buffer_is_binary(mmfs[i].ptr, mmfs[i].size))
-			return error("Cannot merge binary files: %s",
-					argv[i]);
+			return ret;
 	}
 
 	xmp.ancestor = names[1];
-- 
2.37.0.900.g4d0de1cceb2

[PATCH v2 07/11] merge-file: fix memory leaks on error path

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in "merge-file", we need to loop over the "mmfs"
array and free() what we've got so far when we error out. As a result
we can mark a test as passing with SANITIZE=leak using
"TEST_PASSES_SANITIZE_LEAK=true".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/merge-file.c  | 10 ++++++----
 t/t6403-merge-file.sh |  2 ++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index 793817f3cb9..c923bbf2abb 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -87,7 +87,8 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 
 		free(fname);
 		if (ret)
-			return ret;
+			goto cleanup;
+
 	}
 
 	xmp.ancestor = names[1];
@@ -95,9 +96,6 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	xmp.file2 = names[2];
 	ret = xdl_merge(mmfs + 1, mmfs + 0, mmfs + 2, &xmp, &result);
 
-	for (i = 0; i < 3; i++)
-		free(mmfs[i].ptr);
-
 	if (ret >= 0) {
 		const char *filename = argv[0];
 		char *fpath = prefix_filename(prefix, argv[0]);
@@ -118,5 +116,9 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	if (ret > 127)
 		ret = 127;
 
+cleanup:
+	for (i = 0; i < 3; i++)
+		free(mmfs[i].ptr);
+
 	return ret;
 }
diff --git a/t/t6403-merge-file.sh b/t/t6403-merge-file.sh
index 2f421d967ab..1a7082323dd 100755
--- a/t/t6403-merge-file.sh
+++ b/t/t6403-merge-file.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='RCS merge replacement: merge-file'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.37.0.900.g4d0de1cceb2

[PATCH v2 08/11] checkout: avoid "struct unpack_trees_options" leak

[Ævar Arnfjörð Bjarmason]

In 1c41d2805e4 (unpack_trees_options: free messages when done,
2018-05-21) we started calling clear_unpack_trees_porcelain() on this
codepath, but missed this error path.

We could call clear_unpack_trees_porcelain() just before we error()
and return when unmerged_cache() fails, but the more correct fix is to
not have the unmerged_cache() check happen in the middle of our
"topts" setup.

Before 23cbf11b5c0 (merge-recursive: porcelain messages for checkout,
2010-08-11) we would not malloc() to setup our "topts", which is when
this started to leak on the error path.

Before that this code wasn't conflating the setup of "topts" and the
unmerged_cache() call in any meaningful way. The initial version in
782c2d65c24 (Build in checkout, 2008-02-07) just does a "memset" of
it, and initializes a single struct member.

Then in 8ccba008ee3 (unpack-trees: allow Porcelain to give different
error messages, 2008-05-17) we added the initialization of the error
message, which as noted above finally started leaking in 23cbf11b5c0.

Let's fix the memory leak, and avoid future issues by initializing the
"topts" with a helper function. There are no functional changes here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/checkout.c | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2eefda81d8c..1109f1301f4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -710,6 +710,26 @@ static void setup_branch_path(struct branch_info *branch)
 	branch->path = strbuf_detach(&buf, NULL);
 }
 
+static void init_topts(struct unpack_trees_options *topts, int merge,
+		       int show_progress, int overwrite_ignore,
+		       struct commit *old_commit)
+{
+	memset(topts, 0, sizeof(*topts));
+	topts->head_idx = -1;
+	topts->src_index = &the_index;
+	topts->dst_index = &the_index;
+
+	setup_unpack_trees_porcelain(topts, "checkout");
+
+	topts->initial_checkout = is_cache_unborn();
+	topts->update = 1;
+	topts->merge = 1;
+	topts->quiet = merge && old_commit;
+	topts->verbose_update = show_progress;
+	topts->fn = twoway_merge;
+	topts->preserve_ignored = !overwrite_ignore;
+}
+
 static int merge_working_tree(const struct checkout_opts *opts,
 			      struct branch_info *old_branch_info,
 			      struct branch_info *new_branch_info,
@@ -740,13 +760,6 @@ static int merge_working_tree(const struct checkout_opts *opts,
 		struct unpack_trees_options topts;
 		const struct object_id *old_commit_oid;
 
-		memset(&topts, 0, sizeof(topts));
-		topts.head_idx = -1;
-		topts.src_index = &the_index;
-		topts.dst_index = &the_index;
-
-		setup_unpack_trees_porcelain(&topts, "checkout");
-
 		refresh_cache(REFRESH_QUIET);
 
 		if (unmerged_cache()) {
@@ -755,17 +768,12 @@ static int merge_working_tree(const struct checkout_opts *opts,
 		}
 
 		/* 2-way merge to the new branch */
-		topts.initial_checkout = is_cache_unborn();
-		topts.update = 1;
-		topts.merge = 1;
-		topts.quiet = opts->merge && old_branch_info->commit;
-		topts.verbose_update = opts->show_progress;
-		topts.fn = twoway_merge;
+		init_topts(&topts, opts->merge, opts->show_progress,
+			   opts->overwrite_ignore, old_branch_info->commit);
 		init_checkout_metadata(&topts.meta, new_branch_info->refname,
 				       new_branch_info->commit ?
 				       &new_branch_info->commit->object.oid :
 				       &new_branch_info->oid, NULL);
-		topts.preserve_ignored = !opts->overwrite_ignore;
 
 		old_commit_oid = old_branch_info->commit ?
 			&old_branch_info->commit->object.oid :
-- 
2.37.0.900.g4d0de1cceb2

Re: [PATCH v2 08/11] checkout: avoid "struct unpack_trees_options" leak

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 2eefda81d8c..1109f1301f4 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -710,6 +710,26 @@ static void setup_branch_path(struct branch_info *branch)
>  	branch->path = strbuf_detach(&buf, NULL);
>  }
>  
> +static void init_topts(struct unpack_trees_options *topts, int merge,
> +		       int show_progress, int overwrite_ignore,
> +		       struct commit *old_commit)
> +{
> +	memset(topts, 0, sizeof(*topts));
> +	topts->head_idx = -1;
> +	topts->src_index = &the_index;
> +	topts->dst_index = &the_index;
> +
> +	setup_unpack_trees_porcelain(topts, "checkout");
> +
> +	topts->initial_checkout = is_cache_unborn();
> +	topts->update = 1;
> +	topts->merge = 1;
> +	topts->quiet = merge && old_commit;
> +	topts->verbose_update = show_progress;
> +	topts->fn = twoway_merge;
> +	topts->preserve_ignored = !overwrite_ignore;
> +}
> +

I've already expressed my opinion on this step in my response to
[00/11], but I'd say we take the patch as-is.  It is pretty much Meh
between moving some code that is only used once to a helper function
that is called from only one places, and not doing so at all.  Once
the code is written in one way, it is not worth the bandwidth and
brain cycles to replace it with a variant, the difference with which
is mostly irrelevant.

THanks.

[PATCH v2 09/11] gc: fix a memory leak

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in code added in 41abfe15d95 (maintenance: add
pack-refs task, 2021-02-09), we need to call strvec_clear() on the
"struct strvec" that we initialized.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 021e9256ae2..eeff2b760e0 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -168,9 +168,15 @@ struct maintenance_run_opts;
 static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts)
 {
 	struct strvec pack_refs_cmd = STRVEC_INIT;
+	int ret;
+
 	strvec_pushl(&pack_refs_cmd, "pack-refs", "--all", "--prune", NULL);
 
-	return run_command_v_opt(pack_refs_cmd.v, RUN_GIT_CMD);
+	ret = run_command_v_opt(pack_refs_cmd.v, RUN_GIT_CMD);
+
+	strvec_clear(&pack_refs_cmd);
+
+	return ret;
 }
 
 static int too_many_loose_objects(void)
-- 
2.37.0.900.g4d0de1cceb2

[PATCH v2 10/11] cat-file: fix a common "struct object_context" memory leak

[Ævar Arnfjörð Bjarmason]

Fix a memory leak where "cat-file" will leak the "path" member. See
e5fba602e59 (textconv: support for cat_file, 2010-06-15) for the code
that introduced the offending get_oid_with_context() call (called
get_sha1_with_context() at the time).

As a result we can mark several tests as passing with SANITIZE=leak
using "TEST_PASSES_SANITIZE_LEAK=true".

As noted in dc944b65f1d (get_sha1_with_context: dynamically allocate
oc->path, 2017-05-19) callers must free the "path" member. That same
commit added the relevant free() to this function, but we weren't
catching cases where we'd return early.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/cat-file.c                   | 32 +++++++++++++++++++---------
 t/t0028-working-tree-encoding.sh     |  1 +
 t/t1051-large-conversion.sh          |  2 ++
 t/t3304-notes-mixed.sh               |  1 +
 t/t4044-diff-index-unique-abbrev.sh  |  2 ++
 t/t4140-apply-ita.sh                 |  1 +
 t/t5314-pack-cycle-detection.sh      |  4 ++--
 t/t6422-merge-rename-corner-cases.sh |  1 +
 t/t8007-cat-file-textconv.sh         |  2 ++
 t/t8010-cat-file-filters.sh          |  2 ++
 t/t9104-git-svn-follow-parent.sh     |  1 -
 t/t9132-git-svn-broken-symlink.sh    |  1 -
 t/t9301-fast-import-notes.sh         |  1 +
 13 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index ac0459f96be..f42782e955f 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -71,6 +71,7 @@ static int stream_blob(const struct object_id *oid)
 static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 			int unknown_type)
 {
+	int ret;
 	struct object_id oid;
 	enum object_type type;
 	char *buf;
@@ -106,7 +107,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 		if (sb.len) {
 			printf("%s\n", sb.buf);
 			strbuf_release(&sb);
-			return 0;
+			ret = 0;
+			goto cleanup;
 		}
 		break;
 
@@ -115,7 +117,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 		if (oid_object_info_extended(the_repository, &oid, &oi, flags) < 0)
 			die("git cat-file: could not get object info");
 		printf("%"PRIuMAX"\n", (uintmax_t)size);
-		return 0;
+		ret = 0;
+		goto cleanup;
 
 	case 'e':
 		return !has_object_file(&oid);
@@ -123,8 +126,10 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 	case 'w':
 
 		if (filter_object(path, obj_context.mode,
-				  &oid, &buf, &size))
-			return -1;
+				  &oid, &buf, &size)) {
+			ret = -1;
+			goto cleanup;
+		}
 		break;
 
 	case 'c':
@@ -143,11 +148,14 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 			const char *ls_args[3] = { NULL };
 			ls_args[0] =  "ls-tree";
 			ls_args[1] =  obj_name;
-			return cmd_ls_tree(2, ls_args, NULL);
+			ret = cmd_ls_tree(2, ls_args, NULL);
+			goto cleanup;
 		}
 
-		if (type == OBJ_BLOB)
-			return stream_blob(&oid);
+		if (type == OBJ_BLOB) {
+			ret = stream_blob(&oid);
+			goto cleanup;
+		}
 		buf = read_object_file(&oid, &type, &size);
 		if (!buf)
 			die("Cannot read object %s", obj_name);
@@ -172,8 +180,10 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 			} else
 				oidcpy(&blob_oid, &oid);
 
-			if (oid_object_info(the_repository, &blob_oid, NULL) == OBJ_BLOB)
-				return stream_blob(&blob_oid);
+			if (oid_object_info(the_repository, &blob_oid, NULL) == OBJ_BLOB) {
+				ret = stream_blob(&blob_oid);
+				goto cleanup;
+			}
 			/*
 			 * we attempted to dereference a tag to a blob
 			 * and failed; there may be new dereference
@@ -193,9 +203,11 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 		die("git cat-file %s: bad file", obj_name);
 
 	write_or_die(1, buf, size);
+	ret = 0;
+cleanup:
 	free(buf);
 	free(obj_context.path);
-	return 0;
+	return ret;
 }
 
 struct expand_data {
diff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh
index 82905a2156f..416eeabdb94 100755
--- a/t/t0028-working-tree-encoding.sh
+++ b/t/t0028-working-tree-encoding.sh
@@ -5,6 +5,7 @@ test_description='working-tree-encoding 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-encoding.sh"
 
diff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh
index 042b0e44292..f6709c9f569 100755
--- a/t/t1051-large-conversion.sh
+++ b/t/t1051-large-conversion.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test conversion filters on large files'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 set_attr() {
diff --git a/t/t3304-notes-mixed.sh b/t/t3304-notes-mixed.sh
index 03dfcd3954c..2c3a2452668 100755
--- a/t/t3304-notes-mixed.sh
+++ b/t/t3304-notes-mixed.sh
@@ -5,6 +5,7 @@ test_description='Test notes trees that also contain non-notes'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 number_of_commits=100
diff --git a/t/t4044-diff-index-unique-abbrev.sh b/t/t4044-diff-index-unique-abbrev.sh
index 4701796d10e..29e49d22902 100755
--- a/t/t4044-diff-index-unique-abbrev.sh
+++ b/t/t4044-diff-index-unique-abbrev.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test unique sha1 abbreviation on "index from..to" line'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t4140-apply-ita.sh b/t/t4140-apply-ita.sh
index c614eaf04cc..b375aca0d74 100755
--- a/t/t4140-apply-ita.sh
+++ b/t/t4140-apply-ita.sh
@@ -2,6 +2,7 @@
 
 test_description='git apply of i-t-a file'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t5314-pack-cycle-detection.sh b/t/t5314-pack-cycle-detection.sh
index 0aec8619e22..73a241743aa 100755
--- a/t/t5314-pack-cycle-detection.sh
+++ b/t/t5314-pack-cycle-detection.sh
@@ -49,9 +49,9 @@ Then no matter which order we start looking at the packs in, we know that we
 will always find a delta for "file", because its lookup will always come
 immediately after the lookup for "dummy".
 '
-. ./test-lib.sh
-
 
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
 
 # Create a pack containing the tree $1 and blob $1:file, with
 # the latter stored as a delta against $2:file.
diff --git a/t/t6422-merge-rename-corner-cases.sh b/t/t6422-merge-rename-corner-cases.sh
index bf4ce3c63d4..9b65768aed6 100755
--- a/t/t6422-merge-rename-corner-cases.sh
+++ b/t/t6422-merge-rename-corner-cases.sh
@@ -6,6 +6,7 @@ test_description="recursive merge corner cases w/ renames but not criss-crosses"
 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-merge.sh
 
diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh
index b067983ba1c..c8266f17f14 100755
--- a/t/t8007-cat-file-textconv.sh
+++ b/t/t8007-cat-file-textconv.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='git cat-file textconv support'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 cat >helper <<'EOF'
diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
index 31de4b64dc0..ca04242ca01 100755
--- a/t/t8010-cat-file-filters.sh
+++ b/t/t8010-cat-file-filters.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='git cat-file filters support'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup ' '
diff --git a/t/t9104-git-svn-follow-parent.sh b/t/t9104-git-svn-follow-parent.sh
index 5cf2ef4b8b0..85d735861fc 100755
--- a/t/t9104-git-svn-follow-parent.sh
+++ b/t/t9104-git-svn-follow-parent.sh
@@ -5,7 +5,6 @@
 
 test_description='git svn fetching'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 test_expect_success 'initialize repo' '
diff --git a/t/t9132-git-svn-broken-symlink.sh b/t/t9132-git-svn-broken-symlink.sh
index 4d8d0584b79..aeceffaf7b0 100755
--- a/t/t9132-git-svn-broken-symlink.sh
+++ b/t/t9132-git-svn-broken-symlink.sh
@@ -2,7 +2,6 @@
 
 test_description='test that git handles an svn repository with empty symlinks'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 test_expect_success 'load svn dumpfile' '
 	svnadmin load "$rawsvnrepo" <<EOF
diff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh
index 1ae4d7c0d37..58413221e6a 100755
--- a/t/t9301-fast-import-notes.sh
+++ b/t/t9301-fast-import-notes.sh
@@ -7,6 +7,7 @@ test_description='test git fast-import of notes objects'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 
-- 
2.37.0.900.g4d0de1cceb2

[PATCH v2 11/11] pull: fix a "struct oid_array" memory leak

[Ævar Arnfjörð Bjarmason]

Fix a memory leak introduced in 44c175c7a46 (pull: error on no merge
candidates, 2015-06-18). As a result we can mark several tests as
passing with SANITIZE=leak using "TEST_PASSES_SANITIZE_LEAK=true".

Removing the "int ret = 0" assignment added here in a6d7eb2c7a6 (pull:
optionally rebase submodules (remote submodule changes only),
2017-06-23) is not a logic error, it could always have been left
uninitialized (as "int ret"), now that we'll use the "ret" from the
upper scope we can drop the assignment in the "opt_rebase" branch.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/pull.c               | 16 ++++++++++------
 t/t5524-pull-msg.sh          |  1 +
 t/t6417-merge-ours-theirs.sh |  1 +
 t/t9101-git-svn-props.sh     |  1 -
 4 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 01155ba67b2..403a24d7ca6 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -990,6 +990,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	int rebase_unspecified = 0;
 	int can_ff;
 	int divergent;
+	int ret;
 
 	if (!getenv("GIT_REFLOG_ACTION"))
 		set_reflog_message(argc, argv);
@@ -1100,7 +1101,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (is_null_oid(&orig_head)) {
 		if (merge_heads.nr > 1)
 			die(_("Cannot merge multiple branches into empty head."));
-		return pull_into_void(merge_heads.oid, &curr_head);
+		ret = pull_into_void(merge_heads.oid, &curr_head);
+		goto cleanup;
 	}
 	if (merge_heads.nr > 1) {
 		if (opt_rebase)
@@ -1125,8 +1127,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	}
 
 	if (opt_rebase) {
-		int ret = 0;
-
 		struct object_id newbase;
 		struct object_id upstream;
 		get_rebase_newbase_and_upstream(&newbase, &upstream, &curr_head,
@@ -1149,12 +1149,16 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
 			ret = rebase_submodules();
 
-		return ret;
+		goto cleanup;
 	} else {
-		int ret = run_merge();
+		ret = run_merge();
 		if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
 			     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
 			ret = update_submodules();
-		return ret;
+		goto cleanup;
 	}
+
+cleanup:
+	oid_array_clear(&merge_heads);
+	return ret;
 }
diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
index b2be3605f5a..56716e29ddf 100755
--- a/t/t5524-pull-msg.sh
+++ b/t/t5524-pull-msg.sh
@@ -2,6 +2,7 @@
 
 test_description='git pull message generation'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 dollar='$Dollar'
diff --git a/t/t6417-merge-ours-theirs.sh b/t/t6417-merge-ours-theirs.sh
index 62d1406119e..482b73a998f 100755
--- a/t/t6417-merge-ours-theirs.sh
+++ b/t/t6417-merge-ours-theirs.sh
@@ -4,6 +4,7 @@ test_description='Merge-recursive ours and theirs variants'
 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/t9101-git-svn-props.sh b/t/t9101-git-svn-props.sh
index d043e80fc34..52046e60d51 100755
--- a/t/t9101-git-svn-props.sh
+++ b/t/t9101-git-svn-props.sh
@@ -5,7 +5,6 @@
 
 test_description='git svn property tests'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 mkdir import
-- 
2.37.0.900.g4d0de1cceb2

Re: [PATCH v2 00/11] built-ins: fix common memory leaks

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>  * Replace 8/11, maybe that solution is going overboard, we could also
>    just drop it from this series...

The "solution" part is to forbid merge_working_tree() to use
anything that relies on the topts before it bails out due to
unmerged_cache(), that could be done with only a minor reordering of
the logic.

And I think that is a reasonable thing to do---if we are looking at
an unmerged index, we would not want to run unpack-trees on it at
all, so setting up the topts structure does not make much sense
before we fully know we cannot bail out early.

On the other hand, it does make the patch go near overboard to
extract the topt initialization logic for this single user into a
static helper function.  If we later find out that my "that is a
reasonable thing to do" above is not true, or some setting of topts
members need to become conditional, we would probably need to undo
that part of this patch.

The latter "going overboard" part is the majority of change.  We
definitely should *not* discard the essence of this step to stop
leaking, but should think about stopping at the smaller change,
perhaps?

Having said that, all the other patches in the series looked good.

Thanks, will queue.