From 9a48fc1da277f37b602f48e8bec22f4725ebf877 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:13:10 +0200 Subject: [PATCH 01/23] builtin/annotate: fix leaking args vector We're leaking the args vector in git-annotate(1) because we never clear it. Fixing it isn't as easy as calling `strvec_clear()` though because calling `cmd_blame()` will cause the underlying array to be modified. Instead, we also need to pass a shallow copy of the argv array to the function. Do so to plug the memory leaks. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/annotate.c | 20 +++++++++++++++----- t/t8001-annotate.sh | 1 + 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/builtin/annotate.c b/builtin/annotate.c index a99179fe4d..03413c7df8 100644 --- a/builtin/annotate.c +++ b/builtin/annotate.c @@ -15,13 +15,23 @@ int cmd_annotate(int argc, struct repository *repo UNUSED) { struct strvec args = STRVEC_INIT; - int i; + const char **args_copy; + int ret; strvec_pushl(&args, "annotate", "-c", NULL); - - for (i = 1; i < argc; i++) { + for (int i = 1; i < argc; i++) strvec_push(&args, argv[i]); - } - return cmd_blame(args.nr, args.v, prefix, the_repository); + /* + * `cmd_blame()` ends up modifying the array, which causes memory leaks + * if we didn't copy the array here. + */ + CALLOC_ARRAY(args_copy, args.nr + 1); + COPY_ARRAY(args_copy, args.v, args.nr); + + ret = cmd_blame(args.nr, args_copy, prefix, the_repository); + + strvec_clear(&args); + free(args_copy); + return ret; } diff --git a/t/t8001-annotate.sh b/t/t8001-annotate.sh index d7167f5539..d434698919 100755 --- a/t/t8001-annotate.sh +++ b/t/t8001-annotate.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 annotate' From a69d120c077ce4e3f6164e23d41147370a0d687a Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:13:13 +0200 Subject: [PATCH 02/23] read-cache: fix leaking hash context in `do_write_index()` When writing an index with the EOIE extension we allocate a separate hash context. We never free that context though, causing a memory leak. Plug it. This leak is exposed by t9210, but plugging it alone does not make the whole test suite pass. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- read-cache.c | 1 + 1 file changed, 1 insertion(+) diff --git a/read-cache.c b/read-cache.c index 764fdfec46..0fb5e0d372 100644 --- a/read-cache.c +++ b/read-cache.c @@ -3124,6 +3124,7 @@ out: if (f) free_hashfile(f); strbuf_release(&sb); + free(eoie_c); free(ieot); return ret; } From d607bd88161d1826adc236bf7cf758e754becd61 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:13:15 +0200 Subject: [PATCH 03/23] scalar: fix leaking repositories In the scalar code we iterate through multiple repositories, initializing each of them. We never clear them though, causing memory leaks. Plug them. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- scalar.c | 1 + t/t9210-scalar.sh | 1 + t/t9211-scalar-clone.sh | 1 + 3 files changed, 3 insertions(+) diff --git a/scalar.c b/scalar.c index 09560aeab5..ede616ad4f 100644 --- a/scalar.c +++ b/scalar.c @@ -732,6 +732,7 @@ static int cmd_reconfigure(int argc, const char **argv) succeeded = 1; the_repository = old_repo; + repo_clear(&r); loop_end: if (!succeeded) { diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh index e8613990e1..a131a6c029 100755 --- a/t/t9210-scalar.sh +++ b/t/t9210-scalar.sh @@ -2,6 +2,7 @@ test_description='test the `scalar` command' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt,launchctl:true,schtasks:true" diff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh index 7869f45ee6..c16ea67c1d 100755 --- a/t/t9211-scalar-clone.sh +++ b/t/t9211-scalar-clone.sh @@ -2,6 +2,7 @@ test_description='test the `scalar clone` subcommand' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "${TEST_DIRECTORY}/lib-terminal.sh" From c75841687b39f4c62fba56bde08653591b9c2149 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:13:22 +0200 Subject: [PATCH 04/23] shell: fix leaking strings There are two memory leaks in "shell.c". The first one in `run_shell()` is trivial and fixed without further explanation. The second one in `cmd_main()` happens because we overwrite the `prog` variable, which contains an allocated string. In fact though, the memory pointed to by that variable is still in use because we use `split_cmdline()`, which may create pointers into the middle of that string. But as we do not have a direct pointer to the head of the allocated string anymore, we get a complaint by the leak checker. Address this by not overwriting the `prog` pointer. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- shell.c | 6 +++--- t/t9400-git-cvsserver-server.sh | 1 + t/t9850-shell.sh | 2 ++ 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/shell.c b/shell.c index 2ece8b16e2..76333c8068 100644 --- a/shell.c +++ b/shell.c @@ -143,6 +143,7 @@ static void run_shell(void) } free(argv); + free(split_args); free(rawargs); } while (!done); } @@ -216,9 +217,8 @@ int cmd_main(int argc, const char **argv) count = split_cmdline(prog, &user_argv); if (count >= 0) { if (is_valid_cmd_name(user_argv[0])) { - prog = make_cmd(user_argv[0]); - user_argv[0] = prog; - execv(user_argv[0], (char *const *) user_argv); + char *cmd = make_cmd(user_argv[0]); + execv(cmd, (char *const *) user_argv); } free(prog); free(user_argv); diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh index e499c7f955..6da7440e73 100755 --- a/t/t9400-git-cvsserver-server.sh +++ b/t/t9400-git-cvsserver-server.sh @@ -11,6 +11,7 @@ cvs CLI client via git-cvsserver server' 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; then diff --git a/t/t9850-shell.sh b/t/t9850-shell.sh index cfc71c3bd4..f503f16d1b 100755 --- a/t/t9850-shell.sh +++ b/t/t9850-shell.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='git shell tests' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'shell allows upload-pack' ' From 666643fa89e4386c0f4eabd1112e8b3af0cb9cc1 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:13:24 +0200 Subject: [PATCH 05/23] wt-status: fix leaking buffer with sparse directories When hitting a sparse directory in `wt_status_collect_changes_initial()` we use a `struct strbuf` to assemble the directory's name. We never free that buffer though, causing a memory leak. Fix the leak by releasing the buffer. While at it, move the buffer outside of the loop and reset it to save on some wasteful allocations. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/t1092-sparse-checkout-compatibility.sh | 1 + wt-status.c | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index eb32da2a7f..55efafe4e0 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -5,6 +5,7 @@ test_description='compare full workdir to sparse workdir' GIT_TEST_SPLIT_INDEX=0 GIT_TEST_SPARSE_INDEX= +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/wt-status.c b/wt-status.c index 6a6397ca8f..6a8c05d1cf 100644 --- a/wt-status.c +++ b/wt-status.c @@ -717,6 +717,7 @@ static int add_file_to_list(const struct object_id *oid, static void wt_status_collect_changes_initial(struct wt_status *s) { struct index_state *istate = s->repo->index; + struct strbuf base = STRBUF_INIT; int i; for (i = 0; i < istate->cache_nr; i++) { @@ -735,7 +736,6 @@ static void wt_status_collect_changes_initial(struct wt_status *s) * expanding the trees to find the elements that are new in this * tree and marking them with DIFF_STATUS_ADDED. */ - struct strbuf base = STRBUF_INIT; struct pathspec ps = { 0 }; struct tree *tree = lookup_tree(istate->repo, &ce->oid); @@ -743,9 +743,11 @@ static void wt_status_collect_changes_initial(struct wt_status *s) ps.has_wildcard = 1; ps.max_depth = -1; + strbuf_reset(&base); strbuf_add(&base, ce->name, ce->ce_namelen); read_tree_at(istate->repo, tree, &base, 0, &ps, add_file_to_list, s); + continue; } @@ -772,6 +774,8 @@ static void wt_status_collect_changes_initial(struct wt_status *s) s->committable = 1; } } + + strbuf_release(&base); } static void wt_status_collect_untracked(struct wt_status *s) From 5cca114973c602c9f8516e9ed34c76fd75fb999e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:13:27 +0200 Subject: [PATCH 06/23] submodule: fix leaking submodule entry list The submodule entry list returned by `submodules_of_tree()` is never completely free'd by its only caller. Introduce a new function that free's the list for us and call it. While at it, also fix the leaking `branch_point` string. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- branch.c | 8 ++++++-- submodule-config.c | 15 ++++++++++++++- submodule-config.h | 3 +++ t/t3207-branch-submodule.sh | 1 + 4 files changed, 24 insertions(+), 3 deletions(-) diff --git a/branch.c b/branch.c index 08fa4094d2..44977ad0aa 100644 --- a/branch.c +++ b/branch.c @@ -738,6 +738,7 @@ static int submodule_create_branch(struct repository *r, strbuf_release(&child_err); strbuf_release(&out_buf); + free(out_prefix); return ret; } @@ -794,7 +795,7 @@ void create_branches_recursively(struct repository *r, const char *name, create_branch(r, name, start_committish, force, 0, reflog, quiet, BRANCH_TRACK_NEVER, dry_run); if (dry_run) - return; + goto out; /* * NEEDSWORK If tracking was set up in the superproject but not the * submodule, users might expect "git branch --recurse-submodules" to @@ -815,8 +816,11 @@ void create_branches_recursively(struct repository *r, const char *name, die(_("submodule '%s': cannot create branch '%s'"), submodule_entry_list.entries[i].submodule->name, name); - repo_clear(submodule_entry_list.entries[i].repo); } + +out: + submodule_entry_list_release(&submodule_entry_list); + free(branch_point); } void remove_merge_branch_state(struct repository *r) diff --git a/submodule-config.c b/submodule-config.c index 471637a725..9c8c37b259 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -901,8 +901,9 @@ static void traverse_tree_submodules(struct repository *r, struct submodule_tree_entry *st_entry; struct name_entry name_entry; char *tree_path = NULL; + char *tree_buf; - fill_tree_descriptor(r, &tree, treeish_name); + tree_buf = fill_tree_descriptor(r, &tree, treeish_name); while (tree_entry(&tree, &name_entry)) { if (prefix) tree_path = @@ -930,6 +931,8 @@ static void traverse_tree_submodules(struct repository *r, &name_entry.oid, out); free(tree_path); } + + free(tree_buf); } void submodules_of_tree(struct repository *r, @@ -943,6 +946,16 @@ void submodules_of_tree(struct repository *r, traverse_tree_submodules(r, treeish_name, NULL, treeish_name, out); } +void submodule_entry_list_release(struct submodule_entry_list *list) +{ + for (size_t i = 0; i < list->entry_nr; i++) { + free(list->entries[i].name_entry); + repo_clear(list->entries[i].repo); + free(list->entries[i].repo); + } + free(list->entries); +} + void submodule_free(struct repository *r) { if (r->submodule_cache) diff --git a/submodule-config.h b/submodule-config.h index b6133af71b..f55d4e3b61 100644 --- a/submodule-config.h +++ b/submodule-config.h @@ -136,4 +136,7 @@ struct submodule_entry_list { void submodules_of_tree(struct repository *r, const struct object_id *treeish_name, struct submodule_entry_list *ret); + +void submodule_entry_list_release(struct submodule_entry_list *list); + #endif /* SUBMODULE_CONFIG_H */ diff --git a/t/t3207-branch-submodule.sh b/t/t3207-branch-submodule.sh index fe72b24716..904eea7df5 100755 --- a/t/t3207-branch-submodule.sh +++ b/t/t3207-branch-submodule.sh @@ -5,6 +5,7 @@ test_description='git branch submodule 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 From 64fe1e4a8c8731919b0bb26f6e8b9f8a0f4b0477 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:13:30 +0200 Subject: [PATCH 07/23] builtin/stash: fix leaking `pathspec_from_file` The `OPT_PATHSPEC_FROM_FILE()` option maps to `OPT_FILENAME()`, which we know will always allocate memory when passed. We never free the memory though, causing a memory leak. Plug it. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/stash.c | 4 +++- t/t3909-stash-pathspec-file.sh | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/builtin/stash.c b/builtin/stash.c index f1acc918d0..1399a1bbe2 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1759,7 +1759,7 @@ static int push_stash(int argc, const char **argv, const char *prefix, int quiet = 0; int pathspec_file_nul = 0; const char *stash_msg = NULL; - const char *pathspec_from_file = NULL; + char *pathspec_from_file = NULL; struct pathspec ps; struct option options[] = { OPT_BOOL('k', "keep-index", &keep_index, @@ -1821,7 +1821,9 @@ static int push_stash(int argc, const char **argv, const char *prefix, ret = do_push_stash(&ps, stash_msg, quiet, keep_index, patch_mode, include_untracked, only_staged); + clear_pathspec(&ps); + free(pathspec_from_file); return ret; } diff --git a/t/t3909-stash-pathspec-file.sh b/t/t3909-stash-pathspec-file.sh index 73f2dbdeb0..83269d0eb4 100755 --- a/t/t3909-stash-pathspec-file.sh +++ b/t/t3909-stash-pathspec-file.sh @@ -2,6 +2,7 @@ test_description='stash --pathspec-from-file' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_tick From a0f2a2f5813596053cf785302b2bbfa76cbd9783 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:13:32 +0200 Subject: [PATCH 08/23] builtin/pack-redundant: fix various memory leaks There are various different memory leaks in git-pack-redundant(1), mostly caused by not even trying to free allocated memory. Fix them. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/pack-redundant.c | 40 +++++++++++++++++++++++++++++++++------ t/t5323-pack-redundant.sh | 1 + 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c index 81f4494d46..5809613002 100644 --- a/builtin/pack-redundant.c +++ b/builtin/pack-redundant.c @@ -69,6 +69,15 @@ static inline void llist_init(struct llist **list) (*list)->size = 0; } +static void llist_free(struct llist *list) +{ + for (struct llist_item *i = list->front, *next; i; i = next) { + next = i->next; + llist_item_put(i); + } + free(list); +} + static struct llist * llist_copy(struct llist *list) { struct llist *ret; @@ -206,6 +215,14 @@ static inline struct pack_list * pack_list_insert(struct pack_list **pl, return p; } +static void pack_list_free(struct pack_list *pl) +{ + for (struct pack_list *next; pl; pl = next) { + next = pl->next; + free(pl); + } +} + static inline size_t pack_list_size(struct pack_list *pl) { size_t ret = 0; @@ -419,7 +436,8 @@ static void minimize(struct pack_list **min) /* return if there are no objects missing from the unique set */ if (missing->size == 0) { - free(missing); + llist_free(missing); + pack_list_free(non_unique); return; } @@ -434,6 +452,8 @@ static void minimize(struct pack_list **min) } while (non_unique) { + struct pack_list *next; + /* sort the non_unique packs, greater size of remaining_objects first */ sort_pack_list(&non_unique); if (non_unique->remaining_objects->size == 0) @@ -444,8 +464,14 @@ static void minimize(struct pack_list **min) for (pl = non_unique->next; pl && pl->remaining_objects->size > 0; pl = pl->next) llist_sorted_difference_inplace(pl->remaining_objects, non_unique->remaining_objects); - non_unique = non_unique->next; + next = non_unique->next; + free(non_unique); + non_unique = next; } + + pack_list_free(non_unique); + llist_free(unique_pack_objects); + llist_free(missing); } static void load_all_objects(void) @@ -565,7 +591,6 @@ static void load_all(void) int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, struct repository *repo UNUSED) { int i; int i_still_use_this = 0; struct pack_list *min = NULL, *red, *pl; struct llist *ignore; - struct object_id *oid; char buf[GIT_MAX_HEXSZ + 2]; /* hex hash + \n + \0 */ if (argc == 2 && !strcmp(argv[1], "-h")) @@ -625,11 +650,11 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, s /* ignore objects given on stdin */ llist_init(&ignore); if (!isatty(0)) { + struct object_id oid; while (fgets(buf, sizeof(buf), stdin)) { - oid = xmalloc(sizeof(*oid)); - if (get_oid_hex(buf, oid)) + if (get_oid_hex(buf, &oid)) die("Bad object ID on stdin: %s", buf); - llist_insert_sorted_unique(ignore, oid, NULL); + llist_insert_sorted_unique(ignore, &oid, NULL); } } llist_sorted_difference_inplace(all_objects, ignore); @@ -671,5 +696,8 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, s fprintf(stderr, "%luMB of redundant packs in total.\n", (unsigned long)pack_set_bytecount(red)/(1024*1024)); + pack_list_free(red); + pack_list_free(min); + llist_free(ignore); return 0; } diff --git a/t/t5323-pack-redundant.sh b/t/t5323-pack-redundant.sh index 8dbbcc5e51..4e18f5490a 100755 --- a/t/t5323-pack-redundant.sh +++ b/t/t5323-pack-redundant.sh @@ -34,6 +34,7 @@ relationship between packs and objects is as follows: Px2 | s s s x x x ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh main_repo=main.git From 6361dea6e8f4476b495319a9d3eeed2e2e694f8a Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:13:35 +0200 Subject: [PATCH 09/23] builtin/clone: fix leaking repo state when cloning with bundle URIs When cloning with bundle URIs we re-initialize `the_repository` after having fetched the bundle. This causes a bunch of memory leaks though because we do not release its previous state. These leaks can be plugged by calling `repo_clear()` before we call `repo_init()`. But this causes another issue because the remote that we used is tied to the lifetime of the repository's remote state, which would also get released. We thus have to make sure that it does not get free'd under our feet. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/clone.c | 27 ++++++++++++++++++++++++++ t/t5730-protocol-v2-bundle-uri-file.sh | 1 + t/t5731-protocol-v2-bundle-uri-git.sh | 1 + t/t5732-protocol-v2-bundle-uri-http.sh | 1 + 4 files changed, 30 insertions(+) diff --git a/builtin/clone.c b/builtin/clone.c index e77339c847..59fcb317a6 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -1403,8 +1403,17 @@ int cmd_clone(int argc, * data from the --bundle-uri option. */ if (bundle_uri) { + struct remote_state *state; int has_heuristic = 0; + /* + * We need to save the remote state as our remote's lifetime is + * tied to it. + */ + state = the_repository->remote_state; + the_repository->remote_state = NULL; + repo_clear(the_repository); + /* At this point, we need the_repository to match the cloned repo. */ if (repo_init(the_repository, git_dir, work_tree)) warning(_("failed to initialize the repo, skipping bundle URI")); @@ -1413,6 +1422,10 @@ int cmd_clone(int argc, bundle_uri); else if (has_heuristic) git_config_set_gently("fetch.bundleuri", bundle_uri); + + remote_state_clear(the_repository->remote_state); + free(the_repository->remote_state); + the_repository->remote_state = state; } else { /* * Populate transport->got_remote_bundle_uri and @@ -1422,12 +1435,26 @@ int cmd_clone(int argc, if (transport->bundles && hashmap_get_size(&transport->bundles->bundles)) { + struct remote_state *state; + + /* + * We need to save the remote state as our remote's + * lifetime is tied to it. + */ + state = the_repository->remote_state; + the_repository->remote_state = NULL; + repo_clear(the_repository); + /* At this point, we need the_repository to match the cloned repo. */ if (repo_init(the_repository, git_dir, work_tree)) warning(_("failed to initialize the repo, skipping bundle URI")); else if (fetch_bundle_list(the_repository, transport->bundles)) warning(_("failed to fetch advertised bundles")); + + remote_state_clear(the_repository->remote_state); + free(the_repository->remote_state); + the_repository->remote_state = state; } else { clear_bundle_list(transport->bundles); FREE_AND_NULL(transport->bundles); diff --git a/t/t5730-protocol-v2-bundle-uri-file.sh b/t/t5730-protocol-v2-bundle-uri-file.sh index 37bdb725bc..38396df95b 100755 --- a/t/t5730-protocol-v2-bundle-uri-file.sh +++ b/t/t5730-protocol-v2-bundle-uri-file.sh @@ -7,6 +7,7 @@ TEST_NO_CREATE_REPO=1 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Test protocol v2 with 'file://' transport diff --git a/t/t5731-protocol-v2-bundle-uri-git.sh b/t/t5731-protocol-v2-bundle-uri-git.sh index 8add1b37ab..c199e955fe 100755 --- a/t/t5731-protocol-v2-bundle-uri-git.sh +++ b/t/t5731-protocol-v2-bundle-uri-git.sh @@ -7,6 +7,7 @@ TEST_NO_CREATE_REPO=1 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Test protocol v2 with 'git://' transport diff --git a/t/t5732-protocol-v2-bundle-uri-http.sh b/t/t5732-protocol-v2-bundle-uri-http.sh index 129daa0226..a9403e94c6 100755 --- a/t/t5732-protocol-v2-bundle-uri-http.sh +++ b/t/t5732-protocol-v2-bundle-uri-http.sh @@ -7,6 +7,7 @@ TEST_NO_CREATE_REPO=1 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Test protocol v2 with 'http://' transport From 58888c0401a43bfe2953b0508b5f1bd6c3f32b89 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:13:40 +0200 Subject: [PATCH 10/23] t/helper: fix leaking repository in partial-clone helper We initialize but never clear a repository in the partial-clone test helper. Plug this leak. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/helper/test-partial-clone.c | 2 ++ t/t0410-partial-clone.sh | 1 + 2 files changed, 3 insertions(+) diff --git a/t/helper/test-partial-clone.c b/t/helper/test-partial-clone.c index 0ead529167..a1af9710c3 100644 --- a/t/helper/test-partial-clone.c +++ b/t/helper/test-partial-clone.c @@ -26,6 +26,8 @@ static void object_info(const char *gitdir, const char *oid_hex) if (oid_object_info_extended(&r, &oid, &oi, 0)) die("could not obtain object info"); printf("%d\n", (int) size); + + repo_clear(&r); } int cmd__partial_clone(int argc, const char **argv) diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index 34bdb3ab1f..818700fbec 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh @@ -2,6 +2,7 @@ test_description='partial clone' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-terminal.sh From fdf972a9df19915ef7bddf447bfecbcd0d8aea17 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:13:43 +0200 Subject: [PATCH 11/23] builtin/revert: fix leaking `gpg_sign` and `strategy` config We leak the config values when `gpg_sign` or `strategy` options are being overridden via the command line. To fix this we need to free the old value, which requires us to figure out whether the value was changed via an option in the first place. The easy way to do this, which is to initialize local variables with `NULL`, doesn't work because we cannot tell the case where the user has passed e.g. `--no-gpg-sign`. Instead, we use a sentinel value for both values that we can compare against to check whether the user has passed the option. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/revert.c | 17 +++++++++++++---- t/t3514-cherry-pick-revert-gpg.sh | 1 + 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 55ba1092c5..b7917dddd3 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -110,6 +110,9 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, const char * const * usage_str = revert_or_cherry_pick_usage(opts); const char *me = action_name(opts); const char *cleanup_arg = NULL; + const char sentinel_value; + const char *strategy = &sentinel_value; + const char *gpg_sign = &sentinel_value; enum empty_action empty_opt = EMPTY_COMMIT_UNSPECIFIED; int cmd = 0; struct option base_options[] = { @@ -125,10 +128,10 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, OPT_CALLBACK('m', "mainline", opts, N_("parent-number"), N_("select mainline parent"), option_parse_m), OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto), - OPT_STRING(0, "strategy", &opts->strategy, N_("strategy"), N_("merge strategy")), + OPT_STRING(0, "strategy", &strategy, N_("strategy"), N_("merge strategy")), OPT_STRVEC('X', "strategy-option", &opts->xopts, N_("option"), N_("option for merge strategy")), - { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"), + { OPTION_STRING, 'S', "gpg-sign", &gpg_sign, N_("key-id"), N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, OPT_END() }; @@ -240,8 +243,14 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, usage_with_options(usage_str, options); /* These option values will be free()d */ - opts->gpg_sign = xstrdup_or_null(opts->gpg_sign); - opts->strategy = xstrdup_or_null(opts->strategy); + if (gpg_sign != &sentinel_value) { + free(opts->gpg_sign); + opts->gpg_sign = xstrdup_or_null(gpg_sign); + } + if (strategy != &sentinel_value) { + free(opts->strategy); + opts->strategy = xstrdup_or_null(strategy); + } if (!opts->strategy && getenv("GIT_TEST_MERGE_ALGORITHM")) opts->strategy = xstrdup(getenv("GIT_TEST_MERGE_ALGORITHM")); free(options); diff --git a/t/t3514-cherry-pick-revert-gpg.sh b/t/t3514-cherry-pick-revert-gpg.sh index 5b2e250eaa..133dc07217 100755 --- a/t/t3514-cherry-pick-revert-gpg.sh +++ b/t/t3514-cherry-pick-revert-gpg.sh @@ -5,6 +5,7 @@ test_description='test {cherry-pick,revert} --[no-]gpg-sign' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY/lib-gpg.sh" From a5aecb2cdc8c5f2c1501bdbe30c02959948d8442 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:13:45 +0200 Subject: [PATCH 12/23] diff: improve lifecycle management of diff queues The lifecycle management of diff queues is somewhat confusing: - For most of the part this can be attributed to `DIFF_QUEUE_CLEAR()`, which does not release any memory but rather initializes the queue, only. This is in contrast to our common naming schema, where "clearing" means that we release underlying memory and then re-initialize the data structure such that it is ready to use. - A second offender is `diff_free_queue()`, which does not free the queue structure itself. It is rather a release-style function. Refactor the code to make things less confusing. `DIFF_QUEUE_CLEAR()` is replaced by `DIFF_QUEUE_INIT` and `diff_queue_init()`, while `diff_free_queue()` is replaced by `diff_queue_release()`. While on it, adapt callsites where we call `DIFF_QUEUE_CLEAR()` with the intent to release underlying memory to instead call `diff_queue_clear()` to fix memory leaks. This memory leak is exposed by t4211, but plugging it alone does not make the whole test suite pass. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- bloom.c | 8 +------- diff.c | 22 ++++++++++++---------- diffcore-break.c | 8 ++------ diffcore-pickaxe.c | 4 +--- diffcore-rename.c | 3 +-- diffcore-rotate.c | 3 +-- diffcore.h | 10 ++++------ line-log.c | 15 +++++++-------- log-tree.c | 4 ++-- merge-ort.c | 2 +- 10 files changed, 32 insertions(+), 47 deletions(-) diff --git a/bloom.c b/bloom.c index c915f8b1ba..c428634105 100644 --- a/bloom.c +++ b/bloom.c @@ -476,8 +476,6 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r, *last_slash = '\0'; } while (*path); - - diff_free_filepair(diff_queued_diff.queue[i]); } if (hashmap_get_size(&pathmap) > settings->max_changed_paths) { @@ -508,8 +506,6 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r, cleanup: hashmap_clear_and_free(&pathmap, struct pathmap_hash_entry, entry); } else { - for (i = 0; i < diff_queued_diff.nr; i++) - diff_free_filepair(diff_queued_diff.queue[i]); init_truncated_large_filter(filter, settings->hash_version); if (computed) @@ -519,9 +515,7 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r, if (computed) *computed |= BLOOM_COMPUTED; - free(diff_queued_diff.queue); - DIFF_QUEUE_CLEAR(&diff_queued_diff); - + diff_queue_clear(&diff_queued_diff); return filter; } diff --git a/diff.c b/diff.c index 173cbe2bed..3e9137ffed 100644 --- a/diff.c +++ b/diff.c @@ -5983,11 +5983,18 @@ void diff_free_filepair(struct diff_filepair *p) free(p); } -void diff_free_queue(struct diff_queue_struct *q) +void diff_queue_init(struct diff_queue_struct *q) +{ + struct diff_queue_struct blank = DIFF_QUEUE_INIT; + memcpy(q, &blank, sizeof(*q)); +} + +void diff_queue_clear(struct diff_queue_struct *q) { for (int i = 0; i < q->nr; i++) diff_free_filepair(q->queue[i]); free(q->queue); + diff_queue_init(q); } const char *diff_aligned_abbrev(const struct object_id *oid, int len) @@ -6551,8 +6558,7 @@ int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int struct diff_queue_struct *q = &diff_queued_diff; int result = diff_get_patch_id(options, oid, diff_header_only); - diff_free_queue(q); - DIFF_QUEUE_CLEAR(q); + diff_queue_clear(q); return result; } @@ -6835,8 +6841,7 @@ void diff_flush(struct diff_options *options) } free_queue: - diff_free_queue(q); - DIFF_QUEUE_CLEAR(q); + diff_queue_clear(q); diff_free(options); /* @@ -6867,9 +6872,7 @@ static void diffcore_apply_filter(struct diff_options *options) { int i; struct diff_queue_struct *q = &diff_queued_diff; - struct diff_queue_struct outq; - - DIFF_QUEUE_CLEAR(&outq); + struct diff_queue_struct outq = DIFF_QUEUE_INIT; if (!options->filter) return; @@ -6962,8 +6965,7 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt) { int i; struct diff_queue_struct *q = &diff_queued_diff; - struct diff_queue_struct outq; - DIFF_QUEUE_CLEAR(&outq); + struct diff_queue_struct outq = DIFF_QUEUE_INIT; for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; diff --git a/diffcore-break.c b/diffcore-break.c index 02735f80c6..c4c2173f30 100644 --- a/diffcore-break.c +++ b/diffcore-break.c @@ -131,7 +131,7 @@ static int should_break(struct repository *r, void diffcore_break(struct repository *r, int break_score) { struct diff_queue_struct *q = &diff_queued_diff; - struct diff_queue_struct outq; + struct diff_queue_struct outq = DIFF_QUEUE_INIT; /* When the filepair has this much edit (insert and delete), * it is first considered to be a rewrite and broken into a @@ -178,8 +178,6 @@ void diffcore_break(struct repository *r, int break_score) if (!merge_score) merge_score = DEFAULT_MERGE_SCORE; - DIFF_QUEUE_CLEAR(&outq); - for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; int score; @@ -275,11 +273,9 @@ static void merge_broken(struct diff_filepair *p, void diffcore_merge_broken(void) { struct diff_queue_struct *q = &diff_queued_diff; - struct diff_queue_struct outq; + struct diff_queue_struct outq = DIFF_QUEUE_INIT; int i, j; - DIFF_QUEUE_CLEAR(&outq); - for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; if (!p) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index b195fa4eb3..43fef8e8ba 100644 --- a/diffcore-pickaxe.c +++ b/diffcore-pickaxe.c @@ -182,9 +182,7 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o, regex_t *regexp, kwset_t kws, pickaxe_fn fn) { int i; - struct diff_queue_struct outq; - - DIFF_QUEUE_CLEAR(&outq); + struct diff_queue_struct outq = DIFF_QUEUE_INIT; if (o->pickaxe_opts & DIFF_PICKAXE_ALL) { /* Showing the whole changeset if needle exists */ diff --git a/diffcore-rename.c b/diffcore-rename.c index 3d6826baa3..1b1c1a6a1f 100644 --- a/diffcore-rename.c +++ b/diffcore-rename.c @@ -1388,7 +1388,7 @@ void diffcore_rename_extended(struct diff_options *options, int detect_rename = options->detect_rename; int minimum_score = options->rename_score; struct diff_queue_struct *q = &diff_queued_diff; - struct diff_queue_struct outq; + struct diff_queue_struct outq = DIFF_QUEUE_INIT; struct diff_score *mx; int i, j, rename_count, skip_unmodified = 0; int num_destinations, dst_cnt; @@ -1638,7 +1638,6 @@ void diffcore_rename_extended(struct diff_options *options, * are recorded in rename_dst. The original list is still in *q. */ trace2_region_enter("diff", "write back to queue", options->repo); - DIFF_QUEUE_CLEAR(&outq); for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; struct diff_filepair *pair_to_free = NULL; diff --git a/diffcore-rotate.c b/diffcore-rotate.c index 533986cf63..73ca20b331 100644 --- a/diffcore-rotate.c +++ b/diffcore-rotate.c @@ -10,7 +10,7 @@ void diffcore_rotate(struct diff_options *opt) { struct diff_queue_struct *q = &diff_queued_diff; - struct diff_queue_struct outq; + struct diff_queue_struct outq = DIFF_QUEUE_INIT; int rotate_to, i; if (!q->nr) @@ -31,7 +31,6 @@ void diffcore_rotate(struct diff_options *opt) return; } - DIFF_QUEUE_CLEAR(&outq); rotate_to = i; for (i = rotate_to; i < q->nr; i++) diff --git a/diffcore.h b/diffcore.h index 1701ed50b9..2feb325031 100644 --- a/diffcore.h +++ b/diffcore.h @@ -153,18 +153,16 @@ struct diff_queue_struct { int nr; }; -#define DIFF_QUEUE_CLEAR(q) \ - do { \ - (q)->queue = NULL; \ - (q)->nr = (q)->alloc = 0; \ - } while (0) +#define DIFF_QUEUE_INIT { 0 } + +void diff_queue_init(struct diff_queue_struct *q); +void diff_queue_clear(struct diff_queue_struct *q); extern struct diff_queue_struct diff_queued_diff; struct diff_filepair *diff_queue(struct diff_queue_struct *, struct diff_filespec *, struct diff_filespec *); void diff_q(struct diff_queue_struct *, struct diff_filepair *); -void diff_free_queue(struct diff_queue_struct *q); /* dir_rename_relevance: the reason we want rename information for a dir */ enum dir_rename_relevance { diff --git a/line-log.c b/line-log.c index 67c80b39a0..89e0ea4562 100644 --- a/line-log.c +++ b/line-log.c @@ -787,15 +787,14 @@ static void move_diff_queue(struct diff_queue_struct *dst, struct diff_queue_struct *src) { assert(src != dst); - memcpy(dst, src, sizeof(struct diff_queue_struct)); - DIFF_QUEUE_CLEAR(src); + memcpy(dst, src, sizeof(*dst)); + diff_queue_init(src); } static void filter_diffs_for_paths(struct line_log_data *range, int keep_deletions) { int i; - struct diff_queue_struct outq; - DIFF_QUEUE_CLEAR(&outq); + struct diff_queue_struct outq = DIFF_QUEUE_INIT; for (i = 0; i < diff_queued_diff.nr; i++) { struct diff_filepair *p = diff_queued_diff.queue[i]; @@ -850,12 +849,12 @@ static void queue_diffs(struct line_log_data *range, clear_pathspec(&opt->pathspec); parse_pathspec_from_ranges(&opt->pathspec, range); } - DIFF_QUEUE_CLEAR(&diff_queued_diff); + diff_queue_clear(&diff_queued_diff); diff_tree_oid(parent_tree_oid, tree_oid, "", opt); if (opt->detect_rename && diff_might_be_rename()) { /* must look at the full tree diff to detect renames */ clear_pathspec(&opt->pathspec); - DIFF_QUEUE_CLEAR(&diff_queued_diff); + diff_queue_clear(&diff_queued_diff); diff_tree_oid(parent_tree_oid, tree_oid, "", opt); @@ -1097,7 +1096,7 @@ static struct diff_filepair *diff_filepair_dup(struct diff_filepair *pair) static void free_diffqueues(int n, struct diff_queue_struct *dq) { for (int i = 0; i < n; i++) - diff_free_queue(&dq[i]); + diff_queue_clear(&dq[i]); free(dq); } @@ -1200,7 +1199,7 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c if (parent) add_line_range(rev, parent, parent_range); free_line_log_data(parent_range); - diff_free_queue(&queue); + diff_queue_clear(&queue); return changed; } diff --git a/log-tree.c b/log-tree.c index 3758e0d3b8..60774c16b3 100644 --- a/log-tree.c +++ b/log-tree.c @@ -675,7 +675,7 @@ static void show_diff_of_diff(struct rev_info *opt) struct diff_queue_struct dq; memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff)); - DIFF_QUEUE_CLEAR(&diff_queued_diff); + diff_queue_init(&diff_queued_diff); fprintf_ln(opt->diffopt.file, "\n%s", opt->idiff_title); show_interdiff(opt->idiff_oid1, opt->idiff_oid2, 2, @@ -694,7 +694,7 @@ static void show_diff_of_diff(struct rev_info *opt) }; memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff)); - DIFF_QUEUE_CLEAR(&diff_queued_diff); + diff_queue_init(&diff_queued_diff); fprintf_ln(opt->diffopt.file, "\n%s", opt->rdiff_title); /* diff --git a/merge-ort.c b/merge-ort.c index 8b81153e8f..11029c10be 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -3536,7 +3536,7 @@ simple_cleanup: /* Free memory for renames->pairs[] and combined */ for (s = MERGE_SIDE1; s <= MERGE_SIDE2; s++) { free(renames->pairs[s].queue); - DIFF_QUEUE_CLEAR(&renames->pairs[s]); + diff_queue_init(&renames->pairs[s]); } for (i = 0; i < combined.nr; i++) pool_diff_free_filepair(&opt->priv->pool, combined.queue[i]); From 5ce08ed4fbb60c984f46632aaa30d8f781e57e56 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:13:48 +0200 Subject: [PATCH 13/23] line-log: fix several memory leaks As described in "line-log.c" itself, the code is "leaking like a sieve". These leaks are all of rather trivial nature, so this commit plugs them without going too much into details for each of those leaks. The leaks are hit by t4211, but plugging them alone does not make the full test suite pass. The remaining leaks are unrelated to the line-log subsystem. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- line-log.c | 54 +++++++++++++++++++++++++++++++++++------------------- 1 file changed, 35 insertions(+), 19 deletions(-) diff --git a/line-log.c b/line-log.c index 89e0ea4562..ee48988c66 100644 --- a/line-log.c +++ b/line-log.c @@ -248,8 +248,10 @@ static void line_log_data_init(struct line_log_data *r) static void line_log_data_clear(struct line_log_data *r) { range_set_release(&r->ranges); + free(r->path); if (r->pair) diff_free_filepair(r->pair); + diff_ranges_release(&r->diff); } static void free_line_log_data(struct line_log_data *r) @@ -571,7 +573,8 @@ parse_lines(struct repository *r, struct commit *commit, struct line_log_data *p; for_each_string_list_item(item, args) { - const char *name_part, *range_part; + const char *name_part; + char *range_part; char *full_name; struct diff_filespec *spec; long begin = 0, end = 0; @@ -615,6 +618,7 @@ parse_lines(struct repository *r, struct commit *commit, free_filespec(spec); FREE_AND_NULL(ends); + free(range_part); } for (p = ranges; p; p = p->next) @@ -760,15 +764,13 @@ static void parse_pathspec_from_ranges(struct pathspec *pathspec, { struct line_log_data *r; struct strvec array = STRVEC_INIT; - const char **paths; for (r = range; r; r = r->next) strvec_push(&array, r->path); - paths = strvec_detach(&array); - parse_pathspec(pathspec, 0, PATHSPEC_PREFER_FULL, "", paths); - /* strings are now owned by pathspec */ - free(paths); + parse_pathspec(pathspec, 0, PATHSPEC_PREFER_FULL, "", array.v); + + strvec_clear(&array); } void line_log_init(struct rev_info *rev, const char *prefix, struct string_list *args) @@ -781,6 +783,8 @@ void line_log_init(struct rev_info *rev, const char *prefix, struct string_list add_line_range(rev, commit, range); parse_pathspec_from_ranges(&rev->diffopt.pathspec, range); + + free_line_log_data(range); } static void move_diff_queue(struct diff_queue_struct *dst, @@ -1131,10 +1135,18 @@ static int process_all_files(struct line_log_data **range_out, while (rg && strcmp(rg->path, pair->two->path)) rg = rg->next; assert(rg); + if (rg->pair) + diff_free_filepair(rg->pair); rg->pair = diff_filepair_dup(queue->queue[i]); + diff_ranges_release(&rg->diff); memcpy(&rg->diff, pairdiff, sizeof(struct diff_ranges)); + FREE_AND_NULL(pairdiff); + } + + if (pairdiff) { + diff_ranges_release(pairdiff); + free(pairdiff); } - free(pairdiff); } return changed; @@ -1212,12 +1224,13 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm struct commit_list *p; int i; int nparents = commit_list_count(commit->parents); + int ret; if (nparents > 1 && rev->first_parent_only) nparents = 1; ALLOC_ARRAY(diffqueues, nparents); - ALLOC_ARRAY(cand, nparents); + CALLOC_ARRAY(cand, nparents); ALLOC_ARRAY(parents, nparents); p = commit->parents; @@ -1229,7 +1242,6 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm for (i = 0; i < nparents; i++) { int changed; - cand[i] = NULL; changed = process_all_files(&cand[i], rev, &diffqueues[i], range); if (!changed) { /* @@ -1237,13 +1249,10 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm * don't follow any other path in history */ add_line_range(rev, parents[i], cand[i]); - clear_commit_line_range(rev, commit); commit_list_append(parents[i], &commit->parents); - free(parents); - free(cand); - free_diffqueues(nparents, diffqueues); - /* NEEDSWORK leaking like a sieve */ - return 0; + + ret = 0; + goto out; } } @@ -1251,18 +1260,25 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm * No single parent took the blame. We add the candidates * from the above loop to the parents. */ - for (i = 0; i < nparents; i++) { + for (i = 0; i < nparents; i++) add_line_range(rev, parents[i], cand[i]); - } + ret = 1; + +out: clear_commit_line_range(rev, commit); free(parents); + for (i = 0; i < nparents; i++) { + if (!cand[i]) + continue; + line_log_data_clear(cand[i]); + free(cand[i]); + } free(cand); free_diffqueues(nparents, diffqueues); - return 1; + return ret; /* NEEDSWORK evil merge detection stuff */ - /* NEEDSWORK leaking like a sieve */ } int line_log_process_ranges_arbitrary_commit(struct rev_info *rev, struct commit *commit) From 55e563a90cabacd144b529b8bab7c4bb1cecbc49 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:13:51 +0200 Subject: [PATCH 14/23] pseudo-merge: fix various memory leaks Fix various memory leaks hit by the pseudo-merge machinery. These leaks are exposed by t5333, but plugging them does not yet make the whole test suite pass. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- pack-bitmap-write.c | 8 ++++++++ pack-bitmap.c | 4 ++-- pseudo-merge.c | 19 +++++++++++++++++++ pseudo-merge.h | 2 ++ 4 files changed, 31 insertions(+), 2 deletions(-) diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c index 4dc0fe8e40..6413dd1731 100644 --- a/pack-bitmap-write.c +++ b/pack-bitmap-write.c @@ -64,6 +64,12 @@ static void free_pseudo_merge_commit_idx(struct pseudo_merge_commit_idx *idx) free(idx); } +static void pseudo_merge_group_release_cb(void *payload, const char *name UNUSED) +{ + pseudo_merge_group_release(payload); + free(payload); +} + void bitmap_writer_free(struct bitmap_writer *writer) { uint32_t i; @@ -82,6 +88,8 @@ void bitmap_writer_free(struct bitmap_writer *writer) kh_foreach_value(writer->pseudo_merge_commits, idx, free_pseudo_merge_commit_idx(idx)); kh_destroy_oid_map(writer->pseudo_merge_commits); + string_list_clear_func(&writer->pseudo_merge_groups, + pseudo_merge_group_release_cb); for (i = 0; i < writer->selected_nr; i++) { struct bitmapped_commit *bc = &writer->selected[i]; diff --git a/pack-bitmap.c b/pack-bitmap.c index 9d9b8c4bfb..32b222a7af 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -1390,8 +1390,8 @@ static struct bitmap *find_objects(struct bitmap_index *bitmap_git, } base = bitmap_new(); - if (!cascade_pseudo_merges_1(bitmap_git, base, roots_bitmap)) - bitmap_free(roots_bitmap); + cascade_pseudo_merges_1(bitmap_git, base, roots_bitmap); + bitmap_free(roots_bitmap); } /* diff --git a/pseudo-merge.c b/pseudo-merge.c index 10ebd9a4e9..28782a31c6 100644 --- a/pseudo-merge.c +++ b/pseudo-merge.c @@ -97,6 +97,25 @@ static void pseudo_merge_group_init(struct pseudo_merge_group *group) group->stable_size = DEFAULT_PSEUDO_MERGE_STABLE_SIZE; } +void pseudo_merge_group_release(struct pseudo_merge_group *group) +{ + struct hashmap_iter iter; + struct strmap_entry *e; + + regfree(group->pattern); + free(group->pattern); + + strmap_for_each_entry(&group->matches, &iter, e) { + struct pseudo_merge_matches *matches = e->value; + free(matches->stable); + free(matches->unstable); + free(matches); + } + strmap_clear(&group->matches, 0); + + free(group->merges); +} + static int pseudo_merge_config(const char *var, const char *value, const struct config_context *ctx, void *cb_data) diff --git a/pseudo-merge.h b/pseudo-merge.h index 4b5febaa63..29df8a32ec 100644 --- a/pseudo-merge.h +++ b/pseudo-merge.h @@ -51,6 +51,8 @@ struct pseudo_merge_group { timestamp_t stable_threshold; }; +void pseudo_merge_group_release(struct pseudo_merge_group *group); + struct pseudo_merge_matches { struct commit **stable; struct commit **unstable; From d0ab6630a710c8b3f4cf7e1a3f630a685c5d5721 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:13:53 +0200 Subject: [PATCH 15/23] pseudo-merge: fix leaking strmap keys When creating a new pseudo-merge group we collect a set of matchnig commits and put them into a string map. This strmap is initialized such that it does not allocate its keys, and instead we try to pass ownership of the keys to it via `strmap_put()`. This isn't how it works though: the strmap will never try to release these keys, and consequently they end up leaking. Fix this leak by initializing the strmap as duplicating its keys and not trying to hand over ownership. The leak is exposed by t5333, but plugging it does not yet make the full test suite pass. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- pseudo-merge.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pseudo-merge.c b/pseudo-merge.c index 28782a31c6..bb59965ed2 100644 --- a/pseudo-merge.c +++ b/pseudo-merge.c @@ -87,7 +87,7 @@ static void pseudo_merge_group_init(struct pseudo_merge_group *group) { memset(group, 0, sizeof(struct pseudo_merge_group)); - strmap_init_with_options(&group->matches, NULL, 0); + strmap_init_with_options(&group->matches, NULL, 1); group->decay = DEFAULT_PSEUDO_MERGE_DECAY; group->max_merges = DEFAULT_PSEUDO_MERGE_MAX_MERGES; @@ -275,7 +275,7 @@ static int find_pseudo_merge_group_for_ref(const char *refname, matches = strmap_get(&group->matches, group_name.buf); if (!matches) { matches = xcalloc(1, sizeof(*matches)); - strmap_put(&group->matches, strbuf_detach(&group_name, NULL), + strmap_put(&group->matches, group_name.buf, matches); } From 7f97266ee15d2a30dbc12c9a7f282a9f94b32476 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:13:58 +0200 Subject: [PATCH 16/23] pack-bitmap-write: fix leaking OID array Fix a leaking OID array in `write_pseudo_merges()`. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- pack-bitmap-write.c | 1 + t/t5333-pseudo-merge-bitmaps.sh | 1 + 2 files changed, 2 insertions(+) diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c index 6413dd1731..49758e2525 100644 --- a/pack-bitmap-write.c +++ b/pack-bitmap-write.c @@ -913,6 +913,7 @@ static void write_pseudo_merges(struct bitmap_writer *writer, for (i = 0; i < writer->pseudo_merges_nr; i++) bitmap_free(commits_bitmap[i]); + oid_array_clear(&commits); free(pseudo_merge_ofs); free(commits_bitmap); } diff --git a/t/t5333-pseudo-merge-bitmaps.sh b/t/t5333-pseudo-merge-bitmaps.sh index 1dd6284756..eca4a1eb8c 100755 --- a/t/t5333-pseudo-merge-bitmaps.sh +++ b/t/t5333-pseudo-merge-bitmaps.sh @@ -4,6 +4,7 @@ test_description='pseudo-merge bitmaps' GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_pseudo_merges () { From 9d4855eef3644ac00d62e08d797ff7db554ca65d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:14:01 +0200 Subject: [PATCH 17/23] midx-write: fix leaking buffer The buffer used to compute the final MIDX name is never released. Plug this memory leak. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- midx-write.c | 2 ++ t/t5334-incremental-multi-pack-index.sh | 2 ++ 2 files changed, 4 insertions(+) diff --git a/midx-write.c b/midx-write.c index 1ef62c4f4b..625c7d3137 100644 --- a/midx-write.c +++ b/midx-write.c @@ -1445,6 +1445,8 @@ static int write_midx_internal(const char *object_dir, return -1; } + strbuf_release(&final_midx_name); + keep_hashes[ctx.num_multi_pack_indexes_before] = xstrdup(hash_to_hex(midx_hash)); diff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh index c3b08acc73..471994c4bc 100755 --- a/t/t5334-incremental-multi-pack-index.sh +++ b/t/t5334-incremental-multi-pack-index.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='incremental multi-pack-index' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-midx.sh From 4cc2cee5ac1491960afc54cc5ef14b50ddceb4d2 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:14:03 +0200 Subject: [PATCH 18/23] revision: fix memory leaks when rewriting parents Both `rewrite_parents()` and `remove_duplicate_parents()` may end up dropping some parents from a commit without freeing the respective `struct commit_list` items. This causes a bunch of memory leaks. Plug these. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- revision.c | 2 ++ t/t3427-rebase-subtree.sh | 1 + t/t6016-rev-list-graph-simplify-history.sh | 1 + t/t7003-filter-branch.sh | 1 + t/t9350-fast-export.sh | 1 + t/t9402-git-cvsserver-refs.sh | 1 + 6 files changed, 7 insertions(+) diff --git a/revision.c b/revision.c index e79f39e555..6b452ea182 100644 --- a/revision.c +++ b/revision.c @@ -3250,6 +3250,7 @@ static int remove_duplicate_parents(struct rev_info *revs, struct commit *commit struct commit *parent = p->item; if (parent->object.flags & TMP_MARK) { *pp = p->next; + free(p); if (ts) compact_treesame(revs, commit, surviving_parents); continue; @@ -4005,6 +4006,7 @@ int rewrite_parents(struct rev_info *revs, struct commit *commit, break; case rewrite_one_noparents: *pp = parent->next; + free(parent); continue; case rewrite_one_error: return -1; diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh index 1b3e97c875..5e9046e3df 100755 --- a/t/t3427-rebase-subtree.sh +++ b/t/t3427-rebase-subtree.sh @@ -7,6 +7,7 @@ This test runs git rebase and tests the subtree strategy. 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/t6016-rev-list-graph-simplify-history.sh b/t/t6016-rev-list-graph-simplify-history.sh index 54b0a6f5f8..2656d6a6bc 100755 --- a/t/t6016-rev-list-graph-simplify-history.sh +++ b/t/t6016-rev-list-graph-simplify-history.sh @@ -10,6 +10,7 @@ test_description='--graph and simplified history' 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-log-graph.sh diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh index 5ab4d41ee7..bf3e3f0b67 100755 --- a/t/t7003-filter-branch.sh +++ b/t/t7003-filter-branch.sh @@ -4,6 +4,7 @@ test_description='git filter-branch' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY/lib-gpg.sh" diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh index 1eb035ee4c..2bdc02b459 100755 --- a/t/t9350-fast-export.sh +++ b/t/t9350-fast-export.sh @@ -7,6 +7,7 @@ test_description='git fast-export' 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/t9402-git-cvsserver-refs.sh b/t/t9402-git-cvsserver-refs.sh index 2ee41f9443..c847120d52 100755 --- a/t/t9402-git-cvsserver-refs.sh +++ b/t/t9402-git-cvsserver-refs.sh @@ -8,6 +8,7 @@ tags, branches and other git refspecs' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh ######### From 6512d6e473c1c1f9f2e6967e2703a19784109a8b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:14:06 +0200 Subject: [PATCH 19/23] revision: fix leaking saved parents The `saved_parents` slab is used by `--full-diff` to save parents of a commit which we are about to rewrite. We do not release its contents once it's not used anymore, causing a memory leak. Plug it. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- revision.c | 12 ++++++++++-- t/t6012-rev-list-simplify.sh | 1 + 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/revision.c b/revision.c index 6b452ea182..f5f5b84f2b 100644 --- a/revision.c +++ b/revision.c @@ -4207,10 +4207,18 @@ static void save_parents(struct rev_info *revs, struct commit *commit) *pp = EMPTY_PARENT_LIST; } +static void free_saved_parent(struct commit_list **parents) +{ + if (*parents != EMPTY_PARENT_LIST) + free_commit_list(*parents); +} + static void free_saved_parents(struct rev_info *revs) { - if (revs->saved_parents_slab) - clear_saved_parents(revs->saved_parents_slab); + if (!revs->saved_parents_slab) + return; + deep_clear_saved_parents(revs->saved_parents_slab, free_saved_parent); + FREE_AND_NULL(revs->saved_parents_slab); } struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit *commit) diff --git a/t/t6012-rev-list-simplify.sh b/t/t6012-rev-list-simplify.sh index de1e87f162..8ed1a215da 100755 --- a/t/t6012-rev-list-simplify.sh +++ b/t/t6012-rev-list-simplify.sh @@ -5,6 +5,7 @@ test_description='merge simplification' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh note () { From 2f0ee051ddaf880daac06773b56f077c4012a1c7 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:14:08 +0200 Subject: [PATCH 20/23] pack-write: fix return parameter of `write_rev_file_order()` While the return parameter of `write_rev_file_order()` is a string constant, the function may indeed return an allocated string when its first parameter is a `NULL` pointer. This makes for a confusing calling convention, where callers need to be aware of these intricate ownership rules and cast away the constness to free the string in some cases. Adapt the function and its caller `write_rev_file()` to always return an allocated string and adapt callers to always free the return value. Note that this requires us to also adapt `rename_tmp_packfile()`, which compares the pointers to packfile data with each other. Now that the path of the reverse index file gets allocated unconditionally the check will always fail. This is fixed by using strcmp(3P) instead, which also feels way less fragile. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/index-pack.c | 7 +++--- midx-write.c | 3 ++- pack-write.c | 42 +++++++++++++++++-------------- pack.h | 4 +-- t/t5327-multi-pack-bitmaps-rev.sh | 1 + 5 files changed, 31 insertions(+), 26 deletions(-) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index e228c56ff2..9d23b41b3a 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1505,7 +1505,7 @@ static void rename_tmp_packfile(const char **final_name, struct strbuf *name, unsigned char *hash, const char *ext, int make_read_only_if_same) { - if (*final_name != curr_name) { + if (!*final_name || strcmp(*final_name, curr_name)) { if (!*final_name) *final_name = odb_pack_name(name, hash, ext); if (finalize_object_file(curr_name, *final_name)) @@ -1726,7 +1726,7 @@ int cmd_index_pack(int argc, { int i, fix_thin_pack = 0, verify = 0, stat_only = 0, rev_index; const char *curr_index; - const char *curr_rev_index = NULL; + char *curr_rev_index = NULL; const char *index_name = NULL, *pack_name = NULL, *rev_index_name = NULL; const char *keep_msg = NULL; const char *promisor_msg = NULL; @@ -1968,8 +1968,7 @@ int cmd_index_pack(int argc, free((void *) curr_pack); if (!index_name) free((void *) curr_index); - if (!rev_index_name) - free((void *) curr_rev_index); + free(curr_rev_index); /* * Let the caller know this pack is not self contained diff --git a/midx-write.c b/midx-write.c index 625c7d3137..b3a5f6c516 100644 --- a/midx-write.c +++ b/midx-write.c @@ -649,7 +649,7 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash, struct write_midx_context *ctx) { struct strbuf buf = STRBUF_INIT; - const char *tmp_file; + char *tmp_file; trace2_region_enter("midx", "write_midx_reverse_index", the_repository); @@ -662,6 +662,7 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash, die(_("cannot store reverse index file")); strbuf_release(&buf); + free(tmp_file); trace2_region_leave("midx", "write_midx_reverse_index", the_repository); } diff --git a/pack-write.c b/pack-write.c index 27965672f1..9961149e65 100644 --- a/pack-write.c +++ b/pack-write.c @@ -212,15 +212,15 @@ static void write_rev_trailer(struct hashfile *f, const unsigned char *hash) hashwrite(f, hash, the_hash_algo->rawsz); } -const char *write_rev_file(const char *rev_name, - struct pack_idx_entry **objects, - uint32_t nr_objects, - const unsigned char *hash, - unsigned flags) +char *write_rev_file(const char *rev_name, + struct pack_idx_entry **objects, + uint32_t nr_objects, + const unsigned char *hash, + unsigned flags) { uint32_t *pack_order; uint32_t i; - const char *ret; + char *ret; if (!(flags & WRITE_REV) && !(flags & WRITE_REV_VERIFY)) return NULL; @@ -238,13 +238,14 @@ const char *write_rev_file(const char *rev_name, return ret; } -const char *write_rev_file_order(const char *rev_name, - uint32_t *pack_order, - uint32_t nr_objects, - const unsigned char *hash, - unsigned flags) +char *write_rev_file_order(const char *rev_name, + uint32_t *pack_order, + uint32_t nr_objects, + const unsigned char *hash, + unsigned flags) { struct hashfile *f; + char *path; int fd; if ((flags & WRITE_REV) && (flags & WRITE_REV_VERIFY)) @@ -254,12 +255,13 @@ const char *write_rev_file_order(const char *rev_name, if (!rev_name) { struct strbuf tmp_file = STRBUF_INIT; fd = odb_mkstemp(&tmp_file, "pack/tmp_rev_XXXXXX"); - rev_name = strbuf_detach(&tmp_file, NULL); + path = strbuf_detach(&tmp_file, NULL); } else { unlink(rev_name); fd = xopen(rev_name, O_CREAT|O_EXCL|O_WRONLY, 0600); + path = xstrdup(rev_name); } - f = hashfd(fd, rev_name); + f = hashfd(fd, path); } else if (flags & WRITE_REV_VERIFY) { struct stat statbuf; if (stat(rev_name, &statbuf)) { @@ -270,22 +272,24 @@ const char *write_rev_file_order(const char *rev_name, die_errno(_("could not stat: %s"), rev_name); } f = hashfd_check(rev_name); - } else + path = xstrdup(rev_name); + } else { return NULL; + } write_rev_header(f); write_rev_index_positions(f, pack_order, nr_objects); write_rev_trailer(f, hash); - if (rev_name && adjust_shared_perm(rev_name) < 0) - die(_("failed to make %s readable"), rev_name); + if (adjust_shared_perm(path) < 0) + die(_("failed to make %s readable"), path); finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA, CSUM_HASH_IN_STREAM | CSUM_CLOSE | ((flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC)); - return rev_name; + return path; } static void write_mtimes_header(struct hashfile *f) @@ -549,7 +553,7 @@ void stage_tmp_packfiles(struct strbuf *name_buffer, unsigned char hash[], char **idx_tmp_name) { - const char *rev_tmp_name = NULL; + char *rev_tmp_name = NULL; char *mtimes_tmp_name = NULL; if (adjust_shared_perm(pack_tmp_name)) @@ -575,7 +579,7 @@ void stage_tmp_packfiles(struct strbuf *name_buffer, if (mtimes_tmp_name) rename_tmp_packfile(name_buffer, mtimes_tmp_name, "mtimes"); - free((char *)rev_tmp_name); + free(rev_tmp_name); free(mtimes_tmp_name); } diff --git a/pack.h b/pack.h index 3ab9e3f60c..02bbdfb19c 100644 --- a/pack.h +++ b/pack.h @@ -96,8 +96,8 @@ struct ref; void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought); -const char *write_rev_file(const char *rev_name, struct pack_idx_entry **objects, uint32_t nr_objects, const unsigned char *hash, unsigned flags); -const char *write_rev_file_order(const char *rev_name, uint32_t *pack_order, uint32_t nr_objects, const unsigned char *hash, unsigned flags); +char *write_rev_file(const char *rev_name, struct pack_idx_entry **objects, uint32_t nr_objects, const unsigned char *hash, unsigned flags); +char *write_rev_file_order(const char *rev_name, uint32_t *pack_order, uint32_t nr_objects, const unsigned char *hash, unsigned flags); /* * The "hdr" output buffer should be at least this big, which will handle sizes diff --git a/t/t5327-multi-pack-bitmaps-rev.sh b/t/t5327-multi-pack-bitmaps-rev.sh index 9cac03a94b..994a8e6be4 100755 --- a/t/t5327-multi-pack-bitmaps-rev.sh +++ b/t/t5327-multi-pack-bitmaps-rev.sh @@ -2,6 +2,7 @@ test_description='exercise basic multi-pack bitmap functionality (.rev files)' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "${TEST_DIRECTORY}/lib-bitmap.sh" From 12f0fb953891940598486bdbe8edd28b05b38278 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:14:16 +0200 Subject: [PATCH 21/23] t/helper: fix leaks in proc-receive helper Fix trivial leaks in the proc-receive helpe. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/helper/test-proc-receive.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/t/helper/test-proc-receive.c b/t/helper/test-proc-receive.c index 29361c7aab..3703f734f3 100644 --- a/t/helper/test-proc-receive.c +++ b/t/helper/test-proc-receive.c @@ -196,5 +196,12 @@ int cmd__proc_receive(int argc, const char **argv) packet_flush(1); sigchain_pop(SIGPIPE); + while (commands) { + struct command *next = commands->next; + free(commands); + commands = next; + } + string_list_clear(&push_options, 0); + return 0; } From a6c30623d77b5fe759d5d9bedc33957ddaff1b4d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:14:21 +0200 Subject: [PATCH 22/23] remote: fix leaking push reports The push reports that report failures to the user when pushing a reference leak in several places. Plug these leaks by introducing a new function `ref_push_report_free()` that frees the list of reports and call it as required. While at it, fix a trivially leaking error string in the vicinity. These leaks get hit in t5411, but plugging them does not make the whole test suite pass. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 5 ++++- remote.c | 15 +++++++++++++++ remote.h | 4 +++- 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 536d22761d..ab5b20e39c 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -374,6 +374,7 @@ static void write_head_info(void) struct command { struct command *next; const char *error_string; + char *error_string_owned; struct ref_push_report *report; unsigned int skip_update:1, did_not_exist:1, @@ -1083,7 +1084,7 @@ static int read_proc_receive_report(struct packet_reader *reader, hint->run_proc_receive |= RUN_PROC_RECEIVE_RETURNED; if (!strcmp(head, "ng")) { if (p) - hint->error_string = xstrdup(p); + hint->error_string = hint->error_string_owned = xstrdup(p); else hint->error_string = "failed"; code = -1; @@ -2054,6 +2055,8 @@ static void free_commands(struct command *commands) while (commands) { struct command *next = commands->next; + ref_push_report_free(commands->report); + free(commands->error_string_owned); free(commands); commands = next; } diff --git a/remote.c b/remote.c index e291e8ff5c..cd2091ac0c 100644 --- a/remote.c +++ b/remote.c @@ -868,6 +868,20 @@ struct strvec *push_url_of_remote(struct remote *remote) return remote->pushurl.nr ? &remote->pushurl : &remote->url; } +void ref_push_report_free(struct ref_push_report *report) +{ + while (report) { + struct ref_push_report *next = report->next; + + free(report->ref_name); + free(report->old_oid); + free(report->new_oid); + free(report); + + report = next; + } +} + static int match_name_with_pattern(const char *key, const char *name, const char *value, char **result) { @@ -1122,6 +1136,7 @@ void free_one_ref(struct ref *ref) if (!ref) return; free_one_ref(ref->peer_ref); + ref_push_report_free(ref->report); free(ref->remote_status); free(ref->tracking_ref); free(ref->symref); diff --git a/remote.h b/remote.h index ad4513f639..c5e79eb40e 100644 --- a/remote.h +++ b/remote.h @@ -126,13 +126,15 @@ int remote_has_url(struct remote *remote, const char *url); struct strvec *push_url_of_remote(struct remote *remote); struct ref_push_report { - const char *ref_name; + char *ref_name; struct object_id *old_oid; struct object_id *new_oid; unsigned int forced_update:1; struct ref_push_report *next; }; +void ref_push_report_free(struct ref_push_report *); + struct ref { struct ref *next; struct object_id old_oid; From 66893a14d0c0d3850227def321312a7290317610 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 30 Sep 2024 11:14:24 +0200 Subject: [PATCH 23/23] builtin/send-pack: fix leaking list of push options The list of push options is leaking. Plug the leak. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/send-pack.c | 1 + t/t5411-proc-receive-hook.sh | 1 + t/t5545-push-options.sh | 1 + 3 files changed, 3 insertions(+) diff --git a/builtin/send-pack.c b/builtin/send-pack.c index 8b1d46e79a..59b626aae8 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -340,6 +340,7 @@ int cmd_send_pack(int argc, /* stable plumbing output; do not modify or localize */ fprintf(stderr, "Everything up-to-date\n"); + string_list_clear(&push_options, 0); free_refs(remote_refs); free_refs(local_refs); refspec_clear(&rs); diff --git a/t/t5411-proc-receive-hook.sh b/t/t5411-proc-receive-hook.sh index 92cf52c6d4..13d2d310a9 100755 --- a/t/t5411-proc-receive-hook.sh +++ b/t/t5411-proc-receive-hook.sh @@ -8,6 +8,7 @@ test_description='Test proc-receive hook' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/t5411/common-functions.sh diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh index fb13549da7..64ce56d3aa 100755 --- a/t/t5545-push-options.sh +++ b/t/t5545-push-options.sh @@ -8,6 +8,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1 export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh mk_repo_pair () {