From 56931c4d89e7efe41dd96d230b8176e2d32a035d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:19:17 +0200 Subject: [PATCH 01/29] revision: fix memory leak when reversing revisions When reversing revisions in a rev walk, `get_revision()` will allocate a new commit list and assign it to `revs->commits`. It does not free the old list though, which makes it leak. Fix this. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- revision.c | 1 + t/t3508-cherry-pick-many-commits.sh | 1 + 2 files changed, 2 insertions(+) diff --git a/revision.c b/revision.c index 7ddf0f151a3..af95502d925 100644 --- a/revision.c +++ b/revision.c @@ -4430,6 +4430,7 @@ struct commit *get_revision(struct rev_info *revs) reversed = NULL; while ((c = get_revision_internal(revs))) commit_list_insert(c, &reversed); + free_commit_list(revs->commits); revs->commits = reversed; revs->reverse = 0; revs->reverse_output_stage = 1; diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh index 2d53ce754c5..afa7727a4af 100755 --- a/t/t3508-cherry-pick-many-commits.sh +++ b/t/t3508-cherry-pick-many-commits.sh @@ -5,6 +5,7 @@ test_description='test cherry-picking many commits' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh check_head_differs_from() { From 14da26230a7644a2f9dfbc3f43d9d7ab6e0074e9 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:19:22 +0200 Subject: [PATCH 02/29] parse-options: fix leaks for users of OPT_FILENAME The `OPT_FILENAME()` option will, if set, put an allocated string into the user-provided variable. Consequently, that variable thus needs to be free'd by the caller of `parse_options()`. Some callsites don't though and thus leak memory. Fix those. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- apply.c | 1 + apply.h | 2 +- builtin/archive.c | 7 +++++-- builtin/commit.c | 7 +++++-- builtin/fmt-merge-msg.c | 4 +++- builtin/log.c | 4 +++- builtin/multi-pack-index.c | 13 +++++++++---- builtin/sparse-checkout.c | 1 + t/helper/test-parse-options.c | 1 + t/t1512-rev-parse-disambiguation.sh | 1 + t/t2500-untracked-overwriting.sh | 1 + t/t3406-rebase-message.sh | 1 + t/t3407-rebase-abort.sh | 1 + t/t3428-rebase-signoff.sh | 1 + t/t4131-apply-fake-ancestor.sh | 1 + t/t4151-am-abort.sh | 1 + t/t4253-am-keep-cr-dos.sh | 1 + t/t4255-am-submodule.sh | 1 + t/t5407-post-rewrite-hook.sh | 1 + t/t6427-diff3-conflict-markers.sh | 1 + t/t7512-status-help.sh | 1 + 21 files changed, 41 insertions(+), 11 deletions(-) diff --git a/apply.c b/apply.c index 901b67e6255..d8d26a48f13 100644 --- a/apply.c +++ b/apply.c @@ -135,6 +135,7 @@ void clear_apply_state(struct apply_state *state) strset_clear(&state->removed_symlinks); strset_clear(&state->kept_symlinks); strbuf_release(&state->root); + FREE_AND_NULL(state->fake_ancestor); /* &state->fn_table is cleared at the end of apply_patch() */ } diff --git a/apply.h b/apply.h index 7cd38b1443c..36d7c3f70b4 100644 --- a/apply.h +++ b/apply.h @@ -59,7 +59,7 @@ struct apply_state { struct repository *repo; const char *index_file; enum apply_verbosity apply_verbosity; - const char *fake_ancestor; + char *fake_ancestor; const char *patch_input_file; int line_termination; struct strbuf root; diff --git a/builtin/archive.c b/builtin/archive.c index 15ee1ec7bb7..f29c0ef6ad3 100644 --- a/builtin/archive.c +++ b/builtin/archive.c @@ -92,6 +92,7 @@ int cmd_archive(int argc, const char **argv, const char *prefix) N_("path to the remote git-upload-archive command")), OPT_END() }; + int ret; argc = parse_options(argc, argv, prefix, local_opts, NULL, PARSE_OPT_KEEP_ALL); @@ -106,6 +107,8 @@ int cmd_archive(int argc, const char **argv, const char *prefix) setvbuf(stderr, NULL, _IOLBF, BUFSIZ); - UNLEAK(output); - return write_archive(argc, argv, prefix, the_repository, output, 0); + ret = write_archive(argc, argv, prefix, the_repository, output, 0); + + free(output); + return ret; } diff --git a/builtin/commit.c b/builtin/commit.c index f53e7e86ff6..dcaf4efa035 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -106,7 +106,8 @@ static enum { COMMIT_PARTIAL } commit_style; -static const char *logfile, *force_author; +static const char *force_author; +static char *logfile; static char *template_file; /* * The _message variables are commit names from which to take @@ -1309,7 +1310,7 @@ static int parse_and_validate_options(int argc, const char *argv[], !!use_message, "-C", !!logfile, "-F"); if (use_message || edit_message || logfile ||fixup_message || have_option_m) - template_file = NULL; + FREE_AND_NULL(template_file); if (edit_message) use_message = edit_message; if (amend && !use_message && !fixup_message) @@ -1892,5 +1893,7 @@ cleanup: strbuf_release(&author_ident); strbuf_release(&err); strbuf_release(&sb); + free(logfile); + free(template_file); return ret; } diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index 0f9855b680e..957786d1b3a 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -11,7 +11,7 @@ static const char * const fmt_merge_msg_usage[] = { int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix) { - const char *inpath = NULL; + char *inpath = NULL; const char *message = NULL; char *into_name = NULL; int shortlog_len = -1; @@ -66,5 +66,7 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix) if (ret) return ret; write_in_full(STDOUT_FILENO, output.buf, output.len); + + free(inpath); return 0; } diff --git a/builtin/log.c b/builtin/log.c index 78a247d8a94..4e4b645a21d 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -2021,7 +2021,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) const char *rfc = NULL; int creation_factor = -1; const char *signature = git_version_string; - const char *signature_file_arg = NULL; + char *signature_file_arg = NULL; struct keep_callback_data keep_callback_data = { .cfg = &cfg, .revs = &rev, @@ -2559,6 +2559,8 @@ done: strbuf_release(&rdiff1); strbuf_release(&rdiff2); strbuf_release(&rdiff_title); + free(description_file); + free(signature_file_arg); free(to_free); free(rev.message_id); if (rev.ref_message_ids) diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c index 8360932d2e7..9cf1a32d653 100644 --- a/builtin/multi-pack-index.c +++ b/builtin/multi-pack-index.c @@ -50,7 +50,7 @@ static char const * const builtin_multi_pack_index_usage[] = { static struct opts_multi_pack_index { char *object_dir; const char *preferred_pack; - const char *refs_snapshot; + char *refs_snapshot; unsigned long batch_size; unsigned flags; int stdin_packs; @@ -135,6 +135,7 @@ static int cmd_multi_pack_index_write(int argc, const char **argv, N_("refs snapshot for selecting bitmap commits")), OPT_END(), }; + int ret; opts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE; @@ -157,7 +158,6 @@ static int cmd_multi_pack_index_write(int argc, const char **argv, if (opts.stdin_packs) { struct string_list packs = STRING_LIST_INIT_DUP; - int ret; read_packs_from_stdin(&packs); @@ -166,12 +166,17 @@ static int cmd_multi_pack_index_write(int argc, const char **argv, opts.refs_snapshot, opts.flags); string_list_clear(&packs, 0); + free(opts.refs_snapshot); return ret; } - return write_midx_file(opts.object_dir, opts.preferred_pack, - opts.refs_snapshot, opts.flags); + + ret = write_midx_file(opts.object_dir, opts.preferred_pack, + opts.refs_snapshot, opts.flags); + + free(opts.refs_snapshot); + return ret; } static int cmd_multi_pack_index_verify(int argc, const char **argv, diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index 0f52e252493..84a6adf681f 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -1011,6 +1011,7 @@ static int sparse_checkout_check_rules(int argc, const char **argv, const char * ret = check_rules(&pl, check_rules_opts.null_termination); clear_pattern_list(&pl); + free(check_rules_opts.rules_file); return ret; } diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c index ded8116cc51..5250913d99e 100644 --- a/t/helper/test-parse-options.c +++ b/t/helper/test-parse-options.c @@ -207,6 +207,7 @@ int cmd__parse_options(int argc, const char **argv) expect.strdup_strings = 1; string_list_clear(&expect, 0); string_list_clear(&list, 0); + free(file); return ret; } diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh index 70f1e0a998e..f9d68ce74ea 100755 --- a/t/t1512-rev-parse-disambiguation.sh +++ b/t/t1512-rev-parse-disambiguation.sh @@ -23,6 +23,7 @@ one tagged as v1.0.0. They all have one regular file each. GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_cmp_failed_rev_parse () { diff --git a/t/t2500-untracked-overwriting.sh b/t/t2500-untracked-overwriting.sh index 5c0bf4d21fc..714feb83be5 100755 --- a/t/t2500-untracked-overwriting.sh +++ b/t/t2500-untracked-overwriting.sh @@ -2,6 +2,7 @@ test_description='Test handling of overwriting untracked files' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_setup_reset () { diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh index a1d7fa7f7c6..82108b67e67 100755 --- a/t/t3406-rebase-message.sh +++ b/t/t3406-rebase-message.sh @@ -5,6 +5,7 @@ test_description='messages from rebase operation' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh index 9f49c4228b6..2c3f38d45a8 100755 --- a/t/t3407-rebase-abort.sh +++ b/t/t3407-rebase-abort.sh @@ -5,6 +5,7 @@ test_description='git rebase --abort tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh index 6f57aed9fac..365436ebfc3 100755 --- a/t/t3428-rebase-signoff.sh +++ b/t/t3428-rebase-signoff.sh @@ -5,6 +5,7 @@ test_description='git rebase --signoff This test runs git rebase --signoff and make sure that it works. ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-rebase.sh diff --git a/t/t4131-apply-fake-ancestor.sh b/t/t4131-apply-fake-ancestor.sh index b1361ce5469..40c92115a66 100755 --- a/t/t4131-apply-fake-ancestor.sh +++ b/t/t4131-apply-fake-ancestor.sh @@ -5,6 +5,7 @@ test_description='git apply --build-fake-ancestor handling.' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh index edb38da7010..1825a89d6a9 100755 --- a/t/t4151-am-abort.sh +++ b/t/t4151-am-abort.sh @@ -2,6 +2,7 @@ test_description='am --abort' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t4253-am-keep-cr-dos.sh b/t/t4253-am-keep-cr-dos.sh index 0ee69d2a0c4..2bcdd9f34fa 100755 --- a/t/t4253-am-keep-cr-dos.sh +++ b/t/t4253-am-keep-cr-dos.sh @@ -9,6 +9,7 @@ test_description='git-am mbox with dos line ending. GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Three patches which will be added as files with dos line ending. diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh index a7ba08f728c..04f3ccfc41c 100755 --- a/t/t4255-am-submodule.sh +++ b/t/t4255-am-submodule.sh @@ -2,6 +2,7 @@ test_description='git am handling submodules' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-submodule-update.sh diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh index ad7f8c6f002..e99e728236a 100755 --- a/t/t5407-post-rewrite-hook.sh +++ b/t/t5407-post-rewrite-hook.sh @@ -7,6 +7,7 @@ test_description='Test the post-rewrite hook.' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t6427-diff3-conflict-markers.sh b/t/t6427-diff3-conflict-markers.sh index dd5fe6a4021..a13271b3490 100755 --- a/t/t6427-diff3-conflict-markers.sh +++ b/t/t6427-diff3-conflict-markers.sh @@ -5,6 +5,7 @@ test_description='recursive merge diff3 style conflict markers' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Setup: diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh index 802f8f704c6..cdd5f2c6979 100755 --- a/t/t7512-status-help.sh +++ b/t/t7512-status-help.sh @@ -10,6 +10,7 @@ test_description='git status advice' 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-rebase.sh From bb8c43d5cd9648f868e11e462392a38a43354692 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:19:26 +0200 Subject: [PATCH 03/29] notes-utils: free note trees when releasing copied notes While we clear most of the members of `struct notes_rewrite_cfg` in `finish_copy_notes_for_rewrite()`, we do not clear the notes tree. Fix this to plug this memory leak. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- notes-utils.c | 1 + t/t3400-rebase.sh | 1 + t/t7501-commit-basic-functionality.sh | 1 + 3 files changed, 3 insertions(+) diff --git a/notes-utils.c b/notes-utils.c index e33aa86c4b9..671d1969b1f 100644 --- a/notes-utils.c +++ b/notes-utils.c @@ -187,6 +187,7 @@ void finish_copy_notes_for_rewrite(struct repository *r, for (i = 0; c->trees[i]; i++) { commit_notes(r, c->trees[i], msg); free_notes(c->trees[i]); + free(c->trees[i]); } free(c->trees); free(c); diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index e1c8c5f7011..ae34bfad607 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -11,6 +11,7 @@ among other things. GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh GIT_AUTHOR_NAME=author@name diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh index cc12f99f115..52f5e28154e 100755 --- a/t/t7501-commit-basic-functionality.sh +++ b/t/t7501-commit-basic-functionality.sh @@ -9,6 +9,7 @@ test_description='git commit' 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 11ee9a75e7d6f149431f69400c81006f8ccecad5 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:19:31 +0200 Subject: [PATCH 04/29] bundle: plug leaks in `create_bundle()` When creating a bundle, we set up a revision walk, but never release data associated with it. Furthermore, we create a mostly-shallow copy of that revision walk where we only adapt its pending objects such that we can reuse the walk. While that copy must not be released, the pending objects array need to be. Plug those memory leaks by releasing the revision walk and the pending objects of the copied revision walk. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- bundle.c | 29 +++++++++++++++++++---------- t/t5605-clone-local.sh | 1 + t/t5607-clone-bundle.sh | 1 + t/t6020-bundle-misc.sh | 1 + 4 files changed, 22 insertions(+), 10 deletions(-) diff --git a/bundle.c b/bundle.c index 95367c2d0a0..9af558c7a9b 100644 --- a/bundle.c +++ b/bundle.c @@ -500,6 +500,7 @@ int create_bundle(struct repository *r, const char *path, struct rev_info revs, revs_copy; int min_version = 2; struct bundle_prerequisites_info bpi; + int ret; int i; /* init revs to list objects for pack-objects later */ @@ -525,8 +526,8 @@ int create_bundle(struct repository *r, const char *path, min_version = 3; if (argc > 1) { - error(_("unrecognized argument: %s"), argv[1]); - goto err; + ret = error(_("unrecognized argument: %s"), argv[1]); + goto out; } bundle_to_stdout = !strcmp(path, "-"); @@ -591,23 +592,31 @@ int create_bundle(struct repository *r, const char *path, /* write bundle refs */ ref_count = write_bundle_refs(bundle_fd, &revs_copy); - if (!ref_count) + if (!ref_count) { die(_("Refusing to create empty bundle.")); - else if (ref_count < 0) - goto err; + } else if (ref_count < 0) { + ret = -1; + goto out; + } /* write pack */ - if (write_pack_data(bundle_fd, &revs_copy, pack_options)) - goto err; + if (write_pack_data(bundle_fd, &revs_copy, pack_options)) { + ret = -1; + goto out; + } if (!bundle_to_stdout) { if (commit_lock_file(&lock)) die_errno(_("cannot create '%s'"), path); } - return 0; -err: + + ret = 0; + +out: + object_array_clear(&revs_copy.pending); + release_revisions(&revs); rollback_lock_file(&lock); - return -1; + return ret; } int unbundle(struct repository *r, struct bundle_header *header, diff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh index a3055869bc7..9a1390a98fb 100755 --- a/t/t5605-clone-local.sh +++ b/t/t5605-clone-local.sh @@ -4,6 +4,7 @@ test_description='test local clone' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh repo_is_hardlinked() { diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh index 0d1e92d9963..ac5ce9b648a 100755 --- a/t/t5607-clone-bundle.sh +++ b/t/t5607-clone-bundle.sh @@ -4,6 +4,7 @@ test_description='some bundle related tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh index 3e6bcbf30cd..fe75a06572a 100755 --- a/t/t6020-bundle-misc.sh +++ b/t/t6020-bundle-misc.sh @@ -8,6 +8,7 @@ test_description='Test git-bundle' 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-bundle.sh . "$TEST_DIRECTORY"/lib-terminal.sh From afb0653d2373dc72ad4a9c89b7d17b660d194f04 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:19:36 +0200 Subject: [PATCH 05/29] biultin/rev-parse: fix memory leaks in `--parseopt` mode We have a bunch of memory leaks in git-rev-parse(1)'s `--parseopt` mode. Refactor the code to use `struct strvec`s to make it easier for us to track the lifecycle of those leaking variables and then free them. While at it, remove the unneeded static lifetime for some of the variables. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/rev-parse.c | 53 +++++++++++++++++++++++------------------ t/t5150-request-pull.sh | 1 + t/t7006-pager.sh | 1 + 3 files changed, 32 insertions(+), 23 deletions(-) diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index 1e2919fd81c..ab8a8f3b0ed 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -423,12 +423,12 @@ static char *findspace(const char *s) static int cmd_parseopt(int argc, const char **argv, const char *prefix) { - static int keep_dashdash = 0, stop_at_non_option = 0; - static char const * const parseopt_usage[] = { + int keep_dashdash = 0, stop_at_non_option = 0; + char const * const parseopt_usage[] = { N_("git rev-parse --parseopt [] -- [...]"), NULL }; - static struct option parseopt_opts[] = { + struct option parseopt_opts[] = { OPT_BOOL(0, "keep-dashdash", &keep_dashdash, N_("keep the `--` passed as an arg")), OPT_BOOL(0, "stop-at-non-option", &stop_at_non_option, @@ -438,12 +438,11 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix) N_("output in stuck long form")), OPT_END(), }; - static const char * const flag_chars = "*=?!"; - struct strbuf sb = STRBUF_INIT, parsed = STRBUF_INIT; - const char **usage = NULL; + struct strvec longnames = STRVEC_INIT; + struct strvec usage = STRVEC_INIT; struct option *opts = NULL; - int onb = 0, osz = 0, unb = 0, usz = 0; + size_t opts_nr = 0, opts_alloc = 0; strbuf_addstr(&parsed, "set --"); argc = parse_options(argc, argv, prefix, parseopt_opts, parseopt_usage, @@ -453,16 +452,16 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix) /* get the usage up to the first line with a -- on it */ for (;;) { + strbuf_reset(&sb); if (strbuf_getline(&sb, stdin) == EOF) die(_("premature end of input")); - ALLOC_GROW(usage, unb + 1, usz); if (!strcmp("--", sb.buf)) { - if (unb < 1) + if (!usage.nr) die(_("no usage string given before the `--' separator")); - usage[unb] = NULL; break; } - usage[unb++] = strbuf_detach(&sb, NULL); + + strvec_push(&usage, sb.buf); } /* parse: (|,|)[*=?!]*? SP+ */ @@ -474,10 +473,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix) if (!sb.len) continue; - ALLOC_GROW(opts, onb + 1, osz); - memset(opts + onb, 0, sizeof(opts[onb])); + ALLOC_GROW(opts, opts_nr + 1, opts_alloc); + memset(opts + opts_nr, 0, sizeof(*opts)); - o = &opts[onb++]; + o = &opts[opts_nr++]; help = findspace(sb.buf); if (!help || sb.buf == help) { o->type = OPTION_GROUP; @@ -494,20 +493,22 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix) o->callback = &parseopt_dump; /* name(s) */ - s = strpbrk(sb.buf, flag_chars); + s = strpbrk(sb.buf, "*=?!"); if (!s) s = help; if (s == sb.buf) die(_("missing opt-spec before option flags")); - if (s - sb.buf == 1) /* short option only */ + if (s - sb.buf == 1) { /* short option only */ o->short_name = *sb.buf; - else if (sb.buf[1] != ',') /* long option only */ - o->long_name = xmemdupz(sb.buf, s - sb.buf); - else { + } else if (sb.buf[1] != ',') { /* long option only */ + o->long_name = strvec_pushf(&longnames, "%.*s", + (int)(s - sb.buf), sb.buf); + } else { o->short_name = *sb.buf; - o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2); + o->long_name = strvec_pushf(&longnames, "%.*s", + (int)(s - sb.buf - 2), sb.buf + 2); } /* flags */ @@ -537,9 +538,9 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix) strbuf_release(&sb); /* put an OPT_END() */ - ALLOC_GROW(opts, onb + 1, osz); - memset(opts + onb, 0, sizeof(opts[onb])); - argc = parse_options(argc, argv, prefix, opts, usage, + ALLOC_GROW(opts, opts_nr + 1, opts_alloc); + memset(opts + opts_nr, 0, sizeof(*opts)); + argc = parse_options(argc, argv, prefix, opts, usage.v, (keep_dashdash ? PARSE_OPT_KEEP_DASHDASH : 0) | (stop_at_non_option ? PARSE_OPT_STOP_AT_NON_OPTION : 0) | PARSE_OPT_SHELL_EVAL); @@ -547,7 +548,13 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix) strbuf_addstr(&parsed, " --"); sq_quote_argv(&parsed, argv); puts(parsed.buf); + strbuf_release(&parsed); + strbuf_release(&sb); + strvec_clear(&longnames); + strvec_clear(&usage); + free((char *) opts->help); + free(opts); return 0; } diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh index cb67bac1c47..86bee331607 100755 --- a/t/t5150-request-pull.sh +++ b/t/t5150-request-pull.sh @@ -5,6 +5,7 @@ test_description='Test workflows involving pull request.' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh if ! test_have_prereq PERL diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh index e56ca5b0fa8..60e4c90de1b 100755 --- a/t/t7006-pager.sh +++ b/t/t7006-pager.sh @@ -2,6 +2,7 @@ test_description='Test automatic use of a pager.' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-pager.sh . "$TEST_DIRECTORY"/lib-terminal.sh From 3d31d382557527bf945a088931e3e28a717cc547 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:19:40 +0200 Subject: [PATCH 06/29] merge-recursive: fix leaking rename conflict info When computing rename conflicts in our recursive merge algorithm we set up `struct rename_conflict_info`s to track that information. We never free those data structures though and thus leak memory. We need to be a bit more careful here though because the same rename conflict info can be assigned to multiple structures. Accommodate for this by introducing a `rename_conflict_info_owned` bit that we can use to steer whether or not the rename conflict info shall be free'd. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- merge-recursive.c | 8 +++++++- t/t3401-rebase-and-am-rename.sh | 1 + t/t4153-am-resume-override-opts.sh | 1 + t/t7201-co.sh | 1 + 4 files changed, 10 insertions(+), 1 deletion(-) diff --git a/merge-recursive.c b/merge-recursive.c index 8ff29ed09ef..8c8e8b4868b 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -239,7 +239,8 @@ enum rename_type { struct stage_data { struct diff_filespec stages[4]; /* mostly for oid & mode; maybe path */ struct rename_conflict_info *rename_conflict_info; - unsigned processed:1; + unsigned processed:1, + rename_conflict_info_owned:1; }; struct rename { @@ -308,6 +309,7 @@ static inline void setup_rename_conflict_info(enum rename_type rename_type, ci->ren1->dst_entry->processed = 0; ci->ren1->dst_entry->rename_conflict_info = ci; + ci->ren1->dst_entry->rename_conflict_info_owned = 1; if (ren2) { ci->ren2->dst_entry->rename_conflict_info = ci; } @@ -3055,6 +3057,10 @@ static void final_cleanup_rename(struct string_list *rename) for (i = 0; i < rename->nr; i++) { re = rename->items[i].util; diff_free_filepair(re->pair); + if (re->src_entry->rename_conflict_info_owned) + FREE_AND_NULL(re->src_entry->rename_conflict_info); + if (re->dst_entry->rename_conflict_info_owned) + FREE_AND_NULL(re->dst_entry->rename_conflict_info); } string_list_clear(rename, 1); free(rename); diff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh index f18bae94507..328c1d3a3f4 100755 --- a/t/t3401-rebase-and-am-rename.sh +++ b/t/t3401-rebase-and-am-rename.sh @@ -2,6 +2,7 @@ test_description='git rebase + directory rename tests' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-rebase.sh diff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh index 4add7c77578..6bc377b917f 100755 --- a/t/t4153-am-resume-override-opts.sh +++ b/t/t4153-am-resume-override-opts.sh @@ -2,6 +2,7 @@ test_description='git-am command-line options override saved options' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-terminal.sh diff --git a/t/t7201-co.sh b/t/t7201-co.sh index 42352dc0dbe..189d8e341ba 100755 --- a/t/t7201-co.sh +++ b/t/t7201-co.sh @@ -23,6 +23,7 @@ Test switching across them. GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_tick From 97485374377fa62fdd36f4b707e2fcd8f1a7c6c3 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:19:45 +0200 Subject: [PATCH 07/29] revision: fix leaking display notes We never free the display notes options embedded into `struct revision`. Implement a new function `release_display_notes()` that we can call in `release_revisions()` to fix this. There is another gotcha here though: we play some games with the string list used to track extra notes refs, where we sometimes set the bit that indicates that strings should be strdup'd and sometimes unset it. This dance is done to avoid a copy of an already-allocated string when we call `enable_ref_display_notes()`. But this dance is rather pointless as we can instead call `string_list_append_nodup()` to transfer ownership of the allocated string to the list. Refactor the code to do so and drop the `strdup_strings` dance. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- notes.c | 14 ++++++++------ notes.h | 5 +++++ revision.c | 1 + t/t3301-notes.sh | 1 + 4 files changed, 15 insertions(+), 6 deletions(-) diff --git a/notes.c b/notes.c index 53ca25c8147..6a157e34ce6 100644 --- a/notes.c +++ b/notes.c @@ -1060,6 +1060,12 @@ void init_display_notes(struct display_notes_opt *opt) { memset(opt, 0, sizeof(*opt)); opt->use_default_notes = -1; + string_list_init_dup(&opt->extra_notes_refs); +} + +void release_display_notes(struct display_notes_opt *opt) +{ + string_list_clear(&opt->extra_notes_refs, 0); } void enable_default_display_notes(struct display_notes_opt *opt, int *show_notes) @@ -1073,19 +1079,15 @@ void enable_ref_display_notes(struct display_notes_opt *opt, int *show_notes, struct strbuf buf = STRBUF_INIT; strbuf_addstr(&buf, ref); expand_notes_ref(&buf); - string_list_append(&opt->extra_notes_refs, - strbuf_detach(&buf, NULL)); + string_list_append_nodup(&opt->extra_notes_refs, + strbuf_detach(&buf, NULL)); *show_notes = 1; } void disable_display_notes(struct display_notes_opt *opt, int *show_notes) { opt->use_default_notes = -1; - /* we have been strdup'ing ourselves, so trick - * string_list into free()ing strings */ - opt->extra_notes_refs.strdup_strings = 1; string_list_clear(&opt->extra_notes_refs, 0); - opt->extra_notes_refs.strdup_strings = 0; *show_notes = 0; } diff --git a/notes.h b/notes.h index 064fd7143aa..235216944bc 100644 --- a/notes.h +++ b/notes.h @@ -275,6 +275,11 @@ struct display_notes_opt { */ void init_display_notes(struct display_notes_opt *opt); +/* + * Release resources acquired by the display_notes_opt. + */ +void release_display_notes(struct display_notes_opt *opt); + /* * This family of functions enables or disables the display of notes. In * particular, 'enable_default_display_notes' will display the default notes, diff --git a/revision.c b/revision.c index af95502d925..75e71bcaea8 100644 --- a/revision.c +++ b/revision.c @@ -3168,6 +3168,7 @@ void release_revisions(struct rev_info *revs) { free_commit_list(revs->commits); free_commit_list(revs->ancestry_path_bottoms); + release_display_notes(&revs->notes_opt); object_array_clear(&revs->pending); object_array_clear(&revs->boundary_commits); release_revisions_cmdline(&revs->cmdline); diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index cf23c06c098..536bd11ff47 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -5,6 +5,7 @@ test_description='Test commit notes' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh write_script fake_editor <<\EOF From f644dc84949bcc6d6d06274a30feb4b366ae68de Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:19:50 +0200 Subject: [PATCH 08/29] notes: fix memory leak when pruning notes In `prune_notes()` we first store the notes that are to be deleted in a local list, and then iterate through that list to delete those notes one by one. We never free the list though and thus leak its memory. Fix this. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- notes.c | 7 ++++++- t/t3306-notes-prune.sh | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/notes.c b/notes.c index 6a157e34ce6..244b5c4b1c0 100644 --- a/notes.c +++ b/notes.c @@ -1219,11 +1219,16 @@ void prune_notes(struct notes_tree *t, int flags) for_each_note(t, 0, prune_notes_helper, &l); while (l) { + struct note_delete_list *next; + if (flags & NOTES_PRUNE_VERBOSE) printf("%s\n", hash_to_hex(l->sha1)); if (!(flags & NOTES_PRUNE_DRYRUN)) remove_note(t, l->sha1); - l = l->next; + + next = l->next; + free(l); + l = next; } } diff --git a/t/t3306-notes-prune.sh b/t/t3306-notes-prune.sh index 8f4102ff9e4..b6e9f643e3c 100755 --- a/t/t3306-notes-prune.sh +++ b/t/t3306-notes-prune.sh @@ -2,6 +2,7 @@ test_description='Test git notes prune' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup: create a few commits with notes' ' From 61f8bb1ec1ef6022c2e7994b8b896ab158d7070b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:19:55 +0200 Subject: [PATCH 09/29] builtin/rev-list: fix leaking bitmap index when calculating disk usage git-rev-list(1) can speed up its object size calculations for reachable objects via a bitmap walk, if there is any bitmap. This is done in `try_bitmap_disk_usage()`, which tries to optimistically load the bitmap and then use it, if available. It never frees it though, leading to a memory leak. Fix this. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/rev-list.c | 2 ++ t/t6115-rev-list-du.sh | 2 ++ 2 files changed, 4 insertions(+) diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 77803727e07..97d077a9943 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -508,6 +508,8 @@ static int try_bitmap_disk_usage(struct rev_info *revs, size_from_bitmap = get_disk_usage_from_bitmap(bitmap_git, revs); print_disk_usage(size_from_bitmap); + + free_bitmap_index(bitmap_git); return 0; } diff --git a/t/t6115-rev-list-du.sh b/t/t6115-rev-list-du.sh index c0cfda62fa7..21c4a211b15 100755 --- a/t/t6115-rev-list-du.sh +++ b/t/t6115-rev-list-du.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='basic tests of rev-list --disk-usage' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # we want a mix of reachable and unreachable, as well as From f87c55c2647cf3aa0e6b5e45738facb6b62fe37c Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:19:59 +0200 Subject: [PATCH 10/29] object-name: free leaking object contexts While it is documented in `struct object_context::path` that this variable needs to be released by the caller, this fact is rather easy to miss given that we do not ever provide a function to release the object context. And of course, while some callers dutifully release the path, many others don't. Introduce a new `object_context_release()` function that releases the path. Convert callsites that used to free the path to use that new function and add missing calls to callsites that were leaking memory. Refactor those callsites as required to have a single return path, only. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/cat-file.c | 17 ++++++---- builtin/grep.c | 4 +-- builtin/log.c | 6 ++-- builtin/ls-tree.c | 3 +- builtin/rev-parse.c | 2 ++ builtin/stash.c | 12 +++++-- list-objects-filter.c | 2 ++ object-name.c | 40 ++++++++++++++++------- object-name.h | 2 ++ revision.c | 55 ++++++++++++++++++++------------ t/t7012-skip-worktree-writing.sh | 1 + 11 files changed, 97 insertions(+), 47 deletions(-) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 43a1d7ac498..18fe58d6b8b 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -102,7 +102,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name, enum object_type type; char *buf; unsigned long size; - struct object_context obj_context; + struct object_context obj_context = {0}; struct object_info oi = OBJECT_INFO_INIT; struct strbuf sb = STRBUF_INIT; unsigned flags = OBJECT_INFO_LOOKUP_REPLACE; @@ -163,7 +163,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name, goto cleanup; case 'e': - return !repo_has_object_file(the_repository, &oid); + ret = !repo_has_object_file(the_repository, &oid); + goto cleanup; case 'w': @@ -268,7 +269,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name, ret = 0; cleanup: free(buf); - free(obj_context.path); + object_context_release(&obj_context); return ret; } @@ -520,7 +521,7 @@ static void batch_one_object(const char *obj_name, struct batch_options *opt, struct expand_data *data) { - struct object_context ctx; + struct object_context ctx = {0}; int flags = GET_OID_HASH_ANY | (opt->follow_symlinks ? GET_OID_FOLLOW_SYMLINKS : 0); @@ -557,7 +558,8 @@ static void batch_one_object(const char *obj_name, break; } fflush(stdout); - return; + + goto out; } if (ctx.mode == 0) { @@ -565,10 +567,13 @@ static void batch_one_object(const char *obj_name, (uintmax_t)ctx.symlink_path.len, opt->output_delim, ctx.symlink_path.buf, opt->output_delim); fflush(stdout); - return; + goto out; } batch_object_write(obj_name, scratch, opt, data, NULL, 0); + +out: + object_context_release(&ctx); } struct object_cb_data { diff --git a/builtin/grep.c b/builtin/grep.c index 5777ba82a98..dfc3c3e8bd2 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -1114,7 +1114,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) for (i = 0; i < argc; i++) { const char *arg = argv[i]; struct object_id oid; - struct object_context oc; + struct object_context oc = {0}; struct object *object; if (!strcmp(arg, "--")) { @@ -1140,7 +1140,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) if (!seen_dashdash) verify_non_filename(prefix, arg); add_object_array_with_path(object, arg, &list, oc.mode, oc.path); - free(oc.path); + object_context_release(&oc); } /* diff --git a/builtin/log.c b/builtin/log.c index 4e4b645a21d..37ecb3ff8b9 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -682,7 +682,7 @@ static void show_tagger(const char *buf, struct rev_info *rev) static int show_blob_object(const struct object_id *oid, struct rev_info *rev, const char *obj_name) { struct object_id oidc; - struct object_context obj_context; + struct object_context obj_context = {0}; char *buf; unsigned long size; @@ -698,7 +698,7 @@ static int show_blob_object(const struct object_id *oid, struct rev_info *rev, c if (!obj_context.path || !textconv_object(the_repository, obj_context.path, obj_context.mode, &oidc, 1, &buf, &size)) { - free(obj_context.path); + object_context_release(&obj_context); return stream_blob_to_fd(1, oid, NULL, 0); } @@ -706,7 +706,7 @@ static int show_blob_object(const struct object_id *oid, struct rev_info *rev, c die(_("git show %s: bad file"), obj_name); write_or_die(1, buf, size); - free(obj_context.path); + object_context_release(&obj_context); return 0; } diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 7bf84b235ce..bf372c67d77 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -367,7 +367,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) OPT_END() }; struct ls_tree_cmdmode_to_fmt *m2f = ls_tree_cmdmode_format; - struct object_context obj_context; + struct object_context obj_context = {0}; int ret; git_config(git_default_config, NULL); @@ -441,5 +441,6 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) ret = !!read_tree(the_repository, tree, &options.pathspec, fn, &options); clear_pathspec(&options.pathspec); + object_context_release(&obj_context); return ret; } diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index ab8a8f3b0ed..2e64f5bda70 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -1128,6 +1128,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) } if (!get_oid_with_context(the_repository, name, flags, &oid, &unused)) { + object_context_release(&unused); if (output_algo) repo_oid_to_algop(the_repository, &oid, output_algo, &oid); @@ -1137,6 +1138,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) show_rev(type, &oid, name); continue; } + object_context_release(&unused); if (verify) die_no_single_rev(quiet); if (has_dashdash) diff --git a/builtin/stash.c b/builtin/stash.c index 7859bc0866a..628d848a0bc 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1018,13 +1018,14 @@ static int store_stash(int argc, const char **argv, const char *prefix) int quiet = 0; const char *stash_msg = NULL; struct object_id obj; - struct object_context dummy; + struct object_context dummy = {0}; struct option options[] = { OPT__QUIET(&quiet, N_("be quiet")), OPT_STRING('m', "message", &stash_msg, "message", N_("stash message")), OPT_END() }; + int ret; argc = parse_options(argc, argv, prefix, options, git_stash_store_usage, @@ -1043,10 +1044,15 @@ static int store_stash(int argc, const char **argv, const char *prefix) if (!quiet) fprintf_ln(stderr, _("Cannot update %s with %s"), ref_stash, argv[0]); - return -1; + ret = -1; + goto out; } - return do_store_stash(&obj, stash_msg, quiet); + ret = do_store_stash(&obj, stash_msg, quiet); + +out: + object_context_release(&dummy); + return ret; } static void add_pathspecs(struct strvec *args, diff --git a/list-objects-filter.c b/list-objects-filter.c index 4346f8da456..c95ec3509ae 100644 --- a/list-objects-filter.c +++ b/list-objects-filter.c @@ -542,6 +542,8 @@ static void filter_sparse_oid__init( filter->filter_data = d; filter->filter_object_fn = filter_sparse; filter->free_fn = filter_sparse_free; + + object_context_release(&oc); } /* diff --git a/object-name.c b/object-name.c index 523af6f64f3..0471fafc98d 100644 --- a/object-name.c +++ b/object-name.c @@ -1757,6 +1757,11 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name) return check_refname_format(sb->buf, 0); } +void object_context_release(struct object_context *ctx) +{ + free(ctx->path); +} + /* * This is like "get_oid_basic()", except it allows "object ID expressions", * notably "xyz^" for "parent of xyz" @@ -1764,7 +1769,9 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name) int repo_get_oid(struct repository *r, const char *name, struct object_id *oid) { struct object_context unused; - return get_oid_with_context(r, name, 0, oid, &unused); + int ret = get_oid_with_context(r, name, 0, oid, &unused); + object_context_release(&unused); + return ret; } /* @@ -1802,8 +1809,10 @@ int repo_get_oid_committish(struct repository *r, struct object_id *oid) { struct object_context unused; - return get_oid_with_context(r, name, GET_OID_COMMITTISH, - oid, &unused); + int ret = get_oid_with_context(r, name, GET_OID_COMMITTISH, + oid, &unused); + object_context_release(&unused); + return ret; } int repo_get_oid_treeish(struct repository *r, @@ -1811,8 +1820,10 @@ int repo_get_oid_treeish(struct repository *r, struct object_id *oid) { struct object_context unused; - return get_oid_with_context(r, name, GET_OID_TREEISH, - oid, &unused); + int ret = get_oid_with_context(r, name, GET_OID_TREEISH, + oid, &unused); + object_context_release(&unused); + return ret; } int repo_get_oid_commit(struct repository *r, @@ -1820,8 +1831,10 @@ int repo_get_oid_commit(struct repository *r, struct object_id *oid) { struct object_context unused; - return get_oid_with_context(r, name, GET_OID_COMMIT, - oid, &unused); + int ret = get_oid_with_context(r, name, GET_OID_COMMIT, + oid, &unused); + object_context_release(&unused); + return ret; } int repo_get_oid_tree(struct repository *r, @@ -1829,8 +1842,10 @@ int repo_get_oid_tree(struct repository *r, struct object_id *oid) { struct object_context unused; - return get_oid_with_context(r, name, GET_OID_TREE, - oid, &unused); + int ret = get_oid_with_context(r, name, GET_OID_TREE, + oid, &unused); + object_context_release(&unused); + return ret; } int repo_get_oid_blob(struct repository *r, @@ -1838,8 +1853,10 @@ int repo_get_oid_blob(struct repository *r, struct object_id *oid) { struct object_context unused; - return get_oid_with_context(r, name, GET_OID_BLOB, - oid, &unused); + int ret = get_oid_with_context(r, name, GET_OID_BLOB, + oid, &unused); + object_context_release(&unused); + return ret; } /* Must be called only when object_name:filename doesn't exist. */ @@ -2117,6 +2134,7 @@ void maybe_die_on_misspelt_object_name(struct repository *r, struct object_id oid; get_oid_with_context_1(r, name, GET_OID_ONLY_TO_DIE | GET_OID_QUIETLY, prefix, &oid, &oc); + object_context_release(&oc); } enum get_oid_result get_oid_with_context(struct repository *repo, diff --git a/object-name.h b/object-name.h index 064ddc97d1f..8dba4a47a47 100644 --- a/object-name.h +++ b/object-name.h @@ -22,6 +22,8 @@ struct object_context { char *path; }; +void object_context_release(struct object_context *ctx); + /* * Return an abbreviated sha1 unique within this repository's object database. * The result will be at least `len` characters long, and will be NUL diff --git a/revision.c b/revision.c index 75e71bcaea8..82c0aadb42c 100644 --- a/revision.c +++ b/revision.c @@ -2130,30 +2130,26 @@ static int handle_dotdot(const char *arg, struct rev_info *revs, int flags, int cant_be_filename) { - struct object_context a_oc, b_oc; + struct object_context a_oc = {0}, b_oc = {0}; char *dotdot = strstr(arg, ".."); int ret; if (!dotdot) return -1; - memset(&a_oc, 0, sizeof(a_oc)); - memset(&b_oc, 0, sizeof(b_oc)); - *dotdot = '\0'; ret = handle_dotdot_1(arg, dotdot, revs, flags, cant_be_filename, &a_oc, &b_oc); *dotdot = '.'; - free(a_oc.path); - free(b_oc.path); - + object_context_release(&a_oc); + object_context_release(&b_oc); return ret; } static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt) { - struct object_context oc; + struct object_context oc = {0}; char *mark; struct object *object; struct object_id oid; @@ -2161,6 +2157,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl const char *arg = arg_; int cant_be_filename = revarg_opt & REVARG_CANNOT_BE_FILENAME; unsigned get_sha1_flags = GET_OID_RECORD_PATH; + int ret; flags = flags & UNINTERESTING ? flags | BOTTOM : flags & ~BOTTOM; @@ -2169,17 +2166,22 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl * Just ".."? That is not a range but the * pathspec for the parent directory. */ - return -1; + ret = -1; + goto out; } - if (!handle_dotdot(arg, revs, flags, revarg_opt)) - return 0; + if (!handle_dotdot(arg, revs, flags, revarg_opt)) { + ret = 0; + goto out; + } mark = strstr(arg, "^@"); if (mark && !mark[2]) { *mark = 0; - if (add_parents_only(revs, arg, flags, 0)) - return 0; + if (add_parents_only(revs, arg, flags, 0)) { + ret = 0; + goto out; + } *mark = '^'; } mark = strstr(arg, "^!"); @@ -2194,8 +2196,10 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl if (mark[2]) { if (strtol_i(mark + 2, 10, &exclude_parent) || - exclude_parent < 1) - return -1; + exclude_parent < 1) { + ret = -1; + goto out; + } } *mark = 0; @@ -2217,17 +2221,25 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl * should error out if we can't even get an oid, as * `--missing=print` should be able to report missing oids. */ - if (get_oid_with_context(revs->repo, arg, get_sha1_flags, &oid, &oc)) - return revs->ignore_missing ? 0 : -1; + if (get_oid_with_context(revs->repo, arg, get_sha1_flags, &oid, &oc)) { + ret = revs->ignore_missing ? 0 : -1; + goto out; + } if (!cant_be_filename) verify_non_filename(revs->prefix, arg); object = get_reference(revs, arg, &oid, flags ^ local_flags); - if (!object) - return (revs->ignore_missing || revs->do_not_die_on_missing_objects) ? 0 : -1; + if (!object) { + ret = (revs->ignore_missing || revs->do_not_die_on_missing_objects) ? 0 : -1; + goto out; + } add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags); add_pending_object_with_path(revs, object, arg, oc.mode, oc.path); - free(oc.path); - return 0; + + ret = 0; + +out: + object_context_release(&oc); + return ret; } int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt) @@ -3062,6 +3074,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s diagnose_missing_default(revs->def); object = get_reference(revs, revs->def, &oid, 0); add_pending_object_with_mode(revs, object, revs->def, oc.mode); + object_context_release(&oc); } /* Did the user ask for any diff output? Run the diff! */ diff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh index cd5c20fe51b..d984200c173 100755 --- a/t/t7012-skip-worktree-writing.sh +++ b/t/t7012-skip-worktree-writing.sh @@ -5,6 +5,7 @@ test_description='test worktree writing operations when skip-worktree is used' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' From 9e903a5531f239a1ff3ab5fec2f0bb6fda595010 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:20:04 +0200 Subject: [PATCH 11/29] builtin/difftool: plug memory leaks in `run_dir_diff()` We're leaking a bunch of memory leaks in `run_dir_diff()`. Plug them. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/difftool.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/builtin/difftool.c b/builtin/difftool.c index a130faae4f1..63308b1ca7f 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -662,6 +662,9 @@ finish: free(lbase_dir); free(rbase_dir); + strbuf_release(&info); + strbuf_release(&lpath); + strbuf_release(&rpath); strbuf_release(&ldir); strbuf_release(&rdir); strbuf_release(&wtdir); From 3199b22e7d8cf8a95b7fac4e4aaf65638256b226 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:20:09 +0200 Subject: [PATCH 12/29] builtin/merge-recursive: fix leaking object ID bases In `cmd_merge_recursive()` we have a static array of object ID bases that we pass to `merge_recursive_generic()`. This interface is somewhat weird though because the latter function accepts a pointer to a pointer of object IDs, which requires us to allocate the object IDs on the heap. And as we never free those object IDs, the end result is a leak. While we can easily solve this leak by just freeing the respective object IDs, the whole calling convention is somewhat weird. Instead, refactor `merge_recursive_generic()` to accept a plain pointer to object IDs so that we can avoid allocating them altogether. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/am.c | 6 +++--- builtin/merge-recursive.c | 6 ++---- merge-recursive.c | 8 ++++---- merge-recursive.h | 2 +- t/t6432-merge-recursive-space-options.sh | 1 + t/t6434-merge-recursive-rename-options.sh | 1 + 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 36839029d20..4ba44e2d706 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1573,8 +1573,8 @@ static int build_fake_ancestor(const struct am_state *state, const char *index_f */ static int fall_back_threeway(const struct am_state *state, const char *index_path) { - struct object_id orig_tree, their_tree, our_tree; - const struct object_id *bases[1] = { &orig_tree }; + struct object_id their_tree, our_tree; + struct object_id bases[1] = { 0 }; struct merge_options o; struct commit *result; char *their_tree_name; @@ -1588,7 +1588,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa discard_index(the_repository->index); read_index_from(the_repository->index, index_path, get_git_dir()); - if (write_index_as_tree(&orig_tree, the_repository->index, index_path, 0, NULL)) + if (write_index_as_tree(&bases[0], the_repository->index, index_path, 0, NULL)) return error(_("Repository lacks necessary blobs to fall back on 3-way merge.")); say(state, stdout, _("Using index info to reconstruct a base tree...")); diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c index c2ce044a201..82bebea15b3 100644 --- a/builtin/merge-recursive.c +++ b/builtin/merge-recursive.c @@ -23,7 +23,7 @@ static char *better_branch_name(const char *branch) int cmd_merge_recursive(int argc, const char **argv, const char *prefix UNUSED) { - const struct object_id *bases[21]; + struct object_id bases[21]; unsigned bases_count = 0; int i, failed; struct object_id h1, h2; @@ -49,10 +49,8 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix UNUSED) continue; } if (bases_count < ARRAY_SIZE(bases)-1) { - struct object_id *oid = xmalloc(sizeof(struct object_id)); - if (repo_get_oid(the_repository, argv[i], oid)) + if (repo_get_oid(the_repository, argv[i], &bases[bases_count++])) die(_("could not parse object '%s'"), argv[i]); - bases[bases_count++] = oid; } else warning(Q_("cannot handle more than %d base. " diff --git a/merge-recursive.c b/merge-recursive.c index 8c8e8b4868b..eff73dac02e 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3866,7 +3866,7 @@ int merge_recursive_generic(struct merge_options *opt, const struct object_id *head, const struct object_id *merge, int num_merge_bases, - const struct object_id **merge_bases, + const struct object_id *merge_bases, struct commit **result) { int clean; @@ -3879,10 +3879,10 @@ int merge_recursive_generic(struct merge_options *opt, int i; for (i = 0; i < num_merge_bases; ++i) { struct commit *base; - if (!(base = get_ref(opt->repo, merge_bases[i], - oid_to_hex(merge_bases[i])))) + if (!(base = get_ref(opt->repo, &merge_bases[i], + oid_to_hex(&merge_bases[i])))) return err(opt, _("Could not parse object '%s'"), - oid_to_hex(merge_bases[i])); + oid_to_hex(&merge_bases[i])); commit_list_insert(base, &ca); } if (num_merge_bases == 1) diff --git a/merge-recursive.h b/merge-recursive.h index e67d38c3030..839eb6436e4 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -123,7 +123,7 @@ int merge_recursive_generic(struct merge_options *opt, const struct object_id *head, const struct object_id *merge, int num_merge_bases, - const struct object_id **merge_bases, + const struct object_id *merge_bases, struct commit **result); #endif diff --git a/t/t6432-merge-recursive-space-options.sh b/t/t6432-merge-recursive-space-options.sh index db4b77e63d2..c93538b0c38 100755 --- a/t/t6432-merge-recursive-space-options.sh +++ b/t/t6432-merge-recursive-space-options.sh @@ -14,6 +14,7 @@ test_description='merge-recursive space options GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_have_prereq SED_STRIPS_CR && SED_OPTIONS=-b diff --git a/t/t6434-merge-recursive-rename-options.sh b/t/t6434-merge-recursive-rename-options.sh index a11707835b4..df1d0c156c5 100755 --- a/t/t6434-merge-recursive-rename-options.sh +++ b/t/t6434-merge-recursive-rename-options.sh @@ -29,6 +29,7 @@ mentions this in a different context). GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh get_expected_stages () { From 8ff6bd47500dcd55d278fdbfe77fe838cc8dd3b1 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:20:14 +0200 Subject: [PATCH 13/29] merge-recursive: fix memory leak when finalizing merge We do not free some members of `struct merge_options`' private data. Fix this to plug those leaks. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- merge-recursive.c | 3 +++ t/t1004-read-tree-m-u-wf.sh | 1 + t/t1015-read-index-unmerged.sh | 2 ++ t/t3509-cherry-pick-merge-df.sh | 1 + 4 files changed, 7 insertions(+) diff --git a/merge-recursive.c b/merge-recursive.c index eff73dac02e..832c8ef3f34 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3800,6 +3800,9 @@ static void merge_finalize(struct merge_options *opt) if (show(opt, 2)) diff_warn_rename_limit("merge.renamelimit", opt->priv->needed_rename_limit, 0); + hashmap_clear_and_free(&opt->priv->current_file_dir_set, + struct path_hashmap_entry, e); + string_list_clear(&opt->priv->df_conflict_file_set, 0); FREE_AND_NULL(opt->priv); } diff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh index 11bf10424f1..2b9720b0feb 100755 --- a/t/t1004-read-tree-m-u-wf.sh +++ b/t/t1004-read-tree-m-u-wf.sh @@ -5,6 +5,7 @@ test_description='read-tree -m -u checks working tree files' 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-read-tree.sh diff --git a/t/t1015-read-index-unmerged.sh b/t/t1015-read-index-unmerged.sh index 55d22da32cc..da737a32a27 100755 --- a/t/t1015-read-index-unmerged.sh +++ b/t/t1015-read-index-unmerged.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='Test various callers of read_index_unmerged' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup modify/delete + directory/file conflict' ' diff --git a/t/t3509-cherry-pick-merge-df.sh b/t/t3509-cherry-pick-merge-df.sh index f4159246e1e..171cc6d76b7 100755 --- a/t/t3509-cherry-pick-merge-df.sh +++ b/t/t3509-cherry-pick-merge-df.sh @@ -4,6 +4,7 @@ test_description='Test cherry-pick with directory/file conflicts' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'Initialize repository' ' From a282dbeba718db156678aadec6c47ba1d9f13d0f Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:20:19 +0200 Subject: [PATCH 14/29] builtin/log: fix leaking commit list in git-cherry(1) We're storing the list of commits that git-cherry(1) is about to print into a temporary list. This list is never getting free'd and thus leaks. Fix this. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/log.c | 6 +++--- t/t3500-cherry.sh | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index 37ecb3ff8b9..b36fa9d1550 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -2675,16 +2675,16 @@ int cmd_cherry(int argc, const char **argv, const char *prefix) commit_list_insert(commit, &list); } - while (list) { + for (struct commit_list *l = list; l; l = l->next) { char sign = '+'; - commit = list->item; + commit = l->item; if (has_commit_patch_id(commit, &ids)) sign = '-'; print_commit(sign, commit, verbose, abbrev, revs.diffopt.file); - list = list->next; } + free_commit_list(list); free_patch_ids(&ids); return 0; } diff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh index 78c3eac54b5..61ca87512d5 100755 --- a/t/t3500-cherry.sh +++ b/t/t3500-cherry.sh @@ -11,6 +11,7 @@ checks that git cherry only returns the second patch in the local branch GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh GIT_AUTHOR_EMAIL=bogus_email_address From a90a08961190dda2f664e102822fb6a7152e65d5 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:20:24 +0200 Subject: [PATCH 15/29] revision: free diff options There is a todo comment in `release_revisions()` that mentions that we need to free the diff options, which was added via 54c8a7c379 (revisions API: add a TODO for diff_free(&revs->diffopt), 2022-04-14). Releasing the diff options wasn't quite feasible at that time because some call sites rely on its contents to remain even after the revisions have been released. In fact, there really only are a couple of callsites that misbehave here: - `cmd_shortlog()` releases the revisions, but continues to access its file pointer. - `do_diff_cache()` creates a shallow copy of `struct diff_options`, but does not set the `no_free` member. Consequently, we end up releasing resources of the caller-provided diff options. - `diff_free()` and friends do not play nice when being called multiple times as they don't unset data structures that they have just released. Fix all of those cases and enable the call to `diff_free()`, which plugs a bunch of memory leaks. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/shortlog.c | 5 +---- diff-lib.c | 2 ++ diff.c | 8 ++++++-- revision.c | 2 +- t/t4208-log-magic-pathspec.sh | 1 + t/t6000-rev-list-misc.sh | 1 + t/t6001-rev-list-graft.sh | 1 + t/t6013-rev-list-reverse-parents.sh | 1 + t/t6017-rev-list-stdin.sh | 1 + t/t9500-gitweb-standalone-no-errors.sh | 1 + t/t9502-gitweb-standalone-parse-output.sh | 1 + 11 files changed, 17 insertions(+), 7 deletions(-) diff --git a/builtin/shortlog.c b/builtin/shortlog.c index d4daf31e228..5bde7c68c26 100644 --- a/builtin/shortlog.c +++ b/builtin/shortlog.c @@ -460,11 +460,8 @@ parse_done: else get_from_rev(&rev, &log); - release_revisions(&rev); - shortlog_output(&log); - if (log.file != stdout) - fclose(log.file); + release_revisions(&rev); return 0; } diff --git a/diff-lib.c b/diff-lib.c index 5a5a50c5a15..1cbf03bf390 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -662,9 +662,11 @@ 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); revs.diffopt = *opt; + revs.diffopt.no_free = 1; if (diff_cache(&revs, tree_oid, NULL, 1)) exit(128); + release_revisions(&revs); return 0; } diff --git a/diff.c b/diff.c index e70301df767..53e2f5a42e1 100644 --- a/diff.c +++ b/diff.c @@ -6649,8 +6649,10 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o) static void diff_free_file(struct diff_options *options) { - if (options->close_file) + if (options->close_file && options->file) { fclose(options->file); + options->file = NULL; + } } static void diff_free_ignore_regex(struct diff_options *options) @@ -6661,7 +6663,9 @@ static void diff_free_ignore_regex(struct diff_options *options) regfree(options->ignore_regex[i]); free(options->ignore_regex[i]); } - free(options->ignore_regex); + + FREE_AND_NULL(options->ignore_regex); + options->ignore_regex_nr = 0; } void diff_free(struct diff_options *options) diff --git a/revision.c b/revision.c index 82c0aadb42c..99c75c939de 100644 --- a/revision.c +++ b/revision.c @@ -3191,7 +3191,7 @@ void release_revisions(struct rev_info *revs) release_revisions_mailmap(revs->mailmap); free_grep_patterns(&revs->grep_filter); graph_clear(revs->graph); - /* TODO (need to handle "no_free"): diff_free(&revs->diffopt) */ + diff_free(&revs->diffopt); diff_free(&revs->pruning); reflog_walk_info_release(revs->reflog_info); release_revisions_topo_walk_info(revs->topo_walk_info); diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh index 806b2809d40..2a46eb6bedb 100755 --- a/t/t4208-log-magic-pathspec.sh +++ b/t/t4208-log-magic-pathspec.sh @@ -5,6 +5,7 @@ test_description='magic pathspec tests using 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_expect_success 'setup' ' diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh index 6289a2e8b03..f6d17ee9025 100755 --- a/t/t6000-rev-list-misc.sh +++ b/t/t6000-rev-list-misc.sh @@ -5,6 +5,7 @@ test_description='miscellaneous rev-list tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t6001-rev-list-graft.sh b/t/t6001-rev-list-graft.sh index 73a2465aa0e..3553bbbfe73 100755 --- a/t/t6001-rev-list-graft.sh +++ b/t/t6001-rev-list-graft.sh @@ -5,6 +5,7 @@ test_description='Revision traversal vs grafts and path limiter' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t6013-rev-list-reverse-parents.sh b/t/t6013-rev-list-reverse-parents.sh index 39793cbbd66..4128269c1d4 100755 --- a/t/t6013-rev-list-reverse-parents.sh +++ b/t/t6013-rev-list-reverse-parents.sh @@ -5,6 +5,7 @@ test_description='--reverse combines with --parents' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh diff --git a/t/t6017-rev-list-stdin.sh b/t/t6017-rev-list-stdin.sh index 4821b90e747..a0a40fe55cd 100755 --- a/t/t6017-rev-list-stdin.sh +++ b/t/t6017-rev-list-stdin.sh @@ -8,6 +8,7 @@ test_description='log family learns --stdin' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh check () { diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh index 7679780fb87..ccfa4153840 100755 --- a/t/t9500-gitweb-standalone-no-errors.sh +++ b/t/t9500-gitweb-standalone-no-errors.sh @@ -13,6 +13,7 @@ or warnings to log.' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./lib-gitweb.sh # ---------------------------------------------------------------------- diff --git a/t/t9502-gitweb-standalone-parse-output.sh b/t/t9502-gitweb-standalone-parse-output.sh index 81d56255579..b41ea193314 100755 --- a/t/t9502-gitweb-standalone-parse-output.sh +++ b/t/t9502-gitweb-standalone-parse-output.sh @@ -13,6 +13,7 @@ in the HTTP header or the actual script output.' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./lib-gitweb.sh # ---------------------------------------------------------------------- From 748bd0943b696ba2b2942ce6e9d0dbfebf8b1fc3 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:20:28 +0200 Subject: [PATCH 16/29] builtin/stash: fix leak in `show_stash()` We leak the `revision_args()` variable. Fix this. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/stash.c | 2 ++ t/t3420-rebase-autostash.sh | 1 + t/t3907-stash-show-config.sh | 1 + 3 files changed, 4 insertions(+) diff --git a/builtin/stash.c b/builtin/stash.c index 628d848a0bc..1ed0a9a5d96 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -975,7 +975,9 @@ static int show_stash(int argc, const char **argv, const char *prefix) log_tree_diff_flush(&rev); ret = diff_result_code(&rev.diffopt); + cleanup: + strvec_clear(&revision_args); strvec_clear(&stash_args); free_stash_info(&info); release_revisions(&rev); diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh index 1a820f14815..63e400b89f2 100755 --- a/t/t3420-rebase-autostash.sh +++ b/t/t3420-rebase-autostash.sh @@ -7,6 +7,7 @@ test_description='git rebase --autostash tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t3907-stash-show-config.sh b/t/t3907-stash-show-config.sh index 10914bba7b3..7a2eb98b864 100755 --- a/t/t3907-stash-show-config.sh +++ b/t/t3907-stash-show-config.sh @@ -2,6 +2,7 @@ test_description='Test git stash show configuration.' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' From f46ede661face3c57f31097960c00325cf21f67f Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:20:33 +0200 Subject: [PATCH 17/29] rerere: fix various trivial leaks We leak various different string lists in the rerere code. Free those to plug them. Note that the `merge_rr` variable is intentionally being free'd with the `free_util` parameter set to 1. The `util` field is used there to store the IDs of every rerere item and thus needs to be freed, as well. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- rerere.c | 3 +++ t/t1021-rerere-in-workdir.sh | 1 + t/t3504-cherry-pick-rerere.sh | 1 + t/t7600-merge.sh | 1 + 4 files changed, 6 insertions(+) diff --git a/rerere.c b/rerere.c index c7e1f8fd25c..10382da55ce 100644 --- a/rerere.c +++ b/rerere.c @@ -849,6 +849,8 @@ static int do_plain_rerere(struct repository *r, if (update.nr) update_paths(r, &update); + string_list_clear(&conflict, 0); + string_list_clear(&update, 0); return write_rr(rr, fd); } @@ -912,6 +914,7 @@ int repo_rerere(struct repository *r, int flags) return 0; status = do_plain_rerere(r, &merge_rr, fd); free_rerere_dirs(); + string_list_clear(&merge_rr, 1); return status; } diff --git a/t/t1021-rerere-in-workdir.sh b/t/t1021-rerere-in-workdir.sh index 0b892894eb9..69bf9476cbf 100755 --- a/t/t1021-rerere-in-workdir.sh +++ b/t/t1021-rerere-in-workdir.sh @@ -4,6 +4,7 @@ test_description='rerere run in a workdir' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success SYMLINKS setup ' diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh index 4581ae98b87..597c98e9c57 100755 --- a/t/t3504-cherry-pick-rerere.sh +++ b/t/t3504-cherry-pick-rerere.sh @@ -5,6 +5,7 @@ test_description='cherry-pick should rerere for conflicts' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh index e5ff073099a..041f1077b07 100755 --- a/t/t7600-merge.sh +++ b/t/t7600-merge.sh @@ -29,6 +29,7 @@ Testing basic merge operations/option parsing. 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 From c6eb58bfb19a0840841934b91613ea53f0da8651 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:20:38 +0200 Subject: [PATCH 18/29] config: fix leaking "core.notesref" variable The variable used to track the "core.notesref" config is not getting freed before we assign to it and thus leaks. Fix this. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- config.c | 1 + t/t3308-notes-merge.sh | 1 + t/t3309-notes-merge-auto-resolve.sh | 1 + 3 files changed, 3 insertions(+) diff --git a/config.c b/config.c index abce05b7744..742175c1308 100644 --- a/config.c +++ b/config.c @@ -1565,6 +1565,7 @@ static int git_default_core_config(const char *var, const char *value, if (!strcmp(var, "core.notesref")) { if (!value) return config_error_nonbool(var); + free(notes_ref_name); notes_ref_name = xstrdup(value); return 0; } diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh index 202702be1a7..e1d05ff6bc9 100755 --- a/t/t3308-notes-merge.sh +++ b/t/t3308-notes-merge.sh @@ -5,6 +5,7 @@ test_description='Test merging of notes trees' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t3309-notes-merge-auto-resolve.sh b/t/t3309-notes-merge-auto-resolve.sh index 9bd5dbf341f..f55277f499d 100755 --- a/t/t3309-notes-merge-auto-resolve.sh +++ b/t/t3309-notes-merge-auto-resolve.sh @@ -5,6 +5,7 @@ test_description='Test notes merging with auto-resolving strategies' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Set up a notes merge scenario with all kinds of potential conflicts From 63c9bd372e388f5fed77be56771d5ad972f37f8e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:20:42 +0200 Subject: [PATCH 19/29] commit: fix leaking parents when calling `commit_tree_extended()` When creating commits via `commit_tree_extended()`, the caller passes in a string list of parents. This call implicitly transfers ownership of that list to the function, which is quite surprising to begin with. But to make matters worse, `commit_tree_extended()` doesn't even bother to free the list of parents in error cases. The result is a memory leak, and one that the caller cannot fix by themselves because they do not know whether parts of the string list have already been released. Refactor the code such that callers can keep ownership of the list of parents, which is getting indicated by parameter being a constant pointer now. Free the lists at the calling site and add a common exit path to those sites as required. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/am.c | 1 + builtin/commit-tree.c | 11 ++++++++--- builtin/commit.c | 3 ++- builtin/merge.c | 6 +++++- builtin/replay.c | 14 +++++++++----- builtin/stash.c | 9 ++++----- commit.c | 26 ++++++++++++-------------- commit.h | 10 +++++----- notes-merge.c | 1 + notes-utils.c | 8 ++++++-- notes-utils.h | 2 +- sequencer.c | 1 + t/t3403-rebase-skip.sh | 1 + t/t3424-rebase-empty.sh | 1 + t/t3505-cherry-pick-empty.sh | 1 + t/t7505-prepare-commit-msg-hook.sh | 1 + 16 files changed, 59 insertions(+), 37 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 4ba44e2d706..faccc45b136 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1718,6 +1718,7 @@ static void do_commit(const struct am_state *state) run_hooks("post-applypatch"); + free_commit_list(parents); strbuf_release(&sb); } diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c index 1bb78198392..84bb4502229 100644 --- a/builtin/commit-tree.c +++ b/builtin/commit-tree.c @@ -111,6 +111,7 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix) N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, OPT_END() }; + int ret; git_config(git_default_config, NULL); @@ -132,11 +133,15 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix) if (commit_tree(buffer.buf, buffer.len, &tree_oid, parents, &commit_oid, NULL, sign_commit)) { - strbuf_release(&buffer); - return 1; + ret = 1; + goto out; } printf("%s\n", oid_to_hex(&commit_oid)); + ret = 0; + +out: + free_commit_list(parents); strbuf_release(&buffer); - return 0; + return ret; } diff --git a/builtin/commit.c b/builtin/commit.c index dcaf4efa035..d5713455e5f 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1848,7 +1848,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix) rollback_index_files(); die(_("failed to write commit object")); } - free_commit_extra_headers(extra); if (update_head_with_reflog(current_head, &oid, reflog_msg, &sb, &err)) { @@ -1890,6 +1889,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix) apply_autostash_ref(the_repository, "MERGE_AUTOSTASH"); cleanup: + free_commit_extra_headers(extra); + free_commit_list(parents); strbuf_release(&author_ident); strbuf_release(&err); strbuf_release(&sb); diff --git a/builtin/merge.c b/builtin/merge.c index daed2d4e1e2..50b0c87a95f 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -895,7 +895,7 @@ static void prepare_to_commit(struct commit_list *remoteheads) static int merge_trivial(struct commit *head, struct commit_list *remoteheads) { struct object_id result_tree, result_commit; - struct commit_list *parents, **pptr = &parents; + struct commit_list *parents = NULL, **pptr = &parents; if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, SKIP_IF_UNCHANGED, 0, NULL, NULL, @@ -911,7 +911,9 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads) &result_commit, NULL, sign_commit)) die(_("failed to write commit object")); finish(head, remoteheads, &result_commit, "In-index merge"); + remove_merge_branch_state(the_repository); + free_commit_list(parents); return 0; } @@ -937,8 +939,10 @@ static int finish_automerge(struct commit *head, die(_("failed to write commit object")); strbuf_addf(&buf, "Merge made by the '%s' strategy.", wt_strategy); finish(head, remoteheads, &result_commit, buf.buf); + strbuf_release(&buf); remove_merge_branch_state(the_repository); + free_commit_list(parents); return 0; } diff --git a/builtin/replay.c b/builtin/replay.c index 6bf0691f15d..04483266367 100644 --- a/builtin/replay.c +++ b/builtin/replay.c @@ -52,11 +52,11 @@ static struct commit *create_commit(struct tree *tree, struct commit *parent) { struct object_id ret; - struct object *obj; + struct object *obj = NULL; struct commit_list *parents = NULL; char *author; char *sign_commit = NULL; /* FIXME: cli users might want to sign again */ - struct commit_extra_header *extra; + struct commit_extra_header *extra = NULL; struct strbuf msg = STRBUF_INIT; const char *out_enc = get_commit_output_encoding(); const char *message = repo_logmsg_reencode(the_repository, based_on, @@ -73,12 +73,16 @@ static struct commit *create_commit(struct tree *tree, if (commit_tree_extended(msg.buf, msg.len, &tree->object.oid, parents, &ret, author, NULL, sign_commit, extra)) { error(_("failed to write commit object")); - return NULL; + goto out; } - free(author); - strbuf_release(&msg); obj = parse_object(the_repository, &ret); + +out: + free_commit_extra_headers(extra); + free_commit_list(parents); + strbuf_release(&msg); + free(author); return (struct commit *)obj; } diff --git a/builtin/stash.c b/builtin/stash.c index 1ed0a9a5d96..46b981c4dd3 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1416,6 +1416,9 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b goto done; } + free_commit_list(parents); + parents = NULL; + if (include_untracked) { if (save_untracked_files(info, &msg, untracked_files)) { if (!quiet) @@ -1461,11 +1464,6 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b else strbuf_insertf(stash_msg_buf, 0, "On %s: ", branch_name); - /* - * `parents` will be empty after calling `commit_tree()`, so there is - * no need to call `free_commit_list()` - */ - parents = NULL; if (untracked_commit_option) commit_list_insert(lookup_commit(the_repository, &info->u_commit), @@ -1487,6 +1485,7 @@ done: strbuf_release(&commit_tree_label); strbuf_release(&msg); strbuf_release(&untracked_files); + free_commit_list(parents); return ret; } diff --git a/commit.c b/commit.c index 1d08951007b..f674eca3205 100644 --- a/commit.c +++ b/commit.c @@ -1262,7 +1262,7 @@ int remove_signature(struct strbuf *buf) return sigs[0].start != NULL; } -static void handle_signed_tag(struct commit *parent, struct commit_extra_header ***tail) +static void handle_signed_tag(const struct commit *parent, struct commit_extra_header ***tail) { struct merge_remote_desc *desc; struct commit_extra_header *mergetag; @@ -1359,17 +1359,17 @@ void verify_merge_signature(struct commit *commit, int verbosity, signature_check_clear(&signature_check); } -void append_merge_tag_headers(struct commit_list *parents, +void append_merge_tag_headers(const struct commit_list *parents, struct commit_extra_header ***tail) { while (parents) { - struct commit *parent = parents->item; + const struct commit *parent = parents->item; handle_signed_tag(parent, tail); parents = parents->next; } } -static int convert_commit_extra_headers(struct commit_extra_header *orig, +static int convert_commit_extra_headers(const struct commit_extra_header *orig, struct commit_extra_header **result) { const struct git_hash_algo *compat = the_repository->compat_hash_algo; @@ -1403,7 +1403,7 @@ static int convert_commit_extra_headers(struct commit_extra_header *orig, } static void add_extra_header(struct strbuf *buffer, - struct commit_extra_header *extra) + const struct commit_extra_header *extra) { strbuf_addstr(buffer, extra->key); if (extra->len) @@ -1517,7 +1517,7 @@ void free_commit_extra_headers(struct commit_extra_header *extra) } int commit_tree(const char *msg, size_t msg_len, const struct object_id *tree, - struct commit_list *parents, struct object_id *ret, + const struct commit_list *parents, struct object_id *ret, const char *author, const char *sign_commit) { struct commit_extra_header *extra = NULL, **tail = &extra; @@ -1649,7 +1649,7 @@ static void write_commit_tree(struct strbuf *buffer, const char *msg, size_t msg const struct object_id *tree, const struct object_id *parents, size_t parents_len, const char *author, const char *committer, - struct commit_extra_header *extra) + const struct commit_extra_header *extra) { int encoding_is_utf8; size_t i; @@ -1690,10 +1690,10 @@ static void write_commit_tree(struct strbuf *buffer, const char *msg, size_t msg int commit_tree_extended(const char *msg, size_t msg_len, const struct object_id *tree, - struct commit_list *parents, struct object_id *ret, + const struct commit_list *parents, struct object_id *ret, const char *author, const char *committer, const char *sign_commit, - struct commit_extra_header *extra) + const struct commit_extra_header *extra) { struct repository *r = the_repository; int result = 0; @@ -1715,10 +1715,8 @@ int commit_tree_extended(const char *msg, size_t msg_len, nparents = commit_list_count(parents); CALLOC_ARRAY(parent_buf, nparents); i = 0; - while (parents) { - struct commit *parent = pop_commit(&parents); - oidcpy(&parent_buf[i++], &parent->object.oid); - } + for (const struct commit_list *p = parents; p; p = p->next) + oidcpy(&parent_buf[i++], &p->item->object.oid); write_commit_tree(&buffer, msg, msg_len, tree, parent_buf, nparents, author, committer, extra); if (sign_commit && sign_commit_to_strbuf(&sig, &buffer, sign_commit)) { @@ -1814,7 +1812,7 @@ out: define_commit_slab(merge_desc_slab, struct merge_remote_desc *); static struct merge_desc_slab merge_desc_slab = COMMIT_SLAB_INIT(1, merge_desc_slab); -struct merge_remote_desc *merge_remote_util(struct commit *commit) +struct merge_remote_desc *merge_remote_util(const struct commit *commit) { return *merge_desc_slab_at(&merge_desc_slab, commit); } diff --git a/commit.h b/commit.h index 62fe0d77a70..442e50ff245 100644 --- a/commit.h +++ b/commit.h @@ -260,19 +260,19 @@ struct commit_extra_header { size_t len; }; -void append_merge_tag_headers(struct commit_list *parents, +void append_merge_tag_headers(const struct commit_list *parents, struct commit_extra_header ***tail); int commit_tree(const char *msg, size_t msg_len, const struct object_id *tree, - struct commit_list *parents, struct object_id *ret, + const struct commit_list *parents, struct object_id *ret, const char *author, const char *sign_commit); int commit_tree_extended(const char *msg, size_t msg_len, const struct object_id *tree, - struct commit_list *parents, struct object_id *ret, + const struct commit_list *parents, struct object_id *ret, const char *author, const char *committer, - const char *sign_commit, struct commit_extra_header *); + const char *sign_commit, const struct commit_extra_header *); struct commit_extra_header *read_commit_extra_headers(struct commit *, const char **); @@ -306,7 +306,7 @@ struct merge_remote_desc { struct object *obj; /* the named object, could be a tag */ char name[FLEX_ARRAY]; }; -struct merge_remote_desc *merge_remote_util(struct commit *); +struct merge_remote_desc *merge_remote_util(const struct commit *); void set_merge_remote_desc(struct commit *commit, const char *name, struct object *obj); diff --git a/notes-merge.c b/notes-merge.c index 6a9a139b123..f3cc84f45d4 100644 --- a/notes-merge.c +++ b/notes-merge.c @@ -661,6 +661,7 @@ int notes_merge(struct notes_merge_options *o, commit_list_insert(local, &parents); create_notes_commit(o->repo, local_tree, parents, o->commit_msg.buf, o->commit_msg.len, result_oid); + free_commit_list(parents); } found_result: diff --git a/notes-utils.c b/notes-utils.c index 671d1969b1f..3198c14e4df 100644 --- a/notes-utils.c +++ b/notes-utils.c @@ -9,10 +9,11 @@ void create_notes_commit(struct repository *r, struct notes_tree *t, - struct commit_list *parents, + const struct commit_list *parents, const char *msg, size_t msg_len, struct object_id *result_oid) { + struct commit_list *parents_to_free = NULL; struct object_id tree_oid; assert(t->initialized); @@ -27,7 +28,8 @@ void create_notes_commit(struct repository *r, struct commit *parent = lookup_commit(r, &parent_oid); if (repo_parse_commit(r, parent)) die("Failed to find/parse commit %s", t->ref); - commit_list_insert(parent, &parents); + commit_list_insert(parent, &parents_to_free); + parents = parents_to_free; } /* else: t->ref points to nothing, assume root/orphan commit */ } @@ -35,6 +37,8 @@ void create_notes_commit(struct repository *r, if (commit_tree(msg, msg_len, &tree_oid, parents, result_oid, NULL, NULL)) die("Failed to commit notes tree to database"); + + free_commit_list(parents_to_free); } void commit_notes(struct repository *r, struct notes_tree *t, const char *msg) diff --git a/notes-utils.h b/notes-utils.h index d9b3c09eaf0..c54b1fe141f 100644 --- a/notes-utils.h +++ b/notes-utils.h @@ -20,7 +20,7 @@ struct repository; */ void create_notes_commit(struct repository *r, struct notes_tree *t, - struct commit_list *parents, + const struct commit_list *parents, const char *msg, size_t msg_len, struct object_id *result_oid); diff --git a/sequencer.c b/sequencer.c index 30513e87bfb..475646671a8 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1711,6 +1711,7 @@ static int try_to_commit(struct repository *r, out: free_commit_extra_headers(extra); + free_commit_list(parents); strbuf_release(&err); strbuf_release(&commit_msg); free(amend_author); diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh index a1911c4a9d6..4f1d6e8ea64 100755 --- a/t/t3403-rebase-skip.sh +++ b/t/t3403-rebase-skip.sh @@ -8,6 +8,7 @@ test_description='git rebase --merge --skip 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-rebase.sh diff --git a/t/t3424-rebase-empty.sh b/t/t3424-rebase-empty.sh index 1ee6b00fd57..515c949ae37 100755 --- a/t/t3424-rebase-empty.sh +++ b/t/t3424-rebase-empty.sh @@ -2,6 +2,7 @@ test_description='git rebase of commits that start or become empty' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup test repository' ' diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh index 9748443530c..ead3fb46807 100755 --- a/t/t3505-cherry-pick-empty.sh +++ b/t/t3505-cherry-pick-empty.sh @@ -5,6 +5,7 @@ test_description='test cherry-picking an empty commit' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh index 2128142a61c..b88383df9e4 100755 --- a/t/t7505-prepare-commit-msg-hook.sh +++ b/t/t7505-prepare-commit-msg-hook.sh @@ -5,6 +5,7 @@ test_description='prepare-commit-msg hook' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'set up commits for rebasing' ' From 1e5c1601f98afba0772c4548ec6befe6e97761e7 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:20:47 +0200 Subject: [PATCH 20/29] sequencer: fix leaking string buffer in `commit_staged_changes()` We're leaking the `rev` string buffer in various call paths. Refactor the function to have a common exit path so that we can release its memory reliably. This fixes a subset of tests failing with the memory sanitizer in t3404. But as there are more failures, we cannot yet mark the whole test suite as passing. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- sequencer.c | 111 ++++++++++++++++++++++++++++++++++------------------ 1 file changed, 73 insertions(+), 38 deletions(-) diff --git a/sequencer.c b/sequencer.c index 475646671a8..c581061b6db 100644 --- a/sequencer.c +++ b/sequencer.c @@ -5146,33 +5146,47 @@ static int commit_staged_changes(struct repository *r, struct replay_ctx *ctx = opts->ctx; unsigned int flags = ALLOW_EMPTY | EDIT_MSG; unsigned int final_fixup = 0, is_clean; + struct strbuf rev = STRBUF_INIT; + int ret; - if (has_unstaged_changes(r, 1)) - return error(_("cannot rebase: You have unstaged changes.")); + if (has_unstaged_changes(r, 1)) { + ret = error(_("cannot rebase: You have unstaged changes.")); + goto out; + } is_clean = !has_uncommitted_changes(r, 0); if (!is_clean && !file_exists(rebase_path_message())) { const char *gpg_opt = gpg_sign_opt_quoted(opts); - - return error(_(staged_changes_advice), gpg_opt, gpg_opt); + ret = error(_(staged_changes_advice), gpg_opt, gpg_opt); + goto out; } + if (file_exists(rebase_path_amend())) { - struct strbuf rev = STRBUF_INIT; struct object_id head, to_amend; - if (repo_get_oid(r, "HEAD", &head)) - return error(_("cannot amend non-existing commit")); - if (!read_oneliner(&rev, rebase_path_amend(), 0)) - return error(_("invalid file: '%s'"), rebase_path_amend()); - if (get_oid_hex(rev.buf, &to_amend)) - return error(_("invalid contents: '%s'"), - rebase_path_amend()); - if (!is_clean && !oideq(&head, &to_amend)) - return error(_("\nYou have uncommitted changes in your " - "working tree. Please, commit them\n" - "first and then run 'git rebase " - "--continue' again.")); + if (repo_get_oid(r, "HEAD", &head)) { + ret = error(_("cannot amend non-existing commit")); + goto out; + } + + if (!read_oneliner(&rev, rebase_path_amend(), 0)) { + ret = error(_("invalid file: '%s'"), rebase_path_amend()); + goto out; + } + + if (get_oid_hex(rev.buf, &to_amend)) { + ret = error(_("invalid contents: '%s'"), + rebase_path_amend()); + goto out; + } + if (!is_clean && !oideq(&head, &to_amend)) { + ret = error(_("\nYou have uncommitted changes in your " + "working tree. Please, commit them\n" + "first and then run 'git rebase " + "--continue' again.")); + goto out; + } /* * When skipping a failed fixup/squash, we need to edit the * commit message, the current fixup list and count, and if it @@ -5204,9 +5218,11 @@ static int commit_staged_changes(struct repository *r, len--; strbuf_setlen(&ctx->current_fixups, len); if (write_message(p, len, rebase_path_current_fixups(), - 0) < 0) - return error(_("could not write file: '%s'"), - rebase_path_current_fixups()); + 0) < 0) { + ret = error(_("could not write file: '%s'"), + rebase_path_current_fixups()); + goto out; + } /* * If a fixup/squash in a fixup/squash chain failed, the @@ -5236,35 +5252,38 @@ static int commit_staged_changes(struct repository *r, * We need to update the squash message to skip * the latest commit message. */ - int res = 0; struct commit *commit; const char *msg; const char *path = rebase_path_squash_msg(); const char *encoding = get_commit_output_encoding(); - if (parse_head(r, &commit)) - return error(_("could not parse HEAD")); + if (parse_head(r, &commit)) { + ret = error(_("could not parse HEAD")); + goto out; + } p = repo_logmsg_reencode(r, commit, NULL, encoding); if (!p) { - res = error(_("could not parse commit %s"), + ret = error(_("could not parse commit %s"), oid_to_hex(&commit->object.oid)); goto unuse_commit_buffer; } find_commit_subject(p, &msg); if (write_message(msg, strlen(msg), path, 0)) { - res = error(_("could not write file: " + ret = error(_("could not write file: " "'%s'"), path); goto unuse_commit_buffer; } + + ret = 0; + unuse_commit_buffer: repo_unuse_commit_buffer(r, commit, p); - if (res) - return res; + if (ret) + goto out; } } - strbuf_release(&rev); flags |= AMEND_MSG; } @@ -5272,18 +5291,29 @@ static int commit_staged_changes(struct repository *r, if (refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") && refs_delete_ref(get_main_ref_store(r), "", - "CHERRY_PICK_HEAD", NULL, REF_NO_DEREF)) - return error(_("could not remove CHERRY_PICK_HEAD")); - if (unlink(git_path_merge_msg(r)) && errno != ENOENT) - return error_errno(_("could not remove '%s'"), - git_path_merge_msg(r)); - if (!final_fixup) - return 0; + "CHERRY_PICK_HEAD", NULL, REF_NO_DEREF)) { + ret = error(_("could not remove CHERRY_PICK_HEAD")); + goto out; + } + + if (unlink(git_path_merge_msg(r)) && errno != ENOENT) { + ret = error_errno(_("could not remove '%s'"), + git_path_merge_msg(r)); + goto out; + } + + if (!final_fixup) { + ret = 0; + goto out; + } } if (run_git_commit(final_fixup ? NULL : rebase_path_message(), - opts, flags)) - return error(_("could not commit staged changes.")); + opts, flags)) { + ret = error(_("could not commit staged changes.")); + goto out; + } + unlink(rebase_path_amend()); unlink(git_path_merge_head(r)); refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE", @@ -5301,7 +5331,12 @@ static int commit_staged_changes(struct repository *r, strbuf_reset(&ctx->current_fixups); ctx->current_fixup_count = 0; } - return 0; + + ret = 0; + +out: + strbuf_release(&rev); + return ret; } int sequencer_continue(struct repository *r, struct replay_opts *opts) From 4806c55c86f7cc45f60a7ff5d757874072265deb Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:20:52 +0200 Subject: [PATCH 21/29] apply: fix leaking string in `match_fragment()` Before calling `update_pre_post_images()`, we call `strbuf_detach()` to put its buffer into a new string variable that we then pass to that function. Besides being rather pointless, it also causes us to leak memory of that variable because we never free it. Get rid of the variable altogether and instead reach into the `strbuf` directly. While at it, refactor the code to have a common exit path and mark string that do not contain allocated memory as constant. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- apply.c | 87 ++++++++++++++++++++------------ t/t3417-rebase-whitespace-fix.sh | 1 + 2 files changed, 56 insertions(+), 32 deletions(-) diff --git a/apply.c b/apply.c index d8d26a48f13..fd7e3d649fc 100644 --- a/apply.c +++ b/apply.c @@ -2494,18 +2494,21 @@ static int match_fragment(struct apply_state *state, int match_beginning, int match_end) { int i; - char *fixed_buf, *buf, *orig, *target; - struct strbuf fixed; - size_t fixed_len, postlen; + const char *orig, *target; + struct strbuf fixed = STRBUF_INIT; + size_t postlen; int preimage_limit; + int ret; if (preimage->nr + current_lno <= img->nr) { /* * The hunk falls within the boundaries of img. */ preimage_limit = preimage->nr; - if (match_end && (preimage->nr + current_lno != img->nr)) - return 0; + if (match_end && (preimage->nr + current_lno != img->nr)) { + ret = 0; + goto out; + } } else if (state->ws_error_action == correct_ws_error && (ws_rule & WS_BLANK_AT_EOF)) { /* @@ -2522,17 +2525,23 @@ static int match_fragment(struct apply_state *state, * we are not removing blanks at the end, so we * should reject the hunk at this position. */ - return 0; + ret = 0; + goto out; } - if (match_beginning && current_lno) - return 0; + if (match_beginning && current_lno) { + ret = 0; + goto out; + } /* Quick hash check */ - for (i = 0; i < preimage_limit; i++) + for (i = 0; i < preimage_limit; i++) { if ((img->line[current_lno + i].flag & LINE_PATCHED) || - (preimage->line[i].hash != img->line[current_lno + i].hash)) - return 0; + (preimage->line[i].hash != img->line[current_lno + i].hash)) { + ret = 0; + goto out; + } + } if (preimage_limit == preimage->nr) { /* @@ -2545,8 +2554,10 @@ static int match_fragment(struct apply_state *state, if ((match_end ? (current + preimage->len == img->len) : (current + preimage->len <= img->len)) && - !memcmp(img->buf + current, preimage->buf, preimage->len)) - return 1; + !memcmp(img->buf + current, preimage->buf, preimage->len)) { + ret = 1; + goto out; + } } else { /* * The preimage extends beyond the end of img, so @@ -2555,7 +2566,7 @@ static int match_fragment(struct apply_state *state, * There must be one non-blank context line that match * a line before the end of img. */ - char *buf_end; + const char *buf, *buf_end; buf = preimage->buf; buf_end = buf; @@ -2565,8 +2576,10 @@ static int match_fragment(struct apply_state *state, for ( ; buf < buf_end; buf++) if (!isspace(*buf)) break; - if (buf == buf_end) - return 0; + if (buf == buf_end) { + ret = 0; + goto out; + } } /* @@ -2574,12 +2587,16 @@ static int match_fragment(struct apply_state *state, * fuzzy matching. We collect all the line length information because * we need it to adjust whitespace if we match. */ - if (state->ws_ignore_action == ignore_ws_change) - return line_by_line_fuzzy_match(img, preimage, postimage, - current, current_lno, preimage_limit); + if (state->ws_ignore_action == ignore_ws_change) { + ret = line_by_line_fuzzy_match(img, preimage, postimage, + current, current_lno, preimage_limit); + goto out; + } - if (state->ws_error_action != correct_ws_error) - return 0; + if (state->ws_error_action != correct_ws_error) { + ret = 0; + goto out; + } /* * The hunk does not apply byte-by-byte, but the hash says @@ -2608,7 +2625,7 @@ static int match_fragment(struct apply_state *state, * but in this loop we will only handle the part of the * preimage that falls within the file. */ - strbuf_init(&fixed, preimage->len + 1); + strbuf_grow(&fixed, preimage->len + 1); orig = preimage->buf; target = img->buf + current; for (i = 0; i < preimage_limit; i++) { @@ -2644,8 +2661,10 @@ static int match_fragment(struct apply_state *state, postlen += tgtfix.len; strbuf_release(&tgtfix); - if (!match) - goto unmatch_exit; + if (!match) { + ret = 0; + goto out; + } orig += oldlen; target += tgtlen; @@ -2666,9 +2685,13 @@ static int match_fragment(struct apply_state *state, /* Try fixing the line in the preimage */ ws_fix_copy(&fixed, orig, oldlen, ws_rule, NULL); - for (j = fixstart; j < fixed.len; j++) - if (!isspace(fixed.buf[j])) - goto unmatch_exit; + for (j = fixstart; j < fixed.len; j++) { + if (!isspace(fixed.buf[j])) { + ret = 0; + goto out; + } + } + orig += oldlen; } @@ -2678,16 +2701,16 @@ static int match_fragment(struct apply_state *state, * has whitespace breakages unfixed, and fixing them makes the * hunk match. Update the context lines in the postimage. */ - fixed_buf = strbuf_detach(&fixed, &fixed_len); if (postlen < postimage->len) postlen = 0; update_pre_post_images(preimage, postimage, - fixed_buf, fixed_len, postlen); - return 1; + fixed.buf, fixed.len, postlen); - unmatch_exit: + ret = 1; + +out: strbuf_release(&fixed); - return 0; + return ret; } static int find_pos(struct apply_state *state, diff --git a/t/t3417-rebase-whitespace-fix.sh b/t/t3417-rebase-whitespace-fix.sh index 96f2cf22faf..22ee3a20459 100755 --- a/t/t3417-rebase-whitespace-fix.sh +++ b/t/t3417-rebase-whitespace-fix.sh @@ -5,6 +5,7 @@ test_description='git rebase --whitespace=fix This test runs git rebase --whitespace=fix and make sure that it works. ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # prepare initial revision of "file" with a blank line at the end From 8909d6e1a108a46ae9cde70587aa8b2ad4a067d9 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:20:57 +0200 Subject: [PATCH 22/29] builtin/clone: plug leaking HEAD ref in `wanted_peer_refs()` In `wanted_peer_refs()` we first create a copy of the "HEAD" ref. This copy may not actually be passed back to the caller, but is not getting freed in this case. Fix this. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/clone.c | 3 ++- t/t5300-pack-object.sh | 4 ++-- t/t5305-include-tag.sh | 1 + t/t5612-clone-refspec.sh | 1 + 4 files changed, 6 insertions(+), 3 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index 730b3efae60..ae9863ed477 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -530,7 +530,8 @@ static struct ref *wanted_peer_refs(const struct ref *refs, if (!option_branch) remote_head = guess_remote_head(head, refs, 0); else { - local_refs = NULL; + free_one_ref(head); + local_refs = head = NULL; tail = &local_refs; remote_head = copy_ref(find_remote_branch(refs, option_branch)); } diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh index 61e2be2903d..4ad023c846d 100755 --- a/t/t5300-pack-object.sh +++ b/t/t5300-pack-object.sh @@ -3,9 +3,9 @@ # Copyright (c) 2005 Junio C Hamano # -test_description='git pack-object +test_description='git pack-object' -' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t5305-include-tag.sh b/t/t5305-include-tag.sh index 44bd9ef45fd..dc8fe55c82d 100755 --- a/t/t5305-include-tag.sh +++ b/t/t5305-include-tag.sh @@ -4,6 +4,7 @@ test_description='git pack-object --include-tag' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh TRASH=$(pwd) diff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh index 3126cfd7e9d..72762de9774 100755 --- a/t/t5612-clone-refspec.sh +++ b/t/t5612-clone-refspec.sh @@ -4,6 +4,7 @@ test_description='test refspec written by clone-command' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' From 6e95f4ee0394b7ed8ee42b5d0d22d35b20af16a9 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:21:01 +0200 Subject: [PATCH 23/29] sequencer: fix memory leaks in `make_script_with_merges()` Fix some trivial memory leaks in `make_script_with_merges()`. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- sequencer.c | 3 +++ t/t3418-rebase-continue.sh | 1 + t/t3421-rebase-topology-linear.sh | 2 ++ t/t3434-rebase-i18n.sh | 1 + 4 files changed, 7 insertions(+) diff --git a/sequencer.c b/sequencer.c index c581061b6db..20807ea7e58 100644 --- a/sequencer.c +++ b/sequencer.c @@ -5951,6 +5951,9 @@ static int make_script_with_merges(struct pretty_print_context *pp, strbuf_release(&oneline); strbuf_release(&buf); + oidset_clear(&interesting); + oidset_clear(&child_seen); + oidset_clear(&shown); oidmap_free(&commit2todo, 1); oidmap_free(&state.commit2label, 1); hashmap_clear_and_free(&state.labels, struct labels_entry, entry); diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh index 127216f7225..c0d29c2154f 100755 --- a/t/t3418-rebase-continue.sh +++ b/t/t3418-rebase-continue.sh @@ -5,6 +5,7 @@ test_description='git rebase --continue 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-rebase.sh diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh index 62d86d557da..737af80bb3d 100755 --- a/t/t3421-rebase-topology-linear.sh +++ b/t/t3421-rebase-topology-linear.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='basic rebase topology tests' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-rebase.sh diff --git a/t/t3434-rebase-i18n.sh b/t/t3434-rebase-i18n.sh index a4e482d2cd0..26a48d6b103 100755 --- a/t/t3434-rebase-i18n.sh +++ b/t/t3434-rebase-i18n.sh @@ -17,6 +17,7 @@ Initial setup: GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh compare_msg () { From 77241a6b5e3aadbc697632600e7e187ae94c4ca6 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:21:06 +0200 Subject: [PATCH 24/29] builtin/merge: fix leaking `struct cmdnames` in `get_strategy()` In "builtin/merge.c" we use the helper infrastructure to figure out what merge strategies there are. We never free contents of the `cmdnames` structures though and thus leak their memory. Fix this by exposing the already existing `clean_cmdnames()` function to release their memory. As this name isn't quite idiomatic, rename it to `cmdnames_release()` while at it. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/merge.c | 10 +++++++--- help.c | 12 ++++++------ help.h | 2 ++ t/t7606-merge-custom.sh | 1 + 4 files changed, 16 insertions(+), 9 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index 50b0c87a95f..682c6ed868e 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -164,7 +164,7 @@ static struct strategy *get_strategy(const char *name) { int i; struct strategy *ret; - static struct cmdnames main_cmds, other_cmds; + static struct cmdnames main_cmds = {0}, other_cmds = {0}; static int loaded; char *default_strategy = getenv("GIT_TEST_MERGE_ALGORITHM"); @@ -182,10 +182,9 @@ static struct strategy *get_strategy(const char *name) return &all_strategy[i]; if (!loaded) { - struct cmdnames not_strategies; + struct cmdnames not_strategies = {0}; loaded = 1; - memset(¬_strategies, 0, sizeof(struct cmdnames)); load_command_list("git-merge-", &main_cmds, &other_cmds); for (i = 0; i < main_cmds.cnt; i++) { int j, found = 0; @@ -197,6 +196,8 @@ static struct strategy *get_strategy(const char *name) add_cmdname(¬_strategies, ent->name, ent->len); } exclude_cmds(&main_cmds, ¬_strategies); + + cmdnames_release(¬_strategies); } if (!is_in_cmdlist(&main_cmds, name) && !is_in_cmdlist(&other_cmds, name)) { fprintf(stderr, _("Could not find merge strategy '%s'.\n"), name); @@ -216,6 +217,9 @@ static struct strategy *get_strategy(const char *name) CALLOC_ARRAY(ret, 1); ret->name = xstrdup(name); ret->attr = NO_TRIVIAL; + + cmdnames_release(&main_cmds); + cmdnames_release(&other_cmds); return ret; } diff --git a/help.c b/help.c index 1d057aa6073..3686285ca3d 100644 --- a/help.c +++ b/help.c @@ -157,7 +157,7 @@ void add_cmdname(struct cmdnames *cmds, const char *name, int len) cmds->names[cmds->cnt++] = ent; } -static void clean_cmdnames(struct cmdnames *cmds) +void cmdnames_release(struct cmdnames *cmds) { int i; for (i = 0; i < cmds->cnt; ++i) @@ -359,8 +359,8 @@ void list_all_main_cmds(struct string_list *list) for (i = 0; i < main_cmds.cnt; i++) string_list_append(list, main_cmds.names[i]->name); - clean_cmdnames(&main_cmds); - clean_cmdnames(&other_cmds); + cmdnames_release(&main_cmds); + cmdnames_release(&other_cmds); } void list_all_other_cmds(struct string_list *list) @@ -375,8 +375,8 @@ void list_all_other_cmds(struct string_list *list) for (i = 0; i < other_cmds.cnt; i++) string_list_append(list, other_cmds.names[i]->name); - clean_cmdnames(&main_cmds); - clean_cmdnames(&other_cmds); + cmdnames_release(&main_cmds); + cmdnames_release(&other_cmds); } void list_cmds_by_category(struct string_list *list, @@ -689,7 +689,7 @@ const char *help_unknown_cmd(const char *cmd) if (autocorrect && n == 1 && SIMILAR_ENOUGH(best_similarity)) { const char *assumed = main_cmds.names[0]->name; main_cmds.names[0] = NULL; - clean_cmdnames(&main_cmds); + cmdnames_release(&main_cmds); fprintf_ln(stderr, _("WARNING: You called a Git command named '%s', " "which does not exist."), diff --git a/help.h b/help.h index af073a7a026..e716ee27ea1 100644 --- a/help.h +++ b/help.h @@ -13,6 +13,8 @@ struct cmdnames { } **names; }; +void cmdnames_release(struct cmdnames *cmds); + static inline void mput_char(char c, unsigned int num) { while (num--) diff --git a/t/t7606-merge-custom.sh b/t/t7606-merge-custom.sh index 81fb7c474c1..135cb230856 100755 --- a/t/t7606-merge-custom.sh +++ b/t/t7606-merge-custom.sh @@ -14,6 +14,7 @@ Testing a custom strategy. * (tag: c0) c0 " +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'set up custom strategy' ' From 44ec7c575f914d77787a17cefd094e3c46b8b12b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:21:11 +0200 Subject: [PATCH 25/29] merge: fix leaking merge bases When calling either the recursive or the ORT merge machineries we need to provide a list of merge bases. The ownership of that parameter is then implicitly transferred to the callee, which is somewhat fishy. Furthermore, that list may leak in some cases where the merge machinery runs into an error, thus causing a memory leak. Refactor the code such that we stop transferring ownership. Instead, the merge machinery will now create its own local copies of the passed in list as required if they need to modify the list. Free the list at the callsites as required. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/merge-tree.c | 1 + builtin/merge.c | 2 ++ commit.c | 2 +- commit.h | 2 +- log-tree.c | 1 + merge-ort-wrappers.c | 2 +- merge-ort-wrappers.h | 2 +- merge-ort.c | 12 ++++++---- merge-ort.h | 2 +- merge-recursive.c | 49 +++++++++++++++++++++++--------------- merge-recursive.h | 2 +- sequencer.c | 1 + t/t3430-rebase-merges.sh | 1 + t/t6402-merge-rename.sh | 1 + t/t6430-merge-recursive.sh | 1 + t/t6436-merge-overwrite.sh | 1 + t/t7611-merge-abort.sh | 1 + 17 files changed, 54 insertions(+), 29 deletions(-) diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c index 1082d919fd1..dab2fdc2a6b 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c @@ -482,6 +482,7 @@ static int real_merge(struct merge_tree_options *o, die(_("refusing to merge unrelated histories")); merge_bases = reverse_commit_list(merge_bases); merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result); + free_commit_list(merge_bases); } if (result.clean < 0) diff --git a/builtin/merge.c b/builtin/merge.c index 682c6ed868e..1120a6e2f85 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -746,6 +746,8 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, else clean = merge_recursive(&o, head, remoteheads->item, reversed, &result); + free_commit_list(reversed); + if (clean < 0) { rollback_lock_file(&lock); return 2; diff --git a/commit.c b/commit.c index f674eca3205..1386b12df11 100644 --- a/commit.c +++ b/commit.c @@ -680,7 +680,7 @@ unsigned commit_list_count(const struct commit_list *l) return c; } -struct commit_list *copy_commit_list(struct commit_list *list) +struct commit_list *copy_commit_list(const struct commit_list *list) { struct commit_list *head = NULL; struct commit_list **pp = &head; diff --git a/commit.h b/commit.h index 442e50ff245..acabb057857 100644 --- a/commit.h +++ b/commit.h @@ -181,7 +181,7 @@ struct commit_list *commit_list_insert_by_date(struct commit *item, void commit_list_sort_by_date(struct commit_list **list); /* Shallow copy of the input list */ -struct commit_list *copy_commit_list(struct commit_list *list); +struct commit_list *copy_commit_list(const struct commit_list *list); /* Modify list in-place to reverse it, returning new head; list will be tail */ struct commit_list *reverse_commit_list(struct commit_list *list); diff --git a/log-tree.c b/log-tree.c index 41416de4e3f..91b96d83a01 100644 --- a/log-tree.c +++ b/log-tree.c @@ -1047,6 +1047,7 @@ static int do_remerge_diff(struct rev_info *opt, log_tree_diff_flush(opt); /* Cleanup */ + free_commit_list(bases); cleanup_additional_headers(&opt->diffopt); strbuf_release(&parent1_desc); strbuf_release(&parent2_desc); diff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c index 4acedf3c338..d6f61359965 100644 --- a/merge-ort-wrappers.c +++ b/merge-ort-wrappers.c @@ -48,7 +48,7 @@ int merge_ort_nonrecursive(struct merge_options *opt, int merge_ort_recursive(struct merge_options *opt, struct commit *side1, struct commit *side2, - struct commit_list *merge_bases, + const struct commit_list *merge_bases, struct commit **result) { struct tree *head = repo_get_commit_tree(opt->repo, side1); diff --git a/merge-ort-wrappers.h b/merge-ort-wrappers.h index 0c4c57adbb8..90af1f69c55 100644 --- a/merge-ort-wrappers.h +++ b/merge-ort-wrappers.h @@ -19,7 +19,7 @@ int merge_ort_nonrecursive(struct merge_options *opt, int merge_ort_recursive(struct merge_options *opt, struct commit *h1, struct commit *h2, - struct commit_list *ancestors, + const struct commit_list *ancestors, struct commit **result); #endif diff --git a/merge-ort.c b/merge-ort.c index eaede6cead9..8ed8a4c9dcb 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -5071,11 +5071,12 @@ redo: * Originally from merge_recursive_internal(); somewhat adapted, though. */ static void merge_ort_internal(struct merge_options *opt, - struct commit_list *merge_bases, + const struct commit_list *_merge_bases, struct commit *h1, struct commit *h2, struct merge_result *result) { + struct commit_list *merge_bases = copy_commit_list(_merge_bases); struct commit *next; struct commit *merged_merge_bases; const char *ancestor_name; @@ -5085,7 +5086,7 @@ static void merge_ort_internal(struct merge_options *opt, if (repo_get_merge_bases(the_repository, h1, h2, &merge_bases) < 0) { result->clean = -1; - return; + goto out; } /* See merge-ort.h:merge_incore_recursive() declaration NOTE */ merge_bases = reverse_commit_list(merge_bases); @@ -5129,7 +5130,7 @@ static void merge_ort_internal(struct merge_options *opt, opt->branch2 = "Temporary merge branch 2"; merge_ort_internal(opt, NULL, prev, next, result); if (result->clean < 0) - return; + goto out; opt->branch1 = saved_b1; opt->branch2 = saved_b2; opt->priv->call_depth--; @@ -5152,6 +5153,9 @@ static void merge_ort_internal(struct merge_options *opt, result); strbuf_release(&merge_base_abbrev); opt->ancestor = NULL; /* avoid accidental re-use of opt->ancestor */ + +out: + free_commit_list(merge_bases); } void merge_incore_nonrecursive(struct merge_options *opt, @@ -5181,7 +5185,7 @@ void merge_incore_nonrecursive(struct merge_options *opt, } void merge_incore_recursive(struct merge_options *opt, - struct commit_list *merge_bases, + const struct commit_list *merge_bases, struct commit *side1, struct commit *side2, struct merge_result *result) diff --git a/merge-ort.h b/merge-ort.h index ce56ec1a780..6af97c0828b 100644 --- a/merge-ort.h +++ b/merge-ort.h @@ -59,7 +59,7 @@ struct merge_result { * first", 2006-08-09) */ void merge_incore_recursive(struct merge_options *opt, - struct commit_list *merge_bases, + const struct commit_list *merge_bases, struct commit *side1, struct commit *side2, struct merge_result *result); diff --git a/merge-recursive.c b/merge-recursive.c index 832c8ef3f34..1ac0316cce4 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3633,15 +3633,16 @@ static int merge_trees_internal(struct merge_options *opt, static int merge_recursive_internal(struct merge_options *opt, struct commit *h1, struct commit *h2, - struct commit_list *merge_bases, + const struct commit_list *_merge_bases, struct commit **result) { + struct commit_list *merge_bases = copy_commit_list(_merge_bases); struct commit_list *iter; struct commit *merged_merge_bases; struct tree *result_tree; - int clean; const char *ancestor_name; struct strbuf merge_base_abbrev = STRBUF_INIT; + int ret; if (show(opt, 4)) { output(opt, 4, _("Merging:")); @@ -3651,8 +3652,10 @@ static int merge_recursive_internal(struct merge_options *opt, if (!merge_bases) { if (repo_get_merge_bases(the_repository, h1, h2, - &merge_bases) < 0) - return -1; + &merge_bases) < 0) { + ret = -1; + goto out; + } merge_bases = reverse_commit_list(merge_bases); } @@ -3702,14 +3705,18 @@ static int merge_recursive_internal(struct merge_options *opt, opt->branch1 = "Temporary merge branch 1"; opt->branch2 = "Temporary merge branch 2"; if (merge_recursive_internal(opt, merged_merge_bases, iter->item, - NULL, &merged_merge_bases) < 0) - return -1; + NULL, &merged_merge_bases) < 0) { + ret = -1; + goto out; + } opt->branch1 = saved_b1; opt->branch2 = saved_b2; opt->priv->call_depth--; - if (!merged_merge_bases) - return err(opt, _("merge returned no commit")); + if (!merged_merge_bases) { + ret = err(opt, _("merge returned no commit")); + goto out; + } } /* @@ -3726,17 +3733,16 @@ static int merge_recursive_internal(struct merge_options *opt, repo_read_index(opt->repo); opt->ancestor = ancestor_name; - clean = merge_trees_internal(opt, - repo_get_commit_tree(opt->repo, h1), - repo_get_commit_tree(opt->repo, h2), - repo_get_commit_tree(opt->repo, - merged_merge_bases), - &result_tree); - strbuf_release(&merge_base_abbrev); + ret = merge_trees_internal(opt, + repo_get_commit_tree(opt->repo, h1), + repo_get_commit_tree(opt->repo, h2), + repo_get_commit_tree(opt->repo, + merged_merge_bases), + &result_tree); opt->ancestor = NULL; /* avoid accidental re-use of opt->ancestor */ - if (clean < 0) { + if (ret < 0) { flush_output(opt); - return clean; + goto out; } if (opt->priv->call_depth) { @@ -3745,7 +3751,11 @@ static int merge_recursive_internal(struct merge_options *opt, commit_list_insert(h1, &(*result)->parents); commit_list_insert(h2, &(*result)->parents->next); } - return clean; + +out: + strbuf_release(&merge_base_abbrev); + free_commit_list(merge_bases); + return ret; } static int merge_start(struct merge_options *opt, struct tree *head) @@ -3827,7 +3837,7 @@ int merge_trees(struct merge_options *opt, int merge_recursive(struct merge_options *opt, struct commit *h1, struct commit *h2, - struct commit_list *merge_bases, + const struct commit_list *merge_bases, struct commit **result) { int clean; @@ -3895,6 +3905,7 @@ int merge_recursive_generic(struct merge_options *opt, repo_hold_locked_index(opt->repo, &lock, LOCK_DIE_ON_ERROR); clean = merge_recursive(opt, head_commit, next_commit, ca, result); + free_commit_list(ca); if (clean < 0) { rollback_lock_file(&lock); return clean; diff --git a/merge-recursive.h b/merge-recursive.h index 839eb6436e4..3136c7cc2df 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -104,7 +104,7 @@ int merge_trees(struct merge_options *opt, int merge_recursive(struct merge_options *opt, struct commit *h1, struct commit *h2, - struct commit_list *merge_bases, + const struct commit_list *merge_bases, struct commit **result); /* diff --git a/sequencer.c b/sequencer.c index 20807ea7e58..131443c2427 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4315,6 +4315,7 @@ leave_merge: strbuf_release(&ref_name); rollback_lock_file(&lock); free_commit_list(to_merge); + free_commit_list(bases); return ret; } diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 59b5d6b6f27..36ca126bcdf 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -21,6 +21,7 @@ Initial setup: 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-rebase.sh . "$TEST_DIRECTORY"/lib-log-graph.sh diff --git a/t/t6402-merge-rename.sh b/t/t6402-merge-rename.sh index 2738b50c2a9..729aac9842d 100755 --- a/t/t6402-merge-rename.sh +++ b/t/t6402-merge-rename.sh @@ -4,6 +4,7 @@ test_description='Merge-recursive merging renames' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh modify () { diff --git a/t/t6430-merge-recursive.sh b/t/t6430-merge-recursive.sh index ca15e6dd6da..555f00f78a1 100755 --- a/t/t6430-merge-recursive.sh +++ b/t/t6430-merge-recursive.sh @@ -5,6 +5,7 @@ test_description='merge-recursive backend test' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-merge.sh diff --git a/t/t6436-merge-overwrite.sh b/t/t6436-merge-overwrite.sh index 4f4376421e7..ccc620477d4 100755 --- a/t/t6436-merge-overwrite.sh +++ b/t/t6436-merge-overwrite.sh @@ -7,6 +7,7 @@ Do not overwrite changes.' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t7611-merge-abort.sh b/t/t7611-merge-abort.sh index d6975ca48df..992a8f98749 100755 --- a/t/t7611-merge-abort.sh +++ b/t/t7611-merge-abort.sh @@ -25,6 +25,7 @@ Next, test git merge --abort with the following variables: GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' From 4b4f5a911c4d2fedf4620ff079dcb2a5a0edccc7 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:21:15 +0200 Subject: [PATCH 26/29] line-range: plug leaking find functions In `parse_range_funcname()` we may end up allocating a "find function", but never free it. Fix this. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- line-range.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/line-range.c b/line-range.c index 60f0e5ada81..b99f0d98955 100644 --- a/line-range.c +++ b/line-range.c @@ -234,6 +234,8 @@ static const char *parse_range_funcname( } regfree(®exp); + if (xecfg) + xdiff_clear_find_func(xecfg); free(xecfg); free(pattern); From ee6a998583108cebc0db28d70df2aa1b547b6251 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:21:20 +0200 Subject: [PATCH 27/29] blame: fix leaking data for blame scoreboards There are some memory leaks when cleaning up blame scoreboards. Fix those. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- blame.c | 4 ++++ t/t4061-diff-indent.sh | 1 + t/t8002-blame.sh | 1 + t/t8004-blame-with-conflicts.sh | 1 + t/t8006-blame-textconv.sh | 2 ++ t/t8009-blame-vs-topicbranches.sh | 2 ++ t/t8011-blame-split-file.sh | 2 ++ t/t8012-blame-colors.sh | 1 + t/t8014-blame-ignore-fuzzy.sh | 2 ++ 9 files changed, 16 insertions(+) diff --git a/blame.c b/blame.c index 33586b97772..f97d0e9e1a1 100644 --- a/blame.c +++ b/blame.c @@ -2928,6 +2928,10 @@ void setup_blame_bloom_data(struct blame_scoreboard *sb) void cleanup_scoreboard(struct blame_scoreboard *sb) { + free(sb->lineno); + clear_prio_queue(&sb->commits); + oidset_clear(&sb->ignore_list); + if (sb->bloom_data) { int i; for (i = 0; i < sb->bloom_data->nr; i++) { diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh index 7750b87ca16..2942e5d9b93 100755 --- a/t/t4061-diff-indent.sh +++ b/t/t4061-diff-indent.sh @@ -6,6 +6,7 @@ test_description='Test diff indent heuristic. 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 diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh index 0147de304b4..35966340397 100755 --- a/t/t8002-blame.sh +++ b/t/t8002-blame.sh @@ -5,6 +5,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME TEST_CREATE_REPO_NO_TEMPLATE=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh PROG='git blame -c' diff --git a/t/t8004-blame-with-conflicts.sh b/t/t8004-blame-with-conflicts.sh index 35414a53363..2c2a0b33f9b 100755 --- a/t/t8004-blame-with-conflicts.sh +++ b/t/t8004-blame-with-conflicts.sh @@ -6,6 +6,7 @@ test_description='git blame on conflicted files' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup first case' ' diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh index 76835151554..42f8be25a3f 100755 --- a/t/t8006-blame-textconv.sh +++ b/t/t8006-blame-textconv.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='git blame textconv support' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh find_blame() { diff --git a/t/t8009-blame-vs-topicbranches.sh b/t/t8009-blame-vs-topicbranches.sh index 72596e38b25..30331713b95 100755 --- a/t/t8009-blame-vs-topicbranches.sh +++ b/t/t8009-blame-vs-topicbranches.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='blaming trough history with topic branches' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Creates the history shown below. '*'s mark the first parent in the merges. diff --git a/t/t8011-blame-split-file.sh b/t/t8011-blame-split-file.sh index bdda0c03fe9..da1801f4d23 100755 --- a/t/t8011-blame-split-file.sh +++ b/t/t8011-blame-split-file.sh @@ -10,6 +10,8 @@ Note that we need to use "blame -C" to find the commit for all lines. We will not bother testing that the non-C case fails to find it. That is how blame behaves now, but it is not a property we want to make sure is retained. ' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # help avoid typing and reading long strings of similar lines diff --git a/t/t8012-blame-colors.sh b/t/t8012-blame-colors.sh index c3a5f6d01ff..9a79c109f2e 100755 --- a/t/t8012-blame-colors.sh +++ b/t/t8012-blame-colors.sh @@ -5,6 +5,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME TEST_CREATE_REPO_NO_TEMPLATE=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh PROG='git blame -c' diff --git a/t/t8014-blame-ignore-fuzzy.sh b/t/t8014-blame-ignore-fuzzy.sh index 0bd03413018..933222cea15 100755 --- a/t/t8014-blame-ignore-fuzzy.sh +++ b/t/t8014-blame-ignore-fuzzy.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='git blame ignore fuzzy heuristic' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh pick_author='s/^[0-9a-f^]* *(\([^ ]*\) .*/\1/' From 3332f35577ccbb51a50d88d16caafcceaab23767 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:21:25 +0200 Subject: [PATCH 28/29] builtin/blame: fix leaking prefixed paths In `cmd_blame()` we compute prefixed paths by calling `add_prefix()`, which itself calls `prefix_path()`. While `prefix_path()` returns an allocated string, `add_prefix()` pretends to return a constant string. Consequently, this path never gets freed. Fix the return type to be `char *` and free the path to plug the memory leak. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/blame.c | 5 +++-- t/t6130-pathspec-noglob.sh | 2 ++ t/t7010-setup.sh | 1 + t/t8003-blame-corner-cases.sh | 1 + t/t8008-blame-formats.sh | 2 ++ 5 files changed, 9 insertions(+), 2 deletions(-) diff --git a/builtin/blame.c b/builtin/blame.c index e09ff0155a5..17694410ed9 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -687,7 +687,7 @@ static unsigned parse_score(const char *arg) return score; } -static const char *add_prefix(const char *prefix, const char *path) +static char *add_prefix(const char *prefix, const char *path) { return prefix_path(prefix, prefix ? strlen(prefix) : 0, path); } @@ -865,7 +865,7 @@ static void build_ignorelist(struct blame_scoreboard *sb, int cmd_blame(int argc, const char **argv, const char *prefix) { struct rev_info revs; - const char *path; + char *path = NULL; struct blame_scoreboard sb; struct blame_origin *o; struct blame_entry *ent = NULL; @@ -1226,6 +1226,7 @@ parse_done: } cleanup: + free(path); cleanup_scoreboard(&sb); release_revisions(&revs); return 0; diff --git a/t/t6130-pathspec-noglob.sh b/t/t6130-pathspec-noglob.sh index ba7902c9cdc..82de25d549a 100755 --- a/t/t6130-pathspec-noglob.sh +++ b/t/t6130-pathspec-noglob.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='test globbing (and noglob) of pathspec limiting' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'create commits with glob characters' ' diff --git a/t/t7010-setup.sh b/t/t7010-setup.sh index 520f96d09fb..d9add2162e9 100755 --- a/t/t7010-setup.sh +++ b/t/t7010-setup.sh @@ -2,6 +2,7 @@ test_description='setup taking and sanitizing funny paths' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh index 731265541ac..6288352f577 100755 --- a/t/t8003-blame-corner-cases.sh +++ b/t/t8003-blame-corner-cases.sh @@ -4,6 +4,7 @@ test_description='git blame corner cases' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh pick_fc='s/^[0-9a-f^]* *\([^ ]*\) *(\([^ ]*\) .*/\1-\2/' diff --git a/t/t8008-blame-formats.sh b/t/t8008-blame-formats.sh index ae4b579d245..fb5d225a671 100755 --- a/t/t8008-blame-formats.sh +++ b/t/t8008-blame-formats.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='blame output in various formats on a simple case' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' From fbf7a46d881429ef5495af7bbf3a6c3dacbf80b3 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Jun 2024 11:21:29 +0200 Subject: [PATCH 29/29] builtin/blame: fix leaking ignore revs files When parsing the blame configuration we add "blame.ignoreRevsFile" configs to a string list. This string list is declared as with `NODUP`, and thus we hand over the allocated string to that list. We eventually end up calling `string_list_clear()` on that list, but due to it being declared as `NODUP` we will not release the associated strings and thus leak memory. Fix this issue by setting up the list as `DUP` instead and free the config string after insertion. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/blame.c | 3 ++- t/t8013-blame-ignore-revs.sh | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/builtin/blame.c b/builtin/blame.c index 17694410ed9..702fe4fb94c 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -67,7 +67,7 @@ static int no_whole_file_rename; static int show_progress; static char repeated_meta_color[COLOR_MAXLEN]; static int coloring_mode; -static struct string_list ignore_revs_file_list = STRING_LIST_INIT_NODUP; +static struct string_list ignore_revs_file_list = STRING_LIST_INIT_DUP; static int mark_unblamable_lines; static int mark_ignored_lines; @@ -725,6 +725,7 @@ static int git_blame_config(const char *var, const char *value, if (ret) return ret; string_list_insert(&ignore_revs_file_list, str); + free(str); return 0; } if (!strcmp(var, "blame.markunblamablelines")) { diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh index dbfbd86e83a..d33788d8677 100755 --- a/t/t8013-blame-ignore-revs.sh +++ b/t/t8013-blame-ignore-revs.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='ignore revisions when blaming' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Creates: