threads / patch / 57498

patch, 14 partstree-wide: small fixes for memory leaks

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

## tl;dr

37 messages between Mar 2, 2022 and Mar 4, 2022. Diffs are folded; open one to read it.

replies: 36people: 2as markdown or json

Ævar Arnfjörð Bjarmason· Mar 2, 2022, 17:10 UTC · lore

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
Ævar Arnfjörð Bjarmason· Mar 2, 2022, 17:10 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(+)
Show changes to builtin/index-pack.c +5 −0
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
Derrick Stolee· Mar 2, 2022, 20:36 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On 3/2/2022 12:10 PM, Ævar Arnfjörð Bjarmason wrote:
Show 5 quoted lines
> 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/
Show 8 quoted lines
> "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

Ævar Arnfjörð Bjarmason· Mar 3, 2022, 16:19 UTC · re: Derrick Stolee · lore

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

On Wed, Mar 02 2022, Derrick Stolee wrote:
Show 8 quoted lines
> 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!
Show 10 quoted lines
>> "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!
Ævar Arnfjörð Bjarmason· Mar 2, 2022, 17:10 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to builtin/merge-base.c +4 −1
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
Ævar Arnfjörð Bjarmason· Mar 2, 2022, 17:10 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(+)
Show changes to diff.c +1 −0
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
Ævar Arnfjörð Bjarmason· Mar 2, 2022, 17:10 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to remote-curl.c +8 −4
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
Ævar Arnfjörð Bjarmason· Mar 2, 2022, 17:10 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 4 files +8 −1

builtin/config.c, credential.c, urlmatch.c, urlmatch.h

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
Ævar Arnfjörð Bjarmason· Mar 2, 2022, 17:10 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(+)
Show changes to builtin/bundle.c +1 −0
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
Ævar Arnfjörð Bjarmason· Mar 2, 2022, 17:10 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to builtin/submodule--helper.c +4 −1
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
Ævar Arnfjörð Bjarmason· Mar 2, 2022, 17:10 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to transport.c +16 −11
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
Derrick Stolee· Mar 2, 2022, 20:47 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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().
...
Show 13 quoted lines
> +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.

Show 17 quoted lines
>  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

Ævar Arnfjörð Bjarmason· Mar 3, 2022, 16:20 UTC · re: Derrick Stolee · lore

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

On Wed, Mar 02 2022, Derrick Stolee wrote:
Show 61 quoted lines
> 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.
Ævar Arnfjörð Bjarmason· Mar 2, 2022, 17:10 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 3 files +6 −6

builtin/commit-graph.c, commit-graph.c, commit-graph.h

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
Ævar Arnfjörð Bjarmason· Mar 2, 2022, 17:10 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to commit-graph.c +7 −5
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
Ævar Arnfjörð Bjarmason· Mar 2, 2022, 17:10 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 2 files +3 −2

builtin/sparse-checkout.c, commit-graph.c

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
Ævar Arnfjörð Bjarmason· Mar 2, 2022, 17:10 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 3 files +9 −1

apply.c, apply.h, range-diff.c

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
Ævar Arnfjörð Bjarmason· Mar 2, 2022, 17:10 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 3 files +29 −15

path.h, repository.c, repository.h

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
Ævar Arnfjörð Bjarmason· Mar 2, 2022, 17:10 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to range-diff.c +12 −17
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
Derrick Stolee· Mar 2, 2022, 20:58 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On 3/2/2022 12:10 PM, Ævar Arnfjörð Bjarmason wrote:
Show 7 quoted lines
> 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

Ævar Arnfjörð Bjarmason· Mar 4, 2022, 18:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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
Ævar Arnfjörð Bjarmason· Mar 4, 2022, 18:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(+)
Show changes to builtin/index-pack.c +5 −0
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
Ævar Arnfjörð Bjarmason· Mar 4, 2022, 18:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to builtin/merge-base.c +4 −1
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
Ævar Arnfjörð Bjarmason· Mar 4, 2022, 18:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(+)
Show changes to diff.c +1 −0
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
Ævar Arnfjörð Bjarmason· Mar 4, 2022, 18:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 4 files +8 −1

builtin/config.c, credential.c, urlmatch.c, urlmatch.h

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
Ævar Arnfjörð Bjarmason· Mar 4, 2022, 18:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to remote-curl.c +8 −4
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
Ævar Arnfjörð Bjarmason· Mar 4, 2022, 18:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(+)
Show changes to builtin/bundle.c +1 −0
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
Ævar Arnfjörð Bjarmason· Mar 4, 2022, 18:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to transport.c +16 −9
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
Derrick Stolee· Mar 4, 2022, 19:10 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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().
Show 13 quoted lines
> -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
Ævar Arnfjörð Bjarmason· Mar 4, 2022, 18:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 3 files +6 −6

builtin/commit-graph.c, commit-graph.c, commit-graph.h

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
Ævar Arnfjörð Bjarmason· Mar 4, 2022, 18:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 3 files +9 −1

apply.c, apply.h, range-diff.c

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
Ævar Arnfjörð Bjarmason· Mar 4, 2022, 18:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to range-diff.c +12 −17
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
Ævar Arnfjörð Bjarmason· Mar 4, 2022, 18:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 2 files +3 −2

builtin/sparse-checkout.c, commit-graph.c

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
Ævar Arnfjörð Bjarmason· Mar 4, 2022, 18:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 3 files +29 −15

path.h, repository.c, repository.h

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
Ævar Arnfjörð Bjarmason· Mar 4, 2022, 18:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to commit-graph.c +7 −5
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
Ævar Arnfjörð Bjarmason· Mar 4, 2022, 18:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to builtin/submodule--helper.c +4 −1
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
Derrick Stolee· Mar 4, 2022, 19:11 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On 3/4/2022 1:32 PM, Ævar Arnfjörð Bjarmason wrote:
Show 10 quoted lines
> 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

← back to recent threads