{"thread":{"id":"62215","subject":"[PATCH 01/23] builtin/annotate: fix leaking args vector","startedAt":"2024-09-30T09:13:13Z","lastAt":"2024-10-09T07:06:51Z","messageCount":33,"participants":["Patrick Steinhardt","Derrick Stolee","Taylor Blau","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":23},"messages":[{"id":"503700","messageId":"cover.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":null,"subject":"[PATCH 00/23] Memory leak fixes (pt.8)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:13:07Z","receivedAt":"2024-09-30T09:13:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the 8th series of memory leak fixes. As usual, it fixes memory\nleaks all over the place. Once applied, we're down to 35 test suites\nthat do not pass with the memory leak sanitizer.\n\nThis series is built on top of 3857aae53f (Git 2.47-rc0, 2024-09-25)\nwith the following two topisc merged into it:\n\n  - ps/leakfixes-part-7 at 12dfc2475c (diffcore-break: fix leaking\n    filespecs when merging broken pairs, 2024-09-26).\n\n  - jk/http-leakfixes at f4c768c639 (http-push: clean up local_refs at\n    exit, 2024-09-24).\n\nBoth of these topics are part of `next`.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (23):\n  builtin/annotate: fix leaking args vector\n  read-cache: fix leaking hash context in `do_write_index()`\n  scalar: fix leaking repositories\n  shell: fix leaking strings\n  wt-status: fix leaking buffer with sparse directories\n  submodule: fix leaking submodule entry list\n  builtin/stash: fix leaking `pathspec_from_file`\n  builtin/pack-redundant: fix various memory leaks\n  builtin/clone: fix leaking repo state when cloning with bundle URIs\n  t/helper: fix leaking repository in partial-clone helper\n  builtin/revert: fix leaking `gpg_sign` and `strategy` config\n  diff: improve lifecycle management of diff queues\n  line-log: fix several memory leaks\n  pseudo-merge: fix various memory leaks\n  pseudo-merge: fix leaking strmap keys\n  pack-bitmap-write: fix leaking OID array\n  midx-write: fix leaking buffer\n  revision: fix memory leaks when rewriting parents\n  revision: fix leaking saved parents\n  pack-write: fix return parameter of `write_rev_file_order()`\n  t/helper: fix leaks in proc-receive helper\n  remote: fix leaking push reports\n  builtin/send-pack: fix leaking list of push options\n\n bloom.c                                    |  8 +--\n branch.c                                   |  8 ++-\n builtin/annotate.c                         | 20 +++++--\n builtin/clone.c                            | 27 +++++++++\n builtin/index-pack.c                       |  7 +--\n builtin/pack-redundant.c                   | 40 +++++++++++--\n builtin/receive-pack.c                     |  5 +-\n builtin/revert.c                           | 17 ++++--\n builtin/send-pack.c                        |  1 +\n builtin/stash.c                            |  4 +-\n diff.c                                     | 22 +++----\n diffcore-break.c                           |  8 +--\n diffcore-pickaxe.c                         |  4 +-\n diffcore-rename.c                          |  3 +-\n diffcore-rotate.c                          |  3 +-\n diffcore.h                                 | 10 ++--\n line-log.c                                 | 69 +++++++++++++---------\n log-tree.c                                 |  4 +-\n merge-ort.c                                |  2 +-\n midx-write.c                               |  5 +-\n pack-bitmap-write.c                        |  9 +++\n pack-bitmap.c                              |  4 +-\n pack-write.c                               | 42 +++++++------\n pack.h                                     |  4 +-\n pseudo-merge.c                             | 23 +++++++-\n pseudo-merge.h                             |  2 +\n read-cache.c                               |  1 +\n remote.c                                   | 15 +++++\n remote.h                                   |  4 +-\n revision.c                                 | 14 ++++-\n scalar.c                                   |  1 +\n shell.c                                    |  6 +-\n submodule-config.c                         | 15 ++++-\n submodule-config.h                         |  3 +\n t/helper/test-partial-clone.c              |  2 +\n t/helper/test-proc-receive.c               |  7 +++\n t/t0410-partial-clone.sh                   |  1 +\n t/t1092-sparse-checkout-compatibility.sh   |  1 +\n t/t3207-branch-submodule.sh                |  1 +\n t/t3427-rebase-subtree.sh                  |  1 +\n t/t3514-cherry-pick-revert-gpg.sh          |  1 +\n t/t3909-stash-pathspec-file.sh             |  1 +\n t/t5323-pack-redundant.sh                  |  1 +\n t/t5327-multi-pack-bitmaps-rev.sh          |  1 +\n t/t5333-pseudo-merge-bitmaps.sh            |  1 +\n t/t5334-incremental-multi-pack-index.sh    |  2 +\n t/t5411-proc-receive-hook.sh               |  1 +\n t/t5545-push-options.sh                    |  1 +\n t/t5730-protocol-v2-bundle-uri-file.sh     |  1 +\n t/t5731-protocol-v2-bundle-uri-git.sh      |  1 +\n t/t5732-protocol-v2-bundle-uri-http.sh     |  1 +\n t/t6012-rev-list-simplify.sh               |  1 +\n t/t6016-rev-list-graph-simplify-history.sh |  1 +\n t/t7003-filter-branch.sh                   |  1 +\n t/t8001-annotate.sh                        |  1 +\n t/t9210-scalar.sh                          |  1 +\n t/t9211-scalar-clone.sh                    |  1 +\n t/t9350-fast-export.sh                     |  1 +\n t/t9400-git-cvsserver-server.sh            |  1 +\n t/t9402-git-cvsserver-refs.sh              |  1 +\n t/t9850-shell.sh                           |  2 +\n wt-status.c                                |  6 +-\n 62 files changed, 329 insertions(+), 123 deletions(-)\n\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503699","messageId":"0a2a2085159e3817100fb639f2c9908fa7ee2f8b.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 01/23] builtin/annotate: fix leaking args vector","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:13:10Z","receivedAt":"2024-09-30T09:13:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're leaking the args vector in git-annotate(1) because we never clear\nit. Fixing it isn't as easy as calling `strvec_clear()` though because\ncalling `cmd_blame()` will cause the underlying array to be modified.\nInstead, we also need to pass a shallow copy of the argv array to the\nfunction.\n\nDo so to plug the memory leaks.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/annotate.c  | 20 +++++++++++++++-----\n t/t8001-annotate.sh |  1 +\n 2 files changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/annotate.c b/builtin/annotate.c\nindex a99179fe4d..03413c7df8 100644\n--- a/builtin/annotate.c\n+++ b/builtin/annotate.c\n@@ -15,13 +15,23 @@ int cmd_annotate(int argc,\n \t\t struct repository *repo UNUSED)\n {\n \tstruct strvec args = STRVEC_INIT;\n-\tint i;\n+\tconst char **args_copy;\n+\tint ret;\n \n \tstrvec_pushl(&args, \"annotate\", \"-c\", NULL);\n-\n-\tfor (i = 1; i < argc; i++) {\n+\tfor (int i = 1; i < argc; i++)\n \t\tstrvec_push(&args, argv[i]);\n-\t}\n \n-\treturn cmd_blame(args.nr, args.v, prefix, the_repository);\n+\t/*\n+\t * `cmd_blame()` ends up modifying the array, which causes memory leaks\n+\t * if we didn't copy the array here.\n+\t */\n+\tCALLOC_ARRAY(args_copy, args.nr + 1);\n+\tCOPY_ARRAY(args_copy, args.v, args.nr);\n+\n+\tret = cmd_blame(args.nr, args_copy, prefix, the_repository);\n+\n+\tstrvec_clear(&args);\n+\tfree(args_copy);\n+\treturn ret;\n }\ndiff --git a/t/t8001-annotate.sh b/t/t8001-annotate.sh\nindex d7167f5539..d434698919 100755\n--- a/t/t8001-annotate.sh\n+++ b/t/t8001-annotate.sh\n@@ -5,6 +5,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n TEST_CREATE_REPO_NO_TEMPLATE=1\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n PROG='git annotate'\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503701","messageId":"c51f40c5bd0c56967e348363e784222de7884b79.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 02/23] read-cache: fix leaking hash context in `do_write_index()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:13:13Z","receivedAt":"2024-09-30T09:13:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When writing an index with the EOIE extension we allocate a separate\nhash context. We never free that context though, causing a memory leak.\nPlug it.\n\nThis leak is exposed by t9210, but plugging it alone does not make the\nwhole test suite pass.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n read-cache.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 764fdfec46..0fb5e0d372 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -3124,6 +3124,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \tif (f)\n \t\tfree_hashfile(f);\n \tstrbuf_release(&sb);\n+\tfree(eoie_c);\n \tfree(ieot);\n \treturn ret;\n }\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503702","messageId":"fffd1c0ede66caeca677eb296d5b2be9664d666a.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 03/23] scalar: fix leaking repositories","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:13:15Z","receivedAt":"2024-09-30T09:13:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In the scalar code we iterate through multiple repositories,\ninitializing each of them. We never clear them though, causing memory\nleaks. Plug them.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n scalar.c                | 1 +\n t/t9210-scalar.sh       | 1 +\n t/t9211-scalar-clone.sh | 1 +\n 3 files changed, 3 insertions(+)\n\ndiff --git a/scalar.c b/scalar.c\nindex 09560aeab5..ede616ad4f 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -732,6 +732,7 @@ static int cmd_reconfigure(int argc, const char **argv)\n \t\t\tsucceeded = 1;\n \n \t\tthe_repository = old_repo;\n+\t\trepo_clear(&r);\n \n loop_end:\n \t\tif (!succeeded) {\ndiff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh\nindex e8613990e1..a131a6c029 100755\n--- a/t/t9210-scalar.sh\n+++ b/t/t9210-scalar.sh\n@@ -2,6 +2,7 @@\n \n test_description='test the `scalar` command'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n GIT_TEST_MAINT_SCHEDULER=\"crontab:test-tool crontab cron.txt,launchctl:true,schtasks:true\"\ndiff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh\nindex 7869f45ee6..c16ea67c1d 100755\n--- a/t/t9211-scalar-clone.sh\n+++ b/t/t9211-scalar-clone.sh\n@@ -2,6 +2,7 @@\n \n test_description='test the `scalar clone` subcommand'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"${TEST_DIRECTORY}/lib-terminal.sh\"\n \n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503703","messageId":"0fbe431b508c78ab2a12107af0bc00eb5573a392.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 04/23] shell: fix leaking strings","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:13:22Z","receivedAt":"2024-09-30T09:13:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are two memory leaks in \"shell.c\". The first one in `run_shell()`\nis trivial and fixed without further explanation. The second one in\n`cmd_main()` happens because we overwrite the `prog` variable, which\ncontains an allocated string. In fact though, the memory pointed to by\nthat variable is still in use because we use `split_cmdline()`, which\nmay create pointers into the middle of that string. But as we do not\nhave a direct pointer to the head of the allocated string anymore, we\nget a complaint by the leak checker.\n\nAddress this by not overwriting the `prog` pointer.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n shell.c                         | 6 +++---\n t/t9400-git-cvsserver-server.sh | 1 +\n t/t9850-shell.sh                | 2 ++\n 3 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/shell.c b/shell.c\nindex 2ece8b16e2..76333c8068 100644\n--- a/shell.c\n+++ b/shell.c\n@@ -143,6 +143,7 @@ static void run_shell(void)\n \t\t}\n \n \t\tfree(argv);\n+\t\tfree(split_args);\n \t\tfree(rawargs);\n \t} while (!done);\n }\n@@ -216,9 +217,8 @@ int cmd_main(int argc, const char **argv)\n \tcount = split_cmdline(prog, &user_argv);\n \tif (count >= 0) {\n \t\tif (is_valid_cmd_name(user_argv[0])) {\n-\t\t\tprog = make_cmd(user_argv[0]);\n-\t\t\tuser_argv[0] = prog;\n-\t\t\texecv(user_argv[0], (char *const *) user_argv);\n+\t\t\tchar *cmd = make_cmd(user_argv[0]);\n+\t\t\texecv(cmd, (char *const *) user_argv);\n \t\t}\n \t\tfree(prog);\n \t\tfree(user_argv);\ndiff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh\nindex e499c7f955..6da7440e73 100755\n--- a/t/t9400-git-cvsserver-server.sh\n+++ b/t/t9400-git-cvsserver-server.sh\n@@ -11,6 +11,7 @@ cvs CLI client via git-cvsserver server'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n if ! test_have_prereq PERL; then\ndiff --git a/t/t9850-shell.sh b/t/t9850-shell.sh\nindex cfc71c3bd4..f503f16d1b 100755\n--- a/t/t9850-shell.sh\n+++ b/t/t9850-shell.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='git shell tests'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'shell allows upload-pack' '\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503704","messageId":"4196fea159a98ff9a25ca0f65a9c669f45155a7b.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 05/23] wt-status: fix leaking buffer with sparse directories","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:13:24Z","receivedAt":"2024-09-30T09:13:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When hitting a sparse directory in `wt_status_collect_changes_initial()`\nwe use a `struct strbuf` to assemble the directory's name. We never free\nthat buffer though, causing a memory leak.\n\nFix the leak by releasing the buffer. While at it, move the buffer\noutside of the loop and reset it to save on some wasteful allocations.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t1092-sparse-checkout-compatibility.sh | 1 +\n wt-status.c                              | 6 +++++-\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex eb32da2a7f..55efafe4e0 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -5,6 +5,7 @@ test_description='compare full workdir to sparse workdir'\n GIT_TEST_SPLIT_INDEX=0\n GIT_TEST_SPARSE_INDEX=\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/wt-status.c b/wt-status.c\nindex 6a6397ca8f..6a8c05d1cf 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -717,6 +717,7 @@ static int add_file_to_list(const struct object_id *oid,\n static void wt_status_collect_changes_initial(struct wt_status *s)\n {\n \tstruct index_state *istate = s->repo->index;\n+\tstruct strbuf base = STRBUF_INIT;\n \tint i;\n \n \tfor (i = 0; i < istate->cache_nr; i++) {\n@@ -735,7 +736,6 @@ static void wt_status_collect_changes_initial(struct wt_status *s)\n \t\t\t * expanding the trees to find the elements that are new in this\n \t\t\t * tree and marking them with DIFF_STATUS_ADDED.\n \t\t\t */\n-\t\t\tstruct strbuf base = STRBUF_INIT;\n \t\t\tstruct pathspec ps = { 0 };\n \t\t\tstruct tree *tree = lookup_tree(istate->repo, &ce->oid);\n \n@@ -743,9 +743,11 @@ static void wt_status_collect_changes_initial(struct wt_status *s)\n \t\t\tps.has_wildcard = 1;\n \t\t\tps.max_depth = -1;\n \n+\t\t\tstrbuf_reset(&base);\n \t\t\tstrbuf_add(&base, ce->name, ce->ce_namelen);\n \t\t\tread_tree_at(istate->repo, tree, &base, 0, &ps,\n \t\t\t\t     add_file_to_list, s);\n+\n \t\t\tcontinue;\n \t\t}\n \n@@ -772,6 +774,8 @@ static void wt_status_collect_changes_initial(struct wt_status *s)\n \t\t\ts->committable = 1;\n \t\t}\n \t}\n+\n+\tstrbuf_release(&base);\n }\n \n static void wt_status_collect_untracked(struct wt_status *s)\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503705","messageId":"7781d7644e9f76a6638cabc0412d583ad703905d.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 06/23] submodule: fix leaking submodule entry list","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:13:27Z","receivedAt":"2024-09-30T09:13:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The submodule entry list returned by `submodules_of_tree()` is never\ncompletely free'd by its only caller. Introduce a new function that\nfree's the list for us and call it.\n\nWhile at it, also fix the leaking `branch_point` string.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n branch.c                    |  8 ++++++--\n submodule-config.c          | 15 ++++++++++++++-\n submodule-config.h          |  3 +++\n t/t3207-branch-submodule.sh |  1 +\n 4 files changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 08fa4094d2..44977ad0aa 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -738,6 +738,7 @@ static int submodule_create_branch(struct repository *r,\n \n \tstrbuf_release(&child_err);\n \tstrbuf_release(&out_buf);\n+\tfree(out_prefix);\n \treturn ret;\n }\n \n@@ -794,7 +795,7 @@ void create_branches_recursively(struct repository *r, const char *name,\n \tcreate_branch(r, name, start_committish, force, 0, reflog, quiet,\n \t\t      BRANCH_TRACK_NEVER, dry_run);\n \tif (dry_run)\n-\t\treturn;\n+\t\tgoto out;\n \t/*\n \t * NEEDSWORK If tracking was set up in the superproject but not the\n \t * submodule, users might expect \"git branch --recurse-submodules\" to\n@@ -815,8 +816,11 @@ void create_branches_recursively(struct repository *r, const char *name,\n \t\t\tdie(_(\"submodule '%s': cannot create branch '%s'\"),\n \t\t\t    submodule_entry_list.entries[i].submodule->name,\n \t\t\t    name);\n-\t\trepo_clear(submodule_entry_list.entries[i].repo);\n \t}\n+\n+out:\n+\tsubmodule_entry_list_release(&submodule_entry_list);\n+\tfree(branch_point);\n }\n \n void remove_merge_branch_state(struct repository *r)\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 471637a725..9c8c37b259 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -901,8 +901,9 @@ static void traverse_tree_submodules(struct repository *r,\n \tstruct submodule_tree_entry *st_entry;\n \tstruct name_entry name_entry;\n \tchar *tree_path = NULL;\n+\tchar *tree_buf;\n \n-\tfill_tree_descriptor(r, &tree, treeish_name);\n+\ttree_buf = fill_tree_descriptor(r, &tree, treeish_name);\n \twhile (tree_entry(&tree, &name_entry)) {\n \t\tif (prefix)\n \t\t\ttree_path =\n@@ -930,6 +931,8 @@ static void traverse_tree_submodules(struct repository *r,\n \t\t\t\t\t\t &name_entry.oid, out);\n \t\tfree(tree_path);\n \t}\n+\n+\tfree(tree_buf);\n }\n \n void submodules_of_tree(struct repository *r,\n@@ -943,6 +946,16 @@ void submodules_of_tree(struct repository *r,\n \ttraverse_tree_submodules(r, treeish_name, NULL, treeish_name, out);\n }\n \n+void submodule_entry_list_release(struct submodule_entry_list *list)\n+{\n+\tfor (size_t i = 0; i < list->entry_nr; i++) {\n+\t\tfree(list->entries[i].name_entry);\n+\t\trepo_clear(list->entries[i].repo);\n+\t\tfree(list->entries[i].repo);\n+\t}\n+\tfree(list->entries);\n+}\n+\n void submodule_free(struct repository *r)\n {\n \tif (r->submodule_cache)\ndiff --git a/submodule-config.h b/submodule-config.h\nindex b6133af71b..f55d4e3b61 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -136,4 +136,7 @@ struct submodule_entry_list {\n void submodules_of_tree(struct repository *r,\n \t\t\tconst struct object_id *treeish_name,\n \t\t\tstruct submodule_entry_list *ret);\n+\n+void submodule_entry_list_release(struct submodule_entry_list *list);\n+\n #endif /* SUBMODULE_CONFIG_H */\ndiff --git a/t/t3207-branch-submodule.sh b/t/t3207-branch-submodule.sh\nindex fe72b24716..904eea7df5 100755\n--- a/t/t3207-branch-submodule.sh\n+++ b/t/t3207-branch-submodule.sh\n@@ -5,6 +5,7 @@ test_description='git branch submodule tests'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-rebase.sh\n \n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503706","messageId":"38b58f5b7470d3fb07396d62cecda3799d0b944c.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 07/23] builtin/stash: fix leaking `pathspec_from_file`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:13:30Z","receivedAt":"2024-09-30T09:13:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `OPT_PATHSPEC_FROM_FILE()` option maps to `OPT_FILENAME()`, which we\nknow will always allocate memory when passed. We never free the memory\nthough, causing a memory leak. Plug it.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/stash.c                | 4 +++-\n t/t3909-stash-pathspec-file.sh | 1 +\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex f1acc918d0..1399a1bbe2 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1759,7 +1759,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,\n \tint quiet = 0;\n \tint pathspec_file_nul = 0;\n \tconst char *stash_msg = NULL;\n-\tconst char *pathspec_from_file = NULL;\n+\tchar *pathspec_from_file = NULL;\n \tstruct pathspec ps;\n \tstruct option options[] = {\n \t\tOPT_BOOL('k', \"keep-index\", &keep_index,\n@@ -1821,7 +1821,9 @@ static int push_stash(int argc, const char **argv, const char *prefix,\n \n \tret = do_push_stash(&ps, stash_msg, quiet, keep_index, patch_mode,\n \t\t\t    include_untracked, only_staged);\n+\n \tclear_pathspec(&ps);\n+\tfree(pathspec_from_file);\n \treturn ret;\n }\n \ndiff --git a/t/t3909-stash-pathspec-file.sh b/t/t3909-stash-pathspec-file.sh\nindex 73f2dbdeb0..83269d0eb4 100755\n--- a/t/t3909-stash-pathspec-file.sh\n+++ b/t/t3909-stash-pathspec-file.sh\n@@ -2,6 +2,7 @@\n \n test_description='stash --pathspec-from-file'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_tick\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503707","messageId":"31480f336a151e1b7dd984368a5cd54ced40b4ac.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 08/23] builtin/pack-redundant: fix various memory leaks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:13:32Z","receivedAt":"2024-09-30T09:13:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are various different memory leaks in git-pack-redundant(1),\nmostly caused by not even trying to free allocated memory. Fix them.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/pack-redundant.c  | 40 +++++++++++++++++++++++++++++++++------\n t/t5323-pack-redundant.sh |  1 +\n 2 files changed, 35 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex 81f4494d46..5809613002 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -69,6 +69,15 @@ static inline void llist_init(struct llist **list)\n \t(*list)->size = 0;\n }\n \n+static void llist_free(struct llist *list)\n+{\n+\tfor (struct llist_item *i = list->front, *next; i; i = next) {\n+\t\tnext = i->next;\n+\t\tllist_item_put(i);\n+\t}\n+\tfree(list);\n+}\n+\n static struct llist * llist_copy(struct llist *list)\n {\n \tstruct llist *ret;\n@@ -206,6 +215,14 @@ static inline struct pack_list * pack_list_insert(struct pack_list **pl,\n \treturn p;\n }\n \n+static void pack_list_free(struct pack_list *pl)\n+{\n+\tfor (struct pack_list *next; pl; pl = next) {\n+\t\tnext = pl->next;\n+\t\tfree(pl);\n+\t}\n+}\n+\n static inline size_t pack_list_size(struct pack_list *pl)\n {\n \tsize_t ret = 0;\n@@ -419,7 +436,8 @@ static void minimize(struct pack_list **min)\n \n \t/* return if there are no objects missing from the unique set */\n \tif (missing->size == 0) {\n-\t\tfree(missing);\n+\t\tllist_free(missing);\n+\t\tpack_list_free(non_unique);\n \t\treturn;\n \t}\n \n@@ -434,6 +452,8 @@ static void minimize(struct pack_list **min)\n \t}\n \n \twhile (non_unique) {\n+\t\tstruct pack_list *next;\n+\n \t\t/* sort the non_unique packs, greater size of remaining_objects first */\n \t\tsort_pack_list(&non_unique);\n \t\tif (non_unique->remaining_objects->size == 0)\n@@ -444,8 +464,14 @@ static void minimize(struct pack_list **min)\n \t\tfor (pl = non_unique->next; pl && pl->remaining_objects->size > 0;  pl = pl->next)\n \t\t\tllist_sorted_difference_inplace(pl->remaining_objects, non_unique->remaining_objects);\n \n-\t\tnon_unique = non_unique->next;\n+\t\tnext = non_unique->next;\n+\t\tfree(non_unique);\n+\t\tnon_unique = next;\n \t}\n+\n+\tpack_list_free(non_unique);\n+\tllist_free(unique_pack_objects);\n+\tllist_free(missing);\n }\n \n static void load_all_objects(void)\n@@ -565,7 +591,6 @@ static void load_all(void)\n int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, struct repository *repo UNUSED) {\n \tint i; int i_still_use_this = 0; struct pack_list *min = NULL, *red, *pl;\n \tstruct llist *ignore;\n-\tstruct object_id *oid;\n \tchar buf[GIT_MAX_HEXSZ + 2]; /* hex hash + \\n + \\0 */\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n@@ -625,11 +650,11 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, s\n \t/* ignore objects given on stdin */\n \tllist_init(&ignore);\n \tif (!isatty(0)) {\n+\t\tstruct object_id oid;\n \t\twhile (fgets(buf, sizeof(buf), stdin)) {\n-\t\t\toid = xmalloc(sizeof(*oid));\n-\t\t\tif (get_oid_hex(buf, oid))\n+\t\t\tif (get_oid_hex(buf, &oid))\n \t\t\t\tdie(\"Bad object ID on stdin: %s\", buf);\n-\t\t\tllist_insert_sorted_unique(ignore, oid, NULL);\n+\t\t\tllist_insert_sorted_unique(ignore, &oid, NULL);\n \t\t}\n \t}\n \tllist_sorted_difference_inplace(all_objects, ignore);\n@@ -671,5 +696,8 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, s\n \t\tfprintf(stderr, \"%luMB of redundant packs in total.\\n\",\n \t\t\t(unsigned long)pack_set_bytecount(red)/(1024*1024));\n \n+\tpack_list_free(red);\n+\tpack_list_free(min);\n+\tllist_free(ignore);\n \treturn 0;\n }\ndiff --git a/t/t5323-pack-redundant.sh b/t/t5323-pack-redundant.sh\nindex 8dbbcc5e51..4e18f5490a 100755\n--- a/t/t5323-pack-redundant.sh\n+++ b/t/t5323-pack-redundant.sh\n@@ -34,6 +34,7 @@ relationship between packs and objects is as follows:\n     Px2 |         s s s                           x x x\n '\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n main_repo=main.git\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503708","messageId":"2fa76a00fc0469d0c64c66e81b96d622844b8f0d.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 09/23] builtin/clone: fix leaking repo state when cloning with bundle URIs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:13:35Z","receivedAt":"2024-09-30T09:13:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When cloning with bundle URIs we re-initialize `the_repository` after\nhaving fetched the bundle. This causes a bunch of memory leaks though\nbecause we do not release its previous state.\n\nThese leaks can be plugged by calling `repo_clear()` before we call\n`repo_init()`. But this causes another issue because the remote that we\nused is tied to the lifetime of the repository's remote state, which\nwould also get released. We thus have to make sure that it does not get\nfree'd under our feet.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/clone.c                        | 27 ++++++++++++++++++++++++++\n t/t5730-protocol-v2-bundle-uri-file.sh |  1 +\n t/t5731-protocol-v2-bundle-uri-git.sh  |  1 +\n t/t5732-protocol-v2-bundle-uri-http.sh |  1 +\n 4 files changed, 30 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex e77339c847..59fcb317a6 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1403,8 +1403,17 @@ int cmd_clone(int argc,\n \t * data from the --bundle-uri option.\n \t */\n \tif (bundle_uri) {\n+\t\tstruct remote_state *state;\n \t\tint has_heuristic = 0;\n \n+\t\t/*\n+\t\t * We need to save the remote state as our remote's lifetime is\n+\t\t * tied to it.\n+\t\t */\n+\t\tstate = the_repository->remote_state;\n+\t\tthe_repository->remote_state = NULL;\n+\t\trepo_clear(the_repository);\n+\n \t\t/* At this point, we need the_repository to match the cloned repo. */\n \t\tif (repo_init(the_repository, git_dir, work_tree))\n \t\t\twarning(_(\"failed to initialize the repo, skipping bundle URI\"));\n@@ -1413,6 +1422,10 @@ int cmd_clone(int argc,\n \t\t\t\tbundle_uri);\n \t\telse if (has_heuristic)\n \t\t\tgit_config_set_gently(\"fetch.bundleuri\", bundle_uri);\n+\n+\t\tremote_state_clear(the_repository->remote_state);\n+\t\tfree(the_repository->remote_state);\n+\t\tthe_repository->remote_state = state;\n \t} else {\n \t\t/*\n \t\t* Populate transport->got_remote_bundle_uri and\n@@ -1422,12 +1435,26 @@ int cmd_clone(int argc,\n \n \t\tif (transport->bundles &&\n \t\t    hashmap_get_size(&transport->bundles->bundles)) {\n+\t\t\tstruct remote_state *state;\n+\n+\t\t\t/*\n+\t\t\t * We need to save the remote state as our remote's\n+\t\t\t * lifetime is tied to it.\n+\t\t\t */\n+\t\t\tstate = the_repository->remote_state;\n+\t\t\tthe_repository->remote_state = NULL;\n+\t\t\trepo_clear(the_repository);\n+\n \t\t\t/* At this point, we need the_repository to match the cloned repo. */\n \t\t\tif (repo_init(the_repository, git_dir, work_tree))\n \t\t\t\twarning(_(\"failed to initialize the repo, skipping bundle URI\"));\n \t\t\telse if (fetch_bundle_list(the_repository,\n \t\t\t\t\t\t   transport->bundles))\n \t\t\t\twarning(_(\"failed to fetch advertised bundles\"));\n+\n+\t\t\tremote_state_clear(the_repository->remote_state);\n+\t\t\tfree(the_repository->remote_state);\n+\t\t\tthe_repository->remote_state = state;\n \t\t} else {\n \t\t\tclear_bundle_list(transport->bundles);\n \t\t\tFREE_AND_NULL(transport->bundles);\ndiff --git a/t/t5730-protocol-v2-bundle-uri-file.sh b/t/t5730-protocol-v2-bundle-uri-file.sh\nindex 37bdb725bc..38396df95b 100755\n--- a/t/t5730-protocol-v2-bundle-uri-file.sh\n+++ b/t/t5730-protocol-v2-bundle-uri-file.sh\n@@ -7,6 +7,7 @@ TEST_NO_CREATE_REPO=1\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # Test protocol v2 with 'file://' transport\ndiff --git a/t/t5731-protocol-v2-bundle-uri-git.sh b/t/t5731-protocol-v2-bundle-uri-git.sh\nindex 8add1b37ab..c199e955fe 100755\n--- a/t/t5731-protocol-v2-bundle-uri-git.sh\n+++ b/t/t5731-protocol-v2-bundle-uri-git.sh\n@@ -7,6 +7,7 @@ TEST_NO_CREATE_REPO=1\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # Test protocol v2 with 'git://' transport\ndiff --git a/t/t5732-protocol-v2-bundle-uri-http.sh b/t/t5732-protocol-v2-bundle-uri-http.sh\nindex 129daa0226..a9403e94c6 100755\n--- a/t/t5732-protocol-v2-bundle-uri-http.sh\n+++ b/t/t5732-protocol-v2-bundle-uri-http.sh\n@@ -7,6 +7,7 @@ TEST_NO_CREATE_REPO=1\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # Test protocol v2 with 'http://' transport\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503709","messageId":"cfeb6205fb4d788d35559f1091d88bf057b991d8.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 10/23] t/helper: fix leaking repository in partial-clone helper","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:13:40Z","receivedAt":"2024-09-30T09:13:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We initialize but never clear a repository in the partial-clone test\nhelper. Plug this leak.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/helper/test-partial-clone.c | 2 ++\n t/t0410-partial-clone.sh      | 1 +\n 2 files changed, 3 insertions(+)\n\ndiff --git a/t/helper/test-partial-clone.c b/t/helper/test-partial-clone.c\nindex 0ead529167..a1af9710c3 100644\n--- a/t/helper/test-partial-clone.c\n+++ b/t/helper/test-partial-clone.c\n@@ -26,6 +26,8 @@ static void object_info(const char *gitdir, const char *oid_hex)\n \tif (oid_object_info_extended(&r, &oid, &oi, 0))\n \t\tdie(\"could not obtain object info\");\n \tprintf(\"%d\\n\", (int) size);\n+\n+\trepo_clear(&r);\n }\n \n int cmd__partial_clone(int argc, const char **argv)\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 34bdb3ab1f..818700fbec 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -2,6 +2,7 @@\n \n test_description='partial clone'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503710","messageId":"3177c449206c802239720479bc070df7a22b0d0e.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 11/23] builtin/revert: fix leaking `gpg_sign` and `strategy` config","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:13:43Z","receivedAt":"2024-09-30T09:13:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We leak the config values when `gpg_sign` or `strategy` options are\nbeing overridden via the command line. To fix this we need to free the\nold value, which requires us to figure out whether the value was changed\nvia an option in the first place. The easy way to do this, which is to\ninitialize local variables with `NULL`, doesn't work because we cannot\ntell the case where the user has passed e.g. `--no-gpg-sign`. Instead,\nwe use a sentinel value for both values that we can compare against to\ncheck whether the user has passed the option.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/revert.c                  | 17 +++++++++++++----\n t/t3514-cherry-pick-revert-gpg.sh |  1 +\n 2 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 55ba1092c5..b7917dddd3 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -110,6 +110,9 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \tconst char * const * usage_str = revert_or_cherry_pick_usage(opts);\n \tconst char *me = action_name(opts);\n \tconst char *cleanup_arg = NULL;\n+\tconst char sentinel_value;\n+\tconst char *strategy = &sentinel_value;\n+\tconst char *gpg_sign = &sentinel_value;\n \tenum empty_action empty_opt = EMPTY_COMMIT_UNSPECIFIED;\n \tint cmd = 0;\n \tstruct option base_options[] = {\n@@ -125,10 +128,10 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t\tOPT_CALLBACK('m', \"mainline\", opts, N_(\"parent-number\"),\n \t\t\t     N_(\"select mainline parent\"), option_parse_m),\n \t\tOPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),\n-\t\tOPT_STRING(0, \"strategy\", &opts->strategy, N_(\"strategy\"), N_(\"merge strategy\")),\n+\t\tOPT_STRING(0, \"strategy\", &strategy, N_(\"strategy\"), N_(\"merge strategy\")),\n \t\tOPT_STRVEC('X', \"strategy-option\", &opts->xopts, N_(\"option\"),\n \t\t\tN_(\"option for merge strategy\")),\n-\t\t{ OPTION_STRING, 'S', \"gpg-sign\", &opts->gpg_sign, N_(\"key-id\"),\n+\t\t{ OPTION_STRING, 'S', \"gpg-sign\", &gpg_sign, N_(\"key-id\"),\n \t\t  N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n \t\tOPT_END()\n \t};\n@@ -240,8 +243,14 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t\tusage_with_options(usage_str, options);\n \n \t/* These option values will be free()d */\n-\topts->gpg_sign = xstrdup_or_null(opts->gpg_sign);\n-\topts->strategy = xstrdup_or_null(opts->strategy);\n+\tif (gpg_sign != &sentinel_value) {\n+\t\tfree(opts->gpg_sign);\n+\t\topts->gpg_sign = xstrdup_or_null(gpg_sign);\n+\t}\n+\tif (strategy != &sentinel_value) {\n+\t\tfree(opts->strategy);\n+\t\topts->strategy = xstrdup_or_null(strategy);\n+\t}\n \tif (!opts->strategy && getenv(\"GIT_TEST_MERGE_ALGORITHM\"))\n \t\topts->strategy = xstrdup(getenv(\"GIT_TEST_MERGE_ALGORITHM\"));\n \tfree(options);\ndiff --git a/t/t3514-cherry-pick-revert-gpg.sh b/t/t3514-cherry-pick-revert-gpg.sh\nindex 5b2e250eaa..133dc07217 100755\n--- a/t/t3514-cherry-pick-revert-gpg.sh\n+++ b/t/t3514-cherry-pick-revert-gpg.sh\n@@ -5,6 +5,7 @@\n \n test_description='test {cherry-pick,revert} --[no-]gpg-sign'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY/lib-gpg.sh\"\n \n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503711","messageId":"a475d974cc1d060fdae26da31fea834c04b059c5.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 12/23] diff: improve lifecycle management of diff queues","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:13:45Z","receivedAt":"2024-09-30T09:13:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The lifecycle management of diff queues is somewhat confusing:\n\n  - For most of the part this can be attributed to `DIFF_QUEUE_CLEAR()`,\n    which does not release any memory but rather initializes the queue,\n    only. This is in contrast to our common naming schema, where\n    \"clearing\" means that we release underlying memory and then\n    re-initialize the data structure such that it is ready to use.\n\n  - A second offender is `diff_free_queue()`, which does not free the\n    queue structure itself. It is rather a release-style function.\n\nRefactor the code to make things less confusing. `DIFF_QUEUE_CLEAR()` is\nreplaced by `DIFF_QUEUE_INIT` and `diff_queue_init()`, while\n`diff_free_queue()` is replaced by `diff_queue_release()`. While on it,\nadapt callsites where we call `DIFF_QUEUE_CLEAR()` with the intent to\nrelease underlying memory to instead call `diff_queue_clear()` to fix\nmemory leaks.\n\nThis memory leak is exposed by t4211, but plugging it alone does not\nmake the whole test suite pass.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n bloom.c            |  8 +-------\n diff.c             | 22 ++++++++++++----------\n diffcore-break.c   |  8 ++------\n diffcore-pickaxe.c |  4 +---\n diffcore-rename.c  |  3 +--\n diffcore-rotate.c  |  3 +--\n diffcore.h         | 10 ++++------\n line-log.c         | 15 +++++++--------\n log-tree.c         |  4 ++--\n merge-ort.c        |  2 +-\n 10 files changed, 32 insertions(+), 47 deletions(-)\n\ndiff --git a/bloom.c b/bloom.c\nindex c915f8b1ba..c428634105 100644\n--- a/bloom.c\n+++ b/bloom.c\n@@ -476,8 +476,6 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,\n \t\t\t\t*last_slash = '\\0';\n \n \t\t\t} while (*path);\n-\n-\t\t\tdiff_free_filepair(diff_queued_diff.queue[i]);\n \t\t}\n \n \t\tif (hashmap_get_size(&pathmap) > settings->max_changed_paths) {\n@@ -508,8 +506,6 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,\n \tcleanup:\n \t\thashmap_clear_and_free(&pathmap, struct pathmap_hash_entry, entry);\n \t} else {\n-\t\tfor (i = 0; i < diff_queued_diff.nr; i++)\n-\t\t\tdiff_free_filepair(diff_queued_diff.queue[i]);\n \t\tinit_truncated_large_filter(filter, settings->hash_version);\n \n \t\tif (computed)\n@@ -519,9 +515,7 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,\n \tif (computed)\n \t\t*computed |= BLOOM_COMPUTED;\n \n-\tfree(diff_queued_diff.queue);\n-\tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n-\n+\tdiff_queue_clear(&diff_queued_diff);\n \treturn filter;\n }\n \ndiff --git a/diff.c b/diff.c\nindex 173cbe2bed..3e9137ffed 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5983,11 +5983,18 @@ void diff_free_filepair(struct diff_filepair *p)\n \tfree(p);\n }\n \n-void diff_free_queue(struct diff_queue_struct *q)\n+void diff_queue_init(struct diff_queue_struct *q)\n+{\n+\tstruct diff_queue_struct blank = DIFF_QUEUE_INIT;\n+\tmemcpy(q, &blank, sizeof(*q));\n+}\n+\n+void diff_queue_clear(struct diff_queue_struct *q)\n {\n \tfor (int i = 0; i < q->nr; i++)\n \t\tdiff_free_filepair(q->queue[i]);\n \tfree(q->queue);\n+\tdiff_queue_init(q);\n }\n \n const char *diff_aligned_abbrev(const struct object_id *oid, int len)\n@@ -6551,8 +6558,7 @@ int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \tint result = diff_get_patch_id(options, oid, diff_header_only);\n \n-\tdiff_free_queue(q);\n-\tDIFF_QUEUE_CLEAR(q);\n+\tdiff_queue_clear(q);\n \n \treturn result;\n }\n@@ -6835,8 +6841,7 @@ void diff_flush(struct diff_options *options)\n \t}\n \n free_queue:\n-\tdiff_free_queue(q);\n-\tDIFF_QUEUE_CLEAR(q);\n+\tdiff_queue_clear(q);\n \tdiff_free(options);\n \n \t/*\n@@ -6867,9 +6872,7 @@ static void diffcore_apply_filter(struct diff_options *options)\n {\n \tint i;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n-\tstruct diff_queue_struct outq;\n-\n-\tDIFF_QUEUE_CLEAR(&outq);\n+\tstruct diff_queue_struct outq = DIFF_QUEUE_INIT;\n \n \tif (!options->filter)\n \t\treturn;\n@@ -6962,8 +6965,7 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)\n {\n \tint i;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n-\tstruct diff_queue_struct outq;\n-\tDIFF_QUEUE_CLEAR(&outq);\n+\tstruct diff_queue_struct outq = DIFF_QUEUE_INIT;\n \n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\ndiff --git a/diffcore-break.c b/diffcore-break.c\nindex 02735f80c6..c4c2173f30 100644\n--- a/diffcore-break.c\n+++ b/diffcore-break.c\n@@ -131,7 +131,7 @@ static int should_break(struct repository *r,\n void diffcore_break(struct repository *r, int break_score)\n {\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n-\tstruct diff_queue_struct outq;\n+\tstruct diff_queue_struct outq = DIFF_QUEUE_INIT;\n \n \t/* When the filepair has this much edit (insert and delete),\n \t * it is first considered to be a rewrite and broken into a\n@@ -178,8 +178,6 @@ void diffcore_break(struct repository *r, int break_score)\n \tif (!merge_score)\n \t\tmerge_score = DEFAULT_MERGE_SCORE;\n \n-\tDIFF_QUEUE_CLEAR(&outq);\n-\n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n \t\tint score;\n@@ -275,11 +273,9 @@ static void merge_broken(struct diff_filepair *p,\n void diffcore_merge_broken(void)\n {\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n-\tstruct diff_queue_struct outq;\n+\tstruct diff_queue_struct outq = DIFF_QUEUE_INIT;\n \tint i, j;\n \n-\tDIFF_QUEUE_CLEAR(&outq);\n-\n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n \t\tif (!p)\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex b195fa4eb3..43fef8e8ba 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -182,9 +182,7 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,\n \t\t    regex_t *regexp, kwset_t kws, pickaxe_fn fn)\n {\n \tint i;\n-\tstruct diff_queue_struct outq;\n-\n-\tDIFF_QUEUE_CLEAR(&outq);\n+\tstruct diff_queue_struct outq = DIFF_QUEUE_INIT;\n \n \tif (o->pickaxe_opts & DIFF_PICKAXE_ALL) {\n \t\t/* Showing the whole changeset if needle exists */\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 3d6826baa3..1b1c1a6a1f 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -1388,7 +1388,7 @@ void diffcore_rename_extended(struct diff_options *options,\n \tint detect_rename = options->detect_rename;\n \tint minimum_score = options->rename_score;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n-\tstruct diff_queue_struct outq;\n+\tstruct diff_queue_struct outq = DIFF_QUEUE_INIT;\n \tstruct diff_score *mx;\n \tint i, j, rename_count, skip_unmodified = 0;\n \tint num_destinations, dst_cnt;\n@@ -1638,7 +1638,6 @@ void diffcore_rename_extended(struct diff_options *options,\n \t * are recorded in rename_dst.  The original list is still in *q.\n \t */\n \ttrace2_region_enter(\"diff\", \"write back to queue\", options->repo);\n-\tDIFF_QUEUE_CLEAR(&outq);\n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n \t\tstruct diff_filepair *pair_to_free = NULL;\ndiff --git a/diffcore-rotate.c b/diffcore-rotate.c\nindex 533986cf63..73ca20b331 100644\n--- a/diffcore-rotate.c\n+++ b/diffcore-rotate.c\n@@ -10,7 +10,7 @@\n void diffcore_rotate(struct diff_options *opt)\n {\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n-\tstruct diff_queue_struct outq;\n+\tstruct diff_queue_struct outq = DIFF_QUEUE_INIT;\n \tint rotate_to, i;\n \n \tif (!q->nr)\n@@ -31,7 +31,6 @@ void diffcore_rotate(struct diff_options *opt)\n \t\treturn;\n \t}\n \n-\tDIFF_QUEUE_CLEAR(&outq);\n \trotate_to = i;\n \n \tfor (i = rotate_to; i < q->nr; i++)\ndiff --git a/diffcore.h b/diffcore.h\nindex 1701ed50b9..2feb325031 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -153,18 +153,16 @@ struct diff_queue_struct {\n \tint nr;\n };\n \n-#define DIFF_QUEUE_CLEAR(q) \\\n-\tdo { \\\n-\t\t(q)->queue = NULL; \\\n-\t\t(q)->nr = (q)->alloc = 0; \\\n-\t} while (0)\n+#define DIFF_QUEUE_INIT { 0 }\n+\n+void diff_queue_init(struct diff_queue_struct *q);\n+void diff_queue_clear(struct diff_queue_struct *q);\n \n extern struct diff_queue_struct diff_queued_diff;\n struct diff_filepair *diff_queue(struct diff_queue_struct *,\n \t\t\t\t struct diff_filespec *,\n \t\t\t\t struct diff_filespec *);\n void diff_q(struct diff_queue_struct *, struct diff_filepair *);\n-void diff_free_queue(struct diff_queue_struct *q);\n \n /* dir_rename_relevance: the reason we want rename information for a dir */\n enum dir_rename_relevance {\ndiff --git a/line-log.c b/line-log.c\nindex 67c80b39a0..89e0ea4562 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -787,15 +787,14 @@ static void move_diff_queue(struct diff_queue_struct *dst,\n \t\t\t    struct diff_queue_struct *src)\n {\n \tassert(src != dst);\n-\tmemcpy(dst, src, sizeof(struct diff_queue_struct));\n-\tDIFF_QUEUE_CLEAR(src);\n+\tmemcpy(dst, src, sizeof(*dst));\n+\tdiff_queue_init(src);\n }\n \n static void filter_diffs_for_paths(struct line_log_data *range, int keep_deletions)\n {\n \tint i;\n-\tstruct diff_queue_struct outq;\n-\tDIFF_QUEUE_CLEAR(&outq);\n+\tstruct diff_queue_struct outq = DIFF_QUEUE_INIT;\n \n \tfor (i = 0; i < diff_queued_diff.nr; i++) {\n \t\tstruct diff_filepair *p = diff_queued_diff.queue[i];\n@@ -850,12 +849,12 @@ static void queue_diffs(struct line_log_data *range,\n \t\tclear_pathspec(&opt->pathspec);\n \t\tparse_pathspec_from_ranges(&opt->pathspec, range);\n \t}\n-\tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n+\tdiff_queue_clear(&diff_queued_diff);\n \tdiff_tree_oid(parent_tree_oid, tree_oid, \"\", opt);\n \tif (opt->detect_rename && diff_might_be_rename()) {\n \t\t/* must look at the full tree diff to detect renames */\n \t\tclear_pathspec(&opt->pathspec);\n-\t\tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n+\t\tdiff_queue_clear(&diff_queued_diff);\n \n \t\tdiff_tree_oid(parent_tree_oid, tree_oid, \"\", opt);\n \n@@ -1097,7 +1096,7 @@ static struct diff_filepair *diff_filepair_dup(struct diff_filepair *pair)\n static void free_diffqueues(int n, struct diff_queue_struct *dq)\n {\n \tfor (int i = 0; i < n; i++)\n-\t\tdiff_free_queue(&dq[i]);\n+\t\tdiff_queue_clear(&dq[i]);\n \tfree(dq);\n }\n \n@@ -1200,7 +1199,7 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c\n \tif (parent)\n \t\tadd_line_range(rev, parent, parent_range);\n \tfree_line_log_data(parent_range);\n-\tdiff_free_queue(&queue);\n+\tdiff_queue_clear(&queue);\n \treturn changed;\n }\n \ndiff --git a/log-tree.c b/log-tree.c\nindex 3758e0d3b8..60774c16b3 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -675,7 +675,7 @@ static void show_diff_of_diff(struct rev_info *opt)\n \t\tstruct diff_queue_struct dq;\n \n \t\tmemcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));\n-\t\tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n+\t\tdiff_queue_init(&diff_queued_diff);\n \n \t\tfprintf_ln(opt->diffopt.file, \"\\n%s\", opt->idiff_title);\n \t\tshow_interdiff(opt->idiff_oid1, opt->idiff_oid2, 2,\n@@ -694,7 +694,7 @@ static void show_diff_of_diff(struct rev_info *opt)\n \t\t};\n \n \t\tmemcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));\n-\t\tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n+\t\tdiff_queue_init(&diff_queued_diff);\n \n \t\tfprintf_ln(opt->diffopt.file, \"\\n%s\", opt->rdiff_title);\n \t\t/*\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 8b81153e8f..11029c10be 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3536,7 +3536,7 @@ static int detect_and_process_renames(struct merge_options *opt)\n \t/* Free memory for renames->pairs[] and combined */\n \tfor (s = MERGE_SIDE1; s <= MERGE_SIDE2; s++) {\n \t\tfree(renames->pairs[s].queue);\n-\t\tDIFF_QUEUE_CLEAR(&renames->pairs[s]);\n+\t\tdiff_queue_init(&renames->pairs[s]);\n \t}\n \tfor (i = 0; i < combined.nr; i++)\n \t\tpool_diff_free_filepair(&opt->priv->pool, combined.queue[i]);\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503712","messageId":"d3121c2522cc723ab4df71979fd9194260a6a57e.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 13/23] line-log: fix several memory leaks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:13:48Z","receivedAt":"2024-09-30T09:13:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"As described in \"line-log.c\" itself, the code is \"leaking like a sieve\".\nThese leaks are all of rather trivial nature, so this commit plugs them\nwithout going too much into details for each of those leaks.\n\nThe leaks are hit by t4211, but plugging them alone does not make the\nfull test suite pass. The remaining leaks are unrelated to the line-log\nsubsystem.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n line-log.c | 54 +++++++++++++++++++++++++++++++++++-------------------\n 1 file changed, 35 insertions(+), 19 deletions(-)\n\ndiff --git a/line-log.c b/line-log.c\nindex 89e0ea4562..ee48988c66 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -248,8 +248,10 @@ static void line_log_data_init(struct line_log_data *r)\n static void line_log_data_clear(struct line_log_data *r)\n {\n \trange_set_release(&r->ranges);\n+\tfree(r->path);\n \tif (r->pair)\n \t\tdiff_free_filepair(r->pair);\n+\tdiff_ranges_release(&r->diff);\n }\n \n static void free_line_log_data(struct line_log_data *r)\n@@ -571,7 +573,8 @@ parse_lines(struct repository *r, struct commit *commit,\n \tstruct line_log_data *p;\n \n \tfor_each_string_list_item(item, args) {\n-\t\tconst char *name_part, *range_part;\n+\t\tconst char *name_part;\n+\t\tchar *range_part;\n \t\tchar *full_name;\n \t\tstruct diff_filespec *spec;\n \t\tlong begin = 0, end = 0;\n@@ -615,6 +618,7 @@ parse_lines(struct repository *r, struct commit *commit,\n \n \t\tfree_filespec(spec);\n \t\tFREE_AND_NULL(ends);\n+\t\tfree(range_part);\n \t}\n \n \tfor (p = ranges; p; p = p->next)\n@@ -760,15 +764,13 @@ static void parse_pathspec_from_ranges(struct pathspec *pathspec,\n {\n \tstruct line_log_data *r;\n \tstruct strvec array = STRVEC_INIT;\n-\tconst char **paths;\n \n \tfor (r = range; r; r = r->next)\n \t\tstrvec_push(&array, r->path);\n-\tpaths = strvec_detach(&array);\n \n-\tparse_pathspec(pathspec, 0, PATHSPEC_PREFER_FULL, \"\", paths);\n-\t/* strings are now owned by pathspec */\n-\tfree(paths);\n+\tparse_pathspec(pathspec, 0, PATHSPEC_PREFER_FULL, \"\", array.v);\n+\n+\tstrvec_clear(&array);\n }\n \n void line_log_init(struct rev_info *rev, const char *prefix, struct string_list *args)\n@@ -781,6 +783,8 @@ void line_log_init(struct rev_info *rev, const char *prefix, struct string_list\n \tadd_line_range(rev, commit, range);\n \n \tparse_pathspec_from_ranges(&rev->diffopt.pathspec, range);\n+\n+\tfree_line_log_data(range);\n }\n \n static void move_diff_queue(struct diff_queue_struct *dst,\n@@ -1131,10 +1135,18 @@ static int process_all_files(struct line_log_data **range_out,\n \t\t\twhile (rg && strcmp(rg->path, pair->two->path))\n \t\t\t\trg = rg->next;\n \t\t\tassert(rg);\n+\t\t\tif (rg->pair)\n+\t\t\t\tdiff_free_filepair(rg->pair);\n \t\t\trg->pair = diff_filepair_dup(queue->queue[i]);\n+\t\t\tdiff_ranges_release(&rg->diff);\n \t\t\tmemcpy(&rg->diff, pairdiff, sizeof(struct diff_ranges));\n+\t\t\tFREE_AND_NULL(pairdiff);\n+\t\t}\n+\n+\t\tif (pairdiff) {\n+\t\t\tdiff_ranges_release(pairdiff);\n+\t\t\tfree(pairdiff);\n \t\t}\n-\t\tfree(pairdiff);\n \t}\n \n \treturn changed;\n@@ -1212,12 +1224,13 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm\n \tstruct commit_list *p;\n \tint i;\n \tint nparents = commit_list_count(commit->parents);\n+\tint ret;\n \n \tif (nparents > 1 && rev->first_parent_only)\n \t\tnparents = 1;\n \n \tALLOC_ARRAY(diffqueues, nparents);\n-\tALLOC_ARRAY(cand, nparents);\n+\tCALLOC_ARRAY(cand, nparents);\n \tALLOC_ARRAY(parents, nparents);\n \n \tp = commit->parents;\n@@ -1229,7 +1242,6 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm\n \n \tfor (i = 0; i < nparents; i++) {\n \t\tint changed;\n-\t\tcand[i] = NULL;\n \t\tchanged = process_all_files(&cand[i], rev, &diffqueues[i], range);\n \t\tif (!changed) {\n \t\t\t/*\n@@ -1237,13 +1249,10 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm\n \t\t\t * don't follow any other path in history\n \t\t\t */\n \t\t\tadd_line_range(rev, parents[i], cand[i]);\n-\t\t\tclear_commit_line_range(rev, commit);\n \t\t\tcommit_list_append(parents[i], &commit->parents);\n-\t\t\tfree(parents);\n-\t\t\tfree(cand);\n-\t\t\tfree_diffqueues(nparents, diffqueues);\n-\t\t\t/* NEEDSWORK leaking like a sieve */\n-\t\t\treturn 0;\n+\n+\t\t\tret = 0;\n+\t\t\tgoto out;\n \t\t}\n \t}\n \n@@ -1251,18 +1260,25 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm\n \t * No single parent took the blame.  We add the candidates\n \t * from the above loop to the parents.\n \t */\n-\tfor (i = 0; i < nparents; i++) {\n+\tfor (i = 0; i < nparents; i++)\n \t\tadd_line_range(rev, parents[i], cand[i]);\n-\t}\n \n+\tret = 1;\n+\n+out:\n \tclear_commit_line_range(rev, commit);\n \tfree(parents);\n+\tfor (i = 0; i < nparents; i++) {\n+\t\tif (!cand[i])\n+\t\t\tcontinue;\n+\t\tline_log_data_clear(cand[i]);\n+\t\tfree(cand[i]);\n+\t}\n \tfree(cand);\n \tfree_diffqueues(nparents, diffqueues);\n-\treturn 1;\n+\treturn ret;\n \n \t/* NEEDSWORK evil merge detection stuff */\n-\t/* NEEDSWORK leaking like a sieve */\n }\n \n int line_log_process_ranges_arbitrary_commit(struct rev_info *rev, struct commit *commit)\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503713","messageId":"6e7a272c29577536ba992cc73d736d8f66397607.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 14/23] pseudo-merge: fix various memory leaks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:13:51Z","receivedAt":"2024-09-30T09:13:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Fix various memory leaks hit by the pseudo-merge machinery. These leaks\nare exposed by t5333, but plugging them does not yet make the whole test\nsuite pass.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n pack-bitmap-write.c |  8 ++++++++\n pack-bitmap.c       |  4 ++--\n pseudo-merge.c      | 19 +++++++++++++++++++\n pseudo-merge.h      |  2 ++\n 4 files changed, 31 insertions(+), 2 deletions(-)\n\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex 4dc0fe8e40..6413dd1731 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -64,6 +64,12 @@ static void free_pseudo_merge_commit_idx(struct pseudo_merge_commit_idx *idx)\n \tfree(idx);\n }\n \n+static void pseudo_merge_group_release_cb(void *payload, const char *name UNUSED)\n+{\n+\tpseudo_merge_group_release(payload);\n+\tfree(payload);\n+}\n+\n void bitmap_writer_free(struct bitmap_writer *writer)\n {\n \tuint32_t i;\n@@ -82,6 +88,8 @@ void bitmap_writer_free(struct bitmap_writer *writer)\n \tkh_foreach_value(writer->pseudo_merge_commits, idx,\n \t\t\t free_pseudo_merge_commit_idx(idx));\n \tkh_destroy_oid_map(writer->pseudo_merge_commits);\n+\tstring_list_clear_func(&writer->pseudo_merge_groups,\n+\t\t\t       pseudo_merge_group_release_cb);\n \n \tfor (i = 0; i < writer->selected_nr; i++) {\n \t\tstruct bitmapped_commit *bc = &writer->selected[i];\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 9d9b8c4bfb..32b222a7af 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1390,8 +1390,8 @@ static struct bitmap *find_objects(struct bitmap_index *bitmap_git,\n \t\t}\n \n \t\tbase = bitmap_new();\n-\t\tif (!cascade_pseudo_merges_1(bitmap_git, base, roots_bitmap))\n-\t\t\tbitmap_free(roots_bitmap);\n+\t\tcascade_pseudo_merges_1(bitmap_git, base, roots_bitmap);\n+\t\tbitmap_free(roots_bitmap);\n \t}\n \n \t/*\ndiff --git a/pseudo-merge.c b/pseudo-merge.c\nindex 10ebd9a4e9..28782a31c6 100644\n--- a/pseudo-merge.c\n+++ b/pseudo-merge.c\n@@ -97,6 +97,25 @@ static void pseudo_merge_group_init(struct pseudo_merge_group *group)\n \tgroup->stable_size = DEFAULT_PSEUDO_MERGE_STABLE_SIZE;\n }\n \n+void pseudo_merge_group_release(struct pseudo_merge_group *group)\n+{\n+\tstruct hashmap_iter iter;\n+\tstruct strmap_entry *e;\n+\n+\tregfree(group->pattern);\n+\tfree(group->pattern);\n+\n+\tstrmap_for_each_entry(&group->matches, &iter, e) {\n+\t\tstruct pseudo_merge_matches *matches = e->value;\n+\t\tfree(matches->stable);\n+\t\tfree(matches->unstable);\n+\t\tfree(matches);\n+\t}\n+\tstrmap_clear(&group->matches, 0);\n+\n+\tfree(group->merges);\n+}\n+\n static int pseudo_merge_config(const char *var, const char *value,\n \t\t\t       const struct config_context *ctx,\n \t\t\t       void *cb_data)\ndiff --git a/pseudo-merge.h b/pseudo-merge.h\nindex 4b5febaa63..29df8a32ec 100644\n--- a/pseudo-merge.h\n+++ b/pseudo-merge.h\n@@ -51,6 +51,8 @@ struct pseudo_merge_group {\n \ttimestamp_t stable_threshold;\n };\n \n+void pseudo_merge_group_release(struct pseudo_merge_group *group);\n+\n struct pseudo_merge_matches {\n \tstruct commit **stable;\n \tstruct commit **unstable;\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503714","messageId":"9e1161d55f96253bfeb5cddd1bbd381e2dad8a71.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 15/23] pseudo-merge: fix leaking strmap keys","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:13:53Z","receivedAt":"2024-09-30T09:13:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When creating a new pseudo-merge group we collect a set of matchnig\ncommits and put them into a string map. This strmap is initialized such\nthat it does not allocate its keys, and instead we try to pass ownership\nof the keys to it via `strmap_put()`. This isn't how it works though:\nthe strmap will never try to release these keys, and consequently they\nend up leaking.\n\nFix this leak by initializing the strmap as duplicating its keys and not\ntrying to hand over ownership.\n\nThe leak is exposed by t5333, but plugging it does not yet make the full\ntest suite pass.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n pseudo-merge.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/pseudo-merge.c b/pseudo-merge.c\nindex 28782a31c6..bb59965ed2 100644\n--- a/pseudo-merge.c\n+++ b/pseudo-merge.c\n@@ -87,7 +87,7 @@ static void pseudo_merge_group_init(struct pseudo_merge_group *group)\n {\n \tmemset(group, 0, sizeof(struct pseudo_merge_group));\n \n-\tstrmap_init_with_options(&group->matches, NULL, 0);\n+\tstrmap_init_with_options(&group->matches, NULL, 1);\n \n \tgroup->decay = DEFAULT_PSEUDO_MERGE_DECAY;\n \tgroup->max_merges = DEFAULT_PSEUDO_MERGE_MAX_MERGES;\n@@ -275,7 +275,7 @@ static int find_pseudo_merge_group_for_ref(const char *refname,\n \t\tmatches = strmap_get(&group->matches, group_name.buf);\n \t\tif (!matches) {\n \t\t\tmatches = xcalloc(1, sizeof(*matches));\n-\t\t\tstrmap_put(&group->matches, strbuf_detach(&group_name, NULL),\n+\t\t\tstrmap_put(&group->matches, group_name.buf,\n \t\t\t\t   matches);\n \t\t}\n \n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503715","messageId":"98228243be04d7ab296ccf7516e092549bbd1236.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 16/23] pack-bitmap-write: fix leaking OID array","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:13:58Z","receivedAt":"2024-09-30T09:14:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Fix a leaking OID array in `write_pseudo_merges()`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n pack-bitmap-write.c             | 1 +\n t/t5333-pseudo-merge-bitmaps.sh | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex 6413dd1731..49758e2525 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -913,6 +913,7 @@ static void write_pseudo_merges(struct bitmap_writer *writer,\n \tfor (i = 0; i < writer->pseudo_merges_nr; i++)\n \t\tbitmap_free(commits_bitmap[i]);\n \n+\toid_array_clear(&commits);\n \tfree(pseudo_merge_ofs);\n \tfree(commits_bitmap);\n }\ndiff --git a/t/t5333-pseudo-merge-bitmaps.sh b/t/t5333-pseudo-merge-bitmaps.sh\nindex 1dd6284756..eca4a1eb8c 100755\n--- a/t/t5333-pseudo-merge-bitmaps.sh\n+++ b/t/t5333-pseudo-merge-bitmaps.sh\n@@ -4,6 +4,7 @@ test_description='pseudo-merge bitmaps'\n \n GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_pseudo_merges () {\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503716","messageId":"5db4e17db9b9b991e0dbe56d7043b56e5d161097.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 17/23] midx-write: fix leaking buffer","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:14:01Z","receivedAt":"2024-09-30T09:14:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The buffer used to compute the final MIDX name is never released. Plug\nthis memory leak.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n midx-write.c                            | 2 ++\n t/t5334-incremental-multi-pack-index.sh | 2 ++\n 2 files changed, 4 insertions(+)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 1ef62c4f4b..625c7d3137 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1445,6 +1445,8 @@ static int write_midx_internal(const char *object_dir,\n \t\t\treturn -1;\n \t\t}\n \n+\t\tstrbuf_release(&final_midx_name);\n+\n \t\tkeep_hashes[ctx.num_multi_pack_indexes_before] =\n \t\t\txstrdup(hash_to_hex(midx_hash));\n \ndiff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh\nindex c3b08acc73..471994c4bc 100755\n--- a/t/t5334-incremental-multi-pack-index.sh\n+++ b/t/t5334-incremental-multi-pack-index.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='incremental multi-pack-index'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-midx.sh\n \n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503717","messageId":"09e067162aa82fddabe6c7aeffc55f734212ee4e.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 18/23] revision: fix memory leaks when rewriting parents","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:14:03Z","receivedAt":"2024-09-30T09:14:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Both `rewrite_parents()` and `remove_duplicate_parents()` may end up\ndropping some parents from a commit without freeing the respective\n`struct commit_list` items. This causes a bunch of memory leaks. Plug\nthese.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n revision.c                                 | 2 ++\n t/t3427-rebase-subtree.sh                  | 1 +\n t/t6016-rev-list-graph-simplify-history.sh | 1 +\n t/t7003-filter-branch.sh                   | 1 +\n t/t9350-fast-export.sh                     | 1 +\n t/t9402-git-cvsserver-refs.sh              | 1 +\n 6 files changed, 7 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex e79f39e555..6b452ea182 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -3250,6 +3250,7 @@ static int remove_duplicate_parents(struct rev_info *revs, struct commit *commit\n \t\tstruct commit *parent = p->item;\n \t\tif (parent->object.flags & TMP_MARK) {\n \t\t\t*pp = p->next;\n+\t\t\tfree(p);\n \t\t\tif (ts)\n \t\t\t\tcompact_treesame(revs, commit, surviving_parents);\n \t\t\tcontinue;\n@@ -4005,6 +4006,7 @@ int rewrite_parents(struct rev_info *revs, struct commit *commit,\n \t\t\tbreak;\n \t\tcase rewrite_one_noparents:\n \t\t\t*pp = parent->next;\n+\t\t\tfree(parent);\n \t\t\tcontinue;\n \t\tcase rewrite_one_error:\n \t\t\treturn -1;\ndiff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh\nindex 1b3e97c875..5e9046e3df 100755\n--- a/t/t3427-rebase-subtree.sh\n+++ b/t/t3427-rebase-subtree.sh\n@@ -7,6 +7,7 @@ This test runs git rebase and tests the subtree strategy.\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-rebase.sh\n \ndiff --git a/t/t6016-rev-list-graph-simplify-history.sh b/t/t6016-rev-list-graph-simplify-history.sh\nindex 54b0a6f5f8..2656d6a6bc 100755\n--- a/t/t6016-rev-list-graph-simplify-history.sh\n+++ b/t/t6016-rev-list-graph-simplify-history.sh\n@@ -10,6 +10,7 @@ test_description='--graph and simplified history'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-log-graph.sh\n \ndiff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh\nindex 5ab4d41ee7..bf3e3f0b67 100755\n--- a/t/t7003-filter-branch.sh\n+++ b/t/t7003-filter-branch.sh\n@@ -4,6 +4,7 @@ test_description='git filter-branch'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY/lib-gpg.sh\"\n \ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex 1eb035ee4c..2bdc02b459 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -7,6 +7,7 @@ test_description='git fast-export'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t9402-git-cvsserver-refs.sh b/t/t9402-git-cvsserver-refs.sh\nindex 2ee41f9443..c847120d52 100755\n--- a/t/t9402-git-cvsserver-refs.sh\n+++ b/t/t9402-git-cvsserver-refs.sh\n@@ -8,6 +8,7 @@ tags, branches and other git refspecs'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n #########\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503718","messageId":"89892e49e5d1cd0aa72364b686950d1bcbbfd92f.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 19/23] revision: fix leaking saved parents","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:14:06Z","receivedAt":"2024-09-30T09:14:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `saved_parents` slab is used by `--full-diff` to save parents of a\ncommit which we are about to rewrite. We do not release its contents\nonce it's not used anymore, causing a memory leak. Plug it.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n revision.c                   | 12 ++++++++++--\n t/t6012-rev-list-simplify.sh |  1 +\n 2 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 6b452ea182..f5f5b84f2b 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -4207,10 +4207,18 @@ static void save_parents(struct rev_info *revs, struct commit *commit)\n \t\t*pp = EMPTY_PARENT_LIST;\n }\n \n+static void free_saved_parent(struct commit_list **parents)\n+{\n+\tif (*parents != EMPTY_PARENT_LIST)\n+\t\tfree_commit_list(*parents);\n+}\n+\n static void free_saved_parents(struct rev_info *revs)\n {\n-\tif (revs->saved_parents_slab)\n-\t\tclear_saved_parents(revs->saved_parents_slab);\n+\tif (!revs->saved_parents_slab)\n+\t\treturn;\n+\tdeep_clear_saved_parents(revs->saved_parents_slab, free_saved_parent);\n+\tFREE_AND_NULL(revs->saved_parents_slab);\n }\n \n struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit *commit)\ndiff --git a/t/t6012-rev-list-simplify.sh b/t/t6012-rev-list-simplify.sh\nindex de1e87f162..8ed1a215da 100755\n--- a/t/t6012-rev-list-simplify.sh\n+++ b/t/t6012-rev-list-simplify.sh\n@@ -5,6 +5,7 @@ test_description='merge simplification'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n note () {\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503719","messageId":"2f3e17915d750c0ded8bf4f2f57106b81e9d8b3b.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 20/23] pack-write: fix return parameter of `write_rev_file_order()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:14:08Z","receivedAt":"2024-09-30T09:14:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While the return parameter of `write_rev_file_order()` is a string\nconstant, the function may indeed return an allocated string when its\nfirst parameter is a `NULL` pointer. This makes for a confusing calling\nconvention, where callers need to be aware of these intricate ownership\nrules and cast away the constness to free the string in some cases.\n\nAdapt the function and its caller `write_rev_file()` to always return an\nallocated string and adapt callers to always free the return value.\n\nNote that this requires us to also adapt `rename_tmp_packfile()`, which\ncompares the pointers to packfile data with each other. Now that the\npath of the reverse index file gets allocated unconditionally the check\nwill always fail. This is fixed by using strcmp(3P) instead, which also\nfeels way less fragile.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/index-pack.c              |  7 +++---\n midx-write.c                      |  3 ++-\n pack-write.c                      | 42 +++++++++++++++++--------------\n pack.h                            |  4 +--\n t/t5327-multi-pack-bitmaps-rev.sh |  1 +\n 5 files changed, 31 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex e228c56ff2..9d23b41b3a 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1505,7 +1505,7 @@ static void rename_tmp_packfile(const char **final_name,\n \t\t\t\tstruct strbuf *name, unsigned char *hash,\n \t\t\t\tconst char *ext, int make_read_only_if_same)\n {\n-\tif (*final_name != curr_name) {\n+\tif (!*final_name || strcmp(*final_name, curr_name)) {\n \t\tif (!*final_name)\n \t\t\t*final_name = odb_pack_name(name, hash, ext);\n \t\tif (finalize_object_file(curr_name, *final_name))\n@@ -1726,7 +1726,7 @@ int cmd_index_pack(int argc,\n {\n \tint i, fix_thin_pack = 0, verify = 0, stat_only = 0, rev_index;\n \tconst char *curr_index;\n-\tconst char *curr_rev_index = NULL;\n+\tchar *curr_rev_index = NULL;\n \tconst char *index_name = NULL, *pack_name = NULL, *rev_index_name = NULL;\n \tconst char *keep_msg = NULL;\n \tconst char *promisor_msg = NULL;\n@@ -1968,8 +1968,7 @@ int cmd_index_pack(int argc,\n \t\tfree((void *) curr_pack);\n \tif (!index_name)\n \t\tfree((void *) curr_index);\n-\tif (!rev_index_name)\n-\t\tfree((void *) curr_rev_index);\n+\tfree(curr_rev_index);\n \n \t/*\n \t * Let the caller know this pack is not self contained\ndiff --git a/midx-write.c b/midx-write.c\nindex 625c7d3137..b3a5f6c516 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -649,7 +649,7 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,\n \t\t\t\t     struct write_midx_context *ctx)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tconst char *tmp_file;\n+\tchar *tmp_file;\n \n \ttrace2_region_enter(\"midx\", \"write_midx_reverse_index\", the_repository);\n \n@@ -662,6 +662,7 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,\n \t\tdie(_(\"cannot store reverse index file\"));\n \n \tstrbuf_release(&buf);\n+\tfree(tmp_file);\n \n \ttrace2_region_leave(\"midx\", \"write_midx_reverse_index\", the_repository);\n }\ndiff --git a/pack-write.c b/pack-write.c\nindex 27965672f1..9961149e65 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -212,15 +212,15 @@ static void write_rev_trailer(struct hashfile *f, const unsigned char *hash)\n \thashwrite(f, hash, the_hash_algo->rawsz);\n }\n \n-const char *write_rev_file(const char *rev_name,\n-\t\t\t   struct pack_idx_entry **objects,\n-\t\t\t   uint32_t nr_objects,\n-\t\t\t   const unsigned char *hash,\n-\t\t\t   unsigned flags)\n+char *write_rev_file(const char *rev_name,\n+\t\t     struct pack_idx_entry **objects,\n+\t\t     uint32_t nr_objects,\n+\t\t     const unsigned char *hash,\n+\t\t     unsigned flags)\n {\n \tuint32_t *pack_order;\n \tuint32_t i;\n-\tconst char *ret;\n+\tchar *ret;\n \n \tif (!(flags & WRITE_REV) && !(flags & WRITE_REV_VERIFY))\n \t\treturn NULL;\n@@ -238,13 +238,14 @@ const char *write_rev_file(const char *rev_name,\n \treturn ret;\n }\n \n-const char *write_rev_file_order(const char *rev_name,\n-\t\t\t\t uint32_t *pack_order,\n-\t\t\t\t uint32_t nr_objects,\n-\t\t\t\t const unsigned char *hash,\n-\t\t\t\t unsigned flags)\n+char *write_rev_file_order(const char *rev_name,\n+\t\t\t   uint32_t *pack_order,\n+\t\t\t   uint32_t nr_objects,\n+\t\t\t   const unsigned char *hash,\n+\t\t\t   unsigned flags)\n {\n \tstruct hashfile *f;\n+\tchar *path;\n \tint fd;\n \n \tif ((flags & WRITE_REV) && (flags & WRITE_REV_VERIFY))\n@@ -254,12 +255,13 @@ const char *write_rev_file_order(const char *rev_name,\n \t\tif (!rev_name) {\n \t\t\tstruct strbuf tmp_file = STRBUF_INIT;\n \t\t\tfd = odb_mkstemp(&tmp_file, \"pack/tmp_rev_XXXXXX\");\n-\t\t\trev_name = strbuf_detach(&tmp_file, NULL);\n+\t\t\tpath = strbuf_detach(&tmp_file, NULL);\n \t\t} else {\n \t\t\tunlink(rev_name);\n \t\t\tfd = xopen(rev_name, O_CREAT|O_EXCL|O_WRONLY, 0600);\n+\t\t\tpath = xstrdup(rev_name);\n \t\t}\n-\t\tf = hashfd(fd, rev_name);\n+\t\tf = hashfd(fd, path);\n \t} else if (flags & WRITE_REV_VERIFY) {\n \t\tstruct stat statbuf;\n \t\tif (stat(rev_name, &statbuf)) {\n@@ -270,22 +272,24 @@ const char *write_rev_file_order(const char *rev_name,\n \t\t\t\tdie_errno(_(\"could not stat: %s\"), rev_name);\n \t\t}\n \t\tf = hashfd_check(rev_name);\n-\t} else\n+\t\tpath = xstrdup(rev_name);\n+\t} else {\n \t\treturn NULL;\n+\t}\n \n \twrite_rev_header(f);\n \n \twrite_rev_index_positions(f, pack_order, nr_objects);\n \twrite_rev_trailer(f, hash);\n \n-\tif (rev_name && adjust_shared_perm(rev_name) < 0)\n-\t\tdie(_(\"failed to make %s readable\"), rev_name);\n+\tif (adjust_shared_perm(path) < 0)\n+\t\tdie(_(\"failed to make %s readable\"), path);\n \n \tfinalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,\n \t\t\t  CSUM_HASH_IN_STREAM | CSUM_CLOSE |\n \t\t\t  ((flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC));\n \n-\treturn rev_name;\n+\treturn path;\n }\n \n static void write_mtimes_header(struct hashfile *f)\n@@ -549,7 +553,7 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,\n \t\t\t unsigned char hash[],\n \t\t\t char **idx_tmp_name)\n {\n-\tconst char *rev_tmp_name = NULL;\n+\tchar *rev_tmp_name = NULL;\n \tchar *mtimes_tmp_name = NULL;\n \n \tif (adjust_shared_perm(pack_tmp_name))\n@@ -575,7 +579,7 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,\n \tif (mtimes_tmp_name)\n \t\trename_tmp_packfile(name_buffer, mtimes_tmp_name, \"mtimes\");\n \n-\tfree((char *)rev_tmp_name);\n+\tfree(rev_tmp_name);\n \tfree(mtimes_tmp_name);\n }\n \ndiff --git a/pack.h b/pack.h\nindex 3ab9e3f60c..02bbdfb19c 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -96,8 +96,8 @@ struct ref;\n \n void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought);\n \n-const char *write_rev_file(const char *rev_name, struct pack_idx_entry **objects, uint32_t nr_objects, const unsigned char *hash, unsigned flags);\n-const char *write_rev_file_order(const char *rev_name, uint32_t *pack_order, uint32_t nr_objects, const unsigned char *hash, unsigned flags);\n+char *write_rev_file(const char *rev_name, struct pack_idx_entry **objects, uint32_t nr_objects, const unsigned char *hash, unsigned flags);\n+char *write_rev_file_order(const char *rev_name, uint32_t *pack_order, uint32_t nr_objects, const unsigned char *hash, unsigned flags);\n \n /*\n  * The \"hdr\" output buffer should be at least this big, which will handle sizes\ndiff --git a/t/t5327-multi-pack-bitmaps-rev.sh b/t/t5327-multi-pack-bitmaps-rev.sh\nindex 9cac03a94b..994a8e6be4 100755\n--- a/t/t5327-multi-pack-bitmaps-rev.sh\n+++ b/t/t5327-multi-pack-bitmaps-rev.sh\n@@ -2,6 +2,7 @@\n \n test_description='exercise basic multi-pack bitmap functionality (.rev files)'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"${TEST_DIRECTORY}/lib-bitmap.sh\"\n \n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503720","messageId":"f9822f681e76fb14b71a0a60ac8fda11f0a5bb02.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 21/23] t/helper: fix leaks in proc-receive helper","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:14:16Z","receivedAt":"2024-09-30T09:14:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Fix trivial leaks in the proc-receive helpe.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/helper/test-proc-receive.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/t/helper/test-proc-receive.c b/t/helper/test-proc-receive.c\nindex 29361c7aab..3703f734f3 100644\n--- a/t/helper/test-proc-receive.c\n+++ b/t/helper/test-proc-receive.c\n@@ -196,5 +196,12 @@ int cmd__proc_receive(int argc, const char **argv)\n \tpacket_flush(1);\n \tsigchain_pop(SIGPIPE);\n \n+\twhile (commands) {\n+\t\tstruct command *next = commands->next;\n+\t\tfree(commands);\n+\t\tcommands = next;\n+\t}\n+\tstring_list_clear(&push_options, 0);\n+\n \treturn 0;\n }\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503721","messageId":"e94ee74ebdf0ed0fb096254982da2dd5e9500b5f.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 22/23] remote: fix leaking push reports","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:14:21Z","receivedAt":"2024-09-30T09:14:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The push reports that report failures to the user when pushing a\nreference leak in several places. Plug these leaks by introducing a new\nfunction `ref_push_report_free()` that frees the list of reports and\ncall it as required. While at it, fix a trivially leaking error string\nin the vicinity.\n\nThese leaks get hit in t5411, but plugging them does not make the whole\ntest suite pass.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/receive-pack.c |  5 ++++-\n remote.c               | 15 +++++++++++++++\n remote.h               |  4 +++-\n 3 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 536d22761d..ab5b20e39c 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -374,6 +374,7 @@ static void write_head_info(void)\n struct command {\n \tstruct command *next;\n \tconst char *error_string;\n+\tchar *error_string_owned;\n \tstruct ref_push_report *report;\n \tunsigned int skip_update:1,\n \t\t     did_not_exist:1,\n@@ -1083,7 +1084,7 @@ static int read_proc_receive_report(struct packet_reader *reader,\n \t\thint->run_proc_receive |= RUN_PROC_RECEIVE_RETURNED;\n \t\tif (!strcmp(head, \"ng\")) {\n \t\t\tif (p)\n-\t\t\t\thint->error_string = xstrdup(p);\n+\t\t\t\thint->error_string = hint->error_string_owned = xstrdup(p);\n \t\t\telse\n \t\t\t\thint->error_string = \"failed\";\n \t\t\tcode = -1;\n@@ -2054,6 +2055,8 @@ static void free_commands(struct command *commands)\n \twhile (commands) {\n \t\tstruct command *next = commands->next;\n \n+\t\tref_push_report_free(commands->report);\n+\t\tfree(commands->error_string_owned);\n \t\tfree(commands);\n \t\tcommands = next;\n \t}\ndiff --git a/remote.c b/remote.c\nindex e291e8ff5c..cd2091ac0c 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -868,6 +868,20 @@ struct strvec *push_url_of_remote(struct remote *remote)\n \treturn remote->pushurl.nr ? &remote->pushurl : &remote->url;\n }\n \n+void ref_push_report_free(struct ref_push_report *report)\n+{\n+\twhile (report) {\n+\t\tstruct ref_push_report *next = report->next;\n+\n+\t\tfree(report->ref_name);\n+\t\tfree(report->old_oid);\n+\t\tfree(report->new_oid);\n+\t\tfree(report);\n+\n+\t\treport = next;\n+\t}\n+}\n+\n static int match_name_with_pattern(const char *key, const char *name,\n \t\t\t\t   const char *value, char **result)\n {\n@@ -1122,6 +1136,7 @@ void free_one_ref(struct ref *ref)\n \tif (!ref)\n \t\treturn;\n \tfree_one_ref(ref->peer_ref);\n+\tref_push_report_free(ref->report);\n \tfree(ref->remote_status);\n \tfree(ref->tracking_ref);\n \tfree(ref->symref);\ndiff --git a/remote.h b/remote.h\nindex ad4513f639..c5e79eb40e 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -126,13 +126,15 @@ int remote_has_url(struct remote *remote, const char *url);\n struct strvec *push_url_of_remote(struct remote *remote);\n \n struct ref_push_report {\n-\tconst char *ref_name;\n+\tchar *ref_name;\n \tstruct object_id *old_oid;\n \tstruct object_id *new_oid;\n \tunsigned int forced_update:1;\n \tstruct ref_push_report *next;\n };\n \n+void ref_push_report_free(struct ref_push_report *);\n+\n struct ref {\n \tstruct ref *next;\n \tstruct object_id old_oid;\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503722","messageId":"312de7828b646101145975466574434d2dae048e.1727687410.git.ps@pks.im","threadId":"62215","inReplyTo":"cover.1727687410.git.ps@pks.im","subject":"[PATCH 23/23] builtin/send-pack: fix leaking list of push options","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T09:14:24Z","receivedAt":"2024-09-30T09:14:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The list of push options is leaking. Plug the leak.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/send-pack.c          | 1 +\n t/t5411-proc-receive-hook.sh | 1 +\n t/t5545-push-options.sh      | 1 +\n 3 files changed, 3 insertions(+)\n\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 8b1d46e79a..59b626aae8 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -340,6 +340,7 @@ int cmd_send_pack(int argc,\n \t\t/* stable plumbing output; do not modify or localize */\n \t\tfprintf(stderr, \"Everything up-to-date\\n\");\n \n+\tstring_list_clear(&push_options, 0);\n \tfree_refs(remote_refs);\n \tfree_refs(local_refs);\n \trefspec_clear(&rs);\ndiff --git a/t/t5411-proc-receive-hook.sh b/t/t5411-proc-receive-hook.sh\nindex 92cf52c6d4..13d2d310a9 100755\n--- a/t/t5411-proc-receive-hook.sh\n+++ b/t/t5411-proc-receive-hook.sh\n@@ -8,6 +8,7 @@ test_description='Test proc-receive hook'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n . \"$TEST_DIRECTORY\"/t5411/common-functions.sh\ndiff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh\nindex fb13549da7..64ce56d3aa 100755\n--- a/t/t5545-push-options.sh\n+++ b/t/t5545-push-options.sh\n@@ -8,6 +8,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1\n export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n mk_repo_pair () {\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503733","messageId":"076932a7-d88d-4aea-8117-69a2ab83f459@gmail.com","threadId":"62215","inReplyTo":"c51f40c5bd0c56967e348363e784222de7884b79.1727687410.git.ps@pks.im","subject":"Re: [PATCH 02/23] read-cache: fix leaking hash context in `do_write_index()`","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-09-30T14:05:13Z","receivedAt":"2024-09-30T14:05:16Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/30/24 5:13 AM, Patrick Steinhardt wrote:\n> When writing an index with the EOIE extension we allocate a separate\n> hash context. We never free that context though, causing a memory leak.\n> Plug it.\n> \n> This leak is exposed by t9210, but plugging it alone does not make the\n> whole test suite pass.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>   read-cache.c | 1 +\n>   1 file changed, 1 insertion(+)\n> \n> diff --git a/read-cache.c b/read-cache.c\n> index 764fdfec46..0fb5e0d372 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -3124,6 +3124,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n>   \tif (f)\n>   \t\tfree_hashfile(f);\n>   \tstrbuf_release(&sb);\n> +\tfree(eoie_c);\n>   \tfree(ieot);\n>   \treturn ret;\n>   }\n\nThank you for finding and fixing this! This version should be\nused instead of the similar patch I sent today.\n\nThanks,\n-Stolee\n\n"},{"id":"503739","messageId":"ZvrQByUPoHhlDMiF@nand.local","threadId":"62215","inReplyTo":"6e7a272c29577536ba992cc73d736d8f66397607.1727687410.git.ps@pks.im","subject":"Re: [PATCH 14/23] pseudo-merge: fix various memory leaks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-09-30T16:21:27Z","receivedAt":"2024-09-30T16:21:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 30, 2024 at 11:13:51AM +0200, Patrick Steinhardt wrote:\n> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\n> index 4dc0fe8e40..6413dd1731 100644\n> --- a/pack-bitmap-write.c\n> +++ b/pack-bitmap-write.c\n> @@ -64,6 +64,12 @@ static void free_pseudo_merge_commit_idx(struct pseudo_merge_commit_idx *idx)\n>  \tfree(idx);\n>  }\n>\n> +static void pseudo_merge_group_release_cb(void *payload, const char *name UNUSED)\n> +{\n> +\tpseudo_merge_group_release(payload);\n> +\tfree(payload);\n> +}\n> +\n>  void bitmap_writer_free(struct bitmap_writer *writer)\n>  {\n>  \tuint32_t i;\n> @@ -82,6 +88,8 @@ void bitmap_writer_free(struct bitmap_writer *writer)\n>  \tkh_foreach_value(writer->pseudo_merge_commits, idx,\n>  \t\t\t free_pseudo_merge_commit_idx(idx));\n>  \tkh_destroy_oid_map(writer->pseudo_merge_commits);\n> +\tstring_list_clear_func(&writer->pseudo_merge_groups,\n> +\t\t\t       pseudo_merge_group_release_cb);\n\nLooking good, and this is the right spot to free the pseudo-merge\ngroups. Note for other readers: pseudo-merge \"groups\" are a utility\nstructure that is only used while writing pseudo-merge bitmaps, so we\ndon't expect to see any corresponding pseudo_merge_group_release() calls\nsprinkled throughout, e.g., pack-bitmap.c.\n\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index 9d9b8c4bfb..32b222a7af 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -1390,8 +1390,8 @@ static struct bitmap *find_objects(struct bitmap_index *bitmap_git,\n>  \t\t}\n>\n>  \t\tbase = bitmap_new();\n> -\t\tif (!cascade_pseudo_merges_1(bitmap_git, base, roots_bitmap))\n> -\t\t\tbitmap_free(roots_bitmap);\n> +\t\tcascade_pseudo_merges_1(bitmap_git, base, roots_bitmap);\n> +\t\tbitmap_free(roots_bitmap);\n\nI probably would have pulled this leakfix into its own separate patch,\nsince it's not immediately obvious how it's related to other changes in\nthe same patch.\n\nBut the change here is definitely correct. We initialize the\nroots_bitmap field to just the tips of our traversal. I wrote some\ndetails about why in 11d45a6e6a (pack-bitmap.c: use pseudo-merges during\ntraversal, 2024-05-23), but the gist is as follows: We want to avoid\naccidentally thinking that roots which aren't part of some satisfied\npseudo-merge or existing bitmap are part of the reachability closure,\nleaving those bits as dangling and leading to incorrect results.\n\nIn any event, we definitely do not need the roots_bitmap outside of that\nblock (regardless of whether or not we successfully cascaded any\npseudo-merges), so free-ing it here is the right thing to do.\n\n> diff --git a/pseudo-merge.c b/pseudo-merge.c\n> index 10ebd9a4e9..28782a31c6 100644\n> --- a/pseudo-merge.c\n> +++ b/pseudo-merge.c\n> @@ -97,6 +97,25 @@ static void pseudo_merge_group_init(struct pseudo_merge_group *group)\n>  \tgroup->stable_size = DEFAULT_PSEUDO_MERGE_STABLE_SIZE;\n>  }\n>\n> +void pseudo_merge_group_release(struct pseudo_merge_group *group)\n> +{\n> +\tstruct hashmap_iter iter;\n> +\tstruct strmap_entry *e;\n> +\n> +\tregfree(group->pattern);\n> +\tfree(group->pattern);\n> +\n> +\tstrmap_for_each_entry(&group->matches, &iter, e) {\n> +\t\tstruct pseudo_merge_matches *matches = e->value;\n> +\t\tfree(matches->stable);\n> +\t\tfree(matches->unstable);\n> +\t\tfree(matches);\n> +\t}\n> +\tstrmap_clear(&group->matches, 0);\n> +\n> +\tfree(group->merges);\n> +}\n> +\n\nAll looks good here, I didn't see any fields that were missed or\nover-eagerly free'd here.\n\nThanks,\nTaylor\n"},{"id":"503774","messageId":"ZvsWlNPGxt5AOQcK@nand.local","threadId":"62215","inReplyTo":"9e1161d55f96253bfeb5cddd1bbd381e2dad8a71.1727687410.git.ps@pks.im","subject":"Re: [PATCH 15/23] pseudo-merge: fix leaking strmap keys","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-09-30T21:22:28Z","receivedAt":"2024-09-30T21:22:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 30, 2024 at 11:13:53AM +0200, Patrick Steinhardt wrote:\n> When creating a new pseudo-merge group we collect a set of matchnig\n\ns/matchnig/matching/\n\n> commits and put them into a string map. This strmap is initialized such\n> that it does not allocate its keys, and instead we try to pass ownership\n> of the keys to it via `strmap_put()`. This isn't how it works though:\n> the strmap will never try to release these keys, and consequently they\n> end up leaking.\n>\n> Fix this leak by initializing the strmap as duplicating its keys and not\n> trying to hand over ownership.\n\nHmm. I think I am probably splitting hairs, since a few xstrdup() calls\nbetween friends is unlikely to matter here, but... ;-)\n\nIt does seem wasteful if we can avoid it. We already allocated heap\nspace for the matches via the underlying strbuf, and we really do want\nto hand ownership over if we can.\n\nWould doing something like this on top of your previous patch do the\ntrick?\n\n--- >8 ---\ndiff --git a/pseudo-merge.c b/pseudo-merge.c\nindex 28782a31c6..6b6605d3dc 100644\n--- a/pseudo-merge.c\n+++ b/pseudo-merge.c\n@@ -110,6 +110,7 @@ void pseudo_merge_group_release(struct pseudo_merge_group *group)\n \t\tfree(matches->stable);\n \t\tfree(matches->unstable);\n \t\tfree(matches);\n+\t\tfree((char*)e->key);\n \t}\n \tstrmap_clear(&group->matches, 0);\n--- 8< ---\n\nThat introduces an ugly const-cast, but I think it's tolerable (and may\nbe moreso with a comment explaining why it's safe).\n\n> diff --git a/pseudo-merge.c b/pseudo-merge.c\n> index 28782a31c6..bb59965ed2 100644\n> --- a/pseudo-merge.c\n> +++ b/pseudo-merge.c\n> @@ -87,7 +87,7 @@ static void pseudo_merge_group_init(struct pseudo_merge_group *group)\n>  {\n>  \tmemset(group, 0, sizeof(struct pseudo_merge_group));\n>\n> -\tstrmap_init_with_options(&group->matches, NULL, 0);\n> +\tstrmap_init_with_options(&group->matches, NULL, 1);\n>\n>  \tgroup->decay = DEFAULT_PSEUDO_MERGE_DECAY;\n>  \tgroup->max_merges = DEFAULT_PSEUDO_MERGE_MAX_MERGES;\n> @@ -275,7 +275,7 @@ static int find_pseudo_merge_group_for_ref(const char *refname,\n>  \t\tmatches = strmap_get(&group->matches, group_name.buf);\n>  \t\tif (!matches) {\n>  \t\t\tmatches = xcalloc(1, sizeof(*matches));\n> -\t\t\tstrmap_put(&group->matches, strbuf_detach(&group_name, NULL),\n> +\t\t\tstrmap_put(&group->matches, group_name.buf,\n>  \t\t\t\t   matches);\n>  \t\t}\n\nOtherwise, I think what's written here looks good to me.\n\nThanks,\nTaylor\n"},{"id":"503775","messageId":"ZvsWrRW2rwylzETb@nand.local","threadId":"62215","inReplyTo":"98228243be04d7ab296ccf7516e092549bbd1236.1727687410.git.ps@pks.im","subject":"Re: [PATCH 16/23] pack-bitmap-write: fix leaking OID array","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-09-30T21:22:53Z","receivedAt":"2024-09-30T21:22:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 30, 2024 at 11:13:58AM +0200, Patrick Steinhardt wrote:\n> diff --git a/t/t5333-pseudo-merge-bitmaps.sh b/t/t5333-pseudo-merge-bitmaps.sh\n> index 1dd6284756..eca4a1eb8c 100755\n> --- a/t/t5333-pseudo-merge-bitmaps.sh\n> +++ b/t/t5333-pseudo-merge-bitmaps.sh\n> @@ -4,6 +4,7 @@ test_description='pseudo-merge bitmaps'\n>\n>  GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0\n>\n> +TEST_PASSES_SANITIZE_LEAK=true\n>  . ./test-lib.sh\n\nVery nicely done :-).\n\nThanks,\nTaylor\n"},{"id":"503776","messageId":"ZvsXrQadrU78rEha@nand.local","threadId":"62215","inReplyTo":"5db4e17db9b9b991e0dbe56d7043b56e5d161097.1727687410.git.ps@pks.im","subject":"Re: [PATCH 17/23] midx-write: fix leaking buffer","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-09-30T21:27:09Z","receivedAt":"2024-09-30T21:27:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 30, 2024 at 11:14:01AM +0200, Patrick Steinhardt wrote:\n> The buffer used to compute the final MIDX name is never released. Plug\n> this memory leak.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  midx-write.c                            | 2 ++\n>  t/t5334-incremental-multi-pack-index.sh | 2 ++\n>  2 files changed, 4 insertions(+)\n>\n> diff --git a/midx-write.c b/midx-write.c\n> index 1ef62c4f4b..625c7d3137 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -1445,6 +1445,8 @@ static int write_midx_internal(const char *object_dir,\n>  \t\t\treturn -1;\n\nWould you also want to strbuf_release() the final_midx_name buffer here\nas well?\n\nI guess the point is moot since there are a number of other finalization\nsteps that we likewise skip here by returning -1 directly instead of\njumping to the cleanup sub-routine.\n\nIn the above sense I'm OK with it as-is, but it would be nice to verify\nthat this portion of the code is leak-free even when we return early\n(e.g., because we couldn't rename() the tempfile into place).\n\nOf course, because final_midx_name is local to the body of this\nconditional, I think you'd need to do something like:\n\n    if (ctx.incremntal) {\n        struct strbuf final_midx_name = STRBUF_INIT;\n\n        /* ... */\n\n        get_split_midx_filename_ext(&final_midx_name, object_dir,\n                                    midx_hash, MIDX_EXT_MIDX);\n\n        if (rename_tempfile(&incr, final_midx_name.buf) < 0) {\n            result = error_errno(_(\"unable to rename new multi-pack-index layer\"));\n            strbuf_release(&final_midx_name); /* <- here */\n            goto cleanup;\n        }\n\n        strbuf_release(&final_midx_name); /* ... <- and here */\n\n        keep_hashes[ctx.num_multi_pack_indexes_before] =\n          xstrdup(hash_to_hex(midx_hash));\n    }\n\nThanks,\nTaylor\n"},{"id":"504286","messageId":"ZwOstswKzqMNNOUO@pks.im","threadId":"62215","inReplyTo":"ZvsWlNPGxt5AOQcK@nand.local","subject":"Re: [PATCH 15/23] pseudo-merge: fix leaking strmap keys","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-07T09:41:16Z","receivedAt":"2024-10-07T09:41:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Sep 30, 2024 at 05:22:28PM -0400, Taylor Blau wrote:\n> On Mon, Sep 30, 2024 at 11:13:53AM +0200, Patrick Steinhardt wrote:\n> > When creating a new pseudo-merge group we collect a set of matchnig\n> \n> s/matchnig/matching/\n> \n> > commits and put them into a string map. This strmap is initialized such\n> > that it does not allocate its keys, and instead we try to pass ownership\n> > of the keys to it via `strmap_put()`. This isn't how it works though:\n> > the strmap will never try to release these keys, and consequently they\n> > end up leaking.\n> >\n> > Fix this leak by initializing the strmap as duplicating its keys and not\n> > trying to hand over ownership.\n> \n> Hmm. I think I am probably splitting hairs, since a few xstrdup() calls\n> between friends is unlikely to matter here, but... ;-)\n> \n> It does seem wasteful if we can avoid it. We already allocated heap\n> space for the matches via the underlying strbuf, and we really do want\n> to hand ownership over if we can.\n> \n> Would doing something like this on top of your previous patch do the\n> trick?\n> \n> --- >8 ---\n> diff --git a/pseudo-merge.c b/pseudo-merge.c\n> index 28782a31c6..6b6605d3dc 100644\n> --- a/pseudo-merge.c\n> +++ b/pseudo-merge.c\n> @@ -110,6 +110,7 @@ void pseudo_merge_group_release(struct pseudo_merge_group *group)\n>  \t\tfree(matches->stable);\n>  \t\tfree(matches->unstable);\n>  \t\tfree(matches);\n> +\t\tfree((char*)e->key);\n>  \t}\n>  \tstrmap_clear(&group->matches, 0);\n> --- 8< ---\n> \n> That introduces an ugly const-cast, but I think it's tolerable (and may\n> be moreso with a comment explaining why it's safe).\n\nWell, to me this is a tradeoff between complexity and performance. If\nthe performance benefit outweighs the additional complexity that this\nshared ownership of keys brings along with it then I'm okay with the\ncode being intimate with each others lifetime requirements.\n\nBut as far as I can see the number of entries is determined by the group\npattern, and I expect that in most cases it's going to be quite limited.\nSo it does not feel like this should lead to all that many extra\nallocations, and thus I tend to prefer the simpler solution.\n\nBut please let me know in case you disagree with that assessment.\n\nPatrick\n"},{"id":"504287","messageId":"ZwOswYaNe74WkJHJ@pks.im","threadId":"62215","inReplyTo":"ZvsXrQadrU78rEha@nand.local","subject":"Re: [PATCH 17/23] midx-write: fix leaking buffer","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-07T09:41:21Z","receivedAt":"2024-10-07T09:41:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Sep 30, 2024 at 05:27:09PM -0400, Taylor Blau wrote:\n> On Mon, Sep 30, 2024 at 11:14:01AM +0200, Patrick Steinhardt wrote:\n> > The buffer used to compute the final MIDX name is never released. Plug\n> > this memory leak.\n> >\n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> >  midx-write.c                            | 2 ++\n> >  t/t5334-incremental-multi-pack-index.sh | 2 ++\n> >  2 files changed, 4 insertions(+)\n> >\n> > diff --git a/midx-write.c b/midx-write.c\n> > index 1ef62c4f4b..625c7d3137 100644\n> > --- a/midx-write.c\n> > +++ b/midx-write.c\n> > @@ -1445,6 +1445,8 @@ static int write_midx_internal(const char *object_dir,\n> >  \t\t\treturn -1;\n> \n> Would you also want to strbuf_release() the final_midx_name buffer here\n> as well?\n> \n> I guess the point is moot since there are a number of other finalization\n> steps that we likewise skip here by returning -1 directly instead of\n> jumping to the cleanup sub-routine.\n> \n> In the above sense I'm OK with it as-is, but it would be nice to verify\n> that this portion of the code is leak-free even when we return early\n> (e.g., because we couldn't rename() the tempfile into place).\n> \n> Of course, because final_midx_name is local to the body of this\n> conditional, I think you'd need to do something like:\n> \n>     if (ctx.incremntal) {\n>         struct strbuf final_midx_name = STRBUF_INIT;\n> \n>         /* ... */\n> \n>         get_split_midx_filename_ext(&final_midx_name, object_dir,\n>                                     midx_hash, MIDX_EXT_MIDX);\n> \n>         if (rename_tempfile(&incr, final_midx_name.buf) < 0) {\n>             result = error_errno(_(\"unable to rename new multi-pack-index layer\"));\n>             strbuf_release(&final_midx_name); /* <- here */\n>             goto cleanup;\n>         }\n> \n>         strbuf_release(&final_midx_name); /* ... <- and here */\n> \n>         keep_hashes[ctx.num_multi_pack_indexes_before] =\n>           xstrdup(hash_to_hex(midx_hash));\n>     }\n\nPotentially. Until now we never really cared all that much about\nreleasing resources in error paths, and for all I can see this leak\nisn't hit in our test suite. But it is an easy fix to make, so I can\ninclude this in case I'll have to reroll this series due to other\nreasons.\n\nPatrick\n"},{"id":"504405","messageId":"20241008085423.GA1211512@coredump.intra.peff.net","threadId":"62215","inReplyTo":"ZwOstswKzqMNNOUO@pks.im","subject":"Re: [PATCH 15/23] pseudo-merge: fix leaking strmap keys","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-08T08:54:23Z","receivedAt":"2024-10-08T08:54:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 07, 2024 at 11:41:16AM +0200, Patrick Steinhardt wrote:\n\n> > Hmm. I think I am probably splitting hairs, since a few xstrdup() calls\n> > between friends is unlikely to matter here, but... ;-)\n> > \n> > It does seem wasteful if we can avoid it. We already allocated heap\n> > space for the matches via the underlying strbuf, and we really do want\n> > to hand ownership over if we can.\n> > \n> > Would doing something like this on top of your previous patch do the\n> > trick?\n> > \n> > --- >8 ---\n> > diff --git a/pseudo-merge.c b/pseudo-merge.c\n> > index 28782a31c6..6b6605d3dc 100644\n> > --- a/pseudo-merge.c\n> > +++ b/pseudo-merge.c\n> > @@ -110,6 +110,7 @@ void pseudo_merge_group_release(struct pseudo_merge_group *group)\n> >  \t\tfree(matches->stable);\n> >  \t\tfree(matches->unstable);\n> >  \t\tfree(matches);\n> > +\t\tfree((char*)e->key);\n> >  \t}\n> >  \tstrmap_clear(&group->matches, 0);\n> > --- 8< ---\n> > \n> > That introduces an ugly const-cast, but I think it's tolerable (and may\n> > be moreso with a comment explaining why it's safe).\n> \n> Well, to me this is a tradeoff between complexity and performance. If\n> the performance benefit outweighs the additional complexity that this\n> shared ownership of keys brings along with it then I'm okay with the\n> code being intimate with each others lifetime requirements.\n> \n> But as far as I can see the number of entries is determined by the group\n> pattern, and I expect that in most cases it's going to be quite limited.\n> So it does not feel like this should lead to all that many extra\n> allocations, and thus I tend to prefer the simpler solution.\n\nI'd tend to agree with you that the allocations aren't a big deal here.\nBut I think we've run into this kind of strbuf-detaching thing before,\nand there's another pattern that is efficient without getting too\nintimate between modules. Like this (plus your change to set the\nstrmap's strdup_strings mode):\n\ndiff --git a/pseudo-merge.c b/pseudo-merge.c\nindex 10ebd9a4e9..fb1164edfa 100644\n--- a/pseudo-merge.c\n+++ b/pseudo-merge.c\n@@ -210,6 +210,7 @@ static int find_pseudo_merge_group_for_ref(const char *refname,\n \tstruct bitmap_writer *writer = _data;\n \tstruct object_id peeled;\n \tstruct commit *c;\n+\tstruct strbuf group_name = STRBUF_INIT;\n \tuint32_t i;\n \tint has_bitmap;\n \n@@ -227,10 +228,11 @@ static int find_pseudo_merge_group_for_ref(const char *refname,\n \tfor (i = 0; i < writer->pseudo_merge_groups.nr; i++) {\n \t\tstruct pseudo_merge_group *group;\n \t\tstruct pseudo_merge_matches *matches;\n-\t\tstruct strbuf group_name = STRBUF_INIT;\n \t\tregmatch_t captures[16];\n \t\tsize_t j;\n \n+\t\tstrbuf_reset(&group_name);\n+\n \t\tgroup = writer->pseudo_merge_groups.items[i].util;\n \t\tif (regexec(group->pattern, refname, ARRAY_SIZE(captures),\n \t\t\t    captures, 0))\n@@ -256,8 +258,7 @@ static int find_pseudo_merge_group_for_ref(const char *refname,\n \t\tmatches = strmap_get(&group->matches, group_name.buf);\n \t\tif (!matches) {\n \t\t\tmatches = xcalloc(1, sizeof(*matches));\n-\t\t\tstrmap_put(&group->matches, strbuf_detach(&group_name, NULL),\n-\t\t\t\t   matches);\n+\t\t\tstrmap_put(&group->matches, group_name.buf, matches);\n \t\t}\n \n \t\tif (c->date <= group->stable_threshold) {\n@@ -270,9 +271,9 @@ static int find_pseudo_merge_group_for_ref(const char *refname,\n \t\t\tmatches->unstable[matches->unstable_nr++] = c;\n \t\t}\n \n-\t\tstrbuf_release(&group_name);\n \t}\n \n+\tstrbuf_release(&group_name);\n \treturn 0;\n }\n \n\nNow we skip the duplicate allocations because we are reusing the\ngroup_name scratch buffer in the loop over and over. But wait, there's\nmore! We're actually _more_ efficient than the original which did one\nallocation per entry, because:\n\n  1. We can allocate the correct number of bytes for each name, rather\n     than using the over-estimated buffer made by strbuf.\n\n  2. In strdup_strings mode, strmap is smart enough to use a FLEXPTR to\n     stick the name inside the struct. So we've actually reduced the\n     number of allocations per entry by 1.\n\nNow possibly it is not even worth doing this optimization, because this\nis not really a hot path. But it doesn't violate any module boundaries,\nand I think the \"loop over a reusable strbuf\" trick is a general idiom\nfor solving these kinds of allocation problems. So a good thing to keep\nin our toolbox.\n\n-Peff\n"},{"id":"504521","messageId":"ZwYrfCn13aP8fC6H@pks.im","threadId":"62215","inReplyTo":"20241008085423.GA1211512@coredump.intra.peff.net","subject":"Re: [PATCH 15/23] pseudo-merge: fix leaking strmap keys","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-09T07:06:45Z","receivedAt":"2024-10-09T07:06:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Oct 08, 2024 at 04:54:23AM -0400, Jeff King wrote:\n> On Mon, Oct 07, 2024 at 11:41:16AM +0200, Patrick Steinhardt wrote:\n> \n> > > Hmm. I think I am probably splitting hairs, since a few xstrdup() calls\n> > > between friends is unlikely to matter here, but... ;-)\n> > > \n> > > It does seem wasteful if we can avoid it. We already allocated heap\n> > > space for the matches via the underlying strbuf, and we really do want\n> > > to hand ownership over if we can.\n> > > \n> > > Would doing something like this on top of your previous patch do the\n> > > trick?\n> > > \n> > > --- >8 ---\n> > > diff --git a/pseudo-merge.c b/pseudo-merge.c\n> > > index 28782a31c6..6b6605d3dc 100644\n> > > --- a/pseudo-merge.c\n> > > +++ b/pseudo-merge.c\n> > > @@ -110,6 +110,7 @@ void pseudo_merge_group_release(struct pseudo_merge_group *group)\n> > >  \t\tfree(matches->stable);\n> > >  \t\tfree(matches->unstable);\n> > >  \t\tfree(matches);\n> > > +\t\tfree((char*)e->key);\n> > >  \t}\n> > >  \tstrmap_clear(&group->matches, 0);\n> > > --- 8< ---\n> > > \n> > > That introduces an ugly const-cast, but I think it's tolerable (and may\n> > > be moreso with a comment explaining why it's safe).\n> > \n> > Well, to me this is a tradeoff between complexity and performance. If\n> > the performance benefit outweighs the additional complexity that this\n> > shared ownership of keys brings along with it then I'm okay with the\n> > code being intimate with each others lifetime requirements.\n> > \n> > But as far as I can see the number of entries is determined by the group\n> > pattern, and I expect that in most cases it's going to be quite limited.\n> > So it does not feel like this should lead to all that many extra\n> > allocations, and thus I tend to prefer the simpler solution.\n> \n> I'd tend to agree with you that the allocations aren't a big deal here.\n> But I think we've run into this kind of strbuf-detaching thing before,\n> and there's another pattern that is efficient without getting too\n> intimate between modules. Like this (plus your change to set the\n> strmap's strdup_strings mode):\n> \n> diff --git a/pseudo-merge.c b/pseudo-merge.c\n> index 10ebd9a4e9..fb1164edfa 100644\n> --- a/pseudo-merge.c\n> +++ b/pseudo-merge.c\n> @@ -210,6 +210,7 @@ static int find_pseudo_merge_group_for_ref(const char *refname,\n>  \tstruct bitmap_writer *writer = _data;\n>  \tstruct object_id peeled;\n>  \tstruct commit *c;\n> +\tstruct strbuf group_name = STRBUF_INIT;\n>  \tuint32_t i;\n>  \tint has_bitmap;\n>  \n> @@ -227,10 +228,11 @@ static int find_pseudo_merge_group_for_ref(const char *refname,\n>  \tfor (i = 0; i < writer->pseudo_merge_groups.nr; i++) {\n>  \t\tstruct pseudo_merge_group *group;\n>  \t\tstruct pseudo_merge_matches *matches;\n> -\t\tstruct strbuf group_name = STRBUF_INIT;\n>  \t\tregmatch_t captures[16];\n>  \t\tsize_t j;\n>  \n> +\t\tstrbuf_reset(&group_name);\n> +\n>  \t\tgroup = writer->pseudo_merge_groups.items[i].util;\n>  \t\tif (regexec(group->pattern, refname, ARRAY_SIZE(captures),\n>  \t\t\t    captures, 0))\n> @@ -256,8 +258,7 @@ static int find_pseudo_merge_group_for_ref(const char *refname,\n>  \t\tmatches = strmap_get(&group->matches, group_name.buf);\n>  \t\tif (!matches) {\n>  \t\t\tmatches = xcalloc(1, sizeof(*matches));\n> -\t\t\tstrmap_put(&group->matches, strbuf_detach(&group_name, NULL),\n> -\t\t\t\t   matches);\n> +\t\t\tstrmap_put(&group->matches, group_name.buf, matches);\n>  \t\t}\n>  \n>  \t\tif (c->date <= group->stable_threshold) {\n> @@ -270,9 +271,9 @@ static int find_pseudo_merge_group_for_ref(const char *refname,\n>  \t\t\tmatches->unstable[matches->unstable_nr++] = c;\n>  \t\t}\n>  \n> -\t\tstrbuf_release(&group_name);\n>  \t}\n>  \n> +\tstrbuf_release(&group_name);\n>  \treturn 0;\n>  }\n>  \n> \n> Now we skip the duplicate allocations because we are reusing the\n> group_name scratch buffer in the loop over and over. But wait, there's\n> more! We're actually _more_ efficient than the original which did one\n> allocation per entry, because:\n> \n>   1. We can allocate the correct number of bytes for each name, rather\n>      than using the over-estimated buffer made by strbuf.\n> \n>   2. In strdup_strings mode, strmap is smart enough to use a FLEXPTR to\n>      stick the name inside the struct. So we've actually reduced the\n>      number of allocations per entry by 1.\n> \n> Now possibly it is not even worth doing this optimization, because this\n> is not really a hot path. But it doesn't violate any module boundaries,\n> and I think the \"loop over a reusable strbuf\" trick is a general idiom\n> for solving these kinds of allocation problems. So a good thing to keep\n> in our toolbox.\n\nNice, thanks for digging! I see that Junio has already merged the topic\nto `next` though -- is this a mere \"Let this cook before the next cycle\nstarts\" or will it stay in next?\n\nIf the latter then I'll leave this as-is, otherwise I'll send a revised\nversion to make this a bit more efficient.\n\nThanks!\n\nPatrick\n"}]}