From f2bcc69e7ecbd80fe1de81a196c97173f33785b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 4 Mar 2022 19:32:04 +0100 Subject: [PATCH 01/14] index-pack: fix memory leaks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- builtin/index-pack.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 3c2e6aee3c..5fe1adb368 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); From e69fe2e460080771e1de43f2e8b76adda2252c5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 4 Mar 2022 19:32:05 +0100 Subject: [PATCH 02/14] merge-base: free() allocated "struct commit **" list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- 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 26b84980db..a11f8c6e4b 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; } From a18d66cefb9e5ee4fd49be1d60e90523cd89ca0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 4 Mar 2022 19:32:06 +0100 Subject: [PATCH 03/14] diff.c: free "buf" in diff_words_flush() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- diff.c | 1 + 1 file changed, 1 insertion(+) diff --git a/diff.c b/diff.c index c4ccb6b1a3..c5bc9bc512 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; } From a41e8e74674d53a46616b01f2c18e43c7f2f30a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 4 Mar 2022 19:32:07 +0100 Subject: [PATCH 04/14] urlmatch.c: add and use a *_release() function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- 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 542d8d02b2..ebec61868b 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 e7240f3f63..f6389a5068 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 03ad3f30a9..b615adc923 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 34a3ba6d19..9f40b00bfb 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 */ From b07fa8f1b2004fa7ea7091ba453be894bddfeb3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 4 Mar 2022 19:32:08 +0100 Subject: [PATCH 05/14] remote-curl.c: free memory in cmd_main() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- remote-curl.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/remote-curl.c b/remote-curl.c index 0dabef2dd7..ff44f41011 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 []")); - 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; } From bf67dd8d9a29d02e44c622f62762a1cfc58fb3bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 4 Mar 2022 19:32:09 +0100 Subject: [PATCH 06/14] bundle: call strvec_clear() on allocated strvec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- builtin/bundle.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/bundle.c b/builtin/bundle.c index 5a85d7cd0f..2adad545a2 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; } From 0f0d118c650fd0fe117063fbd196782b4f219410 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 4 Mar 2022 19:32:10 +0100 Subject: [PATCH 07/14] transport: stop needlessly copying bundle header references MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- transport.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/transport.c b/transport.c index 253d6671b1..70e9840a90 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; From 8f79015111f879451ceff83bbb9b4a29ebbbeb1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 4 Mar 2022 19:32:11 +0100 Subject: [PATCH 08/14] submodule--helper: fix trivial leak in module_add() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- 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 eeacefcc38..13b4841327 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; } From 4a0479086a9bea5d31c4588b07bd45ae92a12b71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 4 Mar 2022 19:32:12 +0100 Subject: [PATCH 09/14] commit-graph: fix memory leak in misused string_list API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- 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 4247fbde95..51c4040ea6 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 265c010122..d0c94600ba 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 @@ out: } 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 04a94e1830..2e3ac35237 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); From 51a94d8ffe57ff40e1db1d5e46a1864a5ded7f49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 4 Mar 2022 19:32:13 +0100 Subject: [PATCH 10/14] commit-graph: stop fill_oids_from_packs() progress on error and free() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- commit-graph.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/commit-graph.c b/commit-graph.c index d0c94600ba..aab0b29277 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, From ef3fe214484f79afdad4204d56e0ee7ff6e85a0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 4 Mar 2022 19:32:14 +0100 Subject: [PATCH 11/14] lockfile API users: simplify and don't leak "path" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- 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 9c338d33ea..270ad49c2b 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 aab0b29277..b8cde7ea27 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)); From 4998e93fa6e0da45440dd344419974e46d81a165 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 4 Mar 2022 19:32:15 +0100 Subject: [PATCH 12/14] range-diff: plug memory leak in common invocation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- 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 0912307bd9..01f9181642 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 4052da50c0..b9f18ce87d 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 30a4de5c2d..b2a2961f52 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"); From 2d102c2bca1a0c50f17108189f134279cad941cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 4 Mar 2022 19:32:16 +0100 Subject: [PATCH 13/14] range-diff: plug memory leak in read_patches() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- range-diff.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/range-diff.c b/range-diff.c index b2a2961f52..b72eb9fdbe 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, From 759f34073807119ffb935a84aa86e6a8fa7a9bc4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 4 Mar 2022 19:32:17 +0100 Subject: [PATCH 14/14] repository.c: free the "path cache" in repo_clear() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- 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 b68691a86b..0a59c85a62 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 34610c5a33..9b86f3f121 100644 --- a/repository.c +++ b/repository.c @@ -240,6 +240,20 @@ out: 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 ca837cb9e9..e29f361703 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.