From 5f5dd8e297b59f3f7bf61098e978a91c5581388a Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:16:43 +0100 Subject: [PATCH 01/22] builtin/ls-remote: plug leaking server options The list of server options populated via `OPT_STRING_LIST()` is never cleared, causing a memory leak. Plug it. This leak is exposed by t5702, but plugging it alone does not make the whole test suite pass. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/ls-remote.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c index 423318f87e..42f34e1236 100644 --- a/builtin/ls-remote.c +++ b/builtin/ls-remote.c @@ -166,6 +166,7 @@ int cmd_ls_remote(int argc, status = 0; /* we found something */ } + string_list_clear(&server_options, 0); ref_sorting_release(sorting); ref_array_clear(&ref_array); if (transport_disconnect(transport)) From ee3e8c3afa5f432c5232aba80a07b0884d388381 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:16:46 +0100 Subject: [PATCH 02/22] t/helper: fix leaks in "reach" test tool The "reach" test tool doesn't bother to clean up any of its allocated resources, causing various leaks. Plug them. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/helper/test-reach.c | 10 ++++++++++ t/t6600-test-reach.sh | 1 + 2 files changed, 11 insertions(+) diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c index 995e382863..84deee604a 100644 --- a/t/helper/test-reach.c +++ b/t/helper/test-reach.c @@ -127,10 +127,12 @@ int cmd__reach(int ac, const char **av) exit(128); printf("%s(A,X):\n", av[1]); print_sorted_commit_ids(list); + free_commit_list(list); } else if (!strcmp(av[1], "reduce_heads")) { struct commit_list *list = reduce_heads(X); printf("%s(X):\n", av[1]); print_sorted_commit_ids(list); + free_commit_list(list); } else if (!strcmp(av[1], "can_all_from_reach")) { printf("%s(X,Y):%d\n", av[1], can_all_from_reach(X, Y, 1)); } else if (!strcmp(av[1], "can_all_from_reach_with_flag")) { @@ -153,6 +155,7 @@ int cmd__reach(int ac, const char **av) filter.with_commit_tag_algo = 0; printf("%s(_,A,X,_):%d\n", av[1], commit_contains(&filter, A, X, &cache)); + clear_contains_cache(&cache); } else if (!strcmp(av[1], "get_reachable_subset")) { const int reachable_flag = 1; int i, count = 0; @@ -176,7 +179,14 @@ int cmd__reach(int ac, const char **av) die(_("too many commits marked reachable")); print_sorted_commit_ids(list); + free_commit_list(list); } + object_array_clear(&X_obj); + strbuf_release(&buf); + free_commit_list(X); + free_commit_list(Y); + free(X_array); + free(Y_array); return 0; } diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh index 2591f8b8b3..307deefed2 100755 --- a/t/t6600-test-reach.sh +++ b/t/t6600-test-reach.sh @@ -2,6 +2,7 @@ test_description='basic commit reachability tests' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Construct a grid-like commit graph with points (x,y) From a6590ccdd431e2ab7b9c521cac674546725a54d2 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:16:50 +0100 Subject: [PATCH 03/22] grep: fix leak in `grep_splice_or()` In `grep_splice_or()` we search for the next `TRUE` node in our tree of grep expressions and replace it with the given new expression. But we don't free the old node, which causes a memory leak. Plug it. This leak is exposed by t7810, but plugging it alone isn't sufficient to make the test suite pass. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- grep.c | 1 + 1 file changed, 1 insertion(+) diff --git a/grep.c b/grep.c index 701e58de04..e9337f32cb 100644 --- a/grep.c +++ b/grep.c @@ -756,6 +756,7 @@ static struct grep_expr *grep_splice_or(struct grep_expr *x, struct grep_expr *y assert(x->node == GREP_NODE_OR); if (x->u.binary.right && x->u.binary.right->node == GREP_NODE_TRUE) { + free(x->u.binary.right); x->u.binary.right = y; break; } From 43fedde3dfcd1e0ccd638f1a8c70c39219680fb1 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:16:52 +0100 Subject: [PATCH 04/22] builtin/grep: fix leak with `--max-count=0` When executing with `--max-count=0` we'll return early from git-grep(1) without performing any cleanup, which causes memory leaks. Plug these. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/grep.c | 13 ++++++++++--- t/t7810-grep.sh | 1 + 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index f17d46a06e..98b85c7fca 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -906,6 +906,7 @@ int cmd_grep(int argc, int dummy; int use_index = 1; int allow_revs; + int ret; struct option options[] = { OPT_BOOL(0, "cached", &cached, @@ -1172,8 +1173,10 @@ int cmd_grep(int argc, * Optimize out the case where the amount of matches is limited to zero. * We do this to keep results consistent with GNU grep(1). */ - if (opt.max_count == 0) - return 1; + if (opt.max_count == 0) { + ret = 1; + goto out; + } if (show_in_pager) { if (num_threads > 1) @@ -1267,10 +1270,14 @@ int cmd_grep(int argc, hit |= wait_all(); if (hit && show_in_pager) run_pager(&opt, prefix); + + ret = !hit; + +out: clear_pathspec(&pathspec); string_list_clear(&path_list, 0); free_grep_patterns(&opt); object_array_clear(&list); free_repos(); - return !hit; + return ret; } diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh index af2cf2f78a..9e7681f083 100755 --- a/t/t7810-grep.sh +++ b/t/t7810-grep.sh @@ -9,6 +9,7 @@ test_description='git grep various. GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_invalid_grep_expression() { From e29ff075e082ecdc6ee85baea83d32d4a055d186 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:16:58 +0100 Subject: [PATCH 05/22] revision: fix leaking bloom filters The memory allocated by `prepare_to_use_bloom_filter()` is not released by `release_revisions()`, causing a memory leak. Plug it. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- revision.c | 5 +++++ t/t4216-log-bloom.sh | 1 + 2 files changed, 6 insertions(+) diff --git a/revision.c b/revision.c index f5f5b84f2b..8df75b8224 100644 --- a/revision.c +++ b/revision.c @@ -3227,6 +3227,11 @@ void release_revisions(struct rev_info *revs) clear_decoration(&revs->treesame, free); line_log_free(revs); oidset_clear(&revs->missing_commits); + + for (int i = 0; i < revs->bloom_keys_nr; i++) + clear_bloom_key(&revs->bloom_keys[i]); + FREE_AND_NULL(revs->bloom_keys); + revs->bloom_keys_nr = 0; } static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child) diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh index 3f163dc396..8d22338f6a 100755 --- a/t/t4216-log-bloom.sh +++ b/t/t4216-log-bloom.sh @@ -4,6 +4,7 @@ test_description='git log for a path with Bloom filters' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-chunk.sh From 8dd3cb4b45d06959f3344f02b2d45a4ccf8207d8 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:17:00 +0100 Subject: [PATCH 06/22] diff-lib: fix leaking diffopts in `do_diff_cache()` In `do_diff_cache()` we initialize a new `rev_info` and then overwrite its `diffopt` with a user-provided set of options. This can leak memory because `repo_init_revisions()` may end up allocating memory for the `diffopt` itself depending on the configuration. And since that field is overwritten we won't ever free it. Plug the memory leak by releasing the diffopts before we overwrite them. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- diff-lib.c | 1 + t/t7610-mergetool.sh | 1 + 2 files changed, 2 insertions(+) diff --git a/diff-lib.c b/diff-lib.c index 6b14b95962..3cf353946f 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -661,6 +661,7 @@ int do_diff_cache(const struct object_id *tree_oid, struct diff_options *opt) repo_init_revisions(opt->repo, &revs, NULL); copy_pathspec(&revs.prune_data, &opt->pathspec); + diff_free(&revs.diffopt); revs.diffopt = *opt; revs.diffopt.no_free = 1; diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh index 22b3a85b3e..5c5e79e990 100755 --- a/t/t7610-mergetool.sh +++ b/t/t7610-mergetool.sh @@ -10,6 +10,7 @@ Testing basic merge tool invocation' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # All the mergetool test work by checking out a temporary branch based From 0b20a28811390ad8a1f7e22928018e5241738446 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:17:03 +0100 Subject: [PATCH 07/22] pretty: clear signature check The signature check in the formatting context is never getting released. Fix this to plug the resulting memory leak. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- pretty.c | 1 + t/t4202-log.sh | 1 + t/t7031-verify-tag-signed-ssh.sh | 1 + t/t7510-signed-commit.sh | 1 + t/t7528-signed-commit-ssh.sh | 1 + 5 files changed, 5 insertions(+) diff --git a/pretty.c b/pretty.c index 6403e26890..098378720a 100644 --- a/pretty.c +++ b/pretty.c @@ -2032,6 +2032,7 @@ void repo_format_commit_message(struct repository *r, free(context.commit_encoding); repo_unuse_commit_buffer(r, commit, context.message); + signature_check_clear(&context.signature_check); } static void pp_header(struct pretty_print_context *pp, diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 51f7beb59f..35bec4089a 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -5,6 +5,7 @@ test_description='git log' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY/lib-gpg.sh" . "$TEST_DIRECTORY/lib-terminal.sh" diff --git a/t/t7031-verify-tag-signed-ssh.sh b/t/t7031-verify-tag-signed-ssh.sh index 20913b3713..2ee62c0729 100755 --- a/t/t7031-verify-tag-signed-ssh.sh +++ b/t/t7031-verify-tag-signed-ssh.sh @@ -4,6 +4,7 @@ test_description='signed tag tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY/lib-gpg.sh" diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh index 0d2dd29fe6..eb229082e4 100755 --- a/t/t7510-signed-commit.sh +++ b/t/t7510-signed-commit.sh @@ -4,6 +4,7 @@ test_description='signed commit tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh GNUPGHOME_NOT_USED=$GNUPGHOME . "$TEST_DIRECTORY/lib-gpg.sh" diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh index 065f780636..68e18856b6 100755 --- a/t/t7528-signed-commit-ssh.sh +++ b/t/t7528-signed-commit-ssh.sh @@ -4,6 +4,7 @@ test_description='ssh signed commit tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh GNUPGHOME_NOT_USED=$GNUPGHOME . "$TEST_DIRECTORY/lib-gpg.sh" From 3b373150c8ea52ed64db0e7b0bd5a9b483e99a8e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:17:06 +0100 Subject: [PATCH 08/22] upload-pack: fix leaking URI protocols We don't clear `struct upload_pack::uri_protocols`, which causes a memory leak. Fix this. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/t5702-protocol-v2.sh | 1 + upload-pack.c | 1 + 2 files changed, 2 insertions(+) diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index d3df81e785..e4ce059236 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -7,6 +7,7 @@ TEST_NO_CREATE_REPO=1 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Test protocol v2 with 'git://' transport diff --git a/upload-pack.c b/upload-pack.c index 6d6e0f9f98..b4a59c3518 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -166,6 +166,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data) object_array_clear(&data->extra_edge_obj); list_objects_filter_release(&data->filter_options); string_list_clear(&data->allowed_filters, 0); + string_list_clear(&data->uri_protocols, 0); free((char *)data->pack_objects_hook); } From d34b5cbf028ffda45928e50884a6ef3aa533e6e5 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:17:09 +0100 Subject: [PATCH 09/22] builtin/commit: fix leaking change data contents While we free the worktree change data, we never free its contents. Fix this. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/commit.c | 9 ++++++++- t/t7500-commit-template-squash-signoff.sh | 1 + 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/builtin/commit.c b/builtin/commit.c index 8db4e9df0c..18a55bd1b9 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -728,6 +728,13 @@ static void prepare_amend_commit(struct commit *commit, struct strbuf *sb, repo_unuse_commit_buffer(the_repository, commit, buffer); } +static void change_data_free(void *util, const char *str UNUSED) +{ + struct wt_status_change_data *d = util; + free(d->rename_source); + free(d); +} + static int prepare_to_commit(const char *index_file, const char *prefix, struct commit *current_head, struct wt_status *s, @@ -991,7 +998,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, s->use_color = 0; committable = run_status(s->fp, index_file, prefix, 1, s); s->use_color = saved_color_setting; - string_list_clear(&s->change, 1); + string_list_clear_func(&s->change, change_data_free); } else { struct object_id oid; const char *parent = "HEAD"; diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh index 4dca8d97a7..379d3ed341 100755 --- a/t/t7500-commit-template-squash-signoff.sh +++ b/t/t7500-commit-template-squash-signoff.sh @@ -7,6 +7,7 @@ test_description='git commit Tests for template, signoff, squash and -F functions.' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-rebase.sh From 3f692fe5beb817fbb22754281dfb5ebf8863d0a3 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:17:12 +0100 Subject: [PATCH 10/22] trailer: fix leaking trailer values Fix leaking trailer values when replacing the value with a command or when the token value is empty. This leak is exposed by t7513, but plugging it does not make the whole test suite pass. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- trailer.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/trailer.c b/trailer.c index 682d74505b..6bafe92b32 100644 --- a/trailer.c +++ b/trailer.c @@ -249,7 +249,9 @@ static char *apply_command(struct conf_info *conf, const char *arg) static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok) { if (arg_tok->conf.command || arg_tok->conf.cmd) { - const char *arg; + char *value_to_free = NULL; + char *arg; + if (arg_tok->value && arg_tok->value[0]) { arg = arg_tok->value; } else { @@ -257,9 +259,13 @@ static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg arg = xstrdup(in_tok->value); else arg = xstrdup(""); + value_to_free = arg_tok->value; } + arg_tok->value = apply_command(&arg_tok->conf, arg); - free((char *)arg); + + free(value_to_free); + free(arg); } } From ff31b7b941286d91d03ddf4faac22b99149ea4b1 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:17:17 +0100 Subject: [PATCH 11/22] trailer: fix leaking strbufs when formatting trailers When formatting trailer lines we iterate through each of the trailers and munge their respective token/value pairs according to the trailer options. When formatting a trailer that has its `item->token` pointer set we perform the munging in two local buffers. In the case where we figure out that the value is empty and `trim_empty` is set we just skip over the trailer item. But the buffers are local to the loop and we don't release their contents, leading to a memory leak. Plug this leak by lifting the buffers outside of the loop and releasing them on function return. This fixes the memory leaks, but also optimizes the loop as we don't have to reallocate the buffers on every single iteration. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/t7513-interpret-trailers.sh | 1 + trailer.c | 12 +++++++----- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh index 0f7d8938d9..38d6ccaa00 100755 --- a/t/t7513-interpret-trailers.sh +++ b/t/t7513-interpret-trailers.sh @@ -5,6 +5,7 @@ test_description='git interpret-trailers' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # When we want one trailing space at the end of each line, let's use sed diff --git a/trailer.c b/trailer.c index 6bafe92b32..8ba350404d 100644 --- a/trailer.c +++ b/trailer.c @@ -1111,6 +1111,8 @@ void format_trailers(const struct process_trailer_options *opts, struct list_head *trailers, struct strbuf *out) { + struct strbuf tok = STRBUF_INIT; + struct strbuf val = STRBUF_INIT; size_t origlen = out->len; struct list_head *pos; struct trailer_item *item; @@ -1118,9 +1120,9 @@ void format_trailers(const struct process_trailer_options *opts, list_for_each(pos, trailers) { item = list_entry(pos, struct trailer_item, list); if (item->token) { - struct strbuf tok = STRBUF_INIT; - struct strbuf val = STRBUF_INIT; + strbuf_reset(&tok); strbuf_addstr(&tok, item->token); + strbuf_reset(&val); strbuf_addstr(&val, item->value); /* @@ -1151,9 +1153,6 @@ void format_trailers(const struct process_trailer_options *opts, if (!opts->separator) strbuf_addch(out, '\n'); } - strbuf_release(&tok); - strbuf_release(&val); - } else if (!opts->only_trailers) { if (opts->separator && out->len != origlen) { strbuf_addbuf(out, opts->separator); @@ -1165,6 +1164,9 @@ void format_trailers(const struct process_trailer_options *opts, strbuf_addch(out, '\n'); } } + + strbuf_release(&tok); + strbuf_release(&val); } void format_trailers_from_commit(const struct process_trailer_options *opts, From 6ef9f77a15fcb198b59840a9ed3a8f88da5ad53d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:17:20 +0100 Subject: [PATCH 12/22] builtin/commit: fix leaking cleanup config The cleanup string set by the config is leaking when it is being overridden by an option. Fix this by tracking these via two separate variables such that we can free the old value. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/commit.c | 17 ++++++++++++----- t/t7502-commit-porcelain.sh | 1 + 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 18a55bd1b9..71d674138c 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -135,7 +135,7 @@ static struct strvec trailer_args = STRVEC_INIT; * is specified explicitly. */ static enum commit_msg_cleanup_mode cleanup_mode; -static char *cleanup_arg; +static char *cleanup_config; static enum commit_whence whence; static int use_editor = 1, include_status = 1; @@ -1387,8 +1387,6 @@ static int parse_and_validate_options(int argc, const char *argv[], if (0 <= edit_flag) use_editor = edit_flag; - cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor); - handle_untracked_files_arg(s); if (all && argc > 0) @@ -1636,8 +1634,10 @@ static int git_commit_config(const char *k, const char *v, include_status = git_config_bool(k, v); return 0; } - if (!strcmp(k, "commit.cleanup")) - return git_config_string(&cleanup_arg, k, v); + if (!strcmp(k, "commit.cleanup")) { + FREE_AND_NULL(cleanup_config); + return git_config_string(&cleanup_config, k, v); + } if (!strcmp(k, "commit.gpgsign")) { sign_commit = git_config_bool(k, v) ? "" : NULL; return 0; @@ -1658,6 +1658,7 @@ int cmd_commit(int argc, struct repository *repo UNUSED) { static struct wt_status s; + static const char *cleanup_arg = NULL; static struct option builtin_commit_options[] = { OPT__QUIET(&quiet, N_("suppress summary after successful commit")), OPT__VERBOSE(&verbose, N_("show diff in commit message template")), @@ -1757,6 +1758,12 @@ int cmd_commit(int argc, if (verbose == -1) verbose = (config_commit_verbose < 0) ? 0 : config_commit_verbose; + if (cleanup_arg) { + free(cleanup_config); + cleanup_config = xstrdup(cleanup_arg); + } + cleanup_mode = get_cleanup_mode(cleanup_config, use_editor); + if (dry_run) return dry_run_commit(argv, prefix, current_head, &s); index_file = prepare_index(argv, prefix, current_head, 0); diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh index b37e2018a7..84f1ff52b6 100755 --- a/t/t7502-commit-porcelain.sh +++ b/t/t7502-commit-porcelain.sh @@ -5,6 +5,7 @@ test_description='git commit porcelain-ish' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh commit_msg_is () { From 1a99173de0618636fdd534f46b866a9bb39e487d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:17:23 +0100 Subject: [PATCH 13/22] transport-helper: fix leaking import/export marks Fix leaking import and export marks for transport helpers. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/t5801-remote-helpers.sh | 1 + transport-helper.c | 2 ++ 2 files changed, 3 insertions(+) diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index d21877150e..d4882288a3 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -8,6 +8,7 @@ test_description='Test remote-helper import and export commands' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-gpg.sh diff --git a/transport-helper.c b/transport-helper.c index 013ec79dc9..bc27653cde 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -399,6 +399,8 @@ static int release_helper(struct transport *transport) int res = 0; struct helper_data *data = transport->data; refspec_clear(&data->rs); + free(data->import_marks); + free(data->export_marks); res = disconnect_helper(transport); free(transport->data); return res; From d06e3ec858d4863baa5f918db414e890b08891d7 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:17:26 +0100 Subject: [PATCH 14/22] builtin/tag: fix leaking key ID on failure to sign We do not free the key ID when signing a tag fails. Do so by using the common exit path. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/tag.c | 2 +- t/t7004-tag.sh | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/tag.c b/builtin/tag.c index 93d10d5915..c37c0a68fd 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -164,7 +164,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid, int ret = -1; if (sign_buffer(buffer, &sig, keyid)) - return -1; + goto out; if (compat) { const struct git_hash_algo *algo = the_repository->hash_algo; diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index b1316e62f4..42b3327e69 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -10,6 +10,7 @@ Tests for operations with tags.' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-gpg.sh . "$TEST_DIRECTORY"/lib-terminal.sh From 1981d1eb3ecdefd2cdc665184d366728721f76e5 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:17:28 +0100 Subject: [PATCH 15/22] combine-diff: fix leaking lost lines The `cnt` variable tracks the number of lines in a patch diff. It can happen though that there are no newlines, in which case we'd still end up allocating our array of `sline`s. In fact, we always allocate it with `cnt + 2` entries: one extra entry for the deletion hunk at the end, and another entry that we don't seem to ever populate at all but acts as a kind of sentinel value. When we loop through the array to clear it at the end of this function we only loop until `lno < cnt`, and thus we may not end up releasing whatever the two extra `sline`s contain. While that shouldn't matter for the sentinel value, it does matter for the extra deletion hunk sline. Regardless of that, plug this memory leak by releasing both extra entries, which makes the logic a bit easier to reason about. While at it, fix the formatting of a local comment, which incidentally also provides the necessary context for why we overallocate the `sline` array. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- combine-diff.c | 5 +++-- t/t4038-diff-combined.sh | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/combine-diff.c b/combine-diff.c index f6b624dc28..33d0ed7097 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -1185,7 +1185,8 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent, result_file.ptr = result; result_file.size = result_size; - /* Even p_lno[cnt+1] is valid -- that is for the end line number + /* + * Even p_lno[cnt+1] is valid -- that is for the end line number * for deletion hunk at the end. */ CALLOC_ARRAY(sline[0].p_lno, st_mult(st_add(cnt, 2), num_parent)); @@ -1220,7 +1221,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent, } free(result); - for (lno = 0; lno < cnt; lno++) { + for (lno = 0; lno < cnt + 2; lno++) { if (sline[lno].lost) { struct lline *ll = sline[lno].lost; while (ll) { diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh index 2ce26e585c..00190802d8 100755 --- a/t/t4038-diff-combined.sh +++ b/t/t4038-diff-combined.sh @@ -5,6 +5,7 @@ test_description='combined diff' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-diff.sh From e4ba54d47b675e0613a2644f37c0b4f57a833a44 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:17:35 +0100 Subject: [PATCH 16/22] dir: release untracked cache data There are several cases where we invalidate untracked cache directory entries where we do not free the underlying data, but reset the number of entries. This causes us to leak memory because `free_untracked()` will not iterate over any potential entries which we still had in the array. Fix this issue by freeing old entries. The leak is exposed by t7519, but plugging it alone does not make the whole test suite pass. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- dir.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/dir.c b/dir.c index e3ddd5b529..cb9782fa11 100644 --- a/dir.c +++ b/dir.c @@ -1056,6 +1056,8 @@ static void do_invalidate_gitignore(struct untracked_cache_dir *dir) { int i; dir->valid = 0; + for (size_t i = 0; i < dir->untracked_nr; i++) + free(dir->untracked[i]); dir->untracked_nr = 0; for (i = 0; i < dir->dirs_nr; i++) do_invalidate_gitignore(dir->dirs[i]); @@ -1083,6 +1085,8 @@ static void invalidate_directory(struct untracked_cache *uc, uc->dir_invalidated++; dir->valid = 0; + for (size_t i = 0; i < dir->untracked_nr; i++) + free(dir->untracked[i]); dir->untracked_nr = 0; for (i = 0; i < dir->dirs_nr; i++) dir->dirs[i]->recurse = 0; @@ -3573,6 +3577,8 @@ static void write_one_dir(struct untracked_cache_dir *untracked, * for safety.. */ if (!untracked->valid) { + for (size_t i = 0; i < untracked->untracked_nr; i++) + free(untracked->untracked[i]); untracked->untracked_nr = 0; untracked->check_only = 0; } @@ -3905,6 +3911,8 @@ static void invalidate_one_directory(struct untracked_cache *uc, { uc->dir_invalidated++; ucd->valid = 0; + for (size_t i = 0; i < ucd->untracked_nr; i++) + free(ucd->untracked[i]); ucd->untracked_nr = 0; } From 1f5ff83eab03773692fe6f7bab7f10ad82ab031b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:17:38 +0100 Subject: [PATCH 17/22] sparse-index: correctly free EWAH contents While we free the `fsmonitor_dirty` member of `struct index_state`, we do not free the contents of that EWAH. Do so by using `ewah_free()` instead of `FREE_AND_NULL()`. This leak is exposed by t7519, but plugging it alone does not make the test suite pass. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- sparse-index.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/sparse-index.c b/sparse-index.c index 3d7f2164e2..2107840bfc 100644 --- a/sparse-index.c +++ b/sparse-index.c @@ -2,6 +2,7 @@ #include "git-compat-util.h" #include "environment.h" +#include "ewah/ewok.h" #include "gettext.h" #include "name-hash.h" #include "read-cache-ll.h" @@ -242,7 +243,8 @@ int convert_to_sparse(struct index_state *istate, int flags) cache_tree_update(istate, 0); istate->fsmonitor_has_run_once = 0; - FREE_AND_NULL(istate->fsmonitor_dirty); + ewah_free(istate->fsmonitor_dirty); + istate->fsmonitor_dirty = NULL; FREE_AND_NULL(istate->fsmonitor_last_update); istate->sparse_index = INDEX_COLLAPSED; @@ -438,7 +440,8 @@ void expand_index(struct index_state *istate, struct pattern_list *pl) istate->cache_nr = full->cache_nr; istate->cache_alloc = full->cache_alloc; istate->fsmonitor_has_run_once = 0; - FREE_AND_NULL(istate->fsmonitor_dirty); + ewah_free(istate->fsmonitor_dirty); + istate->fsmonitor_dirty = NULL; FREE_AND_NULL(istate->fsmonitor_last_update); strbuf_release(&base); From a53144cf1bfc5ef5b4d8b58311ea83e32e00a69f Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:17:40 +0100 Subject: [PATCH 18/22] t/helper: stop re-initialization of `the_repository` While "common-main.c" already initializes `the_repository` for us, we do so a second time in the "read-cache" test helper. This causes a memory leak because the old repository's contents isn't released. Stop calling `initialize_repository()` to plug this leak. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/helper/test-read-cache.c | 2 -- t/t7519-status-fsmonitor.sh | 1 + 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c index d285c656bd..e277dde8e7 100644 --- a/t/helper/test-read-cache.c +++ b/t/helper/test-read-cache.c @@ -11,8 +11,6 @@ int cmd__read_cache(int argc, const char **argv) int i, cnt = 1; const char *name = NULL; - initialize_repository(the_repository); - if (argc > 1 && skip_prefix(argv[1], "--print-and-refresh=", &name)) { argc--; argv++; diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh index 7ee69ecdd4..0f88a58a81 100755 --- a/t/t7519-status-fsmonitor.sh +++ b/t/t7519-status-fsmonitor.sh @@ -2,6 +2,7 @@ test_description='git status with file system watcher' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Note, after "git reset --hard HEAD" no extensions exist other than 'TREE' From 0bc0fcf0b27271722de4fac668658ad5318bec84 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:17:43 +0100 Subject: [PATCH 19/22] t/helper: fix leaking buffer in "dump-untracked-cache" We never release the local `struct strbuf base` buffer, thus leaking memory. Fix this leak. This leak is exposed by t7063, but plugging it alone does not make the whole test suite pass. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/helper/test-dump-untracked-cache.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c index 4f010d5324..b2e70837a9 100644 --- a/t/helper/test-dump-untracked-cache.c +++ b/t/helper/test-dump-untracked-cache.c @@ -68,5 +68,7 @@ int cmd__dump_untracked_cache(int ac UNUSED, const char **av UNUSED) printf("flags %08x\n", uc->dir_flags); if (uc->root) dump(uc->root, &base); + + strbuf_release(&base); return 0; } From 813b12b6f74d75ce5ad2e7453fb763a4db44bdf8 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:17:46 +0100 Subject: [PATCH 20/22] dir: fix leak when parsing "status.showUntrackedFiles" We use `repo_config_get_string()` to read "status.showUntrackedFiles" from the config subsystem. This function allocates the result, but we never free the result after parsing it. The value never leaves the scope of the calling function, so refactor it to instead use `repo_config_get_string_tmp()`, which does not hand over ownership to the caller. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- dir.c | 4 ++-- t/t7063-status-untracked-cache.sh | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/dir.c b/dir.c index cb9782fa11..7f35a3e317 100644 --- a/dir.c +++ b/dir.c @@ -2872,14 +2872,14 @@ static void set_untracked_ident(struct untracked_cache *uc) static unsigned new_untracked_cache_flags(struct index_state *istate) { struct repository *repo = istate->repo; - char *val; + const char *val; /* * This logic is coordinated with the setting of these flags in * wt-status.c#wt_status_collect_untracked(), and the evaluation * of the config setting in commit.c#git_status_config() */ - if (!repo_config_get_string(repo, "status.showuntrackedfiles", &val) && + if (!repo_config_get_string_tmp(repo, "status.showuntrackedfiles", &val) && !strcmp(val, "all")) return 0; diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh index 8929ef481f..13fea7931c 100755 --- a/t/t7063-status-untracked-cache.sh +++ b/t/t7063-status-untracked-cache.sh @@ -5,6 +5,7 @@ test_description='test untracked cache' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # On some filesystems (e.g. FreeBSD's ext2 and ufs) directory mtime From ff67083ccd77dc80002751c0f81ac50e122abd6c Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:17:49 +0100 Subject: [PATCH 21/22] builtin/merge: release output buffer after performing merge The `obuf` member of `struct merge_options` is used to buffer output in some cases. In order to not discard its allocated memory we only release its contents in `merge_finalize()` when we're not currently recursing into a subtree. This results in some situations where we seemingly do not release the buffer reliably. We thus have calls to `strbuf_release()` for this buffer scattered across the codebase. But we're missing one callsite in git-merge(1), which causes a memory leak. We should ideally refactor this interface so that callers don't have to know about any such internals. But for now, paper over the issue by adding one more `strbuf_release()` call. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/merge.c | 1 + t/t6424-merge-unrelated-index-changes.sh | 1 + 2 files changed, 2 insertions(+) diff --git a/builtin/merge.c b/builtin/merge.c index 84d0f3604b..51038eaca8 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -754,6 +754,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, clean = merge_recursive(&o, head, remoteheads->item, reversed, &result); free_commit_list(reversed); + strbuf_release(&o.obuf); if (clean < 0) { rollback_lock_file(&lock); diff --git a/t/t6424-merge-unrelated-index-changes.sh b/t/t6424-merge-unrelated-index-changes.sh index 7677c5f08d..a7ea8acb84 100755 --- a/t/t6424-merge-unrelated-index-changes.sh +++ b/t/t6424-merge-unrelated-index-changes.sh @@ -2,6 +2,7 @@ test_description="merges with unrelated index changes" +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Testcase for some simple merges From c810549be1dd31e1a0a1de5060a519a461533564 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 5 Nov 2024 07:17:54 +0100 Subject: [PATCH 22/22] list-objects-filter-options: work around reported leak on error This one is a little bit more curious. In t6112, we have a test that exercises the `git rev-list --filter` option with invalid filters. We execute git-rev-list(1) via `test_must_fail`, which means that we check for leaks even though Git exits with an error code. This causes the following leak: Direct leak of 27 byte(s) in 1 object(s) allocated from: #0 0x5555555e6946 in realloc.part.0 lsan_interceptors.cpp.o #1 0x5555558fb4b6 in xrealloc wrapper.c:137:8 #2 0x5555558b6e06 in strbuf_grow strbuf.c:112:2 #3 0x5555558b7550 in strbuf_add strbuf.c:311:2 #4 0x5555557c1a88 in strbuf_addstr strbuf.h:310:2 #5 0x5555557c1d4c in parse_list_objects_filter list-objects-filter-options.c:261:3 #6 0x555555885ead in handle_revision_pseudo_opt revision.c:2899:3 #7 0x555555884e20 in setup_revisions revision.c:3014:11 #8 0x5555556c4b42 in cmd_rev_list builtin/rev-list.c:588:9 #9 0x5555555ec5e3 in run_builtin git.c:483:11 #10 0x5555555eb1e4 in handle_builtin git.c:749:13 #11 0x5555555ec001 in run_argv git.c:819:4 #12 0x5555555eaf94 in cmd_main git.c:954:19 #13 0x5555556fd569 in main common-main.c:64:11 #14 0x7ffff7ca714d in __libc_start_call_main (.../lib/libc.so.6+0x2a14d) #15 0x7ffff7ca7208 in __libc_start_main@GLIBC_2.2.5 (.../libc.so.6+0x2a208) #16 0x5555555ad064 in _start (git+0x59064) This leak is valid, as we call `die()` and do not clean up the memory at all. But what's curious is that this is the only leak reported, because we don't clean up any other allocated memory, either, and I have no idea why the leak sanitizer treats this buffer specially. In any case, we can work around the leak by shuffling things around a bit. Instead of calling `gently_parse_list_objects_filter()` and dying after we have modified the filter spec, we simply do so beforehand. Like this we don't allocate the buffer in the error case, which makes the reported leak go away. It's not pretty, but it manages to make t6112 leak free. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- list-objects-filter-options.c | 17 +++++++---------- t/t6112-rev-list-filters-objects.sh | 1 + 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c index 00611107d2..fa72e81e4a 100644 --- a/list-objects-filter-options.c +++ b/list-objects-filter-options.c @@ -252,16 +252,14 @@ void parse_list_objects_filter( const char *arg) { struct strbuf errbuf = STRBUF_INIT; - int parse_error; if (!filter_options->filter_spec.buf) BUG("filter_options not properly initialized"); if (!filter_options->choice) { + if (gently_parse_list_objects_filter(filter_options, arg, &errbuf)) + die("%s", errbuf.buf); strbuf_addstr(&filter_options->filter_spec, arg); - - parse_error = gently_parse_list_objects_filter( - filter_options, arg, &errbuf); } else { struct list_objects_filter_options *sub; @@ -271,18 +269,17 @@ void parse_list_objects_filter( */ transform_to_combine_type(filter_options); - strbuf_addch(&filter_options->filter_spec, '+'); - filter_spec_append_urlencode(filter_options, arg); ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1, filter_options->sub_alloc); sub = &filter_options->sub[filter_options->sub_nr - 1]; list_objects_filter_init(sub); - parse_error = gently_parse_list_objects_filter(sub, arg, - &errbuf); + if (gently_parse_list_objects_filter(sub, arg, &errbuf)) + die("%s", errbuf.buf); + + strbuf_addch(&filter_options->filter_spec, '+'); + filter_spec_append_urlencode(filter_options, arg); } - if (parse_error) - die("%s", errbuf.buf); } int opt_parse_list_objects_filter(const struct option *opt, diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh index 0387f35a32..71e38491fa 100755 --- a/t/t6112-rev-list-filters-objects.sh +++ b/t/t6112-rev-list-filters-objects.sh @@ -5,6 +5,7 @@ test_description='git rev-list using object filtering' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Test the blob:none filter.