[PATCH 00/14] tree-wide: small fixes for memory leaks

[PATCH 00/14] tree-wide: small fixes for memory leaks

[Ævar Arnfjörð Bjarmason]

This is a collection of various otherwise unrelated tree-wide fixes
for memory leaks.

In fixing more targeted memory leaks in specific areas I've run into
small leaks here & there. Rather than submit e.g. a 2-series patch for
just small bundle leaks, the same for range-diff etc. I thought it
made sense to submit these together.

Ævar Arnfjörð Bjarmason (14):
  index-pack: fix memory leaks
  merge-base: free() allocated "struct commit **" list
  diff.c: free "buf" in diff_words_flush()
  urlmatch.c: add and use a *_release() function
  remote-curl.c: free memory in cmd_main()
  bundle: call strvec_clear() on allocated strvec
  transport: stop needlessly copying bundle header references
  submodule--helper: fix trivial leak in module_add()
  commit-graph: fix memory leak in misused string_list API
  commit-graph: stop fill_oids_from_packs() progress on error and free()
  lockfile API users: simplify and don't leak "path"
  range-diff: plug memory leak in common invocation
  range-diff: plug memory leak in read_patches()
  repository.c: free the "path cache" in repo_clear()

 apply.c                     |  7 ++++++-
 apply.h                     |  2 ++
 builtin/bundle.c            |  1 +
 builtin/commit-graph.c      |  6 +++---
 builtin/config.c            |  2 +-
 builtin/index-pack.c        |  5 +++++
 builtin/merge-base.c        |  5 ++++-
 builtin/sparse-checkout.c   |  3 +--
 builtin/submodule--helper.c |  5 ++++-
 commit-graph.c              | 18 +++++++++++-------
 commit-graph.h              |  2 +-
 credential.c                |  1 +
 diff.c                      |  1 +
 path.h                      | 14 --------------
 range-diff.c                | 30 +++++++++++++-----------------
 remote-curl.c               | 12 ++++++++----
 repository.c                | 16 ++++++++++++++++
 repository.h                | 14 +++++++++++++-
 transport.c                 | 27 ++++++++++++++++-----------
 urlmatch.c                  |  5 +++++
 urlmatch.h                  |  1 +
 21 files changed, 113 insertions(+), 64 deletions(-)

-- 
2.35.1.1228.g56895c6ee86

[PATCH 01/14] index-pack: fix memory leaks

[Ævar Arnfjörð Bjarmason]

Fix various memory leaks in "git index-pack", due to how tightly
coupled this command is with the revision walking this doesn't make
any new tests pass, but e.g. this now passes, and had several failures before:

    ./t5300-pack-object.sh --run=1-2,4,6-27,30-42

it is a bit odd that we'll free "opts.anomaly", since the "opts" is a
"struct pack_idx_option" declared in pack.h. In pack-write.c there's a
reset_pack_idx_option(), but it only wipes the contents, but doesn't
free() anything.

Doing this here in cmd_index_pack() is correct because while the
struct is declared in pack.h, this code in builtin/index-pack.c (in
read_v2_anomalous_offsets()) is what allocates the "opts.anomaly", so
we should also free it here.

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

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 3c2e6aee3cc..5fe1adb3681 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1109,6 +1109,7 @@ static void *threaded_second_pass(void *data)
 			list_add(&child->list, &work_head);
 			base_cache_used += child->size;
 			prune_base_data(NULL);
+			free_base_data(child);
 		} else {
 			/*
 			 * This child does not have its own children. It may be
@@ -1131,6 +1132,7 @@ static void *threaded_second_pass(void *data)
 
 				p = next_p;
 			}
+			FREE_AND_NULL(child);
 		}
 		work_unlock();
 	}
@@ -1424,6 +1426,7 @@ static void fix_unresolved_deltas(struct hashfile *f)
 		 * object).
 		 */
 		append_obj_to_pack(f, d->oid.hash, data, size, type);
+		free(data);
 		threaded_second_pass(NULL);
 
 		display_progress(progress, nr_resolved_deltas);
@@ -1703,6 +1706,7 @@ static void show_pack_info(int stat_only)
 			  i + 1,
 			  chain_histogram[i]);
 	}
+	free(chain_histogram);
 }
 
 int cmd_index_pack(int argc, const char **argv, const char *prefix)
@@ -1932,6 +1936,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 	if (do_fsck_object && fsck_finish(&fsck_options))
 		die(_("fsck error in pack objects"));
 
+	free(opts.anomaly);
 	free(objects);
 	strbuf_release(&index_name_buf);
 	strbuf_release(&rev_index_name_buf);
-- 
2.35.1.1228.g56895c6ee86

Re: [PATCH 01/14] index-pack: fix memory leaks

[Derrick Stolee]

On 3/2/2022 12:10 PM, Ævar Arnfjörð Bjarmason wrote:
> Fix various memory leaks in "git index-pack", due to how tightly
> coupled this command is with the revision walking this doesn't make
> any new tests pass, but e.g. this now passes, and had several failures before:
> 
>     ./t5300-pack-object.sh --run=1-2,4,6-27,30-42

Do you mean that these tests now pass under leak check?
 
> it is a bit odd that we'll free "opts.anomaly", since the "opts" is a

s/it/It/

> "struct pack_idx_option" declared in pack.h. In pack-write.c there's a
> reset_pack_idx_option(), but it only wipes the contents, but doesn't
> free() anything.
> 
> Doing this here in cmd_index_pack() is correct because while the
> struct is declared in pack.h, this code in builtin/index-pack.c (in
> read_v2_anomalous_offsets()) is what allocates the "opts.anomaly", so
> we should also free it here.

Makes sense. Code diff looks good.

Thanks,
-Stolee

Re: [PATCH 01/14] index-pack: fix memory leaks

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 02 2022, Derrick Stolee wrote:

> On 3/2/2022 12:10 PM, Ævar Arnfjörð Bjarmason wrote:
>> Fix various memory leaks in "git index-pack", due to how tightly
>> coupled this command is with the revision walking this doesn't make
>> any new tests pass, but e.g. this now passes, and had several failures before:
>> 
>>     ./t5300-pack-object.sh --run=1-2,4,6-27,30-42
>
> Do you mean that these tests now pass under leak check?

Yes, i.e. test 3, 5 etc. fails before & after, and this change makes
tests in these ranges pass.

>> it is a bit odd that we'll free "opts.anomaly", since the "opts" is a
>
> s/it/It/

Thanks, will fix!

>> "struct pack_idx_option" declared in pack.h. In pack-write.c there's a
>> reset_pack_idx_option(), but it only wipes the contents, but doesn't
>> free() anything.
>> 
>> Doing this here in cmd_index_pack() is correct because while the
>> struct is declared in pack.h, this code in builtin/index-pack.c (in
>> read_v2_anomalous_offsets()) is what allocates the "opts.anomaly", so
>> we should also free it here.
>
> Makes sense. Code diff looks good.

Thanks!

[PATCH 02/14] merge-base: free() allocated "struct commit **" list

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in 53eda89b2fe (merge-base: teach "git merge-base"
to drive underlying merge_bases_many(), 2008-07-30) by calling free()
on the "struct commit **" list used by "git merge-base".

This gets e.g. "t6010-merge-base.sh" closer to passing under
SANITIZE=leak, it failed 8 tests before when compiled with that
option, and now fails only 5 tests.

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

diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index 26b84980dbd..a11f8c6e4bb 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -138,6 +138,7 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix)
 	int rev_nr = 0;
 	int show_all = 0;
 	int cmdmode = 0;
+	int ret;
 
 	struct option options[] = {
 		OPT_BOOL('a', "all", &show_all, N_("output all common ancestors")),
@@ -186,5 +187,7 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix)
 	ALLOC_ARRAY(rev, argc);
 	while (argc-- > 0)
 		rev[rev_nr++] = get_commit_reference(*argv++);
-	return show_merge_base(rev, rev_nr, show_all);
+	ret = show_merge_base(rev, rev_nr, show_all);
+	free(rev);
+	return ret;
 }
-- 
2.35.1.1228.g56895c6ee86

[PATCH 03/14] diff.c: free "buf" in diff_words_flush()

[Ævar Arnfjörð Bjarmason]

Amend the freeing logic added in e6e045f8031 (diff.c: buffer all
output if asked to, 2017-06-29) to free the containing "buf" in
addition to its members.

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

diff --git a/diff.c b/diff.c
index c4ccb6b1a34..c5bc9bc5128 100644
--- a/diff.c
+++ b/diff.c
@@ -2150,6 +2150,7 @@ static void diff_words_flush(struct emit_callback *ecbdata)
 
 		for (i = 0; i < wol->nr; i++)
 			free((void *)wol->buf[i].line);
+		free(wol->buf);
 
 		wol->nr = 0;
 	}
-- 
2.35.1.1228.g56895c6ee86

[PATCH 04/14] urlmatch.c: add and use a *_release() function

[Ævar Arnfjörð Bjarmason]

Plug a memory leak in credential_apply_config() by adding and using a
new urlmatch_config_release() function. This just does a
string_list_clear() on the "vars" member.

This finished up work on normalizing the init/free pattern in this
API, started in 73ee449bbf2 (urlmatch.[ch]: add and use
URLMATCH_CONFIG_INIT, 2021-10-01).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/config.c | 2 +-
 credential.c     | 1 +
 urlmatch.c       | 5 +++++
 urlmatch.h       | 1 +
 4 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/builtin/config.c b/builtin/config.c
index 542d8d02b2b..ebec61868be 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -612,7 +612,7 @@ static int get_urlmatch(const char *var, const char *url)
 
 		strbuf_release(&matched->value);
 	}
-	string_list_clear(&config.vars, 1);
+	urlmatch_config_release(&config);
 	string_list_clear(&values, 1);
 	free(config.url.url);
 
diff --git a/credential.c b/credential.c
index e7240f3f636..f6389a50684 100644
--- a/credential.c
+++ b/credential.c
@@ -130,6 +130,7 @@ static void credential_apply_config(struct credential *c)
 	git_config(urlmatch_config_entry, &config);
 	string_list_clear(&config.vars, 1);
 	free(normalized_url);
+	urlmatch_config_release(&config);
 	strbuf_release(&url);
 
 	c->configured = 1;
diff --git a/urlmatch.c b/urlmatch.c
index 03ad3f30a9c..b615adc923a 100644
--- a/urlmatch.c
+++ b/urlmatch.c
@@ -611,3 +611,8 @@ int urlmatch_config_entry(const char *var, const char *value, void *cb)
 	strbuf_release(&synthkey);
 	return retval;
 }
+
+void urlmatch_config_release(struct urlmatch_config *config)
+{
+	string_list_clear(&config->vars, 1);
+}
diff --git a/urlmatch.h b/urlmatch.h
index 34a3ba6d197..9f40b00bfb8 100644
--- a/urlmatch.h
+++ b/urlmatch.h
@@ -71,5 +71,6 @@ struct urlmatch_config {
 }
 
 int urlmatch_config_entry(const char *var, const char *value, void *cb);
+void urlmatch_config_release(struct urlmatch_config *config);
 
 #endif /* URL_MATCH_H */
-- 
2.35.1.1228.g56895c6ee86

[PATCH 05/14] remote-curl.c: free memory in cmd_main()

[Ævar Arnfjörð Bjarmason]

Plug a trivial memory leak in code added in a2d725b7bdf (Use an
external program to implement fetching with curl, 2009-08-05).

To do this have the cmd_main() use a "goto cleanup" pattern, and to
return an error of 1 unless we can fall through to the http_cleanup()
at the end.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 remote-curl.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 0dabef2dd7c..ff44f41011e 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1472,11 +1472,12 @@ int cmd_main(int argc, const char **argv)
 {
 	struct strbuf buf = STRBUF_INIT;
 	int nongit;
+	int ret = 1;
 
 	setup_git_directory_gently(&nongit);
 	if (argc < 2) {
 		error(_("remote-curl: usage: git remote-curl <remote> [<url>]"));
-		return 1;
+		goto cleanup;
 	}
 
 	options.verbosity = 1;
@@ -1508,7 +1509,7 @@ int cmd_main(int argc, const char **argv)
 		if (strbuf_getline_lf(&buf, stdin) == EOF) {
 			if (ferror(stdin))
 				error(_("remote-curl: error reading command stream from git"));
-			return 1;
+			goto cleanup;
 		}
 		if (buf.len == 0)
 			break;
@@ -1556,12 +1557,15 @@ int cmd_main(int argc, const char **argv)
 				break;
 		} else {
 			error(_("remote-curl: unknown command '%s' from git"), buf.buf);
-			return 1;
+			goto cleanup;
 		}
 		strbuf_reset(&buf);
 	} while (1);
 
 	http_cleanup();
+	ret = 0;
+cleanup:
+	strbuf_release(&buf);
 
-	return 0;
+	return ret;
 }
-- 
2.35.1.1228.g56895c6ee86

[PATCH 06/14] bundle: call strvec_clear() on allocated strvec

[Ævar Arnfjörð Bjarmason]

Fixing this small memory leak in cmd_bundle_create() gets
"t5607-clone-bundle.sh" closer to passing under SANITIZE=leak.

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

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 5a85d7cd0fe..2adad545a2e 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -93,6 +93,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 	if (!startup_info->have_repository)
 		die(_("Need a repository to create a bundle."));
 	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
+	strvec_clear(&pack_opts);
 	free(bundle_file);
 	return ret;
 }
-- 
2.35.1.1228.g56895c6ee86

[PATCH 07/14] transport: stop needlessly copying bundle header references

[Ævar Arnfjörð Bjarmason]

Amend the logic added in fddf2ebe388 (transport: teach all vtables to
allow fetch first, 2019-08-21) and save ourselves pointless work in
fetch_refs_from_bundle().

The fetch_refs_from_bundle() caller doesn't care about the "struct
ref *result" return value of get_refs_from_bundle(), and doesn't need
any of the work we were doing in looping over the
"data->header.references" in get_refs_from_bundle().

So this change saves us work, and also fixes a memory leak that we had
when called from fetch_refs_from_bundle(). The other caller of
get_refs_from_bundle() is the "get_refs_list" member we set up for the
"struct transport_vtable bundle_vtable". That caller does care about
the "struct ref *result" return value.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 transport.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/transport.c b/transport.c
index 253d6671b1f..9a7d32b4b7d 100644
--- a/transport.c
+++ b/transport.c
@@ -125,6 +125,19 @@ struct bundle_transport_data {
 	unsigned get_refs_from_bundle_called : 1;
 };
 
+static void get_refs_from_bundle_inner(struct transport *transport)
+{
+	struct bundle_transport_data *data = transport->data;
+
+	if (data->fd > 0)
+		close(data->fd);
+	data->fd = read_bundle_header(transport->url, &data->header);
+	if (data->fd < 0)
+		die(_("could not read bundle '%s'"), transport->url);
+
+	transport->hash_algo = data->header.hash_algo;
+}
+
 static struct ref *get_refs_from_bundle(struct transport *transport,
 					int for_push,
 					struct transport_ls_refs_options *transport_options)
@@ -136,15 +149,7 @@ static struct ref *get_refs_from_bundle(struct transport *transport,
 	if (for_push)
 		return NULL;
 
-	data->get_refs_from_bundle_called = 1;
-
-	if (data->fd > 0)
-		close(data->fd);
-	data->fd = read_bundle_header(transport->url, &data->header);
-	if (data->fd < 0)
-		die(_("could not read bundle '%s'"), transport->url);
-
-	transport->hash_algo = data->header.hash_algo;
+	get_refs_from_bundle_inner(transport);
 
 	for (i = 0; i < data->header.references.nr; i++) {
 		struct string_list_item *e = data->header.references.items + i;
@@ -168,8 +173,8 @@ static int fetch_refs_from_bundle(struct transport *transport,
 	if (transport->progress)
 		strvec_push(&extra_index_pack_args, "-v");
 
-	if (!data->get_refs_from_bundle_called)
-		get_refs_from_bundle(transport, 0, NULL);
+	if (!data->get_refs_from_bundle_called++)
+		get_refs_from_bundle_inner(transport);
 	ret = unbundle(the_repository, &data->header, data->fd,
 		       &extra_index_pack_args);
 	transport->hash_algo = data->header.hash_algo;
-- 
2.35.1.1228.g56895c6ee86

Re: [PATCH 07/14] transport: stop needlessly copying bundle header references

[Derrick Stolee]

On 3/2/2022 12:10 PM, Ævar Arnfjörð Bjarmason wrote:
> Amend the logic added in fddf2ebe388 (transport: teach all vtables to
> allow fetch first, 2019-08-21) and save ourselves pointless work in
> fetch_refs_from_bundle().
...
> +static void get_refs_from_bundle_inner(struct transport *transport)
> +{
> +	struct bundle_transport_data *data = transport->data;
> +
> +	if (data->fd > 0)
> +		close(data->fd);
> +	data->fd = read_bundle_header(transport->url, &data->header);
> +	if (data->fd < 0)
> +		die(_("could not read bundle '%s'"), transport->url);
> +
> +	transport->hash_algo = data->header.hash_algo;
> +}
> +

There are some subtle changes here that look odd, but it actually
could present a behavior change.

>  static struct ref *get_refs_from_bundle(struct transport *transport,
>  					int for_push,
>  					struct transport_ls_refs_options *transport_options)
> @@ -136,15 +149,7 @@ static struct ref *get_refs_from_bundle(struct transport *transport,
>  	if (for_push)
>  		return NULL;
>  
> -	data->get_refs_from_bundle_called = 1;
> -
> -	if (data->fd > 0)
> -		close(data->fd);
> -	data->fd = read_bundle_header(transport->url, &data->header);
> -	if (data->fd < 0)
> -		die(_("could not read bundle '%s'"), transport->url);
> -
> -	transport->hash_algo = data->header.hash_algo;
> +	get_refs_from_bundle_inner(transport);

The implementation of get_refs_from_bundle_inner() is very close
to these deleted lines, except you removed

	data->get_refs_from_bundle_called = 1;

and instead you added this change:

> -	if (!data->get_refs_from_bundle_called)
> -		get_refs_from_bundle(transport, 0, NULL);
> +	if (!data->get_refs_from_bundle_called++)
> +		get_refs_from_bundle_inner(transport);

So, this is checking to see if data->get_refs_from_bundle_called
_was_ zero while also incrementing it. However, this member is
an unsigned:1, so you're really toggling it on and off each time
we reach this 'if' statement.

Using this "++" would still bother me stylistically, even if the
type was uint64_t and the risk of overflow was minimal. If you
put the "= 1" line into the inner method, then things go back to
working as they did before.

Thanks,
-Stolee

Re: [PATCH 07/14] transport: stop needlessly copying bundle header references

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 02 2022, Derrick Stolee wrote:

> On 3/2/2022 12:10 PM, Ævar Arnfjörð Bjarmason wrote:
>> Amend the logic added in fddf2ebe388 (transport: teach all vtables to
>> allow fetch first, 2019-08-21) and save ourselves pointless work in
>> fetch_refs_from_bundle().
> ...
>> +static void get_refs_from_bundle_inner(struct transport *transport)
>> +{
>> +	struct bundle_transport_data *data = transport->data;
>> +
>> +	if (data->fd > 0)
>> +		close(data->fd);
>> +	data->fd = read_bundle_header(transport->url, &data->header);
>> +	if (data->fd < 0)
>> +		die(_("could not read bundle '%s'"), transport->url);
>> +
>> +	transport->hash_algo = data->header.hash_algo;
>> +}
>> +
>
> There are some subtle changes here that look odd, but it actually
> could present a behavior change.
>
>>  static struct ref *get_refs_from_bundle(struct transport *transport,
>>  					int for_push,
>>  					struct transport_ls_refs_options *transport_options)
>> @@ -136,15 +149,7 @@ static struct ref *get_refs_from_bundle(struct transport *transport,
>>  	if (for_push)
>>  		return NULL;
>>  
>> -	data->get_refs_from_bundle_called = 1;
>> -
>> -	if (data->fd > 0)
>> -		close(data->fd);
>> -	data->fd = read_bundle_header(transport->url, &data->header);
>> -	if (data->fd < 0)
>> -		die(_("could not read bundle '%s'"), transport->url);
>> -
>> -	transport->hash_algo = data->header.hash_algo;
>> +	get_refs_from_bundle_inner(transport);
>
> The implementation of get_refs_from_bundle_inner() is very close
> to these deleted lines, except you removed
>
> 	data->get_refs_from_bundle_called = 1;
>
> and instead you added this change:
>
>> -	if (!data->get_refs_from_bundle_called)
>> -		get_refs_from_bundle(transport, 0, NULL);
>> +	if (!data->get_refs_from_bundle_called++)
>> +		get_refs_from_bundle_inner(transport);
>
> So, this is checking to see if data->get_refs_from_bundle_called
> _was_ zero while also incrementing it. However, this member is
> an unsigned:1, so you're really toggling it on and off each time
> we reach this 'if' statement.
>
> Using this "++" would still bother me stylistically, even if the
> type was uint64_t and the risk of overflow was minimal. If you
> put the "= 1" line into the inner method, then things go back to
> working as they did before.

Ouch, well spotted! Will fix.

[PATCH 08/14] submodule--helper: fix trivial leak in module_add()

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in code added in a6226fd772b (submodule--helper:
convert the bulk of cmd_add() to C, 2021-08-10). If "realrepo" isn't a
copy of the "repo" member we should free() it.

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index eeacefcc383..13b4841327d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -3309,6 +3309,7 @@ static int module_add(int argc, const char **argv, const char *prefix)
 {
 	int force = 0, quiet = 0, progress = 0, dissociate = 0;
 	struct add_data add_data = ADD_DATA_INIT;
+	char *to_free = NULL;
 
 	struct option options[] = {
 		OPT_STRING('b', "branch", &add_data.branch, N_("branch"),
@@ -3360,7 +3361,8 @@ static int module_add(int argc, const char **argv, const char *prefix)
 			      "of the working tree"));
 
 		/* dereference source url relative to parent's url */
-		add_data.realrepo = resolve_relative_url(add_data.repo, NULL, 1);
+		to_free = resolve_relative_url(add_data.repo, NULL, 1);
+		add_data.realrepo = to_free;
 	} else if (is_dir_sep(add_data.repo[0]) || strchr(add_data.repo, ':')) {
 		add_data.realrepo = add_data.repo;
 	} else {
@@ -3413,6 +3415,7 @@ static int module_add(int argc, const char **argv, const char *prefix)
 	}
 	configure_added_submodule(&add_data);
 	free(add_data.sm_path);
+	free(to_free);
 
 	return 0;
 }
-- 
2.35.1.1228.g56895c6ee86

[PATCH 09/14] commit-graph: fix memory leak in misused string_list API

[Ævar Arnfjörð Bjarmason]

When this code was migrated to the string_list API in
d88b14b3fd6 (commit-graph: use string-list API for input, 2018-06-27)
it was made to use used both STRING_LIST_INIT_NODUP and a
strbuf_detach() pattern.

Those should not be used together if string_list_clear() is expected
to free the memory, instead we need to either use STRING_LIST_INIT_DUP
with a string_list_append_nodup(), or a STRING_LIST_INIT_NODUP and
manually fiddle with the "strdup_strings" member before calling
string_list_clear(). Let's do the former.

Since "strdup_strings = 1" is set now other code might be broken by
relying on "pack_indexes" not to duplicate it strings, but that
doesn't happen. When we pass this down to write_commit_graph() that
code uses the "struct string_list" without modifying it. Let's add a
"const" to the variable to have the compiler enforce that assumption.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/commit-graph.c | 6 +++---
 commit-graph.c         | 4 ++--
 commit-graph.h         | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 4247fbde95a..51c4040ea6c 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -192,7 +192,7 @@ static int git_commit_graph_write_config(const char *var, const char *value,
 
 static int graph_write(int argc, const char **argv)
 {
-	struct string_list pack_indexes = STRING_LIST_INIT_NODUP;
+	struct string_list pack_indexes = STRING_LIST_INIT_DUP;
 	struct strbuf buf = STRBUF_INIT;
 	struct oidset commits = OIDSET_INIT;
 	struct object_directory *odb = NULL;
@@ -273,8 +273,8 @@ static int graph_write(int argc, const char **argv)
 
 	if (opts.stdin_packs) {
 		while (strbuf_getline(&buf, stdin) != EOF)
-			string_list_append(&pack_indexes,
-					   strbuf_detach(&buf, NULL));
+			string_list_append_nodup(&pack_indexes,
+						 strbuf_detach(&buf, NULL));
 	} else if (opts.stdin_commits) {
 		oidset_init(&commits, 0);
 		if (opts.progress)
diff --git a/commit-graph.c b/commit-graph.c
index 265c010122e..d0c94600bab 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1679,7 +1679,7 @@ int write_commit_graph_reachable(struct object_directory *odb,
 }
 
 static int fill_oids_from_packs(struct write_commit_graph_context *ctx,
-				struct string_list *pack_indexes)
+				const struct string_list *pack_indexes)
 {
 	uint32_t i;
 	struct strbuf progress_title = STRBUF_INIT;
@@ -2259,7 +2259,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)
 }
 
 int write_commit_graph(struct object_directory *odb,
-		       struct string_list *pack_indexes,
+		       const struct string_list *const pack_indexes,
 		       struct oidset *commits,
 		       enum commit_graph_write_flags flags,
 		       const struct commit_graph_opts *opts)
diff --git a/commit-graph.h b/commit-graph.h
index 04a94e18302..2e3ac35237e 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -142,7 +142,7 @@ int write_commit_graph_reachable(struct object_directory *odb,
 				 enum commit_graph_write_flags flags,
 				 const struct commit_graph_opts *opts);
 int write_commit_graph(struct object_directory *odb,
-		       struct string_list *pack_indexes,
+		       const struct string_list *pack_indexes,
 		       struct oidset *commits,
 		       enum commit_graph_write_flags flags,
 		       const struct commit_graph_opts *opts);
-- 
2.35.1.1228.g56895c6ee86

[PATCH 10/14] commit-graph: stop fill_oids_from_packs() progress on error and free()

[Ævar Arnfjörð Bjarmason]

Fix a bug in fill_oids_from_packs(), we should always stop_progress(),
but did not do so if we returned an error here. This also plugs a
memory leak in those cases by releasing the two "struct strbuf"
variables the function uses.

While I'm at it stop hardcoding "-1" here and just use the return
value of error() instead, which happens to be "-1".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 commit-graph.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index d0c94600bab..aab0b292774 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1685,6 +1685,7 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,
 	struct strbuf progress_title = STRBUF_INIT;
 	struct strbuf packname = STRBUF_INIT;
 	int dirlen;
+	int ret = 0;
 
 	strbuf_addf(&packname, "%s/pack/", ctx->odb->path);
 	dirlen = packname.len;
@@ -1703,12 +1704,12 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,
 		strbuf_addstr(&packname, pack_indexes->items[i].string);
 		p = add_packed_git(packname.buf, packname.len, 1);
 		if (!p) {
-			error(_("error adding pack %s"), packname.buf);
-			return -1;
+			ret = error(_("error adding pack %s"), packname.buf);
+			goto cleanup;
 		}
 		if (open_pack_index(p)) {
-			error(_("error opening index for %s"), packname.buf);
-			return -1;
+			ret = error(_("error opening index for %s"), packname.buf);
+			goto cleanup;
 		}
 		for_each_object_in_pack(p, add_packed_commits, ctx,
 					FOR_EACH_OBJECT_PACK_ORDER);
@@ -1716,11 +1717,12 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,
 		free(p);
 	}
 
+cleanup:
 	stop_progress(&ctx->progress);
 	strbuf_release(&progress_title);
 	strbuf_release(&packname);
 
-	return 0;
+	return ret;
 }
 
 static int fill_oids_from_commits(struct write_commit_graph_context *ctx,
-- 
2.35.1.1228.g56895c6ee86

[PATCH 11/14] lockfile API users: simplify and don't leak "path"

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in code added in 6c622f9f0bb (commit-graph: write
commit-graph chains, 2019-06-18). We needed to free the "lock_name" if
we encounter errors, and the "graph_name" after we'd run unlink() on
it.

For the case of write_commit_graph_file() refactoring the code to free
the "lock_name" after we were done using the "struct lock_file lk"
would have made the control flow more complex. Luckily we can free the
"lock_file" right after the hold_lock_file_for_update() call, if it
makes use of "path" at all it'll have copied its contents to a "struct
strbuf" of its own.

While I'm at it let's fix code added in fb10ca5b543 (sparse-checkout:
write using lockfile, 2019-11-21) in write_patterns_and_update() to
avoid the same complexity that I thought I needed when I wrote the
initial fix for write_commit_graph_file(). We can free the
"sparse_filename" right after calling hold_lock_file_for_update(), we
don't need to wait until we're exiting the function to do so.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/sparse-checkout.c | 3 +--
 commit-graph.c            | 2 ++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 9c338d33ea2..270ad49c2b8 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -328,11 +328,11 @@ static int write_patterns_and_update(struct pattern_list *pl)
 
 	fd = hold_lock_file_for_update(&lk, sparse_filename,
 				      LOCK_DIE_ON_ERROR);
+	free(sparse_filename);
 
 	result = update_working_directory(pl);
 	if (result) {
 		rollback_lock_file(&lk);
-		free(sparse_filename);
 		clear_pattern_list(pl);
 		update_working_directory(NULL);
 		return result;
@@ -348,7 +348,6 @@ static int write_patterns_and_update(struct pattern_list *pl)
 	fflush(fp);
 	commit_lock_file(&lk);
 
-	free(sparse_filename);
 	clear_pattern_list(pl);
 
 	return 0;
diff --git a/commit-graph.c b/commit-graph.c
index aab0b292774..b8cde7ea27d 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1854,6 +1854,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 
 		hold_lock_file_for_update_mode(&lk, lock_name,
 					       LOCK_DIE_ON_ERROR, 0444);
+		free(lock_name);
 
 		fd = git_mkstemp_mode(ctx->graph_name, 0444);
 		if (fd < 0) {
@@ -1978,6 +1979,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 		} else {
 			char *graph_name = get_commit_graph_filename(ctx->odb);
 			unlink(graph_name);
+			free(graph_name);
 		}
 
 		ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1] = xstrdup(hash_to_hex(file_hash));
-- 
2.35.1.1228.g56895c6ee86

[PATCH 12/14] range-diff: plug memory leak in common invocation

[Ævar Arnfjörð Bjarmason]

Create a public release_patch() version of the private free_patch()
function added in 13b5af22f39 (apply: move libified code from
builtin/apply.c to apply.{c,h}, 2016-04-22). Unlike the existing
function this one doesn't free() the "struct patch" itself, so we can
use it for variables on the stack.

Use it in range-diff.c to fix a memory leak in common range-diff
invocations, e.g.:

    git -P range-diff origin/master origin/next origin/seen

Would emit several errors when compiled with SANITIZE=leak, but now
runs cleanly.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 apply.c      | 7 ++++++-
 apply.h      | 2 ++
 range-diff.c | 1 +
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index 0912307bd91..01f91816428 100644
--- a/apply.c
+++ b/apply.c
@@ -219,13 +219,18 @@ static void free_fragment_list(struct fragment *list)
 	}
 }
 
-static void free_patch(struct patch *patch)
+void release_patch(struct patch *patch)
 {
 	free_fragment_list(patch->fragments);
 	free(patch->def_name);
 	free(patch->old_name);
 	free(patch->new_name);
 	free(patch->result);
+}
+
+static void free_patch(struct patch *patch)
+{
+	release_patch(patch);
 	free(patch);
 }
 
diff --git a/apply.h b/apply.h
index 4052da50c06..b9f18ce87d1 100644
--- a/apply.h
+++ b/apply.h
@@ -173,6 +173,8 @@ int parse_git_diff_header(struct strbuf *root,
 			  unsigned int size,
 			  struct patch *patch);
 
+void release_patch(struct patch *patch);
+
 /*
  * Some aspects of the apply behavior are controlled by the following
  * bits in the "options" parameter passed to apply_all_patches().
diff --git a/range-diff.c b/range-diff.c
index 30a4de5c2d8..b2a2961f521 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -165,6 +165,7 @@ static int read_patches(const char *range, struct string_list *list,
 					    patch.old_mode, patch.new_mode);
 
 			strbuf_addstr(&buf, " ##");
+			release_patch(&patch);
 		} else if (in_header) {
 			if (starts_with(line, "Author: ")) {
 				strbuf_addstr(&buf, " ## Metadata ##\n");
-- 
2.35.1.1228.g56895c6ee86

[PATCH 13/14] range-diff: plug memory leak in read_patches()

[Ævar Arnfjörð Bjarmason]

Amend code added in d9c66f0b5bf (range-diff: first rudimentary
implementation, 2018-08-13) to use a "goto cleanup" pattern. This
makes for less code, and frees memory that we'd previously leak.

The reason for changing free(util) to FREE_AND_NULL(util) is because
at the end of the function we append the contents of "util" to a
"struct string_list" if it's non-NULL.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 range-diff.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/range-diff.c b/range-diff.c
index b2a2961f521..b72eb9fdbee 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -40,6 +40,7 @@ static int read_patches(const char *range, struct string_list *list,
 	char *line, *current_filename = NULL;
 	ssize_t len;
 	size_t size;
+	int ret = -1;
 
 	strvec_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
 		     "--reverse", "--date-order", "--decorate=no",
@@ -68,10 +69,10 @@ static int read_patches(const char *range, struct string_list *list,
 	if (strbuf_read(&contents, cp.out, 0) < 0) {
 		error_errno(_("could not read `log` output"));
 		finish_command(&cp);
-		return -1;
+		goto cleanup;
 	}
 	if (finish_command(&cp))
-		return -1;
+		goto cleanup;
 
 	line = contents.buf;
 	size = contents.len;
@@ -95,12 +96,9 @@ static int read_patches(const char *range, struct string_list *list,
 			CALLOC_ARRAY(util, 1);
 			if (get_oid(p, &util->oid)) {
 				error(_("could not parse commit '%s'"), p);
-				free(util);
-				free(current_filename);
+				FREE_AND_NULL(util);
 				string_list_clear(list, 1);
-				strbuf_release(&buf);
-				strbuf_release(&contents);
-				return -1;
+				goto cleanup;
 			}
 			util->matching = -1;
 			in_header = 1;
@@ -111,11 +109,8 @@ static int read_patches(const char *range, struct string_list *list,
 			error(_("could not parse first line of `log` output: "
 				"did not start with 'commit ': '%s'"),
 			      line);
-			free(current_filename);
 			string_list_clear(list, 1);
-			strbuf_release(&buf);
-			strbuf_release(&contents);
-			return -1;
+			goto cleanup;
 		}
 
 		if (starts_with(line, "diff --git")) {
@@ -136,12 +131,9 @@ static int read_patches(const char *range, struct string_list *list,
 			if (len < 0) {
 				error(_("could not parse git header '%.*s'"),
 				      orig_len, line);
-				free(util);
-				free(current_filename);
+				FREE_AND_NULL(util);
 				string_list_clear(list, 1);
-				strbuf_release(&buf);
-				strbuf_release(&contents);
-				return -1;
+				goto cleanup;
 			}
 			strbuf_addstr(&buf, " ## ");
 			if (patch.is_new > 0)
@@ -219,6 +211,9 @@ static int read_patches(const char *range, struct string_list *list,
 		strbuf_addch(&buf, '\n');
 		util->diffsize++;
 	}
+
+	ret = 0;
+cleanup:
 	strbuf_release(&contents);
 
 	if (util)
@@ -226,7 +221,7 @@ static int read_patches(const char *range, struct string_list *list,
 	strbuf_release(&buf);
 	free(current_filename);
 
-	return 0;
+	return ret;
 }
 
 static int patch_util_cmp(const void *dummy, const struct patch_util *a,
-- 
2.35.1.1228.g56895c6ee86

[PATCH 14/14] repository.c: free the "path cache" in repo_clear()

[Ævar Arnfjörð Bjarmason]

The "struct path_cache" added in 102de880d24 (path.c: migrate global
git_path_* to take a repository argument, 2018-05-17) is only used
directly by code in repository.[ch] (but populated in path.[ch]).

Let's move this code to repository.[ch], and stop leaking this memory
when we run repo_clear(). To avoid the cast change it from a "const
char *" to a "char *".

This also removes the "PATH_CACHE_INIT" macro, which has never been
used for anything. For the "struct repository" we already make a hard
assumption that it (and "the_repository") can be identically
initialized by making it a "static" variable, so making use of a
"PATH_CACHE_INIT" somewhere would have been confusing.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 path.h       | 14 --------------
 repository.c | 16 ++++++++++++++++
 repository.h | 14 +++++++++++++-
 3 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/path.h b/path.h
index b68691a86b8..0a59c85a62e 100644
--- a/path.h
+++ b/path.h
@@ -169,20 +169,6 @@ void report_linked_checkout_garbage(void);
 		return r->cached_paths.var; \
 	}
 
-struct path_cache {
-	const char *squash_msg;
-	const char *merge_msg;
-	const char *merge_rr;
-	const char *merge_mode;
-	const char *merge_head;
-	const char *merge_autostash;
-	const char *auto_merge;
-	const char *fetch_head;
-	const char *shallow;
-};
-
-#define PATH_CACHE_INIT { 0 }
-
 const char *git_path_squash_msg(struct repository *r);
 const char *git_path_merge_msg(struct repository *r);
 const char *git_path_merge_rr(struct repository *r);
diff --git a/repository.c b/repository.c
index 34610c5a33e..9b86f3f1214 100644
--- a/repository.c
+++ b/repository.c
@@ -240,6 +240,20 @@ int repo_submodule_init(struct repository *subrepo,
 	return ret;
 }
 
+static void repo_clear_path_cache(struct repo_path_cache *cache)
+{
+	FREE_AND_NULL(cache->squash_msg);
+	FREE_AND_NULL(cache->squash_msg);
+	FREE_AND_NULL(cache->merge_msg);
+	FREE_AND_NULL(cache->merge_rr);
+	FREE_AND_NULL(cache->merge_mode);
+	FREE_AND_NULL(cache->merge_head);
+	FREE_AND_NULL(cache->merge_autostash);
+	FREE_AND_NULL(cache->auto_merge);
+	FREE_AND_NULL(cache->fetch_head);
+	FREE_AND_NULL(cache->shallow);
+}
+
 void repo_clear(struct repository *repo)
 {
 	FREE_AND_NULL(repo->gitdir);
@@ -280,6 +294,8 @@ void repo_clear(struct repository *repo)
 		remote_state_clear(repo->remote_state);
 		FREE_AND_NULL(repo->remote_state);
 	}
+
+	repo_clear_path_cache(&repo->cached_paths);
 }
 
 int repo_read_index(struct repository *repo)
diff --git a/repository.h b/repository.h
index ca837cb9e91..e29f361703d 100644
--- a/repository.h
+++ b/repository.h
@@ -44,6 +44,18 @@ struct repo_settings {
 	int core_multi_pack_index;
 };
 
+struct repo_path_cache {
+	char *squash_msg;
+	char *merge_msg;
+	char *merge_rr;
+	char *merge_mode;
+	char *merge_head;
+	char *merge_autostash;
+	char *auto_merge;
+	char *fetch_head;
+	char *shallow;
+};
+
 struct repository {
 	/* Environment */
 	/*
@@ -82,7 +94,7 @@ struct repository {
 	/*
 	 * Contains path to often used file names.
 	 */
-	struct path_cache cached_paths;
+	struct repo_path_cache cached_paths;
 
 	/*
 	 * Path to the repository's graft file.
-- 
2.35.1.1228.g56895c6ee86

Re: [PATCH 00/14] tree-wide: small fixes for memory leaks

[Derrick Stolee]

On 3/2/2022 12:10 PM, Ævar Arnfjörð Bjarmason wrote:
> This is a collection of various otherwise unrelated tree-wide fixes
> for memory leaks.
> 
> In fixing more targeted memory leaks in specific areas I've run into
> small leaks here & there. Rather than submit e.g. a 2-series patch for
> just small bundle leaks, the same for range-diff etc. I thought it
> made sense to submit these together.

I read these patches, and they were quite small, so it wasn't hard.
The most difficult thing was going into the code just to get more
context than the patches allowed.

Thank you for fixing all of those memory leaks I introduced in the
commit-graph code.

I had a few nitpick comments on the first commit message, but I
think what I pointed out in patch 7 might be a problem that needs
fixing before this merges.

Thanks,
-Stolee

[PATCH v2 00/14] tree-wide: small fixes for memory leaks

[Ævar Arnfjörð Bjarmason]

This is a collection of various otherwise unrelated tree-wide fixes
for memory leaks. See v1 for a (short) overview:
https://lore.kernel.org/git/cover-00.14-00000000000-20220302T170718Z-avarab@gmail.com/

This re-roll addresses issues Derrick Stolee noted. There's a trivial
commit-message change in 1/14, and a rather trivial (but important)
change in how a variable is incremented in 7/14.

That change to v1 is actually small, but the range-diff is big since
"git diff" picks a different way to "anchor" the diff as a result.

Ævar Arnfjörð Bjarmason (14):
  index-pack: fix memory leaks
  merge-base: free() allocated "struct commit **" list
  diff.c: free "buf" in diff_words_flush()
  urlmatch.c: add and use a *_release() function
  remote-curl.c: free memory in cmd_main()
  bundle: call strvec_clear() on allocated strvec
  transport: stop needlessly copying bundle header references
  submodule--helper: fix trivial leak in module_add()
  commit-graph: fix memory leak in misused string_list API
  commit-graph: stop fill_oids_from_packs() progress on error and free()
  lockfile API users: simplify and don't leak "path"
  range-diff: plug memory leak in common invocation
  range-diff: plug memory leak in read_patches()
  repository.c: free the "path cache" in repo_clear()

 apply.c                     |  7 ++++++-
 apply.h                     |  2 ++
 builtin/bundle.c            |  1 +
 builtin/commit-graph.c      |  6 +++---
 builtin/config.c            |  2 +-
 builtin/index-pack.c        |  5 +++++
 builtin/merge-base.c        |  5 ++++-
 builtin/sparse-checkout.c   |  3 +--
 builtin/submodule--helper.c |  5 ++++-
 commit-graph.c              | 18 +++++++++++-------
 commit-graph.h              |  2 +-
 credential.c                |  1 +
 diff.c                      |  1 +
 path.h                      | 14 --------------
 range-diff.c                | 30 +++++++++++++-----------------
 remote-curl.c               | 12 ++++++++----
 repository.c                | 16 ++++++++++++++++
 repository.h                | 14 +++++++++++++-
 transport.c                 | 25 ++++++++++++++++---------
 urlmatch.c                  |  5 +++++
 urlmatch.h                  |  1 +
 21 files changed, 113 insertions(+), 62 deletions(-)

Range-diff against v1:
 1:  bcba06e1d28 !  1:  f46af9ad13f index-pack: fix memory leaks
    @@ Commit message
     
         Fix various memory leaks in "git index-pack", due to how tightly
         coupled this command is with the revision walking this doesn't make
    -    any new tests pass, but e.g. this now passes, and had several failures before:
    +    any new tests pass.
    +
    +    But e.g. this now passes, and had several failures before, i.e. we
    +    still have failures in tests 3, 5 etc., which are being skipped here.
     
             ./t5300-pack-object.sh --run=1-2,4,6-27,30-42
     
    -    it is a bit odd that we'll free "opts.anomaly", since the "opts" is a
    +    It is a bit odd that we'll free "opts.anomaly", since the "opts" is a
         "struct pack_idx_option" declared in pack.h. In pack-write.c there's a
         reset_pack_idx_option(), but it only wipes the contents, but doesn't
         free() anything.
 2:  4c28f056ec2 =  2:  4ee2881adfb merge-base: free() allocated "struct commit **" list
 3:  5d2793039ad =  3:  90517a05582 diff.c: free "buf" in diff_words_flush()
 4:  7f7077e8476 =  4:  d51f6ae0963 urlmatch.c: add and use a *_release() function
 5:  8891fd44c7c =  5:  f0a26db8a87 remote-curl.c: free memory in cmd_main()
 6:  52e2c2a8281 =  6:  c636770b5d6 bundle: call strvec_clear() on allocated strvec
 7:  be62ca89bf5 !  7:  b3f7753a790 transport: stop needlessly copying bundle header references
    @@ transport.c: struct bundle_transport_data {
      	unsigned get_refs_from_bundle_called : 1;
      };
      
    +-static struct ref *get_refs_from_bundle(struct transport *transport,
    +-					int for_push,
    +-					struct transport_ls_refs_options *transport_options)
     +static void get_refs_from_bundle_inner(struct transport *transport)
    + {
    + 	struct bundle_transport_data *data = transport->data;
    +-	struct ref *result = NULL;
    +-	int i;
    +-
    +-	if (for_push)
    +-		return NULL;
    + 
    + 	data->get_refs_from_bundle_called = 1;
    + 
    +@@ transport.c: static struct ref *get_refs_from_bundle(struct transport *transport,
    + 		die(_("could not read bundle '%s'"), transport->url);
    + 
    + 	transport->hash_algo = data->header.hash_algo;
    ++}
    ++
    ++static struct ref *get_refs_from_bundle(struct transport *transport,
    ++					int for_push,
    ++					struct transport_ls_refs_options *transport_options)
     +{
     +	struct bundle_transport_data *data = transport->data;
    ++	struct ref *result = NULL;
    ++	int i;
     +
    -+	if (data->fd > 0)
    -+		close(data->fd);
    -+	data->fd = read_bundle_header(transport->url, &data->header);
    -+	if (data->fd < 0)
    -+		die(_("could not read bundle '%s'"), transport->url);
    -+
    -+	transport->hash_algo = data->header.hash_algo;
    -+}
    ++	if (for_push)
    ++		return NULL;
     +
    - static struct ref *get_refs_from_bundle(struct transport *transport,
    - 					int for_push,
    - 					struct transport_ls_refs_options *transport_options)
    -@@ transport.c: static struct ref *get_refs_from_bundle(struct transport *transport,
    - 	if (for_push)
    - 		return NULL;
    - 
    --	data->get_refs_from_bundle_called = 1;
    --
    --	if (data->fd > 0)
    --		close(data->fd);
    --	data->fd = read_bundle_header(transport->url, &data->header);
    --	if (data->fd < 0)
    --		die(_("could not read bundle '%s'"), transport->url);
    --
    --	transport->hash_algo = data->header.hash_algo;
     +	get_refs_from_bundle_inner(transport);
      
      	for (i = 0; i < data->header.references.nr; i++) {
      		struct string_list_item *e = data->header.references.items + i;
     @@ transport.c: static int fetch_refs_from_bundle(struct transport *transport,
    - 	if (transport->progress)
      		strvec_push(&extra_index_pack_args, "-v");
      
    --	if (!data->get_refs_from_bundle_called)
    + 	if (!data->get_refs_from_bundle_called)
     -		get_refs_from_bundle(transport, 0, NULL);
    -+	if (!data->get_refs_from_bundle_called++)
     +		get_refs_from_bundle_inner(transport);
      	ret = unbundle(the_repository, &data->header, data->fd,
      		       &extra_index_pack_args);
 8:  122fdf7bb41 =  8:  af3ca2f0b5f submodule--helper: fix trivial leak in module_add()
 9:  b5512deb26f =  9:  3fadb265d13 commit-graph: fix memory leak in misused string_list API
10:  27f0883e8d8 = 10:  27f5190ce59 commit-graph: stop fill_oids_from_packs() progress on error and free()
11:  cc8beed10be = 11:  217754edc62 lockfile API users: simplify and don't leak "path"
12:  6d13c2530db = 12:  148382d9529 range-diff: plug memory leak in common invocation
13:  e7b823f70c8 = 13:  c6e61b85491 range-diff: plug memory leak in read_patches()
14:  954de5191c3 = 14:  d70a4394f2b repository.c: free the "path cache" in repo_clear()
-- 
2.35.1.1248.gb68c9165ad8

[PATCH v2 01/14] index-pack: fix memory leaks

[Ævar Arnfjörð Bjarmason]

Fix various memory leaks in "git index-pack", due to how tightly
coupled this command is with the revision walking this doesn't make
any new tests pass.

But e.g. this now passes, and had several failures before, i.e. we
still have failures in tests 3, 5 etc., which are being skipped here.

    ./t5300-pack-object.sh --run=1-2,4,6-27,30-42

It is a bit odd that we'll free "opts.anomaly", since the "opts" is a
"struct pack_idx_option" declared in pack.h. In pack-write.c there's a
reset_pack_idx_option(), but it only wipes the contents, but doesn't
free() anything.

Doing this here in cmd_index_pack() is correct because while the
struct is declared in pack.h, this code in builtin/index-pack.c (in
read_v2_anomalous_offsets()) is what allocates the "opts.anomaly", so
we should also free it here.

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

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 3c2e6aee3cc..5fe1adb3681 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1109,6 +1109,7 @@ static void *threaded_second_pass(void *data)
 			list_add(&child->list, &work_head);
 			base_cache_used += child->size;
 			prune_base_data(NULL);
+			free_base_data(child);
 		} else {
 			/*
 			 * This child does not have its own children. It may be
@@ -1131,6 +1132,7 @@ static void *threaded_second_pass(void *data)
 
 				p = next_p;
 			}
+			FREE_AND_NULL(child);
 		}
 		work_unlock();
 	}
@@ -1424,6 +1426,7 @@ static void fix_unresolved_deltas(struct hashfile *f)
 		 * object).
 		 */
 		append_obj_to_pack(f, d->oid.hash, data, size, type);
+		free(data);
 		threaded_second_pass(NULL);
 
 		display_progress(progress, nr_resolved_deltas);
@@ -1703,6 +1706,7 @@ static void show_pack_info(int stat_only)
 			  i + 1,
 			  chain_histogram[i]);
 	}
+	free(chain_histogram);
 }
 
 int cmd_index_pack(int argc, const char **argv, const char *prefix)
@@ -1932,6 +1936,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 	if (do_fsck_object && fsck_finish(&fsck_options))
 		die(_("fsck error in pack objects"));
 
+	free(opts.anomaly);
 	free(objects);
 	strbuf_release(&index_name_buf);
 	strbuf_release(&rev_index_name_buf);
-- 
2.35.1.1248.gb68c9165ad8

[PATCH v2 02/14] merge-base: free() allocated "struct commit **" list

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in 53eda89b2fe (merge-base: teach "git merge-base"
to drive underlying merge_bases_many(), 2008-07-30) by calling free()
on the "struct commit **" list used by "git merge-base".

This gets e.g. "t6010-merge-base.sh" closer to passing under
SANITIZE=leak, it failed 8 tests before when compiled with that
option, and now fails only 5 tests.

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

diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index 26b84980dbd..a11f8c6e4bb 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -138,6 +138,7 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix)
 	int rev_nr = 0;
 	int show_all = 0;
 	int cmdmode = 0;
+	int ret;
 
 	struct option options[] = {
 		OPT_BOOL('a', "all", &show_all, N_("output all common ancestors")),
@@ -186,5 +187,7 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix)
 	ALLOC_ARRAY(rev, argc);
 	while (argc-- > 0)
 		rev[rev_nr++] = get_commit_reference(*argv++);
-	return show_merge_base(rev, rev_nr, show_all);
+	ret = show_merge_base(rev, rev_nr, show_all);
+	free(rev);
+	return ret;
 }
-- 
2.35.1.1248.gb68c9165ad8

[PATCH v2 03/14] diff.c: free "buf" in diff_words_flush()

[Ævar Arnfjörð Bjarmason]

Amend the freeing logic added in e6e045f8031 (diff.c: buffer all
output if asked to, 2017-06-29) to free the containing "buf" in
addition to its members.

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

diff --git a/diff.c b/diff.c
index c4ccb6b1a34..c5bc9bc5128 100644
--- a/diff.c
+++ b/diff.c
@@ -2150,6 +2150,7 @@ static void diff_words_flush(struct emit_callback *ecbdata)
 
 		for (i = 0; i < wol->nr; i++)
 			free((void *)wol->buf[i].line);
+		free(wol->buf);
 
 		wol->nr = 0;
 	}
-- 
2.35.1.1248.gb68c9165ad8

[PATCH v2 04/14] urlmatch.c: add and use a *_release() function

[Ævar Arnfjörð Bjarmason]

Plug a memory leak in credential_apply_config() by adding and using a
new urlmatch_config_release() function. This just does a
string_list_clear() on the "vars" member.

This finished up work on normalizing the init/free pattern in this
API, started in 73ee449bbf2 (urlmatch.[ch]: add and use
URLMATCH_CONFIG_INIT, 2021-10-01).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/config.c | 2 +-
 credential.c     | 1 +
 urlmatch.c       | 5 +++++
 urlmatch.h       | 1 +
 4 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/builtin/config.c b/builtin/config.c
index 542d8d02b2b..ebec61868be 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -612,7 +612,7 @@ static int get_urlmatch(const char *var, const char *url)
 
 		strbuf_release(&matched->value);
 	}
-	string_list_clear(&config.vars, 1);
+	urlmatch_config_release(&config);
 	string_list_clear(&values, 1);
 	free(config.url.url);
 
diff --git a/credential.c b/credential.c
index e7240f3f636..f6389a50684 100644
--- a/credential.c
+++ b/credential.c
@@ -130,6 +130,7 @@ static void credential_apply_config(struct credential *c)
 	git_config(urlmatch_config_entry, &config);
 	string_list_clear(&config.vars, 1);
 	free(normalized_url);
+	urlmatch_config_release(&config);
 	strbuf_release(&url);
 
 	c->configured = 1;
diff --git a/urlmatch.c b/urlmatch.c
index 03ad3f30a9c..b615adc923a 100644
--- a/urlmatch.c
+++ b/urlmatch.c
@@ -611,3 +611,8 @@ int urlmatch_config_entry(const char *var, const char *value, void *cb)
 	strbuf_release(&synthkey);
 	return retval;
 }
+
+void urlmatch_config_release(struct urlmatch_config *config)
+{
+	string_list_clear(&config->vars, 1);
+}
diff --git a/urlmatch.h b/urlmatch.h
index 34a3ba6d197..9f40b00bfb8 100644
--- a/urlmatch.h
+++ b/urlmatch.h
@@ -71,5 +71,6 @@ struct urlmatch_config {
 }
 
 int urlmatch_config_entry(const char *var, const char *value, void *cb);
+void urlmatch_config_release(struct urlmatch_config *config);
 
 #endif /* URL_MATCH_H */
-- 
2.35.1.1248.gb68c9165ad8

[PATCH v2 05/14] remote-curl.c: free memory in cmd_main()

[Ævar Arnfjörð Bjarmason]

Plug a trivial memory leak in code added in a2d725b7bdf (Use an
external program to implement fetching with curl, 2009-08-05).

To do this have the cmd_main() use a "goto cleanup" pattern, and to
return an error of 1 unless we can fall through to the http_cleanup()
at the end.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 remote-curl.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 0dabef2dd7c..ff44f41011e 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1472,11 +1472,12 @@ int cmd_main(int argc, const char **argv)
 {
 	struct strbuf buf = STRBUF_INIT;
 	int nongit;
+	int ret = 1;
 
 	setup_git_directory_gently(&nongit);
 	if (argc < 2) {
 		error(_("remote-curl: usage: git remote-curl <remote> [<url>]"));
-		return 1;
+		goto cleanup;
 	}
 
 	options.verbosity = 1;
@@ -1508,7 +1509,7 @@ int cmd_main(int argc, const char **argv)
 		if (strbuf_getline_lf(&buf, stdin) == EOF) {
 			if (ferror(stdin))
 				error(_("remote-curl: error reading command stream from git"));
-			return 1;
+			goto cleanup;
 		}
 		if (buf.len == 0)
 			break;
@@ -1556,12 +1557,15 @@ int cmd_main(int argc, const char **argv)
 				break;
 		} else {
 			error(_("remote-curl: unknown command '%s' from git"), buf.buf);
-			return 1;
+			goto cleanup;
 		}
 		strbuf_reset(&buf);
 	} while (1);
 
 	http_cleanup();
+	ret = 0;
+cleanup:
+	strbuf_release(&buf);
 
-	return 0;
+	return ret;
 }
-- 
2.35.1.1248.gb68c9165ad8

[PATCH v2 06/14] bundle: call strvec_clear() on allocated strvec

[Ævar Arnfjörð Bjarmason]

Fixing this small memory leak in cmd_bundle_create() gets
"t5607-clone-bundle.sh" closer to passing under SANITIZE=leak.

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

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 5a85d7cd0fe..2adad545a2e 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -93,6 +93,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 	if (!startup_info->have_repository)
 		die(_("Need a repository to create a bundle."));
 	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
+	strvec_clear(&pack_opts);
 	free(bundle_file);
 	return ret;
 }
-- 
2.35.1.1248.gb68c9165ad8

[PATCH v2 07/14] transport: stop needlessly copying bundle header references

[Ævar Arnfjörð Bjarmason]

Amend the logic added in fddf2ebe388 (transport: teach all vtables to
allow fetch first, 2019-08-21) and save ourselves pointless work in
fetch_refs_from_bundle().

The fetch_refs_from_bundle() caller doesn't care about the "struct
ref *result" return value of get_refs_from_bundle(), and doesn't need
any of the work we were doing in looping over the
"data->header.references" in get_refs_from_bundle().

So this change saves us work, and also fixes a memory leak that we had
when called from fetch_refs_from_bundle(). The other caller of
get_refs_from_bundle() is the "get_refs_list" member we set up for the
"struct transport_vtable bundle_vtable". That caller does care about
the "struct ref *result" return value.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 transport.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/transport.c b/transport.c
index 253d6671b1f..70e9840a90e 100644
--- a/transport.c
+++ b/transport.c
@@ -125,16 +125,9 @@ struct bundle_transport_data {
 	unsigned get_refs_from_bundle_called : 1;
 };
 
-static struct ref *get_refs_from_bundle(struct transport *transport,
-					int for_push,
-					struct transport_ls_refs_options *transport_options)
+static void get_refs_from_bundle_inner(struct transport *transport)
 {
 	struct bundle_transport_data *data = transport->data;
-	struct ref *result = NULL;
-	int i;
-
-	if (for_push)
-		return NULL;
 
 	data->get_refs_from_bundle_called = 1;
 
@@ -145,6 +138,20 @@ static struct ref *get_refs_from_bundle(struct transport *transport,
 		die(_("could not read bundle '%s'"), transport->url);
 
 	transport->hash_algo = data->header.hash_algo;
+}
+
+static struct ref *get_refs_from_bundle(struct transport *transport,
+					int for_push,
+					struct transport_ls_refs_options *transport_options)
+{
+	struct bundle_transport_data *data = transport->data;
+	struct ref *result = NULL;
+	int i;
+
+	if (for_push)
+		return NULL;
+
+	get_refs_from_bundle_inner(transport);
 
 	for (i = 0; i < data->header.references.nr; i++) {
 		struct string_list_item *e = data->header.references.items + i;
@@ -169,7 +176,7 @@ static int fetch_refs_from_bundle(struct transport *transport,
 		strvec_push(&extra_index_pack_args, "-v");
 
 	if (!data->get_refs_from_bundle_called)
-		get_refs_from_bundle(transport, 0, NULL);
+		get_refs_from_bundle_inner(transport);
 	ret = unbundle(the_repository, &data->header, data->fd,
 		       &extra_index_pack_args);
 	transport->hash_algo = data->header.hash_algo;
-- 
2.35.1.1248.gb68c9165ad8

Re: [PATCH v2 07/14] transport: stop needlessly copying bundle header references

[Derrick Stolee]

On 3/4/2022 1:32 PM, Ævar Arnfjörð Bjarmason wrote:
> Amend the logic added in fddf2ebe388 (transport: teach all vtables to
> allow fetch first, 2019-08-21) and save ourselves pointless work in
> fetch_refs_from_bundle().

> -static struct ref *get_refs_from_bundle(struct transport *transport,
> -					int for_push,
> -					struct transport_ls_refs_options *transport_options)
> +static void get_refs_from_bundle_inner(struct transport *transport)
>  {
>  	struct bundle_transport_data *data = transport->data;
> -	struct ref *result = NULL;
> -	int i;
> -
> -	if (for_push)
> -		return NULL;
>  
>  	data->get_refs_from_bundle_called = 1;

The inclusion of this line dramatically changed the shape of the
diff (different areas got selected as adds and deletes) so the
range-diff looked strange, but the end result is good.
  
Thanks,
-Stolee

[PATCH v2 08/14] submodule--helper: fix trivial leak in module_add()

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in code added in a6226fd772b (submodule--helper:
convert the bulk of cmd_add() to C, 2021-08-10). If "realrepo" isn't a
copy of the "repo" member we should free() it.

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index eeacefcc383..13b4841327d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -3309,6 +3309,7 @@ static int module_add(int argc, const char **argv, const char *prefix)
 {
 	int force = 0, quiet = 0, progress = 0, dissociate = 0;
 	struct add_data add_data = ADD_DATA_INIT;
+	char *to_free = NULL;
 
 	struct option options[] = {
 		OPT_STRING('b', "branch", &add_data.branch, N_("branch"),
@@ -3360,7 +3361,8 @@ static int module_add(int argc, const char **argv, const char *prefix)
 			      "of the working tree"));
 
 		/* dereference source url relative to parent's url */
-		add_data.realrepo = resolve_relative_url(add_data.repo, NULL, 1);
+		to_free = resolve_relative_url(add_data.repo, NULL, 1);
+		add_data.realrepo = to_free;
 	} else if (is_dir_sep(add_data.repo[0]) || strchr(add_data.repo, ':')) {
 		add_data.realrepo = add_data.repo;
 	} else {
@@ -3413,6 +3415,7 @@ static int module_add(int argc, const char **argv, const char *prefix)
 	}
 	configure_added_submodule(&add_data);
 	free(add_data.sm_path);
+	free(to_free);
 
 	return 0;
 }
-- 
2.35.1.1248.gb68c9165ad8

[PATCH v2 09/14] commit-graph: fix memory leak in misused string_list API

[Ævar Arnfjörð Bjarmason]

When this code was migrated to the string_list API in
d88b14b3fd6 (commit-graph: use string-list API for input, 2018-06-27)
it was made to use used both STRING_LIST_INIT_NODUP and a
strbuf_detach() pattern.

Those should not be used together if string_list_clear() is expected
to free the memory, instead we need to either use STRING_LIST_INIT_DUP
with a string_list_append_nodup(), or a STRING_LIST_INIT_NODUP and
manually fiddle with the "strdup_strings" member before calling
string_list_clear(). Let's do the former.

Since "strdup_strings = 1" is set now other code might be broken by
relying on "pack_indexes" not to duplicate it strings, but that
doesn't happen. When we pass this down to write_commit_graph() that
code uses the "struct string_list" without modifying it. Let's add a
"const" to the variable to have the compiler enforce that assumption.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/commit-graph.c | 6 +++---
 commit-graph.c         | 4 ++--
 commit-graph.h         | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 4247fbde95a..51c4040ea6c 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -192,7 +192,7 @@ static int git_commit_graph_write_config(const char *var, const char *value,
 
 static int graph_write(int argc, const char **argv)
 {
-	struct string_list pack_indexes = STRING_LIST_INIT_NODUP;
+	struct string_list pack_indexes = STRING_LIST_INIT_DUP;
 	struct strbuf buf = STRBUF_INIT;
 	struct oidset commits = OIDSET_INIT;
 	struct object_directory *odb = NULL;
@@ -273,8 +273,8 @@ static int graph_write(int argc, const char **argv)
 
 	if (opts.stdin_packs) {
 		while (strbuf_getline(&buf, stdin) != EOF)
-			string_list_append(&pack_indexes,
-					   strbuf_detach(&buf, NULL));
+			string_list_append_nodup(&pack_indexes,
+						 strbuf_detach(&buf, NULL));
 	} else if (opts.stdin_commits) {
 		oidset_init(&commits, 0);
 		if (opts.progress)
diff --git a/commit-graph.c b/commit-graph.c
index 265c010122e..d0c94600bab 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1679,7 +1679,7 @@ int write_commit_graph_reachable(struct object_directory *odb,
 }
 
 static int fill_oids_from_packs(struct write_commit_graph_context *ctx,
-				struct string_list *pack_indexes)
+				const struct string_list *pack_indexes)
 {
 	uint32_t i;
 	struct strbuf progress_title = STRBUF_INIT;
@@ -2259,7 +2259,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)
 }
 
 int write_commit_graph(struct object_directory *odb,
-		       struct string_list *pack_indexes,
+		       const struct string_list *const pack_indexes,
 		       struct oidset *commits,
 		       enum commit_graph_write_flags flags,
 		       const struct commit_graph_opts *opts)
diff --git a/commit-graph.h b/commit-graph.h
index 04a94e18302..2e3ac35237e 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -142,7 +142,7 @@ int write_commit_graph_reachable(struct object_directory *odb,
 				 enum commit_graph_write_flags flags,
 				 const struct commit_graph_opts *opts);
 int write_commit_graph(struct object_directory *odb,
-		       struct string_list *pack_indexes,
+		       const struct string_list *pack_indexes,
 		       struct oidset *commits,
 		       enum commit_graph_write_flags flags,
 		       const struct commit_graph_opts *opts);
-- 
2.35.1.1248.gb68c9165ad8

[PATCH v2 10/14] commit-graph: stop fill_oids_from_packs() progress on error and free()

[Ævar Arnfjörð Bjarmason]

Fix a bug in fill_oids_from_packs(), we should always stop_progress(),
but did not do so if we returned an error here. This also plugs a
memory leak in those cases by releasing the two "struct strbuf"
variables the function uses.

While I'm at it stop hardcoding "-1" here and just use the return
value of error() instead, which happens to be "-1".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 commit-graph.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index d0c94600bab..aab0b292774 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1685,6 +1685,7 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,
 	struct strbuf progress_title = STRBUF_INIT;
 	struct strbuf packname = STRBUF_INIT;
 	int dirlen;
+	int ret = 0;
 
 	strbuf_addf(&packname, "%s/pack/", ctx->odb->path);
 	dirlen = packname.len;
@@ -1703,12 +1704,12 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,
 		strbuf_addstr(&packname, pack_indexes->items[i].string);
 		p = add_packed_git(packname.buf, packname.len, 1);
 		if (!p) {
-			error(_("error adding pack %s"), packname.buf);
-			return -1;
+			ret = error(_("error adding pack %s"), packname.buf);
+			goto cleanup;
 		}
 		if (open_pack_index(p)) {
-			error(_("error opening index for %s"), packname.buf);
-			return -1;
+			ret = error(_("error opening index for %s"), packname.buf);
+			goto cleanup;
 		}
 		for_each_object_in_pack(p, add_packed_commits, ctx,
 					FOR_EACH_OBJECT_PACK_ORDER);
@@ -1716,11 +1717,12 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,
 		free(p);
 	}
 
+cleanup:
 	stop_progress(&ctx->progress);
 	strbuf_release(&progress_title);
 	strbuf_release(&packname);
 
-	return 0;
+	return ret;
 }
 
 static int fill_oids_from_commits(struct write_commit_graph_context *ctx,
-- 
2.35.1.1248.gb68c9165ad8

[PATCH v2 11/14] lockfile API users: simplify and don't leak "path"

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in code added in 6c622f9f0bb (commit-graph: write
commit-graph chains, 2019-06-18). We needed to free the "lock_name" if
we encounter errors, and the "graph_name" after we'd run unlink() on
it.

For the case of write_commit_graph_file() refactoring the code to free
the "lock_name" after we were done using the "struct lock_file lk"
would have made the control flow more complex. Luckily we can free the
"lock_file" right after the hold_lock_file_for_update() call, if it
makes use of "path" at all it'll have copied its contents to a "struct
strbuf" of its own.

While I'm at it let's fix code added in fb10ca5b543 (sparse-checkout:
write using lockfile, 2019-11-21) in write_patterns_and_update() to
avoid the same complexity that I thought I needed when I wrote the
initial fix for write_commit_graph_file(). We can free the
"sparse_filename" right after calling hold_lock_file_for_update(), we
don't need to wait until we're exiting the function to do so.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/sparse-checkout.c | 3 +--
 commit-graph.c            | 2 ++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 9c338d33ea2..270ad49c2b8 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -328,11 +328,11 @@ static int write_patterns_and_update(struct pattern_list *pl)
 
 	fd = hold_lock_file_for_update(&lk, sparse_filename,
 				      LOCK_DIE_ON_ERROR);
+	free(sparse_filename);
 
 	result = update_working_directory(pl);
 	if (result) {
 		rollback_lock_file(&lk);
-		free(sparse_filename);
 		clear_pattern_list(pl);
 		update_working_directory(NULL);
 		return result;
@@ -348,7 +348,6 @@ static int write_patterns_and_update(struct pattern_list *pl)
 	fflush(fp);
 	commit_lock_file(&lk);
 
-	free(sparse_filename);
 	clear_pattern_list(pl);
 
 	return 0;
diff --git a/commit-graph.c b/commit-graph.c
index aab0b292774..b8cde7ea27d 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1854,6 +1854,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 
 		hold_lock_file_for_update_mode(&lk, lock_name,
 					       LOCK_DIE_ON_ERROR, 0444);
+		free(lock_name);
 
 		fd = git_mkstemp_mode(ctx->graph_name, 0444);
 		if (fd < 0) {
@@ -1978,6 +1979,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 		} else {
 			char *graph_name = get_commit_graph_filename(ctx->odb);
 			unlink(graph_name);
+			free(graph_name);
 		}
 
 		ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1] = xstrdup(hash_to_hex(file_hash));
-- 
2.35.1.1248.gb68c9165ad8

[PATCH v2 12/14] range-diff: plug memory leak in common invocation

[Ævar Arnfjörð Bjarmason]

Create a public release_patch() version of the private free_patch()
function added in 13b5af22f39 (apply: move libified code from
builtin/apply.c to apply.{c,h}, 2016-04-22). Unlike the existing
function this one doesn't free() the "struct patch" itself, so we can
use it for variables on the stack.

Use it in range-diff.c to fix a memory leak in common range-diff
invocations, e.g.:

    git -P range-diff origin/master origin/next origin/seen

Would emit several errors when compiled with SANITIZE=leak, but now
runs cleanly.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 apply.c      | 7 ++++++-
 apply.h      | 2 ++
 range-diff.c | 1 +
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index 0912307bd91..01f91816428 100644
--- a/apply.c
+++ b/apply.c
@@ -219,13 +219,18 @@ static void free_fragment_list(struct fragment *list)
 	}
 }
 
-static void free_patch(struct patch *patch)
+void release_patch(struct patch *patch)
 {
 	free_fragment_list(patch->fragments);
 	free(patch->def_name);
 	free(patch->old_name);
 	free(patch->new_name);
 	free(patch->result);
+}
+
+static void free_patch(struct patch *patch)
+{
+	release_patch(patch);
 	free(patch);
 }
 
diff --git a/apply.h b/apply.h
index 4052da50c06..b9f18ce87d1 100644
--- a/apply.h
+++ b/apply.h
@@ -173,6 +173,8 @@ int parse_git_diff_header(struct strbuf *root,
 			  unsigned int size,
 			  struct patch *patch);
 
+void release_patch(struct patch *patch);
+
 /*
  * Some aspects of the apply behavior are controlled by the following
  * bits in the "options" parameter passed to apply_all_patches().
diff --git a/range-diff.c b/range-diff.c
index 30a4de5c2d8..b2a2961f521 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -165,6 +165,7 @@ static int read_patches(const char *range, struct string_list *list,
 					    patch.old_mode, patch.new_mode);
 
 			strbuf_addstr(&buf, " ##");
+			release_patch(&patch);
 		} else if (in_header) {
 			if (starts_with(line, "Author: ")) {
 				strbuf_addstr(&buf, " ## Metadata ##\n");
-- 
2.35.1.1248.gb68c9165ad8

[PATCH v2 13/14] range-diff: plug memory leak in read_patches()

[Ævar Arnfjörð Bjarmason]

Amend code added in d9c66f0b5bf (range-diff: first rudimentary
implementation, 2018-08-13) to use a "goto cleanup" pattern. This
makes for less code, and frees memory that we'd previously leak.

The reason for changing free(util) to FREE_AND_NULL(util) is because
at the end of the function we append the contents of "util" to a
"struct string_list" if it's non-NULL.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 range-diff.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/range-diff.c b/range-diff.c
index b2a2961f521..b72eb9fdbee 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -40,6 +40,7 @@ static int read_patches(const char *range, struct string_list *list,
 	char *line, *current_filename = NULL;
 	ssize_t len;
 	size_t size;
+	int ret = -1;
 
 	strvec_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
 		     "--reverse", "--date-order", "--decorate=no",
@@ -68,10 +69,10 @@ static int read_patches(const char *range, struct string_list *list,
 	if (strbuf_read(&contents, cp.out, 0) < 0) {
 		error_errno(_("could not read `log` output"));
 		finish_command(&cp);
-		return -1;
+		goto cleanup;
 	}
 	if (finish_command(&cp))
-		return -1;
+		goto cleanup;
 
 	line = contents.buf;
 	size = contents.len;
@@ -95,12 +96,9 @@ static int read_patches(const char *range, struct string_list *list,
 			CALLOC_ARRAY(util, 1);
 			if (get_oid(p, &util->oid)) {
 				error(_("could not parse commit '%s'"), p);
-				free(util);
-				free(current_filename);
+				FREE_AND_NULL(util);
 				string_list_clear(list, 1);
-				strbuf_release(&buf);
-				strbuf_release(&contents);
-				return -1;
+				goto cleanup;
 			}
 			util->matching = -1;
 			in_header = 1;
@@ -111,11 +109,8 @@ static int read_patches(const char *range, struct string_list *list,
 			error(_("could not parse first line of `log` output: "
 				"did not start with 'commit ': '%s'"),
 			      line);
-			free(current_filename);
 			string_list_clear(list, 1);
-			strbuf_release(&buf);
-			strbuf_release(&contents);
-			return -1;
+			goto cleanup;
 		}
 
 		if (starts_with(line, "diff --git")) {
@@ -136,12 +131,9 @@ static int read_patches(const char *range, struct string_list *list,
 			if (len < 0) {
 				error(_("could not parse git header '%.*s'"),
 				      orig_len, line);
-				free(util);
-				free(current_filename);
+				FREE_AND_NULL(util);
 				string_list_clear(list, 1);
-				strbuf_release(&buf);
-				strbuf_release(&contents);
-				return -1;
+				goto cleanup;
 			}
 			strbuf_addstr(&buf, " ## ");
 			if (patch.is_new > 0)
@@ -219,6 +211,9 @@ static int read_patches(const char *range, struct string_list *list,
 		strbuf_addch(&buf, '\n');
 		util->diffsize++;
 	}
+
+	ret = 0;
+cleanup:
 	strbuf_release(&contents);
 
 	if (util)
@@ -226,7 +221,7 @@ static int read_patches(const char *range, struct string_list *list,
 	strbuf_release(&buf);
 	free(current_filename);
 
-	return 0;
+	return ret;
 }
 
 static int patch_util_cmp(const void *dummy, const struct patch_util *a,
-- 
2.35.1.1248.gb68c9165ad8

[PATCH v2 14/14] repository.c: free the "path cache" in repo_clear()

[Ævar Arnfjörð Bjarmason]

The "struct path_cache" added in 102de880d24 (path.c: migrate global
git_path_* to take a repository argument, 2018-05-17) is only used
directly by code in repository.[ch] (but populated in path.[ch]).

Let's move this code to repository.[ch], and stop leaking this memory
when we run repo_clear(). To avoid the cast change it from a "const
char *" to a "char *".

This also removes the "PATH_CACHE_INIT" macro, which has never been
used for anything. For the "struct repository" we already make a hard
assumption that it (and "the_repository") can be identically
initialized by making it a "static" variable, so making use of a
"PATH_CACHE_INIT" somewhere would have been confusing.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 path.h       | 14 --------------
 repository.c | 16 ++++++++++++++++
 repository.h | 14 +++++++++++++-
 3 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/path.h b/path.h
index b68691a86b8..0a59c85a62e 100644
--- a/path.h
+++ b/path.h
@@ -169,20 +169,6 @@ void report_linked_checkout_garbage(void);
 		return r->cached_paths.var; \
 	}
 
-struct path_cache {
-	const char *squash_msg;
-	const char *merge_msg;
-	const char *merge_rr;
-	const char *merge_mode;
-	const char *merge_head;
-	const char *merge_autostash;
-	const char *auto_merge;
-	const char *fetch_head;
-	const char *shallow;
-};
-
-#define PATH_CACHE_INIT { 0 }
-
 const char *git_path_squash_msg(struct repository *r);
 const char *git_path_merge_msg(struct repository *r);
 const char *git_path_merge_rr(struct repository *r);
diff --git a/repository.c b/repository.c
index 34610c5a33e..9b86f3f1214 100644
--- a/repository.c
+++ b/repository.c
@@ -240,6 +240,20 @@ int repo_submodule_init(struct repository *subrepo,
 	return ret;
 }
 
+static void repo_clear_path_cache(struct repo_path_cache *cache)
+{
+	FREE_AND_NULL(cache->squash_msg);
+	FREE_AND_NULL(cache->squash_msg);
+	FREE_AND_NULL(cache->merge_msg);
+	FREE_AND_NULL(cache->merge_rr);
+	FREE_AND_NULL(cache->merge_mode);
+	FREE_AND_NULL(cache->merge_head);
+	FREE_AND_NULL(cache->merge_autostash);
+	FREE_AND_NULL(cache->auto_merge);
+	FREE_AND_NULL(cache->fetch_head);
+	FREE_AND_NULL(cache->shallow);
+}
+
 void repo_clear(struct repository *repo)
 {
 	FREE_AND_NULL(repo->gitdir);
@@ -280,6 +294,8 @@ void repo_clear(struct repository *repo)
 		remote_state_clear(repo->remote_state);
 		FREE_AND_NULL(repo->remote_state);
 	}
+
+	repo_clear_path_cache(&repo->cached_paths);
 }
 
 int repo_read_index(struct repository *repo)
diff --git a/repository.h b/repository.h
index ca837cb9e91..e29f361703d 100644
--- a/repository.h
+++ b/repository.h
@@ -44,6 +44,18 @@ struct repo_settings {
 	int core_multi_pack_index;
 };
 
+struct repo_path_cache {
+	char *squash_msg;
+	char *merge_msg;
+	char *merge_rr;
+	char *merge_mode;
+	char *merge_head;
+	char *merge_autostash;
+	char *auto_merge;
+	char *fetch_head;
+	char *shallow;
+};
+
 struct repository {
 	/* Environment */
 	/*
@@ -82,7 +94,7 @@ struct repository {
 	/*
 	 * Contains path to often used file names.
 	 */
-	struct path_cache cached_paths;
+	struct repo_path_cache cached_paths;
 
 	/*
 	 * Path to the repository's graft file.
-- 
2.35.1.1248.gb68c9165ad8

Re: [PATCH v2 00/14] tree-wide: small fixes for memory leaks

[Derrick Stolee]

On 3/4/2022 1:32 PM, Ævar Arnfjörð Bjarmason wrote:
> This is a collection of various otherwise unrelated tree-wide fixes
> for memory leaks. See v1 for a (short) overview:
> https://lore.kernel.org/git/cover-00.14-00000000000-20220302T170718Z-avarab@gmail.com/
> 
> This re-roll addresses issues Derrick Stolee noted. There's a trivial
> commit-message change in 1/14, and a rather trivial (but important)
> change in how a variable is incremented in 7/14.
> 
> That change to v1 is actually small, but the range-diff is big since
> "git diff" picks a different way to "anchor" the diff as a result.

Yep. This version looks good to go. Thanks for the re-roll.

-Stolee