{"thread":{"id":"62002","subject":"[PATCH 00/22] Memory leak fixes (pt.6)","startedAt":"2024-08-26T07:21:36Z","lastAt":"2024-09-12T20:30:01Z","messageCount":70,"participants":["Patrick Steinhardt","Junio C Hamano","Calvin Wan","Josh Steadmon","Toon claes","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":22},"messages":[{"id":"501642","messageId":"cover.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":null,"subject":"[PATCH 00/22] Memory leak fixes (pt.6)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:21:31Z","receivedAt":"2024-08-26T07:21:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nhere's another round of memory leak fixes. This is the second-last round\nof \"general\" leak fixes -- so once this and the next series are merged,\nI have made a full pass over all failing test suites. There will still\nbe a bunch of memory leaks, namely ~86 failing test suites.\n\nI have documented a bunch of remaining leaks at [1]. Most of them are a\nbit more involved to fix, so they will likely require separate series.\nThese issues relate to:\n\n - Simplifying commit history causes us to rewrite their parents, but\n we don't free the old parents in some cases. Hit e.g. by t6012.\n\n - The describe atom infrastructure as used by git-for-each-ref(1) is\n leaky. Hit e.g. by t6300, t6302.\n\n - Breaking rewrites causes memory leaks. Hit e.g. by t7524.\n\n - Cloning repositories causes us to re-initialize `the_repository`.\n This surfaces lifetime management issues with the `struct remote`\n that we use, which reuses storage held by `the_repository`. Hit e.g.\n by t5558.\n\n - The GPG interface is leaky. Hit e.g. by t7510, t7528.\n\nIf you want to do some puzzling, please feel free to pick up any of\nthese leaks :) I won't start working on them before I have the final\npart of memory leak fixes merged.\n\nThis patch series builds on top of 6a09c36371 (The eighth batch,\n2024-08-23) with Junio's ps/leakfixes-part-5 at 13b23d2da5 (transport:\nfix leaking negotiation tips, 2024-08-22) merged into it.\n\nPatrick\n\n[1]: https://gitlab.com/groups/gitlab-org/-/epics/14943\n\nPatrick Steinhardt (22):\n t/test-lib: allow skipping leak checks for passing tests\n fetch-pack: fix memory leaks on fetch negotiation\n send-pack: fix leaking common object IDs\n builtin/push: fix leaking refspec query result\n upload-pack: fix leaking child process data on reachability checks\n submodule: fix leaking fetch task data\n builtin/submodule--helper: fix leaking refs on push-check\n remote: fix leaking tracking refs\n remote: fix leak in reachability check of a remote-tracking ref\n send-pack: fix leaking push cert nonce\n gpg-interface: fix misdesigned signing key interfaces\n object: clear grafts when clearing parsed object pool\n shallow: free grafts when unregistering them\n shallow: fix leaking members of `struct shallow_info`\n negotiator/skipping: fix leaking commit entries\n builtin/repack: fix leaking line buffer when packing promisors\n builtin/pack-objects: plug leaking list of keep-packs\n builtin/grep: fix leaking object context\n builtin/fmt-merge-msg: fix leaking buffers\n match-trees: fix leaking prefixes in `shift_tree()`\n merge-ort: fix two leaks when handling directory rename modifications\n builtin/repack: fix leaking keep-pack list\n\n builtin/fmt-merge-msg.c | 2 ++\n builtin/grep.c | 1 +\n builtin/pack-objects.c | 1 +\n builtin/push.c | 8 +++--\n builtin/repack.c | 3 ++\n builtin/submodule--helper.c | 2 ++\n builtin/tag.c | 3 +-\n commit.c | 23 ++++--------\n commit.h | 3 +-\n fetch-pack.c | 3 ++\n gpg-interface.c | 26 ++++++++------\n gpg-interface.h | 4 +--\n match-trees.c | 10 ++++--\n merge-ort.c | 4 ++-\n negotiator/skipping.c | 7 ++--\n object.c | 14 +++++++-\n object.h | 4 ++-\n remote.c | 6 +++-\n repository.c | 2 +-\n send-pack.c | 52 ++++++++++++++++++----------\n shallow.c | 15 ++++++--\n submodule.c | 2 ++\n t/t5516-fetch-push.sh | 1 +\n t/t5526-fetch-submodules.sh | 1 +\n t/t5531-deep-submodule-push.sh | 1 +\n t/t5533-push-cas.sh | 1 +\n t/t5534-push-signed.sh | 1 +\n t/t5537-fetch-shallow.sh | 1 +\n t/t5538-push-shallow.sh | 1 +\n t/t5549-fetch-push-http.sh | 1 +\n t/t5552-skipping-fetch-negotiator.sh | 2 ++\n t/t5616-partial-clone.sh | 1 +\n t/t6132-pathspec-exclude.sh | 1 +\n t/t6135-pathspec-with-attrs.sh | 2 ++\n t/t6200-fmt-merge-msg.sh | 1 +\n t/t6409-merge-subtree.sh | 1 +\n t/t6423-merge-rename-directories.sh | 1 +\n t/t6500-gc.sh | 1 +\n t/t7703-repack-geometric.sh | 1 +\n t/test-lib.sh | 11 +++++-\n upload-pack.c | 22 ++++++++----\n 41 files changed, 175 insertions(+), 72 deletions(-)\n\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501643","messageId":"7c158acadf40b44edb3cf186860a3f60818f76f0.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 01/22] t/test-lib: allow skipping leak checks for passing tests","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:21:34Z","receivedAt":"2024-08-26T07:21:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"With `GIT_TEST_PASSING_SANITIZE_LEAK=check`, one can double check\nwhether a memory leak fix caused some test suites to become leak free.\nIt is somewhat slow to execute though because it runs all of our test\nsuites with the leak sanitizer enabled. It is also pointless in most\ncases, because the only test suites that need to be checked are those\nwhich _aren't_ yet marked with `TEST_PASSES_SANITIZE_LEAK=true`.\n\nIntroduce a new value \"check-failing\". If set, we will only check those\ntests which are not yet marked as leak free.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/test-lib.sh | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 54247604cbc..64bd36531c1 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1558,8 +1558,16 @@ then\n \t\tpasses_sanitize_leak=t\n \tfi\n \n-\tif test \"$GIT_TEST_PASSING_SANITIZE_LEAK\" = \"check\"\n+\tif test \"$GIT_TEST_PASSING_SANITIZE_LEAK\" = \"check\" ||\n+\t test \"$GIT_TEST_PASSING_SANITIZE_LEAK\" = \"check-failing\"\n \tthen\n+\t\tif test \"$GIT_TEST_PASSING_SANITIZE_LEAK\" = \"check-failing\" &&\n+\t\t test -n \"$passes_sanitize_leak\"\n+\t\tthen\n+\t\t\tskip_all=\"skipping leak-free $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=check-failing\"\n+\t\t\ttest_done\n+\t\tfi\n+\n \t\tsanitize_leak_check=t\n \t\tif test -n \"$invert_exit_code\"\n \t\tthen\n@@ -1597,6 +1605,7 @@ then\n \texport LSAN_OPTIONS\n \n elif test \"$GIT_TEST_PASSING_SANITIZE_LEAK\" = \"check\" ||\n+ test \"$GIT_TEST_PASSING_SANITIZE_LEAK\" = \"check-failing\" ||\n test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false\n then\n \tBAIL_OUT_ENV_NEEDS_SANITIZE_LEAK \"GIT_TEST_PASSING_SANITIZE_LEAK=true\"\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501644","messageId":"33bc728a3450ea087ca786e9077c67a4cac0596a.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 02/22] fetch-pack: fix memory leaks on fetch negotiation","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:21:37Z","receivedAt":"2024-08-26T07:21:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We leak both the `nt_object_array` and `negotiator` structures in\n`negotiate_using_fetch()`. Plug both of these leaks.\n\nThese leaks were exposed by t5516, but fixing them is not sufficient to\nmake the whole test suite leak free.\n\nSigned-off-by: Patrick Steinhardt \n---\n fetch-pack.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 58b4581ad80..0ed82feda14 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -2227,7 +2227,10 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,\n \ttrace2_region_leave(\"fetch-pack\", \"negotiate_using_fetch\", the_repository);\n \ttrace2_data_intmax(\"negotiate_using_fetch\", the_repository,\n \t\t\t \"total_rounds\", negotiation_round);\n+\n \tclear_common_flag(acked_commits);\n+\tobject_array_clear(&nt_object_array);\n+\tnegotiator.release(&negotiator);\n \tstrbuf_release(&req_buf);\n }\n \n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501645","messageId":"a13db9777f096e2000bfecc3eaf900df7e58ddb8.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 03/22] send-pack: fix leaking common object IDs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:21:39Z","receivedAt":"2024-08-26T07:21:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're leaking the array of common object IDs in `send_pack()`. Fix this\nby creating a common exit path where we free the leaking data. While at\nit, unify some other cleanups now that we have a central place to put\nthem.\n\nSigned-off-by: Patrick Steinhardt \n---\n send-pack.c | 34 ++++++++++++++++++++++------------\n t/t5549-fetch-push-http.sh | 1 +\n 2 files changed, 23 insertions(+), 12 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex fa2f5eec17b..b224ef9fc5e 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -508,7 +508,8 @@ int send_pack(struct send_pack_args *args,\n \tif (!remote_refs) {\n \t\tfprintf(stderr, \"No refs in common and none specified; doing nothing.\\n\"\n \t\t\t\"Perhaps you should specify a branch.\\n\");\n-\t\treturn 0;\n+\t\tret = 0;\n+\t\tgoto out;\n \t}\n \n \tgit_config_get_bool(\"push.negotiate\", &push_negotiate);\n@@ -615,12 +616,11 @@ int send_pack(struct send_pack_args *args,\n \t\t\t * atomically, abort the whole operation.\n \t\t\t */\n \t\t\tif (use_atomic) {\n-\t\t\t\tstrbuf_release(&req_buf);\n-\t\t\t\tstrbuf_release(&cap_buf);\n \t\t\t\treject_atomic_push(remote_refs, args->send_mirror);\n \t\t\t\terror(\"atomic push failed for ref %s. status: %d\\n\",\n \t\t\t\t ref->name, ref->status);\n-\t\t\t\treturn args->porcelain ? 0 : -1;\n+\t\t\t\tret = args->porcelain ? 0 : -1;\n+\t\t\t\tgoto out;\n \t\t\t}\n \t\t\t/* else fallthrough */\n \t\tdefault:\n@@ -682,8 +682,6 @@ int send_pack(struct send_pack_args *args,\n \t\twrite_or_die(out, req_buf.buf, req_buf.len);\n \t\tpacket_flush(out);\n \t}\n-\tstrbuf_release(&req_buf);\n-\tstrbuf_release(&cap_buf);\n \n \tif (use_sideband && cmds_sent) {\n \t\tmemset(&demux, 0, sizeof(demux));\n@@ -721,7 +719,9 @@ int send_pack(struct send_pack_args *args,\n \t\t\t\tfinish_async(&demux);\n \t\t\t}\n \t\t\tfd[1] = -1;\n-\t\t\treturn -1;\n+\n+\t\t\tret = -1;\n+\t\t\tgoto out;\n \t\t}\n \t\tif (!args->stateless_rpc)\n \t\t\t/* Closed by pack_objects() via start_command() */\n@@ -746,10 +746,12 @@ int send_pack(struct send_pack_args *args,\n \t}\n \n \tif (ret < 0)\n-\t\treturn ret;\n+\t\tgoto out;\n \n-\tif (args->porcelain)\n-\t\treturn 0;\n+\tif (args->porcelain) {\n+\t\tret = 0;\n+\t\tgoto out;\n+\t}\n \n \tfor (ref = remote_refs; ref; ref = ref->next) {\n \t\tswitch (ref->status) {\n@@ -758,8 +760,16 @@ int send_pack(struct send_pack_args *args,\n \t\tcase REF_STATUS_OK:\n \t\t\tbreak;\n \t\tdefault:\n-\t\t\treturn -1;\n+\t\t\tret = -1;\n+\t\t\tgoto out;\n \t\t}\n \t}\n-\treturn 0;\n+\n+\tret = 0;\n+\n+out:\n+\toid_array_clear(&commons);\n+\tstrbuf_release(&req_buf);\n+\tstrbuf_release(&cap_buf);\n+\treturn ret;\n }\ndiff --git a/t/t5549-fetch-push-http.sh b/t/t5549-fetch-push-http.sh\nindex 2cdebcb7356..6377fb6d993 100755\n--- a/t/t5549-fetch-push-http.sh\n+++ b/t/t5549-fetch-push-http.sh\n@@ -5,6 +5,7 @@ test_description='fetch/push functionality using the HTTP protocol'\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-httpd.sh\n start_httpd\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501646","messageId":"92fc97b3db86bb0bdf610a2f76c03a96a99bfe8d.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 04/22] builtin/push: fix leaking refspec query result","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:21:45Z","receivedAt":"2024-08-26T07:21:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When appending a refspec via `refspec_append_mapped()` we leak the\nresult of `query_refspecs()`. The overall logic around refspec queries\nis quite weird, as callers are expected to either set the `src` or `dst`\npointers, and then the (allocated) result will be in the respective\nother struct member.\n\nAs we have the `src` member set, plugging the memory leak is thus as\neasy as just freeing the `dst` member. While at it, use designated\ninitializers to initialize the structure.\n\nThis leak was exposed by t5516, but fixing it is not sufficient to make\nthe whole test suite leak free.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/push.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 7a67398124f..0b123eb9c1e 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -72,13 +72,15 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,\n \tconst char *branch_name;\n \n \tif (remote->push.nr) {\n-\t\tstruct refspec_item query;\n-\t\tmemset(&query, 0, sizeof(struct refspec_item));\n-\t\tquery.src = matched->name;\n+\t\tstruct refspec_item query = {\n+\t\t\t.src = matched->name,\n+\t\t};\n+\n \t\tif (!query_refspecs(&remote->push, &query) && query.dst) {\n \t\t\trefspec_appendf(refspec, \"%s%s:%s\",\n \t\t\t\t\tquery.force ? \"+\" : \"\",\n \t\t\t\t\tquery.src, query.dst);\n+\t\t\tfree(query.dst);\n \t\t\treturn;\n \t\t}\n \t}\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501647","messageId":"77023421e189aee6837cea9b25841ea258bc06e3.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 05/22] upload-pack: fix leaking child process data on reachability checks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:21:48Z","receivedAt":"2024-08-26T07:21:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We spawn a git-rev-list(1) command to perform reachability checks in\n\"upload-pack.c\". We do not release memory associated with the process\nin error cases though, thus leaking memory.\n\nFix these by calling `child_process_clear()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5516-fetch-push.sh | 1 +\n upload-pack.c | 22 ++++++++++++++++------\n 2 files changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 9d693eb57f7..331778bd42c 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -19,6 +19,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 D=$(pwd)\ndiff --git a/upload-pack.c b/upload-pack.c\nindex f03ba3e98be..c84c3c3b1f5 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -709,10 +709,13 @@ static int get_reachable_list(struct upload_pack_data *data,\n \tstruct object *o;\n \tchar namebuf[GIT_MAX_HEXSZ + 2]; /* ^ + hash + LF */\n \tconst unsigned hexsz = the_hash_algo->hexsz;\n+\tint ret;\n \n \tif (do_reachable_revlist(&cmd, &data->shallows, reachable,\n-\t\t\t\t data->allow_uor) < 0)\n-\t\treturn -1;\n+\t\t\t\t data->allow_uor) < 0) {\n+\t\tret = -1;\n+\t\tgoto out;\n+\t}\n \n \twhile ((i = read_in_full(cmd.out, namebuf, hexsz + 1)) == hexsz + 1) {\n \t\tstruct object_id oid;\n@@ -736,10 +739,16 @@ static int get_reachable_list(struct upload_pack_data *data,\n \t}\n \tclose(cmd.out);\n \n-\tif (finish_command(&cmd))\n-\t\treturn -1;\n+\tif (finish_command(&cmd)) {\n+\t\tret = -1;\n+\t\tgoto out;\n+\t}\n \n-\treturn 0;\n+\tret = 0;\n+\n+out:\n+\tchild_process_clear(&cmd);\n+\treturn ret;\n }\n \n static int has_unreachable(struct object_array *src, enum allow_uor allow_uor)\n@@ -749,7 +758,7 @@ static int has_unreachable(struct object_array *src, enum allow_uor allow_uor)\n \tint i;\n \n \tif (do_reachable_revlist(&cmd, src, NULL, allow_uor) < 0)\n-\t\treturn 1;\n+\t\tgoto error;\n \n \t/*\n \t * The commits out of the rev-list are not ancestors of\n@@ -775,6 +784,7 @@ static int has_unreachable(struct object_array *src, enum allow_uor allow_uor)\n error:\n \tif (cmd.out >= 0)\n \t\tclose(cmd.out);\n+\tchild_process_clear(&cmd);\n \treturn 1;\n }\n \n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501648","messageId":"b4b65c3783ca496a62aa4a0ac3d82deca4793062.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 06/22] submodule: fix leaking fetch task data","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:21:50Z","receivedAt":"2024-08-26T07:21:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `submodule_parallel_fetch` structure contains various data\nstructures that we use to set up parallel fetches of submodules. We do\nnot free some of its data though, causing memory leaks. Plug those.\n\nSigned-off-by: Patrick Steinhardt \n---\n submodule.c | 2 ++\n t/t5526-fetch-submodules.sh | 1 +\n 2 files changed, 3 insertions(+)\n\ndiff --git a/submodule.c b/submodule.c\nindex 97516b0fec1..97d0d47b561 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1883,6 +1883,8 @@ int fetch_submodules(struct repository *r,\n out:\n \tfree_submodules_data(&spf.changed_submodule_names);\n \tstring_list_clear(&spf.seen_submodule_names, 0);\n+\tstrbuf_release(&spf.submodules_with_errors);\n+\tfree(spf.oid_fetch_tasks);\n \treturn spf.result;\n }\n \ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 5e566205ba4..2cfb5bd6bb1 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -6,6 +6,7 @@ test_description='Recursive \"git fetch\" for submodules'\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 pwd=$(pwd)\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501649","messageId":"e3d1ac1712fff01d74d4c6ffdc1c04fddb19404f.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 07/22] builtin/submodule--helper: fix leaking refs on push-check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:21:53Z","receivedAt":"2024-08-26T07:21:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In the push-check subcommand of the submodule helper we acquire a list\nof local refs, but never free that list. Fix this memory leak.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/submodule--helper.c | 2 ++\n t/t5531-deep-submodule-push.sh | 1 +\n 2 files changed, 3 insertions(+)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 85fb23dee84..642a0edabf0 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2958,7 +2958,9 @@ static int push_check(int argc, const char **argv, const char *prefix UNUSED)\n \t\t\t\t rs->src);\n \t\t\t}\n \t\t}\n+\n \t\trefspec_clear(&refspec);\n+\t\tfree_refs(local_refs);\n \t}\n \tfree(head);\n \ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nindex f3fff557447..135823630a3 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.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 test_expect_success setup '\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501650","messageId":"7fafcc53d23672bb0339c44ea3874b3628785f08.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 08/22] remote: fix leaking tracking refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:21:56Z","receivedAt":"2024-08-26T07:21:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When computing the remote tracking ref we cause two memory leaks:\n\n - We leak when `remote_tracking()` fails.\n\n - We leak when the call to `remote_tracking()` succeeds and sets\n `ref->tracking_ref()`.\n\nFix both of these leaks.\n\nSigned-off-by: Patrick Steinhardt \n---\n remote.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex 8f3dee13186..240311619ab 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1123,6 +1123,7 @@ void free_one_ref(struct ref *ref)\n \t\treturn;\n \tfree_one_ref(ref->peer_ref);\n \tfree(ref->remote_status);\n+\tfree(ref->tracking_ref);\n \tfree(ref->symref);\n \tfree(ref);\n }\n@@ -2620,8 +2621,10 @@ static int remote_tracking(struct remote *remote, const char *refname,\n \tdst = apply_refspecs(&remote->fetch, refname);\n \tif (!dst)\n \t\treturn -1; /* no tracking ref for refname at remote */\n-\tif (refs_read_ref(get_main_ref_store(the_repository), dst, oid))\n+\tif (refs_read_ref(get_main_ref_store(the_repository), dst, oid)) {\n+\t\tfree(dst);\n \t\treturn -1; /* we know what the tracking ref is but we cannot read it */\n+\t}\n \n \t*dst_refname = dst;\n \treturn 0;\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501651","messageId":"1446e42f0bab9b31768305283d51b0f6d1df9b48.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 09/22] remote: fix leak in reachability check of a remote-tracking ref","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:21:59Z","receivedAt":"2024-08-26T07:22:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `check_if_includes_upstream()` we retrieve the local ref\ncorresponding to a remote-tracking ref we want to check reachability\nfor. We never free that local ref and thus cause a memory leak. Fix\nthis.\n\nSigned-off-by: Patrick Steinhardt \n---\n remote.c | 1 +\n t/t5533-push-cas.sh | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/remote.c b/remote.c\nindex 240311619ab..bff54046b2c 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -2774,6 +2774,7 @@ static void check_if_includes_upstream(struct ref *remote)\n \n \tif (is_reachable_in_reflog(local->name, remote) <= 0)\n \t\tremote->unreachable = 1;\n+\tfree_one_ref(local);\n }\n \n static void apply_cas(struct push_cas_option *cas,\ndiff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh\nindex cba26a872dd..6365d99777e 100755\n--- a/t/t5533-push-cas.sh\n+++ b/t/t5533-push-cas.sh\n@@ -5,6 +5,7 @@ test_description='compare & swap push force/delete safety'\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 setup_srcdst_basic () {\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501652","messageId":"138a5ded35a43d3aeaa5058ba316a45b7b50b9ef.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 10/22] send-pack: fix leaking push cert nonce","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:22:04Z","receivedAt":"2024-08-26T07:22:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When retrieving the push cert nonce from the server, we first store the\nconstant returned by `server_feature_value()` and then, if the nonce is\nvalid, we duplicate the nonce memory to extend its lifetime. We never\nfree the latter and thus cause a memory leak.\n\nFix this by storing the limited-lifetime nonce into a scope-local\nvariable such that the long-lived, allocated nonce can be easily freed\nwithout having to cast away its constness.\n\nThis leak was exposed by t5534, but fixing it is not sufficient to make\nthe whole test suite leak free.\n\nSigned-off-by: Patrick Steinhardt \n---\n send-pack.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex b224ef9fc5e..c37f6ab3c07 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -501,7 +501,7 @@ int send_pack(struct send_pack_args *args,\n \tunsigned cmds_sent = 0;\n \tint ret;\n \tstruct async demux;\n-\tconst char *push_cert_nonce = NULL;\n+\tchar *push_cert_nonce = NULL;\n \tstruct packet_reader reader;\n \tint use_bitmaps;\n \n@@ -550,10 +550,11 @@ int send_pack(struct send_pack_args *args,\n \n \tif (args->push_cert != SEND_PACK_PUSH_CERT_NEVER) {\n \t\tsize_t len;\n-\t\tpush_cert_nonce = server_feature_value(\"push-cert\", &len);\n-\t\tif (push_cert_nonce) {\n-\t\t\treject_invalid_nonce(push_cert_nonce, len);\n-\t\t\tpush_cert_nonce = xmemdupz(push_cert_nonce, len);\n+\t\tconst char *nonce = server_feature_value(\"push-cert\", &len);\n+\n+\t\tif (nonce) {\n+\t\t\treject_invalid_nonce(nonce, len);\n+\t\t\tpush_cert_nonce = xmemdupz(nonce, len);\n \t\t} else if (args->push_cert == SEND_PACK_PUSH_CERT_ALWAYS) {\n \t\t\tdie(_(\"the receiving end does not support --signed push\"));\n \t\t} else if (args->push_cert == SEND_PACK_PUSH_CERT_IF_ASKED) {\n@@ -771,5 +772,6 @@ int send_pack(struct send_pack_args *args,\n \toid_array_clear(&commons);\n \tstrbuf_release(&req_buf);\n \tstrbuf_release(&cap_buf);\n+\tfree(push_cert_nonce);\n \treturn ret;\n }\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501653","messageId":"a1fca8104b345bff4005fcd7e4c8edb67424fd20.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 11/22] gpg-interface: fix misdesigned signing key interfaces","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:22:07Z","receivedAt":"2024-08-26T07:22:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The interfaces to retrieve signing keys and their IDs are misdesigned as\nthey return string constants even though they indeed allocate memory,\nwhich leads to memory leaks. Refactor the code to instead always return\nallocated strings and let the callers free them accordingly.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/tag.c | 3 ++-\n commit.c | 9 ++++++---\n gpg-interface.c | 26 +++++++++++++++-----------\n gpg-interface.h | 4 ++--\n send-pack.c | 6 ++++--\n t/t5534-push-signed.sh | 1 +\n 6 files changed, 30 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex a1fb218512c..ab3b500543d 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -160,7 +160,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid,\n \tconst struct git_hash_algo *compat = the_repository->compat_hash_algo;\n \tstruct strbuf sig = STRBUF_INIT, compat_sig = STRBUF_INIT;\n \tstruct strbuf compat_buf = STRBUF_INIT;\n-\tconst char *keyid = get_signing_key();\n+\tchar *keyid = get_signing_key();\n \tint ret = -1;\n \n \tif (sign_buffer(buffer, &sig, keyid))\n@@ -190,6 +190,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid,\n \tstrbuf_release(&sig);\n \tstrbuf_release(&compat_sig);\n \tstrbuf_release(&compat_buf);\n+\tfree(keyid);\n \treturn ret;\n }\n \ndiff --git a/commit.c b/commit.c\nindex 24ab5c1b509..ec9efc189d5 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1150,11 +1150,14 @@ int add_header_signature(struct strbuf *buf, struct strbuf *sig, const struct gi\n \n static int sign_commit_to_strbuf(struct strbuf *sig, struct strbuf *buf, const char *keyid)\n {\n+\tchar *keyid_to_free = NULL;\n+\tint ret = 0;\n \tif (!keyid || !*keyid)\n-\t\tkeyid = get_signing_key();\n+\t\tkeyid = keyid_to_free = get_signing_key();\n \tif (sign_buffer(buf, sig, keyid))\n-\t\treturn -1;\n-\treturn 0;\n+\t\tret = -1;\n+\tfree(keyid_to_free);\n+\treturn ret;\n }\n \n int parse_signed_commit(const struct commit *commit,\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 6587085cd19..cf6126b5aa0 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -45,8 +45,8 @@ struct gpg_format {\n \t\t\t\t size_t signature_size);\n \tint (*sign_buffer)(struct strbuf *buffer, struct strbuf *signature,\n \t\t\t const char *signing_key);\n-\tconst char *(*get_default_key)(void);\n-\tconst char *(*get_key_id)(void);\n+\tchar *(*get_default_key)(void);\n+\tchar *(*get_key_id)(void);\n };\n \n static const char *openpgp_verify_args[] = {\n@@ -86,9 +86,9 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,\n static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n \t\t\t const char *signing_key);\n \n-static const char *get_default_ssh_signing_key(void);\n+static char *get_default_ssh_signing_key(void);\n \n-static const char *get_ssh_key_id(void);\n+static char *get_ssh_key_id(void);\n \n static struct gpg_format gpg_format[] = {\n \t{\n@@ -847,7 +847,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key)\n }\n \n /* Returns the first public key from an ssh-agent to use for signing */\n-static const char *get_default_ssh_signing_key(void)\n+static char *get_default_ssh_signing_key(void)\n {\n \tstruct child_process ssh_default_key = CHILD_PROCESS_INIT;\n \tint ret = -1;\n@@ -899,12 +899,16 @@ static const char *get_default_ssh_signing_key(void)\n \treturn default_key;\n }\n \n-static const char *get_ssh_key_id(void) {\n-\treturn get_ssh_key_fingerprint(get_signing_key());\n+static char *get_ssh_key_id(void)\n+{\n+\tchar *signing_key = get_signing_key();\n+\tchar *key_id = get_ssh_key_fingerprint(signing_key);\n+\tfree(signing_key);\n+\treturn key_id;\n }\n \n /* Returns a textual but unique representation of the signing key */\n-const char *get_signing_key_id(void)\n+char *get_signing_key_id(void)\n {\n \tgpg_interface_lazy_init();\n \n@@ -916,17 +920,17 @@ const char *get_signing_key_id(void)\n \treturn get_signing_key();\n }\n \n-const char *get_signing_key(void)\n+char *get_signing_key(void)\n {\n \tgpg_interface_lazy_init();\n \n \tif (configured_signing_key)\n-\t\treturn configured_signing_key;\n+\t\treturn xstrdup(configured_signing_key);\n \tif (use_format->get_default_key) {\n \t\treturn use_format->get_default_key();\n \t}\n \n-\treturn git_committer_info(IDENT_STRICT | IDENT_NO_DATE);\n+\treturn xstrdup(git_committer_info(IDENT_STRICT | IDENT_NO_DATE));\n }\n \n const char *gpg_trust_level_to_str(enum signature_trust_level level)\ndiff --git a/gpg-interface.h b/gpg-interface.h\nindex 7cd98161f7f..e09f12e8d04 100644\n--- a/gpg-interface.h\n+++ b/gpg-interface.h\n@@ -80,13 +80,13 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature,\n const char *gpg_trust_level_to_str(enum signature_trust_level level);\n \n void set_signing_key(const char *);\n-const char *get_signing_key(void);\n+char *get_signing_key(void);\n \n /*\n * Returns a textual unique representation of the signing key in use\n * Either a GPG KeyID or a SSH Key Fingerprint\n */\n-const char *get_signing_key_id(void);\n+char *get_signing_key_id(void);\n int check_signature(struct signature_check *sigc,\n \t\t const char *signature, size_t slen);\n void print_signature_buffer(const struct signature_check *sigc,\ndiff --git a/send-pack.c b/send-pack.c\nindex c37f6ab3c07..31a62e6a98c 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -348,7 +348,8 @@ static int generate_push_cert(struct strbuf *req_buf,\n {\n \tconst struct ref *ref;\n \tstruct string_list_item *item;\n-\tchar *signing_key_id = xstrdup(get_signing_key_id());\n+\tchar *signing_key_id = get_signing_key_id();\n+\tchar *signing_key = get_signing_key();\n \tconst char *cp, *np;\n \tstruct strbuf cert = STRBUF_INIT;\n \tint update_seen = 0;\n@@ -381,7 +382,7 @@ static int generate_push_cert(struct strbuf *req_buf,\n \tif (!update_seen)\n \t\tgoto free_return;\n \n-\tif (sign_buffer(&cert, &cert, get_signing_key()))\n+\tif (sign_buffer(&cert, &cert, signing_key))\n \t\tdie(_(\"failed to sign the push certificate\"));\n \n \tpacket_buf_write(req_buf, \"push-cert%c%s\", 0, cap_string);\n@@ -394,6 +395,7 @@ static int generate_push_cert(struct strbuf *req_buf,\n \n free_return:\n \tfree(signing_key_id);\n+\tfree(signing_key);\n \tstrbuf_release(&cert);\n \treturn update_seen;\n }\ndiff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh\nindex c91a62b77af..d43aee0c327 100755\n--- a/t/t5534-push-signed.sh\n+++ b/t/t5534-push-signed.sh\n@@ -5,6 +5,7 @@ test_description='signed push'\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 \n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501654","messageId":"922b8640a55c74e91ce69f6976d20f948be55765.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 12/22] object: clear grafts when clearing parsed object pool","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:22:10Z","receivedAt":"2024-08-26T07:22:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We do not clear grafts part of the parsed object pool when clearing the\npool itself, which can lead to memory leaks when a repository is being\ncleared.\n\nFix this by moving `reset_commit_grafts()` into \"object.c\" and making it\npart of the `struct parsed_object_pool` interface such that we can call\nit from `parsed_object_pool_clear()`. Adapt `parsed_object_pool_new()`\nto take and store a reference to its owning repository, which is needed\nby `unparse_commit()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n commit.c | 14 +-------------\n commit.h | 3 ++-\n object.c | 14 +++++++++++++-\n object.h | 4 +++-\n repository.c | 2 +-\n shallow.c | 2 +-\n 6 files changed, 21 insertions(+), 18 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex ec9efc189d5..bbef0e81c65 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -177,7 +177,7 @@ int commit_graft_pos(struct repository *r, const struct object_id *oid)\n \t\t commit_graft_oid_access);\n }\n \n-static void unparse_commit(struct repository *r, const struct object_id *oid)\n+void unparse_commit(struct repository *r, const struct object_id *oid)\n {\n \tstruct commit *c = lookup_commit(r, oid);\n \n@@ -318,18 +318,6 @@ int for_each_commit_graft(each_commit_graft_fn fn, void *cb_data)\n \treturn ret;\n }\n \n-void reset_commit_grafts(struct repository *r)\n-{\n-\tint i;\n-\n-\tfor (i = 0; i < r->parsed_objects->grafts_nr; i++) {\n-\t\tunparse_commit(r, &r->parsed_objects->grafts[i]->oid);\n-\t\tfree(r->parsed_objects->grafts[i]);\n-\t}\n-\tr->parsed_objects->grafts_nr = 0;\n-\tr->parsed_objects->commit_graft_prepared = 0;\n-}\n-\n struct commit_buffer {\n \tvoid *buffer;\n \tunsigned long size;\ndiff --git a/commit.h b/commit.h\nindex d62b1d93f95..5ba0f77b1eb 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -108,6 +108,8 @@ static inline int repo_parse_commit_no_graph(struct repository *r,\n \n void parse_commit_or_die(struct commit *item);\n \n+void unparse_commit(struct repository *r, const struct object_id *oid);\n+\n struct buffer_slab;\n struct buffer_slab *allocate_commit_buffer_slab(void);\n void free_commit_buffer_slab(struct buffer_slab *bs);\n@@ -240,7 +242,6 @@ int commit_graft_pos(struct repository *r, const struct object_id *oid);\n int register_commit_graft(struct repository *r, struct commit_graft *, int);\n void prepare_commit_graft(struct repository *r);\n struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid);\n-void reset_commit_grafts(struct repository *r);\n \n struct commit *get_fork_point(const char *refname, struct commit *commit);\n \ndiff --git a/object.c b/object.c\nindex d756c7f2ea3..94ea8fb8d2c 100644\n--- a/object.c\n+++ b/object.c\n@@ -545,11 +545,12 @@ void repo_clear_commit_marks(struct repository *r, unsigned int flags)\n \t}\n }\n \n-struct parsed_object_pool *parsed_object_pool_new(void)\n+struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)\n {\n \tstruct parsed_object_pool *o = xmalloc(sizeof(*o));\n \tmemset(o, 0, sizeof(*o));\n \n+\to->repo = repo;\n \to->blob_state = allocate_alloc_state();\n \to->tree_state = allocate_alloc_state();\n \to->commit_state = allocate_alloc_state();\n@@ -628,6 +629,16 @@ void raw_object_store_clear(struct raw_object_store *o)\n \thashmap_clear(&o->pack_map);\n }\n \n+void parsed_object_pool_reset_commit_grafts(struct parsed_object_pool *o)\n+{\n+\tfor (int i = 0; i < o->grafts_nr; i++) {\n+\t\tunparse_commit(o->repo, &o->grafts[i]->oid);\n+\t\tfree(o->grafts[i]);\n+\t}\n+\to->grafts_nr = 0;\n+\to->commit_graft_prepared = 0;\n+}\n+\n void parsed_object_pool_clear(struct parsed_object_pool *o)\n {\n \t/*\n@@ -659,6 +670,7 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)\n \tfree_commit_buffer_slab(o->buffer_slab);\n \to->buffer_slab = NULL;\n \n+\tparsed_object_pool_reset_commit_grafts(o);\n \tclear_alloc_state(o->blob_state);\n \tclear_alloc_state(o->tree_state);\n \tclear_alloc_state(o->commit_state);\ndiff --git a/object.h b/object.h\nindex 05691486ebf..17f32f1103e 100644\n--- a/object.h\n+++ b/object.h\n@@ -7,6 +7,7 @@ struct buffer_slab;\n struct repository;\n \n struct parsed_object_pool {\n+\tstruct repository *repo;\n \tstruct object **obj_hash;\n \tint nr_objs, obj_hash_size;\n \n@@ -31,8 +32,9 @@ struct parsed_object_pool {\n \tstruct buffer_slab *buffer_slab;\n };\n \n-struct parsed_object_pool *parsed_object_pool_new(void);\n+struct parsed_object_pool *parsed_object_pool_new(struct repository *repo);\n void parsed_object_pool_clear(struct parsed_object_pool *o);\n+void parsed_object_pool_reset_commit_grafts(struct parsed_object_pool *o);\n \n struct object_list {\n \tstruct object *item;\ndiff --git a/repository.c b/repository.c\nindex 9825a308993..e6fc2c6aa9d 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -54,7 +54,7 @@ void initialize_repository(struct repository *repo)\n {\n \trepo->objects = raw_object_store_new();\n \trepo->remote_state = remote_state_new();\n-\trepo->parsed_objects = parsed_object_pool_new();\n+\trepo->parsed_objects = parsed_object_pool_new(repo);\n \tALLOC_ARRAY(repo->index, 1);\n \tindex_state_init(repo->index, repo);\n \ndiff --git a/shallow.c b/shallow.c\nindex b8cd051e3b6..a10cf9e9d5d 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -97,7 +97,7 @@ static void reset_repository_shallow(struct repository *r)\n {\n \tr->parsed_objects->is_shallow = -1;\n \tstat_validity_clear(r->parsed_objects->shallow_stat);\n-\treset_commit_grafts(r);\n+\tparsed_object_pool_reset_commit_grafts(r->parsed_objects);\n }\n \n int commit_shallow_file(struct repository *r, struct shallow_lock *lk)\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501655","messageId":"ec9a51432414cf3a62a1041542f0c9706377b235.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 13/22] shallow: free grafts when unregistering them","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:22:13Z","receivedAt":"2024-08-26T07:22:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When removing a graft via `unregister_shallow()` we remove it from the\ngrafts array, but do not free the structure. Fix this to plug the leak.\n\nSigned-off-by: Patrick Steinhardt \n---\n shallow.c | 4 +++-\n t/t5537-fetch-shallow.sh | 1 +\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/shallow.c b/shallow.c\nindex a10cf9e9d5d..7e0ee96ead9 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -51,10 +51,12 @@ int unregister_shallow(const struct object_id *oid)\n \tint pos = commit_graft_pos(the_repository, oid);\n \tif (pos < 0)\n \t\treturn -1;\n-\tif (pos + 1 < the_repository->parsed_objects->grafts_nr)\n+\tif (pos + 1 < the_repository->parsed_objects->grafts_nr) {\n+\t\tfree(the_repository->parsed_objects->grafts[pos]);\n \t\tMOVE_ARRAY(the_repository->parsed_objects->grafts + pos,\n \t\t\t the_repository->parsed_objects->grafts + pos + 1,\n \t\t\t the_repository->parsed_objects->grafts_nr - pos - 1);\n+\t}\n \tthe_repository->parsed_objects->grafts_nr--;\n \treturn 0;\n }\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 37f7547a4ca..cae4d400f32 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -5,6 +5,7 @@ test_description='fetch/clone from a shallow clone'\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 commit() {\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501656","messageId":"2a63030ff09f938d705c117406b501ecf81f67de.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 14/22] shallow: fix leaking members of `struct shallow_info`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:22:16Z","receivedAt":"2024-08-26T07:22:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We do not free several struct members in `clear_shallow_info()`. Fix\nthis to plug the resulting leaks.\n\nSigned-off-by: Patrick Steinhardt \n---\n shallow.c | 9 +++++++++\n t/t5538-push-shallow.sh | 1 +\n 2 files changed, 10 insertions(+)\n\ndiff --git a/shallow.c b/shallow.c\nindex 7e0ee96ead9..dcebc263d70 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -489,6 +489,15 @@ void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa)\n \n void clear_shallow_info(struct shallow_info *info)\n {\n+\tif (info->used_shallow) {\n+\t\tfor (size_t i = 0; i < info->shallow->nr; i++)\n+\t\t\tfree(info->used_shallow[i]);\n+\t\tfree(info->used_shallow);\n+\t}\n+\n+\tfree(info->need_reachability_test);\n+\tfree(info->reachable);\n+\tfree(info->shallow_ref);\n \tfree(info->ours);\n \tfree(info->theirs);\n }\ndiff --git a/t/t5538-push-shallow.sh b/t/t5538-push-shallow.sh\nindex e91fcc173e8..6adc3a20a45 100755\n--- a/t/t5538-push-shallow.sh\n+++ b/t/t5538-push-shallow.sh\n@@ -5,6 +5,7 @@ test_description='push from/to a shallow clone'\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 commit() {\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501657","messageId":"920db3a2912c609d4ac1fca2fc6b137513c8eceb.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 15/22] negotiator/skipping: fix leaking commit entries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:22:18Z","receivedAt":"2024-08-26T07:22:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When releasing the skipping negotiator we free its priority queue, but\nnot the contained entries. Fix this to plug a memory leak.\n\nSigned-off-by: Patrick Steinhardt \n---\n negotiator/skipping.c | 7 +++++--\n t/t5552-skipping-fetch-negotiator.sh | 2 ++\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/negotiator/skipping.c b/negotiator/skipping.c\nindex f65d47858b4..b738fe4faef 100644\n--- a/negotiator/skipping.c\n+++ b/negotiator/skipping.c\n@@ -247,8 +247,11 @@ static int ack(struct fetch_negotiator *n, struct commit *c)\n \n static void release(struct fetch_negotiator *n)\n {\n-\tclear_prio_queue(&((struct data *)n->data)->rev_list);\n-\tFREE_AND_NULL(n->data);\n+\tstruct data *data = n->data;\n+\tfor (int i = 0; i < data->rev_list.nr; i++)\n+\t\tfree(data->rev_list.array[i].data);\n+\tclear_prio_queue(&data->rev_list);\n+\tFREE_AND_NULL(data);\n }\n \n void skipping_negotiator_init(struct fetch_negotiator *negotiator)\ndiff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh\nindex b55a9f65e6b..4f2e5ae8dfa 100755\n--- a/t/t5552-skipping-fetch-negotiator.sh\n+++ b/t/t5552-skipping-fetch-negotiator.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='test skipping fetch negotiator'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'fetch.negotiationalgorithm config' '\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501658","messageId":"19eb9073482144d0983de1ff9529fe050b3fc6a0.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 16/22] builtin/repack: fix leaking line buffer when packing promisors","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:22:24Z","receivedAt":"2024-08-26T07:22:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `repack_promisor_objects()` we read output from git-pack-objects(1)\nline by line, using `strbuf_getline_lf()`. We never free the line\nbuffer, causing a memory leak. Plug it.\n\nThis leak is being hit in t5616, but plugging it alone is not\nsufficient to make the whole test suite leak free.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/repack.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 8bb875532b4..a382754feee 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -425,9 +425,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \n \t\tfree(promisor_name);\n \t}\n+\n \tfclose(out);\n \tif (finish_command(&cmd))\n \t\tdie(_(\"could not finish pack-objects to repack promisor objects\"));\n+\tstrbuf_release(&line);\n }\n \n struct pack_geometry {\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501659","messageId":"017713f5a9c31994b18387d8e934fbab61e4aaef.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 17/22] builtin/pack-objects: plug leaking list of keep-packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:22:26Z","receivedAt":"2024-08-26T07:22:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `--keep-pack` option of git-pack-objects(1) populates the arguments\ninto a string list. And while the list is marked as `NODUP` and thus\nwon't duplicate the strings, the list entries themselves still need to\nbe free'd. We don't though, causing a leak.\n\nPlug it.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-objects.c | 1 +\n t/t5616-partial-clone.sh | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c481feadbfa..ab78d72e273 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4641,6 +4641,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n cleanup:\n \tclear_packing_data(&to_pack);\n \tlist_objects_filter_release(&filter_options);\n+\tstring_list_clear(&keep_pack_list, 0);\n \tstrvec_clear(&rp);\n \n \treturn 0;\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 2da7291e379..467c46dccf6 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -5,6 +5,7 @@ test_description='git partial clone'\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 # create a normal \"src\" repo where we can later create new commits.\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501660","messageId":"0722cb38ea9d6b158a07da6f31ad0fe83424cb2e.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 18/22] builtin/grep: fix leaking object context","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:22:29Z","receivedAt":"2024-08-26T07:22:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Even when `get_oid_with_context()` fails it may have allocated some data\nin tthe object context. But we do not release it in git-grep(1) when the\ncall fails, leading to a memory leak. Plug it.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/grep.c | 1 +\n t/t6132-pathspec-exclude.sh | 1 +\n t/t6135-pathspec-with-attrs.sh | 2 ++\n 3 files changed, 4 insertions(+)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex dfc3c3e8bd2..dda4582d646 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1133,6 +1133,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t &oid, &oc)) {\n \t\t\tif (seen_dashdash)\n \t\t\t\tdie(_(\"unable to resolve revision: %s\"), arg);\n+\t\t\tobject_context_release(&oc);\n \t\t\tbreak;\n \t\t}\n \ndiff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh\nindex 9fdafeb1e90..f31c09c056f 100755\n--- a/t/t6132-pathspec-exclude.sh\n+++ b/t/t6132-pathspec-exclude.sh\n@@ -2,6 +2,7 @@\n \n test_description='test case exclude pathspec'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex 120dcd74a51..794bc7daf05 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='test labels in pathspecs'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup a tree' '\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501661","messageId":"b37391c0d6bec8a017968bb78f1a2284a408e9b0.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 19/22] builtin/fmt-merge-msg: fix leaking buffers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:22:32Z","receivedAt":"2024-08-26T07:22:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Fix leaking input and output buffers in git-fmt-merge-msg(1).\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fmt-merge-msg.c | 2 ++\n t/t6200-fmt-merge-msg.sh | 1 +\n 2 files changed, 3 insertions(+)\n\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 957786d1b3a..0b162f8fab1 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -67,6 +67,8 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)\n \t\treturn ret;\n \twrite_in_full(STDOUT_FILENO, output.buf, output.len);\n \n+\tstrbuf_release(&input);\n+\tstrbuf_release(&output);\n \tfree(inpath);\n \treturn 0;\n }\ndiff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh\nindex 5a221f8ef1f..ac57b0e4ae3 100755\n--- a/t/t6200-fmt-merge-msg.sh\n+++ b/t/t6200-fmt-merge-msg.sh\n@@ -8,6 +8,7 @@ test_description='fmt-merge-msg test'\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 \n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501662","messageId":"05461e3b1c02488046ef480f20c109b51b9b7691.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 20/22] match-trees: fix leaking prefixes in `shift_tree()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:22:34Z","receivedAt":"2024-08-26T07:22:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `shift_tree()` we allocate two empty strings that we end up\npassing to `match_trees()`. If that function finds a better match it\nwill update these pointers to point to a newly allocated strings,\nfreeing the old strings. We never free the final results though, neither\nthe ones we have allocated ourselves, nor the one that `match_trees()`\nmight've returned to us.\n\nFix the resulting memory leaks by creating a common exit path where we\nfree them.\n\nSigned-off-by: Patrick Steinhardt \n---\n match-trees.c | 10 +++++++---\n t/t6409-merge-subtree.sh | 1 +\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/match-trees.c b/match-trees.c\nindex f17c74d483f..147b03abf18 100644\n--- a/match-trees.c\n+++ b/match-trees.c\n@@ -294,18 +294,22 @@ void shift_tree(struct repository *r,\n \t\tunsigned short mode;\n \n \t\tif (!*del_prefix)\n-\t\t\treturn;\n+\t\t\tgoto out;\n \n \t\tif (get_tree_entry(r, hash2, del_prefix, shifted, &mode))\n \t\t\tdie(\"cannot find path %s in tree %s\",\n \t\t\t del_prefix, oid_to_hex(hash2));\n-\t\treturn;\n+\t\tgoto out;\n \t}\n \n \tif (!*add_prefix)\n-\t\treturn;\n+\t\tgoto out;\n \n \tsplice_tree(hash1, add_prefix, hash2, shifted);\n+\n+out:\n+\tfree(add_prefix);\n+\tfree(del_prefix);\n }\n \n /*\ndiff --git a/t/t6409-merge-subtree.sh b/t/t6409-merge-subtree.sh\nindex e9ba6f1690d..528615b981f 100755\n--- a/t/t6409-merge-subtree.sh\n+++ b/t/t6409-merge-subtree.sh\n@@ -5,6 +5,7 @@ test_description='subtree merge 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 \n test_expect_success setup '\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501663","messageId":"da1c23a9ccf8c797ebcbe6ce5a8243c1d051fad6.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 21/22] merge-ort: fix two leaks when handling directory rename modifications","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:22:37Z","receivedAt":"2024-08-26T07:22:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are two leaks in `apply_directory_rename_modifications()`:\n\n - We do not release the `dirs_to_insert` string list.\n\n - We do not release some `conflict_info` we put into the\n `opt->priv->paths` string map.\n\nThe former is trivial to fix. The latter is a bit less straight forward:\nthe `util` pointer of the string map may sometimes point to data that\nhas been allocated via `CALLOC()`, while at other times it may point to\ndata that has been allocated via a `mem_pool`.\n\nIt very much seems like an oversight that we didn't also allocate the\nconflict info in this code path via the memory pool, though. So let's\nfix that, which will also plug the memory leak for us.\n\nSigned-off-by: Patrick Steinhardt \n---\n merge-ort.c | 4 +++-\n t/t6423-merge-rename-directories.sh | 1 +\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 3752c7e595d..0ed3cd06b1a 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2710,7 +2710,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,\n \t\tstruct conflict_info *dir_ci;\n \t\tchar *cur_dir = dirs_to_insert.items[i].string;\n \n-\t\tCALLOC_ARRAY(dir_ci, 1);\n+\t\tdir_ci = mem_pool_calloc(&opt->priv->pool, 1, sizeof(*dir_ci));\n \n \t\tdir_ci->merged.directory_name = parent_name;\n \t\tlen = strlen(parent_name);\n@@ -2838,6 +2838,8 @@ static void apply_directory_rename_modifications(struct merge_options *opt,\n \t * Finally, record the new location.\n \t */\n \tpair->two->path = new_path;\n+\n+\tstring_list_clear(&dirs_to_insert, 0);\n }\n \n /*** Function Grouping: functions related to regular rename detection ***/\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex 88d1cf2cde9..4aaaf38be68 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -25,6 +25,7 @@ test_description=\"recursive merge with directory renames\"\n # underscore notation is to differentiate different\n # files that might be renamed into each other's paths.)\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-merge.sh\n \n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501664","messageId":"3d30c727fbcb930f34f363a72cf4fbf7168b7504.1724656120.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH 22/22] builtin/repack: fix leaking keep-pack list","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T07:22:42Z","receivedAt":"2024-08-26T07:22:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The list of packs to keep is populated via a command line option but\nnever free'd. Plug this memory leak.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/repack.c | 1 +\n t/t6500-gc.sh | 1 +\n t/t7703-repack-geometric.sh | 1 +\n 3 files changed, 3 insertions(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex a382754feee..3ee8cfa732f 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -1543,6 +1543,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t}\n \n cleanup:\n+\tstring_list_clear(&keep_pack_list, 0);\n \tstring_list_clear(&names, 1);\n \texisting_packs_release(&existing);\n \tfree_pack_geometry(&geometry);\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 1b5909d1b70..58654b3437e 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -3,6 +3,7 @@\n test_description='basic git gc tests\n '\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 9fc1626fbfd..8877aea98ba 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -2,6 +2,7 @@\n \n test_description='git repack --geometric works correctly'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n GIT_TEST_MULTI_PACK_INDEX=0\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501740","messageId":"xmqqbk1d7fr0.fsf@gitster.g","threadId":"62002","inReplyTo":"7c158acadf40b44edb3cf186860a3f60818f76f0.1724656120.git.ps@pks.im","subject":"Re: [PATCH 01/22] t/test-lib: allow skipping leak checks for passing tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-27T22:38:59Z","receivedAt":"2024-08-27T22:39:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> With `GIT_TEST_PASSING_SANITIZE_LEAK=check`, one can double check\n> whether a memory leak fix caused some test suites to become leak free.\n> It is somewhat slow to execute though because it runs all of our test\n> suites with the leak sanitizer enabled. It is also pointless in most\n> cases, because the only test suites that need to be checked are those\n> which _aren't_ yet marked with `TEST_PASSES_SANITIZE_LEAK=true`.\n>\n> Introduce a new value \"check-failing\". If set, we will only check those\n> tests which are not yet marked as leak free.\n\nA very welcome addition. I am already liking it while running\nlocally.\n\nThanks.\n\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> t/test-lib.sh | 11 ++++++++++-\n> 1 file changed, 10 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 54247604cbc..64bd36531c1 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1558,8 +1558,16 @@ then\n> \t\tpasses_sanitize_leak=t\n> \tfi\n> \n> -\tif test \"$GIT_TEST_PASSING_SANITIZE_LEAK\" = \"check\"\n> +\tif test \"$GIT_TEST_PASSING_SANITIZE_LEAK\" = \"check\" ||\n> +\t test \"$GIT_TEST_PASSING_SANITIZE_LEAK\" = \"check-failing\"\n> \tthen\n> +\t\tif test \"$GIT_TEST_PASSING_SANITIZE_LEAK\" = \"check-failing\" &&\n> +\t\t test -n \"$passes_sanitize_leak\"\n> +\t\tthen\n> +\t\t\tskip_all=\"skipping leak-free $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=check-failing\"\n> +\t\t\ttest_done\n> +\t\tfi\n> +\n> \t\tsanitize_leak_check=t\n> \t\tif test -n \"$invert_exit_code\"\n> \t\tthen\n> @@ -1597,6 +1605,7 @@ then\n> \texport LSAN_OPTIONS\n> \n> elif test \"$GIT_TEST_PASSING_SANITIZE_LEAK\" = \"check\" ||\n> + test \"$GIT_TEST_PASSING_SANITIZE_LEAK\" = \"check-failing\" ||\n> test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false\n> then\n> \tBAIL_OUT_ENV_NEEDS_SANITIZE_LEAK \"GIT_TEST_PASSING_SANITIZE_LEAK=true\"\n"},{"id":"501790","messageId":"20240828202902.4156688-1-calvinwan@google.com","threadId":"62002","inReplyTo":"920db3a2912c609d4ac1fca2fc6b137513c8eceb.1724656120.git.ps@pks.im","subject":"Re: [PATCH 15/22] negotiator/skipping: fix leaking commit entries","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-08-28T20:29:02Z","receivedAt":"2024-08-28T20:29:05Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"Patrick Steinhardt writes:\n> When releasing the skipping negotiator we free its priority queue, but\n> not the contained entries. Fix this to plug a memory leak.\n> \n> Signed-off-by: Patrick Steinhardt \n> ---\n> negotiator/skipping.c | 7 +++++--\n> t/t5552-skipping-fetch-negotiator.sh | 2 ++\n> 2 files changed, 7 insertions(+), 2 deletions(-)\n> \n> diff --git a/negotiator/skipping.c b/negotiator/skipping.c\n> index f65d47858b4..b738fe4faef 100644\n> --- a/negotiator/skipping.c\n> +++ b/negotiator/skipping.c\n> @@ -247,8 +247,11 @@ static int ack(struct fetch_negotiator *n, struct commit *c)\n> \n> static void release(struct fetch_negotiator *n)\n> {\n> -\tclear_prio_queue(&((struct data *)n->data)->rev_list);\n> -\tFREE_AND_NULL(n->data);\n> +\tstruct data *data = n->data;\n> +\tfor (int i = 0; i < data->rev_list.nr; i++)\n> +\t\tfree(data->rev_list.array[i].data);\n> +\tclear_prio_queue(&data->rev_list);\n> +\tFREE_AND_NULL(data);\n> }\n> \n\nIt seems unintuitive that clear_prio_queue() doesn't also free the data\nunderneath and that a caller would have to know to free that as well to\navoid leaking memory. Would it make more sense to add this change to\nclear_prio_queue() instead? Patch 14 has that pattern already.\n\nThanks again for the cleanups -- I'm tempted to take a stab at some of\nthe other memory leaks you mentioned during our biweekly hackathon. All\nof the other patches look reasonable to me.\n"},{"id":"501795","messageId":"fazvh3gejzofti6ii4x2fkoh66hni6lcc6axc7w4tspte4lioy@hgnvhue2pm26","threadId":"62002","inReplyTo":"20240828202902.4156688-1-calvinwan@google.com","subject":"Re: [PATCH 15/22] negotiator/skipping: fix leaking commit entries","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-08-28T22:19:10Z","receivedAt":"2024-08-28T22:19:16Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.08.28 20:29, Calvin Wan wrote:\n> Patrick Steinhardt writes:\n> > When releasing the skipping negotiator we free its priority queue, but\n> > not the contained entries. Fix this to plug a memory leak.\n> > \n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > negotiator/skipping.c | 7 +++++--\n> > t/t5552-skipping-fetch-negotiator.sh | 2 ++\n> > 2 files changed, 7 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/negotiator/skipping.c b/negotiator/skipping.c\n> > index f65d47858b4..b738fe4faef 100644\n> > --- a/negotiator/skipping.c\n> > +++ b/negotiator/skipping.c\n> > @@ -247,8 +247,11 @@ static int ack(struct fetch_negotiator *n, struct commit *c)\n> > \n> > static void release(struct fetch_negotiator *n)\n> > {\n> > -\tclear_prio_queue(&((struct data *)n->data)->rev_list);\n> > -\tFREE_AND_NULL(n->data);\n> > +\tstruct data *data = n->data;\n> > +\tfor (int i = 0; i < data->rev_list.nr; i++)\n> > +\t\tfree(data->rev_list.array[i].data);\n> > +\tclear_prio_queue(&data->rev_list);\n> > +\tFREE_AND_NULL(data);\n> > }\n> > \n> \n> It seems unintuitive that clear_prio_queue() doesn't also free the data\n> underneath and that a caller would have to know to free that as well to\n> avoid leaking memory. Would it make more sense to add this change to\n> clear_prio_queue() instead? Patch 14 has that pattern already.\n\nI'm assuming the reasoning is that clear_prio_queue() can't know if its\nitems need more complicated cleanup of their own, so if the caller\n(potentially) needs to clean up items individually anyway, the caller\ncan also free them at the same time?\n\n> Thanks again for the cleanups -- I'm tempted to take a stab at some of\n> the other memory leaks you mentioned during our biweekly hackathon. All\n> of the other patches look reasonable to me.\n\nThe series also looks good to me, thanks!\n\nReviewed-by: Josh Steadmon \n"},{"id":"501802","messageId":"ZtA0Oj0o3UXmSIDP@tanuki","threadId":"62002","inReplyTo":"fazvh3gejzofti6ii4x2fkoh66hni6lcc6axc7w4tspte4lioy@hgnvhue2pm26","subject":"Re: [PATCH 15/22] negotiator/skipping: fix leaking commit entries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-29T08:41:37Z","receivedAt":"2024-08-29T08:41:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Aug 28, 2024 at 03:19:10PM -0700, Josh Steadmon wrote:\n> On 2024.08.28 20:29, Calvin Wan wrote:\n> > Patrick Steinhardt writes:\n> > > When releasing the skipping negotiator we free its priority queue, but\n> > > not the contained entries. Fix this to plug a memory leak.\n> > > \n> > > Signed-off-by: Patrick Steinhardt \n> > > ---\n> > > negotiator/skipping.c | 7 +++++--\n> > > t/t5552-skipping-fetch-negotiator.sh | 2 ++\n> > > 2 files changed, 7 insertions(+), 2 deletions(-)\n> > > \n> > > diff --git a/negotiator/skipping.c b/negotiator/skipping.c\n> > > index f65d47858b4..b738fe4faef 100644\n> > > --- a/negotiator/skipping.c\n> > > +++ b/negotiator/skipping.c\n> > > @@ -247,8 +247,11 @@ static int ack(struct fetch_negotiator *n, struct commit *c)\n> > > \n> > > static void release(struct fetch_negotiator *n)\n> > > {\n> > > -\tclear_prio_queue(&((struct data *)n->data)->rev_list);\n> > > -\tFREE_AND_NULL(n->data);\n> > > +\tstruct data *data = n->data;\n> > > +\tfor (int i = 0; i < data->rev_list.nr; i++)\n> > > +\t\tfree(data->rev_list.array[i].data);\n> > > +\tclear_prio_queue(&data->rev_list);\n> > > +\tFREE_AND_NULL(data);\n> > > }\n> > > \n> > \n> > It seems unintuitive that clear_prio_queue() doesn't also free the data\n> > underneath and that a caller would have to know to free that as well to\n> > avoid leaking memory. Would it make more sense to add this change to\n> > clear_prio_queue() instead? Patch 14 has that pattern already.\n> \n> I'm assuming the reasoning is that clear_prio_queue() can't know if its\n> items need more complicated cleanup of their own, so if the caller\n> (potentially) needs to clean up items individually anyway, the caller\n> can also free them at the same time?\n\nYeah, that's mostly the reason. We have e.g. `string_list_clear_func()`\nthat works around this issue by making the caller provide the cleanup\nfunction, and we could use the same pattern here. But it seems as if\nmost of the callers of `clear_prio_queue()` don't need this because they\nalready drain the queue during normal operations anyway.\n\nWith patch 14 you probably refer to `clear_shallow_info()`? We're not\nusing a priority queue there, so it is not quite related to the prio\nqueue we have here. So I'm inclined to leave this as-is, and if we ever\nsee that we have more callsites that want to clean up the prio queue and\nits contents we can introduce `prio_queue_clear_func()`. Does that work\nfor you?\n\n> > Thanks again for the cleanups -- I'm tempted to take a stab at some of\n> > the other memory leaks you mentioned during our biweekly hackathon. All\n> > of the other patches look reasonable to me.\n\nI'd certainly be happy to shed some of the load here. If you do, please\ngive me a quick ping so that we don't duplicate any work by accident.\n\n> The series also looks good to me, thanks!\n> \n> Reviewed-by: Josh Steadmon \n\nThanks!\n\nPatrick\n"},{"id":"501833","messageId":"87frqnjty9.fsf@iotcl.com","threadId":"62002","inReplyTo":"7c158acadf40b44edb3cf186860a3f60818f76f0.1724656120.git.ps@pks.im","subject":"Re: [PATCH 01/22] t/test-lib: allow skipping leak checks for passing tests","fromName":"Toon claes","fromEmail":"toon@iotcl.com","sentAt":"2024-08-29T14:15:58Z","receivedAt":"2024-08-29T14:16:07Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Patrick Steinhardt writes:\n\n> With `GIT_TEST_PASSING_SANITIZE_LEAK=check`, one can double check\n> whether a memory leak fix caused some test suites to become leak free.\n> It is somewhat slow to execute though because it runs all of our test\n> suites with the leak sanitizer enabled. It is also pointless in most\n> cases, because the only test suites that need to be checked are those\n> which _aren't_ yet marked with `TEST_PASSES_SANITIZE_LEAK=true`.\n\nWhat I understand from `t/README` the \"check\" value is used to test\nwhether the presence or absence of `TEST_PASSES_SANITIZE_LEAK=true`\nmatches the expectations. I think it's better to express that in the\nfirst sentence, because it sounds a bit misleading at the moment if you\ndon't know that.\n\n> Introduce a new value \"check-failing\". If set, we will only check those\n> tests which are not yet marked as leak free.\n\nI would also mention this still has the effect that tests which *are*\nleak-free but do not have `TEST_PASSES_SANITIZE_LEAK=true` fail due to\nthe use of \"--invert-exit-code\".\n\nAlso, can you add a short paragraph about this value in \"t/README\"?\n\n--\nToon\n"},{"id":"501834","messageId":"87ed67jtxy.fsf@iotcl.com","threadId":"62002","inReplyTo":"2a63030ff09f938d705c117406b501ecf81f67de.1724656120.git.ps@pks.im","subject":"Re: [PATCH 14/22] shallow: fix leaking members of `struct shallow_info`","fromName":"Toon claes","fromEmail":"toon@iotcl.com","sentAt":"2024-08-29T14:16:09Z","receivedAt":"2024-08-29T14:16:19Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Patrick Steinhardt writes:\n\n> We do not free several struct members in `clear_shallow_info()`. Fix\n> this to plug the resulting leaks.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> shallow.c | 9 +++++++++\n> t/t5538-push-shallow.sh | 1 +\n> 2 files changed, 10 insertions(+)\n>\n> diff --git a/shallow.c b/shallow.c\n> index 7e0ee96ead9..dcebc263d70 100644\n> --- a/shallow.c\n> +++ b/shallow.c\n> @@ -489,6 +489,15 @@ void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa)\n> \n> void clear_shallow_info(struct shallow_info *info)\n> {\n> +\tif (info->used_shallow) {\n> +\t\tfor (size_t i = 0; i < info->shallow->nr; i++)\n> +\t\t\tfree(info->used_shallow[i]);\n> +\t\tfree(info->used_shallow);\n> +\t}\n> +\n> +\tfree(info->need_reachability_test);\n> +\tfree(info->reachable);\n> +\tfree(info->shallow_ref);\n> \tfree(info->ours);\n> \tfree(info->theirs);\n> }\n\nRecently was agreed in the CodingGuidelines `S_clear()` functions do a\n`S_release()` + `S_init()`. I see we're not initializing the struct for\nfuture use (i.e. we don't reset the `nr_*` fields to 0). But we cannot\nreally do an init, because that would be calling\n`prepare_shallow_info()`, which allocates new memory. So would it be\nworth to rename this function to `release_shallow_info()`?\n\n--\nToon\n"},{"id":"501840","messageId":"xmqqr0a7wbvw.fsf@gitster.g","threadId":"62002","inReplyTo":"87ed67jtxy.fsf@iotcl.com","subject":"Re: [PATCH 14/22] shallow: fix leaking members of `struct shallow_info`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-29T16:07:47Z","receivedAt":"2024-08-29T16:07:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Toon claes writes:\n\n>> void clear_shallow_info(struct shallow_info *info)\n>> {\n>> +\tif (info->used_shallow) {\n>> +\t\tfor (size_t i = 0; i < info->shallow->nr; i++)\n>> +\t\t\tfree(info->used_shallow[i]);\n>> +\t\tfree(info->used_shallow);\n>> +\t}\n>> +\n>> +\tfree(info->need_reachability_test);\n>> +\tfree(info->reachable);\n>> +\tfree(info->shallow_ref);\n>> \tfree(info->ours);\n>> \tfree(info->theirs);\n>> }\n>\n> `prepare_shallow_info()`, which allocates new memory. So would it be\n> worth to rename this function to `release_shallow_info()`?\n\nIn the longer term in a separate \"renaming everything\" effort, yes.\nIn the context of \"plug many resource leaks\" series, probably no.\n\nThanks.\n"},{"id":"501844","messageId":"CAFySSZAPJUkZmVrjzoaL6JqdJrmfphecq58X+yZZkwqu5yJGZA@mail.gmail.com","threadId":"62002","inReplyTo":"ZtA0Oj0o3UXmSIDP@tanuki","subject":"Re: [PATCH 15/22] negotiator/skipping: fix leaking commit entries","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-08-29T17:29:22Z","receivedAt":"2024-08-29T17:29:35Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"On Thu, Aug 29, 2024 at 1:41 AM Patrick Steinhardt wrote:\n>\n> Yeah, that's mostly the reason. We have e.g. `string_list_clear_func()`\n> that works around this issue by making the caller provide the cleanup\n> function, and we could use the same pattern here. But it seems as if\n> most of the callers of `clear_prio_queue()` don't need this because they\n> already drain the queue during normal operations anyway.\n>\n> With patch 14 you probably refer to `clear_shallow_info()`? We're not\n> using a priority queue there, so it is not quite related to the prio\n> queue we have here. So I'm inclined to leave this as-is, and if we ever\n> see that we have more callsites that want to clean up the prio queue and\n> its contents we can introduce `prio_queue_clear_func()`. Does that work\n> for you?\n\nYes it does! Thanks for the clarification\n"},{"id":"501893","messageId":"ZtGKJfgBo17Vz8wY@tanuki","threadId":"62002","inReplyTo":"87frqnjty9.fsf@iotcl.com","subject":"Re: [PATCH 01/22] t/test-lib: allow skipping leak checks for passing tests","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-30T09:00:26Z","receivedAt":"2024-08-30T09:00:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Aug 29, 2024 at 04:15:58PM +0200, Toon claes wrote:\n> Patrick Steinhardt writes:\n> \n> > With `GIT_TEST_PASSING_SANITIZE_LEAK=check`, one can double check\n> > whether a memory leak fix caused some test suites to become leak free.\n> > It is somewhat slow to execute though because it runs all of our test\n> > suites with the leak sanitizer enabled. It is also pointless in most\n> > cases, because the only test suites that need to be checked are those\n> > which _aren't_ yet marked with `TEST_PASSES_SANITIZE_LEAK=true`.\n> \n> What I understand from `t/README` the \"check\" value is used to test\n> whether the presence or absence of `TEST_PASSES_SANITIZE_LEAK=true`\n> matches the expectations. I think it's better to express that in the\n> first sentence, because it sounds a bit misleading at the moment if you\n> don't know that.\n> \n> > Introduce a new value \"check-failing\". If set, we will only check those\n> > tests which are not yet marked as leak free.\n> \n> I would also mention this still has the effect that tests which *are*\n> leak-free but do not have `TEST_PASSES_SANITIZE_LEAK=true` fail due to\n> the use of \"--invert-exit-code\".\n\nI don't want to go too much into the implementation details here, but\nwill note that we continue to behave the same as with \"check\", except\nthat we skip already-leak-free tests.\n\n> Also, can you add a short paragraph about this value in \"t/README\"?\n\nYup, will do.\n\nPatrick\n"},{"id":"501894","messageId":"ZtGKL1m2MVOVYwfT@tanuki","threadId":"62002","inReplyTo":"xmqqr0a7wbvw.fsf@gitster.g","subject":"Re: [PATCH 14/22] shallow: fix leaking members of `struct shallow_info`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-30T09:00:31Z","receivedAt":"2024-08-30T09:00:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Aug 29, 2024 at 09:07:47AM -0700, Junio C Hamano wrote:\n> Toon claes writes:\n> \n> >> void clear_shallow_info(struct shallow_info *info)\n> >> {\n> >> +\tif (info->used_shallow) {\n> >> +\t\tfor (size_t i = 0; i < info->shallow->nr; i++)\n> >> +\t\t\tfree(info->used_shallow[i]);\n> >> +\t\tfree(info->used_shallow);\n> >> +\t}\n> >> +\n> >> +\tfree(info->need_reachability_test);\n> >> +\tfree(info->reachable);\n> >> +\tfree(info->shallow_ref);\n> >> \tfree(info->ours);\n> >> \tfree(info->theirs);\n> >> }\n> >\n> > `prepare_shallow_info()`, which allocates new memory. So would it be\n> > worth to rename this function to `release_shallow_info()`?\n> \n> In the longer term in a separate \"renaming everything\" effort, yes.\n> In the context of \"plug many resource leaks\" series, probably no.\n\nYeah, agreed. And note that `release_shallow_info()` does not match our\nstyle guide, either: it should be `shallow_info_release()`.\n\nPatrick\n"},{"id":"501943","messageId":"xmqqttf1pt96.fsf@gitster.g","threadId":"62002","inReplyTo":"92fc97b3db86bb0bdf610a2f76c03a96a99bfe8d.1724656120.git.ps@pks.im","subject":"Re: [PATCH 04/22] builtin/push: fix leaking refspec query result","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-30T21:59:01Z","receivedAt":"2024-08-30T21:59:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> When appending a refspec via `refspec_append_mapped()` we leak the\n> result of `query_refspecs()`. The overall logic around refspec queries\n> is quite weird, as callers are expected to either set the `src` or `dst`\n> pointers, and then the (allocated) result will be in the respective\n> other struct member.\n\nHmph, is it necessary to say \"quite weird\" for the purpose of this\nchange? The query interface is designed to be usable to query both\nways and within that constraints, I find it designed very nicely\n(but I do not think that is necessary to be said for the purpose of\nthis change, either)..\n\n> As we have the `src` member set, plugging the memory leak is thus as\n> easy as just freeing the `dst` member. While at it, use designated\n> initializers to initialize the structure.\n\nIn order to understand this paragraph, of course, it helps for a\nreader to understand that the query_refspecs() gives an answer by\npopulating the side other than the query side, and the answers are\nwhat we want to release.\n\n> This leak was exposed by t5516, but fixing it is not sufficient to make\n> the whole test suite leak free.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/push.c | 8 +++++---\n> 1 file changed, 5 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/push.c b/builtin/push.c\n> index 7a67398124f..0b123eb9c1e 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -72,13 +72,15 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,\n> \tconst char *branch_name;\n> \n> \tif (remote->push.nr) {\n> -\t\tstruct refspec_item query;\n> -\t\tmemset(&query, 0, sizeof(struct refspec_item));\n> -\t\tquery.src = matched->name;\n> +\t\tstruct refspec_item query = {\n> +\t\t\t.src = matched->name,\n> +\t\t};\n\nThis is \"while at it\" change that does not contribute to the leak or\nthe leakfix; the resulting code is easier to read.\n\n> \t\tif (!query_refspecs(&remote->push, &query) && query.dst) {\n> \t\t\trefspec_appendf(refspec, \"%s%s:%s\",\n> \t\t\t\t\tquery.force ? \"+\" : \"\",\n> \t\t\t\t\tquery.src, query.dst);\n> +\t\t\tfree(query.dst);\n\nAnd this is the real fix, which looks good.\n\nThanks.\n\n> \t\t\treturn;\n> \t\t}\n> \t}\n"},{"id":"501945","messageId":"xmqqplppprsg.fsf@gitster.g","threadId":"62002","inReplyTo":"77023421e189aee6837cea9b25841ea258bc06e3.1724656120.git.ps@pks.im","subject":"Re: [PATCH 05/22] upload-pack: fix leaking child process data on reachability checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-30T22:30:39Z","receivedAt":"2024-08-30T22:30:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> We spawn a git-rev-list(1) command to perform reachability checks in\n> \"upload-pack.c\". We do not release memory associated with the process\n> in error cases though, thus leaking memory.\n\nThanks for plugging this leak dating back in ages.\n\nBoth changes look sensible to me.\n\n"},{"id":"501972","messageId":"ZtWFCnJ5DZU2m8Q3@tanuki","threadId":"62002","inReplyTo":"xmqqttf1pt96.fsf@gitster.g","subject":"Re: [PATCH 04/22] builtin/push: fix leaking refspec query result","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-02T09:27:44Z","receivedAt":"2024-09-02T09:27:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Aug 30, 2024 at 02:59:01PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > When appending a refspec via `refspec_append_mapped()` we leak the\n> > result of `query_refspecs()`. The overall logic around refspec queries\n> > is quite weird, as callers are expected to either set the `src` or `dst`\n> > pointers, and then the (allocated) result will be in the respective\n> > other struct member.\n> \n> Hmph, is it necessary to say \"quite weird\" for the purpose of this\n> change? The query interface is designed to be usable to query both\n> ways and within that constraints, I find it designed very nicely\n> (but I do not think that is necessary to be said for the purpose of\n> this change, either)..\n\nI don't quite agree that it's nicely designed -- I find it rather hard\nto use and reason about, and the fact that so many callsites get this\ninterface wrong seems to indicate that there is at least some sort of\ntruth to this assessment.\n\nBut I don't mind removing this subjective opinion from the commit\nmessage. I'll do that in case I end up rerolling this patch series.\n\nThanks!\n\nPatrick\n"},{"id":"502176","messageId":"xmqq34mf84x0.fsf@gitster.g","threadId":"62002","inReplyTo":"7fafcc53d23672bb0339c44ea3874b3628785f08.1724656120.git.ps@pks.im","subject":"Re: [PATCH 08/22] remote: fix leaking tracking refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-04T21:50:19Z","receivedAt":"2024-09-04T21:50:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> @@ -1123,6 +1123,7 @@ void free_one_ref(struct ref *ref)\n> \t\treturn;\n> \tfree_one_ref(ref->peer_ref);\n> \tfree(ref->remote_status);\n> +\tfree(ref->tracking_ref);\n> \tfree(ref->symref);\n> \tfree(ref);\n> }\n> @@ -2620,8 +2621,10 @@ static int remote_tracking(struct remote *remote, const char *refname,\n> \tdst = apply_refspecs(&remote->fetch, refname);\n> \tif (!dst)\n> \t\treturn -1; /* no tracking ref for refname at remote */\n> -\tif (refs_read_ref(get_main_ref_store(the_repository), dst, oid))\n> +\tif (refs_read_ref(get_main_ref_store(the_repository), dst, oid)) {\n> +\t\tfree(dst);\n> \t\treturn -1; /* we know what the tracking ref is but we cannot read it */\n> +\t}\n> \n> \t*dst_refname = dst;\n> \treturn 0;\n\nLooking good.\n"},{"id":"502177","messageId":"xmqqr09z6pj3.fsf@gitster.g","threadId":"62002","inReplyTo":"138a5ded35a43d3aeaa5058ba316a45b7b50b9ef.1724656120.git.ps@pks.im","subject":"Re: [PATCH 10/22] send-pack: fix leaking push cert nonce","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-04T22:08:00Z","receivedAt":"2024-09-04T22:08:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> When retrieving the push cert nonce from the server, we first store the\n> constant returned by `server_feature_value()` and then, if the nonce is\n> valid, we duplicate the nonce memory to extend its lifetime. We never\n> free the latter and thus cause a memory leak.\n\n\"to extend its lifetime\" -> \"to a NUL-terminated string, so that we\ncan pass it to generate_push_cert()\".\n\nWhat is going on in this code path is this.\n\nThe other side may send a nonce in the server capability. We die if\nthat nonce is bogus. Otherwise we make a xmemdupz() copy because we\nneed to pass the nonce to generate_push_cert() that expects nonce to\nbe a NUL terminated string (the original server capability is a long\nconcatenation of capabilities and we learn the for the\nnonce). The function cryptographically signs the ref update request\nwe have, together with the nonce we got from the server, so that the\nother side can validate that it is signed by us, and the nonce serves\nas a protection against replay attacks.\n\n> diff --git a/send-pack.c b/send-pack.c\n> index b224ef9fc5e..c37f6ab3c07 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -501,7 +501,7 @@ int send_pack(struct send_pack_args *args,\n> \tunsigned cmds_sent = 0;\n> \tint ret;\n> \tstruct async demux;\n> -\tconst char *push_cert_nonce = NULL;\n> +\tchar *push_cert_nonce = NULL;\n> \tstruct packet_reader reader;\n> \tint use_bitmaps;\n\nThis is a change necessary to avoid having to cast the parameter to\nfree().\n\n> @@ -550,10 +550,11 @@ int send_pack(struct send_pack_args *args,\n> \n> \tif (args->push_cert != SEND_PACK_PUSH_CERT_NEVER) {\n> \t\tsize_t len;\n> -\t\tpush_cert_nonce = server_feature_value(\"push-cert\", &len);\n> -\t\tif (push_cert_nonce) {\n> -\t\t\treject_invalid_nonce(push_cert_nonce, len);\n> -\t\t\tpush_cert_nonce = xmemdupz(push_cert_nonce, len);\n> +\t\tconst char *nonce = server_feature_value(\"push-cert\", &len);\n> +\n> +\t\tif (nonce) {\n> +\t\t\treject_invalid_nonce(nonce, len);\n> +\t\t\tpush_cert_nonce = xmemdupz(nonce, len);\n\nAnd this hunk become needed because push_cert_nonce cannot receive\nthe return value from server_feature_value() without stripping\nconstness.\n\n> \t\t} else if (args->push_cert == SEND_PACK_PUSH_CERT_ALWAYS) {\n> \t\t\tdie(_(\"the receiving end does not support --signed push\"));\n> \t\t} else if (args->push_cert == SEND_PACK_PUSH_CERT_IF_ASKED) {\n> @@ -771,5 +772,6 @@ int send_pack(struct send_pack_args *args,\n> \toid_array_clear(&commons);\n> \tstrbuf_release(&req_buf);\n> \tstrbuf_release(&cap_buf);\n> +\tfree(push_cert_nonce);\n\nAnd this is my fault to forget freeing. Thanks for spotting and fixing.\n\n> \treturn ret;\n> }\n"},{"id":"502178","messageId":"xmqqmskn6phb.fsf@gitster.g","threadId":"62002","inReplyTo":"a1fca8104b345bff4005fcd7e4c8edb67424fd20.1724656120.git.ps@pks.im","subject":"Re: [PATCH 11/22] gpg-interface: fix misdesigned signing key interfaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-04T22:09:04Z","receivedAt":"2024-09-04T22:09:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> The interfaces to retrieve signing keys and their IDs are misdesigned as\n> they return string constants even though they indeed allocate memory,\n> which leads to memory leaks. Refactor the code to instead always return\n> allocated strings and let the callers free them accordingly.\n\nMakes sense.\n"},{"id":"502179","messageId":"xmqqikvb6ome.fsf@gitster.g","threadId":"62002","inReplyTo":"19eb9073482144d0983de1ff9529fe050b3fc6a0.1724656120.git.ps@pks.im","subject":"Re: [PATCH 16/22] builtin/repack: fix leaking line buffer when packing promisors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-04T22:27:37Z","receivedAt":"2024-09-04T22:27:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> In `repack_promisor_objects()` we read output from git-pack-objects(1)\n> line by line, using `strbuf_getline_lf()`. We never free the line\n> buffer, causing a memory leak. Plug it.\n>\n> This leak is being hit in t5616, but plugging it alone is not\n> sufficient to make the whole test suite leak free.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/repack.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 8bb875532b4..a382754feee 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -425,9 +425,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n> \n> \t\tfree(promisor_name);\n> \t}\n> +\n> \tfclose(out);\n> \tif (finish_command(&cmd))\n> \t\tdie(_(\"could not finish pack-objects to repack promisor objects\"));\n> +\tstrbuf_release(&line);\n> }\n\nObviously correct.\n\nThanks.\n"},{"id":"502180","messageId":"xmqqed5z6o7y.fsf@gitster.g","threadId":"62002","inReplyTo":"0722cb38ea9d6b158a07da6f31ad0fe83424cb2e.1724656120.git.ps@pks.im","subject":"Re: [PATCH 18/22] builtin/grep: fix leaking object context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-04T22:36:17Z","receivedAt":"2024-09-04T22:36:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Even when `get_oid_with_context()` fails it may have allocated some data\n> in tthe object context. But we do not release it in git-grep(1) when the\n\n\"tthe\" -> \"the\".\n\n> call fails, leading to a memory leak. Plug it.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/grep.c | 1 +\n> t/t6132-pathspec-exclude.sh | 1 +\n> t/t6135-pathspec-with-attrs.sh | 2 ++\n> 3 files changed, 4 insertions(+)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index dfc3c3e8bd2..dda4582d646 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -1133,6 +1133,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \t\t\t\t\t &oid, &oc)) {\n> \t\t\tif (seen_dashdash)\n> \t\t\t\tdie(_(\"unable to resolve revision: %s\"), arg);\n> +\t\t\tobject_context_release(&oc);\n> \t\t\tbreak;\n> \t\t}\n\nOK. This is the \"oh, this is not a revision argument\" codepath. It\nis perfectly normal for get_oid_with_context() to fail here, and we\nshould make sure we clear the context variable.\n\nThanks.\n"},{"id":"502181","messageId":"xmqqa5gn6nwu.fsf@gitster.g","threadId":"62002","inReplyTo":"05461e3b1c02488046ef480f20c109b51b9b7691.1724656120.git.ps@pks.im","subject":"Re: [PATCH 20/22] match-trees: fix leaking prefixes in `shift_tree()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-04T22:42:57Z","receivedAt":"2024-09-04T22:43:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> In `shift_tree()` we allocate two empty strings that we end up\n> passing to `match_trees()`. If that function finds a better match it\n> will update these pointers to point to a newly allocated strings,\n> freeing the old strings. We never free the final results though, neither\n> the ones we have allocated ourselves, nor the one that `match_trees()`\n> might've returned to us.\n>\n> Fix the resulting memory leaks by creating a common exit path where we\n> free them.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> match-trees.c | 10 +++++++---\n> t/t6409-merge-subtree.sh | 1 +\n> 2 files changed, 8 insertions(+), 3 deletions(-)\n\nWe are not going to take the \"best_match\" out of this\nfunction, so somebody ought to free it, and that somebody must be\nthis function.\n\nMakes sense.\n"},{"id":"502182","messageId":"xmqq5xrb6naw.fsf@gitster.g","threadId":"62002","inReplyTo":"da1c23a9ccf8c797ebcbe6ce5a8243c1d051fad6.1724656120.git.ps@pks.im","subject":"Re: [PATCH 21/22] merge-ort: fix two leaks when handling directory rename modifications","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-04T22:56:07Z","receivedAt":"2024-09-04T22:56:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> There are two leaks in `apply_directory_rename_modifications()`:\n>\n> - We do not release the `dirs_to_insert` string list.\n>\n> - We do not release some `conflict_info` we put into the\n> `opt->priv->paths` string map.\n>\n> The former is trivial to fix. The latter is a bit less straight forward:\n> the `util` pointer of the string map may sometimes point to data that\n> has been allocated via `CALLOC()`, while at other times it may point to\n> data that has been allocated via a `mem_pool`.\n>\n> It very much seems like an oversight that we didn't also allocate the\n> conflict info in this code path via the memory pool, though. So let's\n> fix that, which will also plug the memory leak for us.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n\nMay be nice if we can hear from the original author and the area\nexpert.\n\n> merge-ort.c | 4 +++-\n> t/t6423-merge-rename-directories.sh | 1 +\n> 2 files changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 3752c7e595d..0ed3cd06b1a 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -2710,7 +2710,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,\n> \t\tstruct conflict_info *dir_ci;\n> \t\tchar *cur_dir = dirs_to_insert.items[i].string;\n> \n> -\t\tCALLOC_ARRAY(dir_ci, 1);\n> +\t\tdir_ci = mem_pool_calloc(&opt->priv->pool, 1, sizeof(*dir_ci));\n> \n> \t\tdir_ci->merged.directory_name = parent_name;\n> \t\tlen = strlen(parent_name);\n> @@ -2838,6 +2838,8 @@ static void apply_directory_rename_modifications(struct merge_options *opt,\n> \t * Finally, record the new location.\n> \t */\n> \tpair->two->path = new_path;\n> +\n> +\tstring_list_clear(&dirs_to_insert, 0);\n> }\n> \n> /*** Function Grouping: functions related to regular rename detection ***/\n> diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\n> index 88d1cf2cde9..4aaaf38be68 100755\n> --- a/t/t6423-merge-rename-directories.sh\n> +++ b/t/t6423-merge-rename-directories.sh\n> @@ -25,6 +25,7 @@ test_description=\"recursive merge with directory renames\"\n> # underscore notation is to differentiate different\n> # files that might be renamed into each other's paths.)\n> \n> +TEST_PASSES_SANITIZE_LEAK=true\n> . ./test-lib.sh\n> . \"$TEST_DIRECTORY\"/lib-merge.sh\n"},{"id":"502183","messageId":"xmqq1q1z6n24.fsf@gitster.g","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"Re: [PATCH 00/22] Memory leak fixes (pt.6)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-04T23:01:23Z","receivedAt":"2024-09-04T23:01:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> here's another round of memory leak fixes. This is the second-last round\n> of \"general\" leak fixes -- so once this and the next series are merged,\n> I have made a full pass over all failing test suites. There will still\n> be a bunch of memory leaks, namely ~86 failing test suites.\n\nI've read (or at least briefly scanned) most of the changes in this\nseries over the course of several days, and did not spot anything\nglaringly wrong, other than a small typo or two whose fix should be\ntrivial.\n\nThanks. \n"},{"id":"502186","messageId":"CABPp-BHAnAfyqfRiMg7Xr0DyAqP0CGgT_XOA=tp42+T-knCReg@mail.gmail.com","threadId":"62002","inReplyTo":"xmqq5xrb6naw.fsf@gitster.g","subject":"Re: [PATCH 21/22] merge-ort: fix two leaks when handling directory rename modifications","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-09-05T02:01:52Z","receivedAt":"2024-09-05T02:02:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Sep 4, 2024 at 3:56 PM Junio C Hamano wrote:\n>\n> Patrick Steinhardt writes:\n>\n> > There are two leaks in `apply_directory_rename_modifications()`:\n> >\n> > - We do not release the `dirs_to_insert` string list.\n> >\n> > - We do not release some `conflict_info` we put into the\n> > `opt->priv->paths` string map.\n> >\n> > The former is trivial to fix. The latter is a bit less straight forward:\n> > the `util` pointer of the string map may sometimes point to data that\n> > has been allocated via `CALLOC()`, while at other times it may point to\n> > data that has been allocated via a `mem_pool`.\n\nOops.\n\n> > It very much seems like an oversight that we didn't also allocate the\n> > conflict info in this code path via the memory pool, though.\n\nYep.\n\n> > So let's fix that, which will also plug the memory leak for us.\n> >\n> > Signed-off-by: Patrick Steinhardt \n> > ---\n>\n> May be nice if we can hear from the original author and the area\n> expert.\n>\n> > merge-ort.c | 4 +++-\n> > t/t6423-merge-rename-directories.sh | 1 +\n> > 2 files changed, 4 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/merge-ort.c b/merge-ort.c\n> > index 3752c7e595d..0ed3cd06b1a 100644\n> > --- a/merge-ort.c\n> > +++ b/merge-ort.c\n> > @@ -2710,7 +2710,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,\n> > struct conflict_info *dir_ci;\n> > char *cur_dir = dirs_to_insert.items[i].string;\n> >\n> > - CALLOC_ARRAY(dir_ci, 1);\n> > + dir_ci = mem_pool_calloc(&opt->priv->pool, 1, sizeof(*dir_ci));\n> >\n> > dir_ci->merged.directory_name = parent_name;\n> > len = strlen(parent_name);\n> > @@ -2838,6 +2838,8 @@ static void apply_directory_rename_modifications(struct merge_options *opt,\n> > * Finally, record the new location.\n> > */\n> > pair->two->path = new_path;\n> > +\n> > + string_list_clear(&dirs_to_insert, 0);\n> > }\n> >\n> > /*** Function Grouping: functions related to regular rename detection ***/\n> > diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\n> > index 88d1cf2cde9..4aaaf38be68 100755\n> > --- a/t/t6423-merge-rename-directories.sh\n> > +++ b/t/t6423-merge-rename-directories.sh\n> > @@ -25,6 +25,7 @@ test_description=\"recursive merge with directory renames\"\n> > # underscore notation is to differentiate different\n> > # files that might be renamed into each other's paths.)\n> >\n> > +TEST_PASSES_SANITIZE_LEAK=true\n> > . ./test-lib.sh\n> > . \"$TEST_DIRECTORY\"/lib-merge.sh\n\nLooks good to me; thanks for fixing up my bugs.\n\nReviewed-by: Elijah Newren \n"},{"id":"502207","messageId":"cover.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1724656120.git.ps@pks.im","subject":"[PATCH v2 00/22] Memory leak fixes (pt.6)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:08:35Z","receivedAt":"2024-09-05T10:08:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of this round of memory leak fixes.\n\nThere are only some smallish changes compared to v1:\n\n - Explain leak checking a bit more carefully and document the new\n `GIT_TEST_PASSING_SANITIZE_LEAK=check-failing` value in t/README.\n\n - Some more trivial commit message improvements.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (22):\n t/test-lib: allow skipping leak checks for passing tests\n fetch-pack: fix memory leaks on fetch negotiation\n send-pack: fix leaking common object IDs\n builtin/push: fix leaking refspec query result\n upload-pack: fix leaking child process data on reachability checks\n submodule: fix leaking fetch task data\n builtin/submodule--helper: fix leaking refs on push-check\n remote: fix leaking tracking refs\n remote: fix leak in reachability check of a remote-tracking ref\n send-pack: fix leaking push cert nonce\n gpg-interface: fix misdesigned signing key interfaces\n object: clear grafts when clearing parsed object pool\n shallow: free grafts when unregistering them\n shallow: fix leaking members of `struct shallow_info`\n negotiator/skipping: fix leaking commit entries\n builtin/repack: fix leaking line buffer when packing promisors\n builtin/pack-objects: plug leaking list of keep-packs\n builtin/grep: fix leaking object context\n builtin/fmt-merge-msg: fix leaking buffers\n match-trees: fix leaking prefixes in `shift_tree()`\n merge-ort: fix two leaks when handling directory rename modifications\n builtin/repack: fix leaking keep-pack list\n\n builtin/fmt-merge-msg.c | 2 ++\n builtin/grep.c | 1 +\n builtin/pack-objects.c | 1 +\n builtin/push.c | 8 +++--\n builtin/repack.c | 3 ++\n builtin/submodule--helper.c | 2 ++\n builtin/tag.c | 3 +-\n commit.c | 23 ++++--------\n commit.h | 3 +-\n fetch-pack.c | 3 ++\n gpg-interface.c | 26 ++++++++------\n gpg-interface.h | 4 +--\n match-trees.c | 10 ++++--\n merge-ort.c | 4 ++-\n negotiator/skipping.c | 7 ++--\n object.c | 14 +++++++-\n object.h | 4 ++-\n remote.c | 6 +++-\n repository.c | 2 +-\n send-pack.c | 52 ++++++++++++++++++----------\n shallow.c | 15 ++++++--\n submodule.c | 2 ++\n t/README | 3 ++\n t/t5516-fetch-push.sh | 1 +\n t/t5526-fetch-submodules.sh | 1 +\n t/t5531-deep-submodule-push.sh | 1 +\n t/t5533-push-cas.sh | 1 +\n t/t5534-push-signed.sh | 1 +\n t/t5537-fetch-shallow.sh | 1 +\n t/t5538-push-shallow.sh | 1 +\n t/t5549-fetch-push-http.sh | 1 +\n t/t5552-skipping-fetch-negotiator.sh | 2 ++\n t/t5616-partial-clone.sh | 1 +\n t/t6132-pathspec-exclude.sh | 1 +\n t/t6135-pathspec-with-attrs.sh | 2 ++\n t/t6200-fmt-merge-msg.sh | 1 +\n t/t6409-merge-subtree.sh | 1 +\n t/t6423-merge-rename-directories.sh | 1 +\n t/t6500-gc.sh | 1 +\n t/t7703-repack-geometric.sh | 1 +\n t/test-lib.sh | 11 +++++-\n upload-pack.c | 22 ++++++++----\n 42 files changed, 178 insertions(+), 72 deletions(-)\n\nRange-diff against v1:\n 1: 7c158acadf4 ! 1: fa62d0106a5 t/test-lib: allow skipping leak checks for passing tests\n @@ Commit message\n \n With `GIT_TEST_PASSING_SANITIZE_LEAK=check`, one can double check\n whether a memory leak fix caused some test suites to become leak free.\n + This is done by running all tests with the leak checker enabled. If a\n + test suite does not declare `TEST_PASSES_SANITIZE_LEAK=true` but still\n + finishes successfully with the leak checker enabled, then this indicates\n + that the test is leak free and thus missing the annotation.\n +\n It is somewhat slow to execute though because it runs all of our test\n suites with the leak sanitizer enabled. It is also pointless in most\n cases, because the only test suites that need to be checked are those\n which _aren't_ yet marked with `TEST_PASSES_SANITIZE_LEAK=true`.\n \n - Introduce a new value \"check-failing\". If set, we will only check those\n - tests which are not yet marked as leak free.\n + Introduce a new value \"check-failing\". When set, we behave the same as\n + if \"check\" was passed, except that we only check those tests which do\n + not have `TEST_PASSES_SANITIZE_LEAK=true` set. This is significantly\n + faster than running all test suites but still fulfills the usecase of\n + finding newly-leak-free test suites.\n \n Signed-off-by: Patrick Steinhardt \n \n + ## t/README ##\n +@@ t/README: GIT_TEST_PASSING_SANITIZE_LEAK=check when combined with \"--immediate\"\n + will run to completion faster, and result in the same failing\n + tests.\n + \n ++GIT_TEST_PASSING_SANITIZE_LEAK=check-failing behaves the same as \"check\",\n ++but skips all tests which are already marked as leak-free.\n ++\n + GIT_TEST_PROTOCOL_VERSION=, when set, makes 'protocol.version'\n + default to n.\n + \n +\n ## t/test-lib.sh ##\n @@ t/test-lib.sh: then\n \t\tpasses_sanitize_leak=t\n 2: 33bc728a345 = 2: 611a29d1ca3 fetch-pack: fix memory leaks on fetch negotiation\n 3: a13db9777f0 = 3: 0d969962a39 send-pack: fix leaking common object IDs\n 4: 92fc97b3db8 = 4: 834a184c855 builtin/push: fix leaking refspec query result\n 5: 77023421e18 = 5: 17b0c4b577a upload-pack: fix leaking child process data on reachability checks\n 6: b4b65c3783c = 6: 872f39faece submodule: fix leaking fetch task data\n 7: e3d1ac1712f = 7: 3cbd6fe808e builtin/submodule--helper: fix leaking refs on push-check\n 8: 7fafcc53d23 = 8: 90647301de5 remote: fix leaking tracking refs\n 9: 1446e42f0ba = 9: bb845fc9ff1 remote: fix leak in reachability check of a remote-tracking ref\n10: 138a5ded35a ! 10: a1baba39bc5 send-pack: fix leaking push cert nonce\n @@ Commit message\n \n When retrieving the push cert nonce from the server, we first store the\n constant returned by `server_feature_value()` and then, if the nonce is\n - valid, we duplicate the nonce memory to extend its lifetime. We never\n - free the latter and thus cause a memory leak.\n + valid, we duplicate the nonce memory to a NUL-terminated string, so that\n + we can pass it to `generate_push_cert()`. We never free the latter and\n + thus cause a memory leak.\n \n Fix this by storing the limited-lifetime nonce into a scope-local\n variable such that the long-lived, allocated nonce can be easily freed\n11: a1fca8104b3 = 11: ddebe2f6f6b gpg-interface: fix misdesigned signing key interfaces\n12: 922b8640a55 = 12: 242f6b76db3 object: clear grafts when clearing parsed object pool\n13: ec9a5143241 = 13: 4747042cb6a shallow: free grafts when unregistering them\n14: 2a63030ff09 = 14: d3996c92d80 shallow: fix leaking members of `struct shallow_info`\n15: 920db3a2912 = 15: 66ed1151449 negotiator/skipping: fix leaking commit entries\n16: 19eb9073482 = 16: 78c1e5e1772 builtin/repack: fix leaking line buffer when packing promisors\n17: 017713f5a9c = 17: d2e108040fd builtin/pack-objects: plug leaking list of keep-packs\n18: 0722cb38ea9 ! 18: 9fd891f5222 builtin/grep: fix leaking object context\n @@ Commit message\n builtin/grep: fix leaking object context\n \n Even when `get_oid_with_context()` fails it may have allocated some data\n - in tthe object context. But we do not release it in git-grep(1) when the\n + in the object context. But we do not release it in git-grep(1) when the\n call fails, leading to a memory leak. Plug it.\n \n Signed-off-by: Patrick Steinhardt \n19: b37391c0d6b = 19: 84b68affa0d builtin/fmt-merge-msg: fix leaking buffers\n20: 05461e3b1c0 = 20: bbb8ab20229 match-trees: fix leaking prefixes in `shift_tree()`\n21: da1c23a9ccf = 21: db0e7a8733a merge-ort: fix two leaks when handling directory rename modifications\n22: 3d30c727fbc = 22: 2368924995f builtin/repack: fix leaking keep-pack list\n\nbase-commit: 098ca6ba562006675df6a6d0948400d2d955458b\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502208","messageId":"fa62d0106a5d46678e594864ab36f4a656284878.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 01/22] t/test-lib: allow skipping leak checks for passing tests","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:08:38Z","receivedAt":"2024-09-05T10:08:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"With `GIT_TEST_PASSING_SANITIZE_LEAK=check`, one can double check\nwhether a memory leak fix caused some test suites to become leak free.\nThis is done by running all tests with the leak checker enabled. If a\ntest suite does not declare `TEST_PASSES_SANITIZE_LEAK=true` but still\nfinishes successfully with the leak checker enabled, then this indicates\nthat the test is leak free and thus missing the annotation.\n\nIt is somewhat slow to execute though because it runs all of our test\nsuites with the leak sanitizer enabled. It is also pointless in most\ncases, because the only test suites that need to be checked are those\nwhich _aren't_ yet marked with `TEST_PASSES_SANITIZE_LEAK=true`.\n\nIntroduce a new value \"check-failing\". When set, we behave the same as\nif \"check\" was passed, except that we only check those tests which do\nnot have `TEST_PASSES_SANITIZE_LEAK=true` set. This is significantly\nfaster than running all test suites but still fulfills the usecase of\nfinding newly-leak-free test suites.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/README | 3 +++\n t/test-lib.sh | 11 ++++++++++-\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/t/README b/t/README\nindex 44c02d81298..8dcb778e260 100644\n--- a/t/README\n+++ b/t/README\n@@ -386,6 +386,9 @@ GIT_TEST_PASSING_SANITIZE_LEAK=check when combined with \"--immediate\"\n will run to completion faster, and result in the same failing\n tests.\n \n+GIT_TEST_PASSING_SANITIZE_LEAK=check-failing behaves the same as \"check\",\n+but skips all tests which are already marked as leak-free.\n+\n GIT_TEST_PROTOCOL_VERSION=, when set, makes 'protocol.version'\n default to n.\n \ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 54247604cbc..64bd36531c1 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1558,8 +1558,16 @@ then\n \t\tpasses_sanitize_leak=t\n \tfi\n \n-\tif test \"$GIT_TEST_PASSING_SANITIZE_LEAK\" = \"check\"\n+\tif test \"$GIT_TEST_PASSING_SANITIZE_LEAK\" = \"check\" ||\n+\t test \"$GIT_TEST_PASSING_SANITIZE_LEAK\" = \"check-failing\"\n \tthen\n+\t\tif test \"$GIT_TEST_PASSING_SANITIZE_LEAK\" = \"check-failing\" &&\n+\t\t test -n \"$passes_sanitize_leak\"\n+\t\tthen\n+\t\t\tskip_all=\"skipping leak-free $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=check-failing\"\n+\t\t\ttest_done\n+\t\tfi\n+\n \t\tsanitize_leak_check=t\n \t\tif test -n \"$invert_exit_code\"\n \t\tthen\n@@ -1597,6 +1605,7 @@ then\n \texport LSAN_OPTIONS\n \n elif test \"$GIT_TEST_PASSING_SANITIZE_LEAK\" = \"check\" ||\n+ test \"$GIT_TEST_PASSING_SANITIZE_LEAK\" = \"check-failing\" ||\n test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false\n then\n \tBAIL_OUT_ENV_NEEDS_SANITIZE_LEAK \"GIT_TEST_PASSING_SANITIZE_LEAK=true\"\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502209","messageId":"611a29d1ca30759b7c91597a580d93578382955c.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 02/22] fetch-pack: fix memory leaks on fetch negotiation","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:08:40Z","receivedAt":"2024-09-05T10:08:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We leak both the `nt_object_array` and `negotiator` structures in\n`negotiate_using_fetch()`. Plug both of these leaks.\n\nThese leaks were exposed by t5516, but fixing them is not sufficient to\nmake the whole test suite leak free.\n\nSigned-off-by: Patrick Steinhardt \n---\n fetch-pack.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 58b4581ad80..0ed82feda14 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -2227,7 +2227,10 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,\n \ttrace2_region_leave(\"fetch-pack\", \"negotiate_using_fetch\", the_repository);\n \ttrace2_data_intmax(\"negotiate_using_fetch\", the_repository,\n \t\t\t \"total_rounds\", negotiation_round);\n+\n \tclear_common_flag(acked_commits);\n+\tobject_array_clear(&nt_object_array);\n+\tnegotiator.release(&negotiator);\n \tstrbuf_release(&req_buf);\n }\n \n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502210","messageId":"0d969962a39bb48bc1831129fb154997bd06093e.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 03/22] send-pack: fix leaking common object IDs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:08:43Z","receivedAt":"2024-09-05T10:08:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're leaking the array of common object IDs in `send_pack()`. Fix this\nby creating a common exit path where we free the leaking data. While at\nit, unify some other cleanups now that we have a central place to put\nthem.\n\nSigned-off-by: Patrick Steinhardt \n---\n send-pack.c | 34 ++++++++++++++++++++++------------\n t/t5549-fetch-push-http.sh | 1 +\n 2 files changed, 23 insertions(+), 12 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex fa2f5eec17b..b224ef9fc5e 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -508,7 +508,8 @@ int send_pack(struct send_pack_args *args,\n \tif (!remote_refs) {\n \t\tfprintf(stderr, \"No refs in common and none specified; doing nothing.\\n\"\n \t\t\t\"Perhaps you should specify a branch.\\n\");\n-\t\treturn 0;\n+\t\tret = 0;\n+\t\tgoto out;\n \t}\n \n \tgit_config_get_bool(\"push.negotiate\", &push_negotiate);\n@@ -615,12 +616,11 @@ int send_pack(struct send_pack_args *args,\n \t\t\t * atomically, abort the whole operation.\n \t\t\t */\n \t\t\tif (use_atomic) {\n-\t\t\t\tstrbuf_release(&req_buf);\n-\t\t\t\tstrbuf_release(&cap_buf);\n \t\t\t\treject_atomic_push(remote_refs, args->send_mirror);\n \t\t\t\terror(\"atomic push failed for ref %s. status: %d\\n\",\n \t\t\t\t ref->name, ref->status);\n-\t\t\t\treturn args->porcelain ? 0 : -1;\n+\t\t\t\tret = args->porcelain ? 0 : -1;\n+\t\t\t\tgoto out;\n \t\t\t}\n \t\t\t/* else fallthrough */\n \t\tdefault:\n@@ -682,8 +682,6 @@ int send_pack(struct send_pack_args *args,\n \t\twrite_or_die(out, req_buf.buf, req_buf.len);\n \t\tpacket_flush(out);\n \t}\n-\tstrbuf_release(&req_buf);\n-\tstrbuf_release(&cap_buf);\n \n \tif (use_sideband && cmds_sent) {\n \t\tmemset(&demux, 0, sizeof(demux));\n@@ -721,7 +719,9 @@ int send_pack(struct send_pack_args *args,\n \t\t\t\tfinish_async(&demux);\n \t\t\t}\n \t\t\tfd[1] = -1;\n-\t\t\treturn -1;\n+\n+\t\t\tret = -1;\n+\t\t\tgoto out;\n \t\t}\n \t\tif (!args->stateless_rpc)\n \t\t\t/* Closed by pack_objects() via start_command() */\n@@ -746,10 +746,12 @@ int send_pack(struct send_pack_args *args,\n \t}\n \n \tif (ret < 0)\n-\t\treturn ret;\n+\t\tgoto out;\n \n-\tif (args->porcelain)\n-\t\treturn 0;\n+\tif (args->porcelain) {\n+\t\tret = 0;\n+\t\tgoto out;\n+\t}\n \n \tfor (ref = remote_refs; ref; ref = ref->next) {\n \t\tswitch (ref->status) {\n@@ -758,8 +760,16 @@ int send_pack(struct send_pack_args *args,\n \t\tcase REF_STATUS_OK:\n \t\t\tbreak;\n \t\tdefault:\n-\t\t\treturn -1;\n+\t\t\tret = -1;\n+\t\t\tgoto out;\n \t\t}\n \t}\n-\treturn 0;\n+\n+\tret = 0;\n+\n+out:\n+\toid_array_clear(&commons);\n+\tstrbuf_release(&req_buf);\n+\tstrbuf_release(&cap_buf);\n+\treturn ret;\n }\ndiff --git a/t/t5549-fetch-push-http.sh b/t/t5549-fetch-push-http.sh\nindex 2cdebcb7356..6377fb6d993 100755\n--- a/t/t5549-fetch-push-http.sh\n+++ b/t/t5549-fetch-push-http.sh\n@@ -5,6 +5,7 @@ test_description='fetch/push functionality using the HTTP protocol'\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-httpd.sh\n start_httpd\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502211","messageId":"834a184c855c6e7b788771ccfd6a4f63dbacc358.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 04/22] builtin/push: fix leaking refspec query result","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:08:46Z","receivedAt":"2024-09-05T10:08:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When appending a refspec via `refspec_append_mapped()` we leak the\nresult of `query_refspecs()`. The overall logic around refspec queries\nis quite weird, as callers are expected to either set the `src` or `dst`\npointers, and then the (allocated) result will be in the respective\nother struct member.\n\nAs we have the `src` member set, plugging the memory leak is thus as\neasy as just freeing the `dst` member. While at it, use designated\ninitializers to initialize the structure.\n\nThis leak was exposed by t5516, but fixing it is not sufficient to make\nthe whole test suite leak free.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/push.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 7a67398124f..0b123eb9c1e 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -72,13 +72,15 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,\n \tconst char *branch_name;\n \n \tif (remote->push.nr) {\n-\t\tstruct refspec_item query;\n-\t\tmemset(&query, 0, sizeof(struct refspec_item));\n-\t\tquery.src = matched->name;\n+\t\tstruct refspec_item query = {\n+\t\t\t.src = matched->name,\n+\t\t};\n+\n \t\tif (!query_refspecs(&remote->push, &query) && query.dst) {\n \t\t\trefspec_appendf(refspec, \"%s%s:%s\",\n \t\t\t\t\tquery.force ? \"+\" : \"\",\n \t\t\t\t\tquery.src, query.dst);\n+\t\t\tfree(query.dst);\n \t\t\treturn;\n \t\t}\n \t}\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502212","messageId":"17b0c4b577afc0edbedb52299528c89e65ee5a92.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 05/22] upload-pack: fix leaking child process data on reachability checks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:08:48Z","receivedAt":"2024-09-05T10:08:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We spawn a git-rev-list(1) command to perform reachability checks in\n\"upload-pack.c\". We do not release memory associated with the process\nin error cases though, thus leaking memory.\n\nFix these by calling `child_process_clear()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5516-fetch-push.sh | 1 +\n upload-pack.c | 22 ++++++++++++++++------\n 2 files changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 9d693eb57f7..331778bd42c 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -19,6 +19,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 D=$(pwd)\ndiff --git a/upload-pack.c b/upload-pack.c\nindex f03ba3e98be..c84c3c3b1f5 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -709,10 +709,13 @@ static int get_reachable_list(struct upload_pack_data *data,\n \tstruct object *o;\n \tchar namebuf[GIT_MAX_HEXSZ + 2]; /* ^ + hash + LF */\n \tconst unsigned hexsz = the_hash_algo->hexsz;\n+\tint ret;\n \n \tif (do_reachable_revlist(&cmd, &data->shallows, reachable,\n-\t\t\t\t data->allow_uor) < 0)\n-\t\treturn -1;\n+\t\t\t\t data->allow_uor) < 0) {\n+\t\tret = -1;\n+\t\tgoto out;\n+\t}\n \n \twhile ((i = read_in_full(cmd.out, namebuf, hexsz + 1)) == hexsz + 1) {\n \t\tstruct object_id oid;\n@@ -736,10 +739,16 @@ static int get_reachable_list(struct upload_pack_data *data,\n \t}\n \tclose(cmd.out);\n \n-\tif (finish_command(&cmd))\n-\t\treturn -1;\n+\tif (finish_command(&cmd)) {\n+\t\tret = -1;\n+\t\tgoto out;\n+\t}\n \n-\treturn 0;\n+\tret = 0;\n+\n+out:\n+\tchild_process_clear(&cmd);\n+\treturn ret;\n }\n \n static int has_unreachable(struct object_array *src, enum allow_uor allow_uor)\n@@ -749,7 +758,7 @@ static int has_unreachable(struct object_array *src, enum allow_uor allow_uor)\n \tint i;\n \n \tif (do_reachable_revlist(&cmd, src, NULL, allow_uor) < 0)\n-\t\treturn 1;\n+\t\tgoto error;\n \n \t/*\n \t * The commits out of the rev-list are not ancestors of\n@@ -775,6 +784,7 @@ static int has_unreachable(struct object_array *src, enum allow_uor allow_uor)\n error:\n \tif (cmd.out >= 0)\n \t\tclose(cmd.out);\n+\tchild_process_clear(&cmd);\n \treturn 1;\n }\n \n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502213","messageId":"872f39faece60cba10aeb4606fd3ff28b59ee568.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 06/22] submodule: fix leaking fetch task data","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:08:53Z","receivedAt":"2024-09-05T10:08:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `submodule_parallel_fetch` structure contains various data\nstructures that we use to set up parallel fetches of submodules. We do\nnot free some of its data though, causing memory leaks. Plug those.\n\nSigned-off-by: Patrick Steinhardt \n---\n submodule.c | 2 ++\n t/t5526-fetch-submodules.sh | 1 +\n 2 files changed, 3 insertions(+)\n\ndiff --git a/submodule.c b/submodule.c\nindex 97516b0fec1..97d0d47b561 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1883,6 +1883,8 @@ int fetch_submodules(struct repository *r,\n out:\n \tfree_submodules_data(&spf.changed_submodule_names);\n \tstring_list_clear(&spf.seen_submodule_names, 0);\n+\tstrbuf_release(&spf.submodules_with_errors);\n+\tfree(spf.oid_fetch_tasks);\n \treturn spf.result;\n }\n \ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 5e566205ba4..2cfb5bd6bb1 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -6,6 +6,7 @@ test_description='Recursive \"git fetch\" for submodules'\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 pwd=$(pwd)\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502214","messageId":"3cbd6fe808eb934407c031faeece08459531c487.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 07/22] builtin/submodule--helper: fix leaking refs on push-check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:08:56Z","receivedAt":"2024-09-05T10:09:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In the push-check subcommand of the submodule helper we acquire a list\nof local refs, but never free that list. Fix this memory leak.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/submodule--helper.c | 2 ++\n t/t5531-deep-submodule-push.sh | 1 +\n 2 files changed, 3 insertions(+)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 85fb23dee84..642a0edabf0 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2958,7 +2958,9 @@ static int push_check(int argc, const char **argv, const char *prefix UNUSED)\n \t\t\t\t rs->src);\n \t\t\t}\n \t\t}\n+\n \t\trefspec_clear(&refspec);\n+\t\tfree_refs(local_refs);\n \t}\n \tfree(head);\n \ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nindex f3fff557447..135823630a3 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.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 test_expect_success setup '\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502215","messageId":"90647301de58b519f36fb0d677ed7be99924769b.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 08/22] remote: fix leaking tracking refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:08:59Z","receivedAt":"2024-09-05T10:09:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When computing the remote tracking ref we cause two memory leaks:\n\n - We leak when `remote_tracking()` fails.\n\n - We leak when the call to `remote_tracking()` succeeds and sets\n `ref->tracking_ref()`.\n\nFix both of these leaks.\n\nSigned-off-by: Patrick Steinhardt \n---\n remote.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex 8f3dee13186..240311619ab 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1123,6 +1123,7 @@ void free_one_ref(struct ref *ref)\n \t\treturn;\n \tfree_one_ref(ref->peer_ref);\n \tfree(ref->remote_status);\n+\tfree(ref->tracking_ref);\n \tfree(ref->symref);\n \tfree(ref);\n }\n@@ -2620,8 +2621,10 @@ static int remote_tracking(struct remote *remote, const char *refname,\n \tdst = apply_refspecs(&remote->fetch, refname);\n \tif (!dst)\n \t\treturn -1; /* no tracking ref for refname at remote */\n-\tif (refs_read_ref(get_main_ref_store(the_repository), dst, oid))\n+\tif (refs_read_ref(get_main_ref_store(the_repository), dst, oid)) {\n+\t\tfree(dst);\n \t\treturn -1; /* we know what the tracking ref is but we cannot read it */\n+\t}\n \n \t*dst_refname = dst;\n \treturn 0;\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502216","messageId":"bb845fc9ff14f2bba8443ad6ab6ab243e8542e47.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 09/22] remote: fix leak in reachability check of a remote-tracking ref","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:09:01Z","receivedAt":"2024-09-05T10:09:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `check_if_includes_upstream()` we retrieve the local ref\ncorresponding to a remote-tracking ref we want to check reachability\nfor. We never free that local ref and thus cause a memory leak. Fix\nthis.\n\nSigned-off-by: Patrick Steinhardt \n---\n remote.c | 1 +\n t/t5533-push-cas.sh | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/remote.c b/remote.c\nindex 240311619ab..bff54046b2c 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -2774,6 +2774,7 @@ static void check_if_includes_upstream(struct ref *remote)\n \n \tif (is_reachable_in_reflog(local->name, remote) <= 0)\n \t\tremote->unreachable = 1;\n+\tfree_one_ref(local);\n }\n \n static void apply_cas(struct push_cas_option *cas,\ndiff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh\nindex cba26a872dd..6365d99777e 100755\n--- a/t/t5533-push-cas.sh\n+++ b/t/t5533-push-cas.sh\n@@ -5,6 +5,7 @@ test_description='compare & swap push force/delete safety'\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 setup_srcdst_basic () {\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502217","messageId":"a1baba39bc555fc8b9d82b0148c7ad5449ed3bf9.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 10/22] send-pack: fix leaking push cert nonce","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:09:04Z","receivedAt":"2024-09-05T10:09:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When retrieving the push cert nonce from the server, we first store the\nconstant returned by `server_feature_value()` and then, if the nonce is\nvalid, we duplicate the nonce memory to a NUL-terminated string, so that\nwe can pass it to `generate_push_cert()`. We never free the latter and\nthus cause a memory leak.\n\nFix this by storing the limited-lifetime nonce into a scope-local\nvariable such that the long-lived, allocated nonce can be easily freed\nwithout having to cast away its constness.\n\nThis leak was exposed by t5534, but fixing it is not sufficient to make\nthe whole test suite leak free.\n\nSigned-off-by: Patrick Steinhardt \n---\n send-pack.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex b224ef9fc5e..c37f6ab3c07 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -501,7 +501,7 @@ int send_pack(struct send_pack_args *args,\n \tunsigned cmds_sent = 0;\n \tint ret;\n \tstruct async demux;\n-\tconst char *push_cert_nonce = NULL;\n+\tchar *push_cert_nonce = NULL;\n \tstruct packet_reader reader;\n \tint use_bitmaps;\n \n@@ -550,10 +550,11 @@ int send_pack(struct send_pack_args *args,\n \n \tif (args->push_cert != SEND_PACK_PUSH_CERT_NEVER) {\n \t\tsize_t len;\n-\t\tpush_cert_nonce = server_feature_value(\"push-cert\", &len);\n-\t\tif (push_cert_nonce) {\n-\t\t\treject_invalid_nonce(push_cert_nonce, len);\n-\t\t\tpush_cert_nonce = xmemdupz(push_cert_nonce, len);\n+\t\tconst char *nonce = server_feature_value(\"push-cert\", &len);\n+\n+\t\tif (nonce) {\n+\t\t\treject_invalid_nonce(nonce, len);\n+\t\t\tpush_cert_nonce = xmemdupz(nonce, len);\n \t\t} else if (args->push_cert == SEND_PACK_PUSH_CERT_ALWAYS) {\n \t\t\tdie(_(\"the receiving end does not support --signed push\"));\n \t\t} else if (args->push_cert == SEND_PACK_PUSH_CERT_IF_ASKED) {\n@@ -771,5 +772,6 @@ int send_pack(struct send_pack_args *args,\n \toid_array_clear(&commons);\n \tstrbuf_release(&req_buf);\n \tstrbuf_release(&cap_buf);\n+\tfree(push_cert_nonce);\n \treturn ret;\n }\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502218","messageId":"ddebe2f6f6b088cf1d2bfea5c89ef7e3fe96026e.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 11/22] gpg-interface: fix misdesigned signing key interfaces","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:09:07Z","receivedAt":"2024-09-05T10:09:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The interfaces to retrieve signing keys and their IDs are misdesigned as\nthey return string constants even though they indeed allocate memory,\nwhich leads to memory leaks. Refactor the code to instead always return\nallocated strings and let the callers free them accordingly.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/tag.c | 3 ++-\n commit.c | 9 ++++++---\n gpg-interface.c | 26 +++++++++++++++-----------\n gpg-interface.h | 4 ++--\n send-pack.c | 6 ++++--\n t/t5534-push-signed.sh | 1 +\n 6 files changed, 30 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex a1fb218512c..ab3b500543d 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -160,7 +160,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid,\n \tconst struct git_hash_algo *compat = the_repository->compat_hash_algo;\n \tstruct strbuf sig = STRBUF_INIT, compat_sig = STRBUF_INIT;\n \tstruct strbuf compat_buf = STRBUF_INIT;\n-\tconst char *keyid = get_signing_key();\n+\tchar *keyid = get_signing_key();\n \tint ret = -1;\n \n \tif (sign_buffer(buffer, &sig, keyid))\n@@ -190,6 +190,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid,\n \tstrbuf_release(&sig);\n \tstrbuf_release(&compat_sig);\n \tstrbuf_release(&compat_buf);\n+\tfree(keyid);\n \treturn ret;\n }\n \ndiff --git a/commit.c b/commit.c\nindex 24ab5c1b509..ec9efc189d5 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1150,11 +1150,14 @@ int add_header_signature(struct strbuf *buf, struct strbuf *sig, const struct gi\n \n static int sign_commit_to_strbuf(struct strbuf *sig, struct strbuf *buf, const char *keyid)\n {\n+\tchar *keyid_to_free = NULL;\n+\tint ret = 0;\n \tif (!keyid || !*keyid)\n-\t\tkeyid = get_signing_key();\n+\t\tkeyid = keyid_to_free = get_signing_key();\n \tif (sign_buffer(buf, sig, keyid))\n-\t\treturn -1;\n-\treturn 0;\n+\t\tret = -1;\n+\tfree(keyid_to_free);\n+\treturn ret;\n }\n \n int parse_signed_commit(const struct commit *commit,\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 6587085cd19..cf6126b5aa0 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -45,8 +45,8 @@ struct gpg_format {\n \t\t\t\t size_t signature_size);\n \tint (*sign_buffer)(struct strbuf *buffer, struct strbuf *signature,\n \t\t\t const char *signing_key);\n-\tconst char *(*get_default_key)(void);\n-\tconst char *(*get_key_id)(void);\n+\tchar *(*get_default_key)(void);\n+\tchar *(*get_key_id)(void);\n };\n \n static const char *openpgp_verify_args[] = {\n@@ -86,9 +86,9 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,\n static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n \t\t\t const char *signing_key);\n \n-static const char *get_default_ssh_signing_key(void);\n+static char *get_default_ssh_signing_key(void);\n \n-static const char *get_ssh_key_id(void);\n+static char *get_ssh_key_id(void);\n \n static struct gpg_format gpg_format[] = {\n \t{\n@@ -847,7 +847,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key)\n }\n \n /* Returns the first public key from an ssh-agent to use for signing */\n-static const char *get_default_ssh_signing_key(void)\n+static char *get_default_ssh_signing_key(void)\n {\n \tstruct child_process ssh_default_key = CHILD_PROCESS_INIT;\n \tint ret = -1;\n@@ -899,12 +899,16 @@ static const char *get_default_ssh_signing_key(void)\n \treturn default_key;\n }\n \n-static const char *get_ssh_key_id(void) {\n-\treturn get_ssh_key_fingerprint(get_signing_key());\n+static char *get_ssh_key_id(void)\n+{\n+\tchar *signing_key = get_signing_key();\n+\tchar *key_id = get_ssh_key_fingerprint(signing_key);\n+\tfree(signing_key);\n+\treturn key_id;\n }\n \n /* Returns a textual but unique representation of the signing key */\n-const char *get_signing_key_id(void)\n+char *get_signing_key_id(void)\n {\n \tgpg_interface_lazy_init();\n \n@@ -916,17 +920,17 @@ const char *get_signing_key_id(void)\n \treturn get_signing_key();\n }\n \n-const char *get_signing_key(void)\n+char *get_signing_key(void)\n {\n \tgpg_interface_lazy_init();\n \n \tif (configured_signing_key)\n-\t\treturn configured_signing_key;\n+\t\treturn xstrdup(configured_signing_key);\n \tif (use_format->get_default_key) {\n \t\treturn use_format->get_default_key();\n \t}\n \n-\treturn git_committer_info(IDENT_STRICT | IDENT_NO_DATE);\n+\treturn xstrdup(git_committer_info(IDENT_STRICT | IDENT_NO_DATE));\n }\n \n const char *gpg_trust_level_to_str(enum signature_trust_level level)\ndiff --git a/gpg-interface.h b/gpg-interface.h\nindex 7cd98161f7f..e09f12e8d04 100644\n--- a/gpg-interface.h\n+++ b/gpg-interface.h\n@@ -80,13 +80,13 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature,\n const char *gpg_trust_level_to_str(enum signature_trust_level level);\n \n void set_signing_key(const char *);\n-const char *get_signing_key(void);\n+char *get_signing_key(void);\n \n /*\n * Returns a textual unique representation of the signing key in use\n * Either a GPG KeyID or a SSH Key Fingerprint\n */\n-const char *get_signing_key_id(void);\n+char *get_signing_key_id(void);\n int check_signature(struct signature_check *sigc,\n \t\t const char *signature, size_t slen);\n void print_signature_buffer(const struct signature_check *sigc,\ndiff --git a/send-pack.c b/send-pack.c\nindex c37f6ab3c07..31a62e6a98c 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -348,7 +348,8 @@ static int generate_push_cert(struct strbuf *req_buf,\n {\n \tconst struct ref *ref;\n \tstruct string_list_item *item;\n-\tchar *signing_key_id = xstrdup(get_signing_key_id());\n+\tchar *signing_key_id = get_signing_key_id();\n+\tchar *signing_key = get_signing_key();\n \tconst char *cp, *np;\n \tstruct strbuf cert = STRBUF_INIT;\n \tint update_seen = 0;\n@@ -381,7 +382,7 @@ static int generate_push_cert(struct strbuf *req_buf,\n \tif (!update_seen)\n \t\tgoto free_return;\n \n-\tif (sign_buffer(&cert, &cert, get_signing_key()))\n+\tif (sign_buffer(&cert, &cert, signing_key))\n \t\tdie(_(\"failed to sign the push certificate\"));\n \n \tpacket_buf_write(req_buf, \"push-cert%c%s\", 0, cap_string);\n@@ -394,6 +395,7 @@ static int generate_push_cert(struct strbuf *req_buf,\n \n free_return:\n \tfree(signing_key_id);\n+\tfree(signing_key);\n \tstrbuf_release(&cert);\n \treturn update_seen;\n }\ndiff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh\nindex c91a62b77af..d43aee0c327 100755\n--- a/t/t5534-push-signed.sh\n+++ b/t/t5534-push-signed.sh\n@@ -5,6 +5,7 @@ test_description='signed push'\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 \n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502219","messageId":"242f6b76db397cddf2b3f1ced69bdd0b36711192.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 12/22] object: clear grafts when clearing parsed object pool","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:09:12Z","receivedAt":"2024-09-05T10:09:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We do not clear grafts part of the parsed object pool when clearing the\npool itself, which can lead to memory leaks when a repository is being\ncleared.\n\nFix this by moving `reset_commit_grafts()` into \"object.c\" and making it\npart of the `struct parsed_object_pool` interface such that we can call\nit from `parsed_object_pool_clear()`. Adapt `parsed_object_pool_new()`\nto take and store a reference to its owning repository, which is needed\nby `unparse_commit()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n commit.c | 14 +-------------\n commit.h | 3 ++-\n object.c | 14 +++++++++++++-\n object.h | 4 +++-\n repository.c | 2 +-\n shallow.c | 2 +-\n 6 files changed, 21 insertions(+), 18 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex ec9efc189d5..bbef0e81c65 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -177,7 +177,7 @@ int commit_graft_pos(struct repository *r, const struct object_id *oid)\n \t\t commit_graft_oid_access);\n }\n \n-static void unparse_commit(struct repository *r, const struct object_id *oid)\n+void unparse_commit(struct repository *r, const struct object_id *oid)\n {\n \tstruct commit *c = lookup_commit(r, oid);\n \n@@ -318,18 +318,6 @@ int for_each_commit_graft(each_commit_graft_fn fn, void *cb_data)\n \treturn ret;\n }\n \n-void reset_commit_grafts(struct repository *r)\n-{\n-\tint i;\n-\n-\tfor (i = 0; i < r->parsed_objects->grafts_nr; i++) {\n-\t\tunparse_commit(r, &r->parsed_objects->grafts[i]->oid);\n-\t\tfree(r->parsed_objects->grafts[i]);\n-\t}\n-\tr->parsed_objects->grafts_nr = 0;\n-\tr->parsed_objects->commit_graft_prepared = 0;\n-}\n-\n struct commit_buffer {\n \tvoid *buffer;\n \tunsigned long size;\ndiff --git a/commit.h b/commit.h\nindex d62b1d93f95..5ba0f77b1eb 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -108,6 +108,8 @@ static inline int repo_parse_commit_no_graph(struct repository *r,\n \n void parse_commit_or_die(struct commit *item);\n \n+void unparse_commit(struct repository *r, const struct object_id *oid);\n+\n struct buffer_slab;\n struct buffer_slab *allocate_commit_buffer_slab(void);\n void free_commit_buffer_slab(struct buffer_slab *bs);\n@@ -240,7 +242,6 @@ int commit_graft_pos(struct repository *r, const struct object_id *oid);\n int register_commit_graft(struct repository *r, struct commit_graft *, int);\n void prepare_commit_graft(struct repository *r);\n struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid);\n-void reset_commit_grafts(struct repository *r);\n \n struct commit *get_fork_point(const char *refname, struct commit *commit);\n \ndiff --git a/object.c b/object.c\nindex d756c7f2ea3..94ea8fb8d2c 100644\n--- a/object.c\n+++ b/object.c\n@@ -545,11 +545,12 @@ void repo_clear_commit_marks(struct repository *r, unsigned int flags)\n \t}\n }\n \n-struct parsed_object_pool *parsed_object_pool_new(void)\n+struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)\n {\n \tstruct parsed_object_pool *o = xmalloc(sizeof(*o));\n \tmemset(o, 0, sizeof(*o));\n \n+\to->repo = repo;\n \to->blob_state = allocate_alloc_state();\n \to->tree_state = allocate_alloc_state();\n \to->commit_state = allocate_alloc_state();\n@@ -628,6 +629,16 @@ void raw_object_store_clear(struct raw_object_store *o)\n \thashmap_clear(&o->pack_map);\n }\n \n+void parsed_object_pool_reset_commit_grafts(struct parsed_object_pool *o)\n+{\n+\tfor (int i = 0; i < o->grafts_nr; i++) {\n+\t\tunparse_commit(o->repo, &o->grafts[i]->oid);\n+\t\tfree(o->grafts[i]);\n+\t}\n+\to->grafts_nr = 0;\n+\to->commit_graft_prepared = 0;\n+}\n+\n void parsed_object_pool_clear(struct parsed_object_pool *o)\n {\n \t/*\n@@ -659,6 +670,7 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)\n \tfree_commit_buffer_slab(o->buffer_slab);\n \to->buffer_slab = NULL;\n \n+\tparsed_object_pool_reset_commit_grafts(o);\n \tclear_alloc_state(o->blob_state);\n \tclear_alloc_state(o->tree_state);\n \tclear_alloc_state(o->commit_state);\ndiff --git a/object.h b/object.h\nindex 05691486ebf..17f32f1103e 100644\n--- a/object.h\n+++ b/object.h\n@@ -7,6 +7,7 @@ struct buffer_slab;\n struct repository;\n \n struct parsed_object_pool {\n+\tstruct repository *repo;\n \tstruct object **obj_hash;\n \tint nr_objs, obj_hash_size;\n \n@@ -31,8 +32,9 @@ struct parsed_object_pool {\n \tstruct buffer_slab *buffer_slab;\n };\n \n-struct parsed_object_pool *parsed_object_pool_new(void);\n+struct parsed_object_pool *parsed_object_pool_new(struct repository *repo);\n void parsed_object_pool_clear(struct parsed_object_pool *o);\n+void parsed_object_pool_reset_commit_grafts(struct parsed_object_pool *o);\n \n struct object_list {\n \tstruct object *item;\ndiff --git a/repository.c b/repository.c\nindex 9825a308993..e6fc2c6aa9d 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -54,7 +54,7 @@ void initialize_repository(struct repository *repo)\n {\n \trepo->objects = raw_object_store_new();\n \trepo->remote_state = remote_state_new();\n-\trepo->parsed_objects = parsed_object_pool_new();\n+\trepo->parsed_objects = parsed_object_pool_new(repo);\n \tALLOC_ARRAY(repo->index, 1);\n \tindex_state_init(repo->index, repo);\n \ndiff --git a/shallow.c b/shallow.c\nindex b8cd051e3b6..a10cf9e9d5d 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -97,7 +97,7 @@ static void reset_repository_shallow(struct repository *r)\n {\n \tr->parsed_objects->is_shallow = -1;\n \tstat_validity_clear(r->parsed_objects->shallow_stat);\n-\treset_commit_grafts(r);\n+\tparsed_object_pool_reset_commit_grafts(r->parsed_objects);\n }\n \n int commit_shallow_file(struct repository *r, struct shallow_lock *lk)\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502220","messageId":"4747042cb6a779c87eef35f23e127795828281db.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 13/22] shallow: free grafts when unregistering them","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:09:15Z","receivedAt":"2024-09-05T10:09:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When removing a graft via `unregister_shallow()` we remove it from the\ngrafts array, but do not free the structure. Fix this to plug the leak.\n\nSigned-off-by: Patrick Steinhardt \n---\n shallow.c | 4 +++-\n t/t5537-fetch-shallow.sh | 1 +\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/shallow.c b/shallow.c\nindex a10cf9e9d5d..7e0ee96ead9 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -51,10 +51,12 @@ int unregister_shallow(const struct object_id *oid)\n \tint pos = commit_graft_pos(the_repository, oid);\n \tif (pos < 0)\n \t\treturn -1;\n-\tif (pos + 1 < the_repository->parsed_objects->grafts_nr)\n+\tif (pos + 1 < the_repository->parsed_objects->grafts_nr) {\n+\t\tfree(the_repository->parsed_objects->grafts[pos]);\n \t\tMOVE_ARRAY(the_repository->parsed_objects->grafts + pos,\n \t\t\t the_repository->parsed_objects->grafts + pos + 1,\n \t\t\t the_repository->parsed_objects->grafts_nr - pos - 1);\n+\t}\n \tthe_repository->parsed_objects->grafts_nr--;\n \treturn 0;\n }\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 37f7547a4ca..cae4d400f32 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -5,6 +5,7 @@ test_description='fetch/clone from a shallow clone'\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 commit() {\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502221","messageId":"d3996c92d801dcd0bae463e04d4197b4d619482e.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 14/22] shallow: fix leaking members of `struct shallow_info`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:09:17Z","receivedAt":"2024-09-05T10:09:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We do not free several struct members in `clear_shallow_info()`. Fix\nthis to plug the resulting leaks.\n\nSigned-off-by: Patrick Steinhardt \n---\n shallow.c | 9 +++++++++\n t/t5538-push-shallow.sh | 1 +\n 2 files changed, 10 insertions(+)\n\ndiff --git a/shallow.c b/shallow.c\nindex 7e0ee96ead9..dcebc263d70 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -489,6 +489,15 @@ void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa)\n \n void clear_shallow_info(struct shallow_info *info)\n {\n+\tif (info->used_shallow) {\n+\t\tfor (size_t i = 0; i < info->shallow->nr; i++)\n+\t\t\tfree(info->used_shallow[i]);\n+\t\tfree(info->used_shallow);\n+\t}\n+\n+\tfree(info->need_reachability_test);\n+\tfree(info->reachable);\n+\tfree(info->shallow_ref);\n \tfree(info->ours);\n \tfree(info->theirs);\n }\ndiff --git a/t/t5538-push-shallow.sh b/t/t5538-push-shallow.sh\nindex e91fcc173e8..6adc3a20a45 100755\n--- a/t/t5538-push-shallow.sh\n+++ b/t/t5538-push-shallow.sh\n@@ -5,6 +5,7 @@ test_description='push from/to a shallow clone'\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 commit() {\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502222","messageId":"66ed1151449aad1d39d0d6fc10844e7938ea9209.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 15/22] negotiator/skipping: fix leaking commit entries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:09:20Z","receivedAt":"2024-09-05T10:09:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When releasing the skipping negotiator we free its priority queue, but\nnot the contained entries. Fix this to plug a memory leak.\n\nSigned-off-by: Patrick Steinhardt \n---\n negotiator/skipping.c | 7 +++++--\n t/t5552-skipping-fetch-negotiator.sh | 2 ++\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/negotiator/skipping.c b/negotiator/skipping.c\nindex f65d47858b4..b738fe4faef 100644\n--- a/negotiator/skipping.c\n+++ b/negotiator/skipping.c\n@@ -247,8 +247,11 @@ static int ack(struct fetch_negotiator *n, struct commit *c)\n \n static void release(struct fetch_negotiator *n)\n {\n-\tclear_prio_queue(&((struct data *)n->data)->rev_list);\n-\tFREE_AND_NULL(n->data);\n+\tstruct data *data = n->data;\n+\tfor (int i = 0; i < data->rev_list.nr; i++)\n+\t\tfree(data->rev_list.array[i].data);\n+\tclear_prio_queue(&data->rev_list);\n+\tFREE_AND_NULL(data);\n }\n \n void skipping_negotiator_init(struct fetch_negotiator *negotiator)\ndiff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh\nindex b55a9f65e6b..4f2e5ae8dfa 100755\n--- a/t/t5552-skipping-fetch-negotiator.sh\n+++ b/t/t5552-skipping-fetch-negotiator.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='test skipping fetch negotiator'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'fetch.negotiationalgorithm config' '\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502223","messageId":"78c1e5e1772d8920859588c4eaf67faab0b5730e.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 16/22] builtin/repack: fix leaking line buffer when packing promisors","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:09:23Z","receivedAt":"2024-09-05T10:09:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `repack_promisor_objects()` we read output from git-pack-objects(1)\nline by line, using `strbuf_getline_lf()`. We never free the line\nbuffer, causing a memory leak. Plug it.\n\nThis leak is being hit in t5616, but plugging it alone is not\nsufficient to make the whole test suite leak free.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/repack.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 8bb875532b4..a382754feee 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -425,9 +425,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \n \t\tfree(promisor_name);\n \t}\n+\n \tfclose(out);\n \tif (finish_command(&cmd))\n \t\tdie(_(\"could not finish pack-objects to repack promisor objects\"));\n+\tstrbuf_release(&line);\n }\n \n struct pack_geometry {\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502224","messageId":"d2e108040fd1064a25bc9cb416d301504529781d.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 17/22] builtin/pack-objects: plug leaking list of keep-packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:09:26Z","receivedAt":"2024-09-05T10:09:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `--keep-pack` option of git-pack-objects(1) populates the arguments\ninto a string list. And while the list is marked as `NODUP` and thus\nwon't duplicate the strings, the list entries themselves still need to\nbe free'd. We don't though, causing a leak.\n\nPlug it.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-objects.c | 1 +\n t/t5616-partial-clone.sh | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c481feadbfa..ab78d72e273 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4641,6 +4641,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n cleanup:\n \tclear_packing_data(&to_pack);\n \tlist_objects_filter_release(&filter_options);\n+\tstring_list_clear(&keep_pack_list, 0);\n \tstrvec_clear(&rp);\n \n \treturn 0;\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 2da7291e379..467c46dccf6 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -5,6 +5,7 @@ test_description='git partial clone'\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 # create a normal \"src\" repo where we can later create new commits.\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502225","messageId":"9fd891f5222163d1cc61281ec9f91e9e35481eeb.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 18/22] builtin/grep: fix leaking object context","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:09:31Z","receivedAt":"2024-09-05T10:09:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Even when `get_oid_with_context()` fails it may have allocated some data\nin the object context. But we do not release it in git-grep(1) when the\ncall fails, leading to a memory leak. Plug it.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/grep.c | 1 +\n t/t6132-pathspec-exclude.sh | 1 +\n t/t6135-pathspec-with-attrs.sh | 2 ++\n 3 files changed, 4 insertions(+)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex dfc3c3e8bd2..dda4582d646 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1133,6 +1133,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t &oid, &oc)) {\n \t\t\tif (seen_dashdash)\n \t\t\t\tdie(_(\"unable to resolve revision: %s\"), arg);\n+\t\t\tobject_context_release(&oc);\n \t\t\tbreak;\n \t\t}\n \ndiff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh\nindex 9fdafeb1e90..f31c09c056f 100755\n--- a/t/t6132-pathspec-exclude.sh\n+++ b/t/t6132-pathspec-exclude.sh\n@@ -2,6 +2,7 @@\n \n test_description='test case exclude pathspec'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex 120dcd74a51..794bc7daf05 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='test labels in pathspecs'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup a tree' '\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502226","messageId":"84b68affa0d8420ee907a6bb1af15c7f37b13485.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 19/22] builtin/fmt-merge-msg: fix leaking buffers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:09:34Z","receivedAt":"2024-09-05T10:09:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Fix leaking input and output buffers in git-fmt-merge-msg(1).\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fmt-merge-msg.c | 2 ++\n t/t6200-fmt-merge-msg.sh | 1 +\n 2 files changed, 3 insertions(+)\n\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 957786d1b3a..0b162f8fab1 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -67,6 +67,8 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)\n \t\treturn ret;\n \twrite_in_full(STDOUT_FILENO, output.buf, output.len);\n \n+\tstrbuf_release(&input);\n+\tstrbuf_release(&output);\n \tfree(inpath);\n \treturn 0;\n }\ndiff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh\nindex 5a221f8ef1f..ac57b0e4ae3 100755\n--- a/t/t6200-fmt-merge-msg.sh\n+++ b/t/t6200-fmt-merge-msg.sh\n@@ -8,6 +8,7 @@ test_description='fmt-merge-msg test'\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 \n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502227","messageId":"bbb8ab2022968c4f195d21d307da754f561aaa98.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 20/22] match-trees: fix leaking prefixes in `shift_tree()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:09:36Z","receivedAt":"2024-09-05T10:09:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `shift_tree()` we allocate two empty strings that we end up\npassing to `match_trees()`. If that function finds a better match it\nwill update these pointers to point to a newly allocated strings,\nfreeing the old strings. We never free the final results though, neither\nthe ones we have allocated ourselves, nor the one that `match_trees()`\nmight've returned to us.\n\nFix the resulting memory leaks by creating a common exit path where we\nfree them.\n\nSigned-off-by: Patrick Steinhardt \n---\n match-trees.c | 10 +++++++---\n t/t6409-merge-subtree.sh | 1 +\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/match-trees.c b/match-trees.c\nindex f17c74d483f..147b03abf18 100644\n--- a/match-trees.c\n+++ b/match-trees.c\n@@ -294,18 +294,22 @@ void shift_tree(struct repository *r,\n \t\tunsigned short mode;\n \n \t\tif (!*del_prefix)\n-\t\t\treturn;\n+\t\t\tgoto out;\n \n \t\tif (get_tree_entry(r, hash2, del_prefix, shifted, &mode))\n \t\t\tdie(\"cannot find path %s in tree %s\",\n \t\t\t del_prefix, oid_to_hex(hash2));\n-\t\treturn;\n+\t\tgoto out;\n \t}\n \n \tif (!*add_prefix)\n-\t\treturn;\n+\t\tgoto out;\n \n \tsplice_tree(hash1, add_prefix, hash2, shifted);\n+\n+out:\n+\tfree(add_prefix);\n+\tfree(del_prefix);\n }\n \n /*\ndiff --git a/t/t6409-merge-subtree.sh b/t/t6409-merge-subtree.sh\nindex e9ba6f1690d..528615b981f 100755\n--- a/t/t6409-merge-subtree.sh\n+++ b/t/t6409-merge-subtree.sh\n@@ -5,6 +5,7 @@ test_description='subtree merge 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 \n test_expect_success setup '\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502228","messageId":"db0e7a8733ae52e5fe0f513ebbfacac5a4ac5bbb.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 21/22] merge-ort: fix two leaks when handling directory rename modifications","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:09:39Z","receivedAt":"2024-09-05T10:09:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are two leaks in `apply_directory_rename_modifications()`:\n\n - We do not release the `dirs_to_insert` string list.\n\n - We do not release some `conflict_info` we put into the\n `opt->priv->paths` string map.\n\nThe former is trivial to fix. The latter is a bit less straight forward:\nthe `util` pointer of the string map may sometimes point to data that\nhas been allocated via `CALLOC()`, while at other times it may point to\ndata that has been allocated via a `mem_pool`.\n\nIt very much seems like an oversight that we didn't also allocate the\nconflict info in this code path via the memory pool, though. So let's\nfix that, which will also plug the memory leak for us.\n\nSigned-off-by: Patrick Steinhardt \n---\n merge-ort.c | 4 +++-\n t/t6423-merge-rename-directories.sh | 1 +\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 3752c7e595d..0ed3cd06b1a 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2710,7 +2710,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,\n \t\tstruct conflict_info *dir_ci;\n \t\tchar *cur_dir = dirs_to_insert.items[i].string;\n \n-\t\tCALLOC_ARRAY(dir_ci, 1);\n+\t\tdir_ci = mem_pool_calloc(&opt->priv->pool, 1, sizeof(*dir_ci));\n \n \t\tdir_ci->merged.directory_name = parent_name;\n \t\tlen = strlen(parent_name);\n@@ -2838,6 +2838,8 @@ static void apply_directory_rename_modifications(struct merge_options *opt,\n \t * Finally, record the new location.\n \t */\n \tpair->two->path = new_path;\n+\n+\tstring_list_clear(&dirs_to_insert, 0);\n }\n \n /*** Function Grouping: functions related to regular rename detection ***/\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex 88d1cf2cde9..4aaaf38be68 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -25,6 +25,7 @@ test_description=\"recursive merge with directory renames\"\n # underscore notation is to differentiate different\n # files that might be renamed into each other's paths.)\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-merge.sh\n \n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502229","messageId":"2368924995ff5cbb6b95dcbaa46a7ee18cd52d03.1725530720.git.ps@pks.im","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"[PATCH v2 22/22] builtin/repack: fix leaking keep-pack list","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:09:41Z","receivedAt":"2024-09-05T10:09:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The list of packs to keep is populated via a command line option but\nnever free'd. Plug this memory leak.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/repack.c | 1 +\n t/t6500-gc.sh | 1 +\n t/t7703-repack-geometric.sh | 1 +\n 3 files changed, 3 insertions(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex a382754feee..3ee8cfa732f 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -1543,6 +1543,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t}\n \n cleanup:\n+\tstring_list_clear(&keep_pack_list, 0);\n \tstring_list_clear(&names, 1);\n \texisting_packs_release(&existing);\n \tfree_pack_geometry(&geometry);\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 1b5909d1b70..58654b3437e 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -3,6 +3,7 @@\n test_description='basic git gc tests\n '\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 9fc1626fbfd..8877aea98ba 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -2,6 +2,7 @@\n \n test_description='git repack --geometric works correctly'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n GIT_TEST_MULTI_PACK_INDEX=0\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502413","messageId":"xmqq1q1tx1u2.fsf@gitster.g","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"Re: [PATCH v2 00/22] Memory leak fixes (pt.6)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-08T21:39:01Z","receivedAt":"2024-09-08T21:39:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> There are only some smallish changes compared to v1:\n>\n> - Explain leak checking a bit more carefully and document the new\n> `GIT_TEST_PASSING_SANITIZE_LEAK=check-failing` value in t/README.\n>\n> - Some more trivial commit message improvements.\n\nThese changes looked trivially good ;-) Shall we mark the topic for\n'next' soonish, or are there others who want to comment?\n\nThanks.\n"},{"id":"502715","messageId":"xmqq1q1o4nui.fsf@gitster.g","threadId":"62002","inReplyTo":"cover.1725530720.git.ps@pks.im","subject":"Re: [PATCH v2 00/22] Memory leak fixes (pt.6)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-12T20:29:57Z","receivedAt":"2024-09-12T20:30:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Hi,\n>\n> this is the second version of this round of memory leak fixes.\n>\n> There are only some smallish changes compared to v1:\n>\n> - Explain leak checking a bit more carefully and document the new\n> `GIT_TEST_PASSING_SANITIZE_LEAK=check-failing` value in t/README.\n>\n> - Some more trivial commit message improvements.\n>\n> Thanks!\n\nLooking good. Let me mark the topic for 'next' soonish.\n\nThanks.\n"}]}