{"thread":{"id":"62187","subject":"[PATCH 0/28] leak fixes for http fetch/push","startedAt":"2024-09-24T21:49:32Z","lastAt":"2024-09-27T03:55:37Z","messageCount":49,"participants":["Jeff King","René Scharfe","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":28},"messages":[{"id":"503407","messageId":"20240924214930.GA1143523@coredump.intra.peff.net","threadId":"62187","inReplyTo":null,"subject":"[PATCH 0/28] leak fixes for http fetch/push","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T21:49:30Z","receivedAt":"2024-09-24T21:49:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Patrick asked me to take a look at the remaining leaks in the http\npush/fetch code, especially the dumb variants. So here are enough\npatches to all of these scripts running leak-free:\n\n t5500-fetch-pack.sh\n t5539-fetch-http-shallow.sh\n t5540-http-push-webdav.sh\n t5541-http-push-smart.sh\n t5542-push-http-shallow.sh\n t5550-http-fetch-dumb.sh\n t5551-http-fetch-smart.sh\n t5582-fetch-negative-refspec.sh\n t5619-clone-local-ambiguous-transport.sh\n\nI did have to cheat a little and steal some of the patches from his\n\"leak fixes part 8\" work-in-progress (which are at the front of this\nseries). But otherwise this should be independent of the other\nleak-fixing work and can be applied directly on master.\n\nI tried to put patches touching similar areas together, but there's\nprobably still a bit of a jumble, just because I got there by picking at\nthe pile of leak logs for each script. :)\n\nMost of them are pretty obvious \"add a free call\" one-liners. If you\nreally want to put on your thinking cap, try patches 7, 14, and 21 (I'm\ngoing to pretend I spaced them out evenly for your reading pleasure).\n\n [01/28]: http-fetch: clear leaking git-index-pack(1) arguments\n [02/28]: shallow: fix leak when unregistering last shallow root\n [03/28]: fetch-pack: fix leaking sought refs\n [04/28]: connect: clear child process before freeing in diagnostic mode\n [05/28]: fetch-pack: free object filter before exiting\n [06/28]: fetch-pack, send-pack: clean up shallow oid array\n [07/28]: commit: avoid leaking already-saved buffer\n [08/28]: send-pack: free cas options before exit\n [09/28]: transport-helper: fix strbuf leak in push_refs_with_push()\n [10/28]: fetch: free \"raw\" string when shrinking refspec\n [11/28]: fetch-pack: clear pack lockfiles list\n [12/28]: transport-helper: fix leak of dummy refs_list\n [13/28]: http: fix leak when redacting cookies from curl trace\n [14/28]: http: fix leak of http_object_request struct\n [15/28]: http: call git_inflate_end() when releasing http_object_request\n [16/28]: http: stop leaking buffer in http_get_info_packs()\n [17/28]: remote-curl: free HEAD ref with free_one_ref()\n [18/28]: http-walker: free fake packed_git list\n [19/28]: http-push: clear refspecs before exiting\n [20/28]: http-push: free repo->url string\n [21/28]: http-push: free curl header lists\n [22/28]: http-push: free transfer_request dest field\n [23/28]: http-push: free transfer_request strbuf\n [24/28]: http-push: free remote_ls_ctx.dentry_name\n [25/28]: http-push: free xml_ctx.cdata after use\n [26/28]: http-push: clean up objects list\n [27/28]: http-push: clean up loose request when falling back to packed\n [28/28]: http-push: clean up local_refs at exit\n\n builtin/fetch-pack.c | 14 +++++++-\n builtin/fetch.c | 1 +\n builtin/push.c | 1 +\n builtin/send-pack.c | 2 ++\n commit.c | 3 +-\n connect.c | 1 +\n http-fetch.c | 16 ++++++---\n http-push.c | 40 +++++++++++++++-------\n http-walker.c | 18 +++++++---\n http.c | 16 ++++++---\n http.h | 4 +--\n refspec.c | 2 +-\n refspec.h | 2 +-\n remote-curl.c | 2 +-\n remote.c | 2 +-\n remote.h | 1 +\n shallow.c | 5 ++-\n t/t5500-fetch-pack.sh | 1 +\n t/t5539-fetch-http-shallow.sh | 1 +\n t/t5540-http-push-webdav.sh | 1 +\n t/t5541-http-push-smart.sh | 1 +\n t/t5542-push-http-shallow.sh | 1 +\n t/t5550-http-fetch-dumb.sh | 1 +\n t/t5551-http-fetch-smart.sh | 1 +\n t/t5582-fetch-negative-refspec.sh | 1 +\n t/t5619-clone-local-ambiguous-transport.sh | 1 +\n t/t5700-protocol-v1.sh | 1 +\n transport-helper.c | 11 ++++--\n transport.c | 11 +++++-\n 29 files changed, 123 insertions(+), 39 deletions(-)\n\n-Peff\n"},{"id":"503408","messageId":"20240924215009.GA1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 01/28] http-fetch: clear leaking git-index-pack(1) arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T21:50:09Z","receivedAt":"2024-09-24T21:50:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Patrick Steinhardt \n\nWe never clear the arguments that we pass to git-index-pack(1). Create a\ncommon exit path and release them there to plug this leak.\n\nThis is leak is exposed by t5702, but plugging the leak does not make\nthe whole test suite pass.\n\nSigned-off-by: Patrick Steinhardt \nSigned-off-by: Jeff King \n---\nObviously I didn't write this or the next two patches, but FWIW, I\nreviewed them and they all look good to me.\n\n http-fetch.c | 16 +++++++++++-----\n 1 file changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/http-fetch.c b/http-fetch.c\nindex d460bb1837..02ab80533f 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -106,6 +106,7 @@ int cmd_main(int argc, const char **argv)\n \tint nongit;\n \tstruct object_id packfile_hash;\n \tstruct strvec index_pack_args = STRVEC_INIT;\n+\tint ret;\n \n \tsetup_git_directory_gently(&nongit);\n \n@@ -157,8 +158,8 @@ int cmd_main(int argc, const char **argv)\n \n \t\tfetch_single_packfile(&packfile_hash, argv[arg],\n \t\t\t\t index_pack_args.v);\n-\n-\t\treturn 0;\n+\t\tret = 0;\n+\t\tgoto out;\n \t}\n \n \tif (index_pack_args.nr)\n@@ -170,7 +171,12 @@ int cmd_main(int argc, const char **argv)\n \t\tcommit_id = (char **) &argv[arg++];\n \t\tcommits = 1;\n \t}\n-\treturn fetch_using_walker(argv[arg], get_verbosely, get_recover,\n-\t\t\t\t commits, commit_id, write_ref,\n-\t\t\t\t commits_on_stdin);\n+\n+\tret = fetch_using_walker(argv[arg], get_verbosely, get_recover,\n+\t\t\t\t commits, commit_id, write_ref,\n+\t\t\t\t commits_on_stdin);\n+\n+out:\n+\tstrvec_clear(&index_pack_args);\n+\treturn ret;\n }\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503409","messageId":"20240924215039.GB1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 02/28] shallow: fix leak when unregistering last shallow root","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T21:50:39Z","receivedAt":"2024-09-24T21:50:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Patrick Steinhardt \n\nWhen unregistering a shallow root we shrink the array of grafts by one\nand move remaining grafts one to the left. This can of course only\nhappen when there are any grafts left, because otherwise there is\nnothing to move. As such, this code is guarded by a condition that only\nperforms the move in case there are grafts after the position of the\ngraft to be unregistered.\n\nBy mistake we also put the call to free the unregistered graft into that\ncondition. But that doesn't make any sense, as we want to always free\nthe graft when it exists. Fix the resulting memory leak by doing so.\n\nThis leak is exposed by t5500, but plugging it does not make the whole\ntest suite pass.\n\nSigned-off-by: Patrick Steinhardt \nSigned-off-by: Jeff King \n---\n shallow.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/shallow.c b/shallow.c\nindex dcebc263d7..4bb1518dbc 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -51,12 +51,11 @@ 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-\t\tfree(the_repository->parsed_objects->grafts[pos]);\n+\tfree(the_repository->parsed_objects->grafts[pos]);\n+\tif (pos + 1 < the_repository->parsed_objects->grafts_nr)\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 }\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503410","messageId":"20240924215103.GC1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 03/28] fetch-pack: fix leaking sought refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T21:51:03Z","receivedAt":"2024-09-24T21:51:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Patrick Steinhardt \n\nWhen calling `fetch_pack()` the caller is expected to pass in a set of\nsought-after refs that they want to fetch. This array gets massaged to\nnot contain duplicate entries, which is done by replacing duplicate refs\nwith `NULL` pointers. This modifies the caller-provided array, and in\ncase we do unset any pointers the caller now loses track of that ref and\ncannot free it anymore.\n\nNow the obvious fix would be to not only unset these pointers, but to\nalso free their contents. But this doesn't work because callers continue\nto use those refs. Another potential solution would be to copy the array\nin `fetch_pack()` so that we dont modify the caller-provided one. But\nthat doesn't work either because the NULL-ness of those entries is used\nby callers to skip over ref entries that we didn't even try to fetch in\n`report_unmatched_refs()`.\n\nInstead, we make it the responsibility of our callers to duplicate these\narrays as needed. It ain't pretty, but it works to plug the memory leak.\n\nSigned-off-by: Patrick Steinhardt \nSigned-off-by: Jeff King \n---\n builtin/fetch-pack.c | 11 ++++++++++-\n t/t5700-protocol-v1.sh | 1 +\n transport.c | 11 ++++++++++-\n 3 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 49222a36fa..c5319232a5 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -53,6 +53,7 @@ int cmd_fetch_pack(int argc,\n \tstruct ref *fetched_refs = NULL, *remote_refs = NULL;\n \tconst char *dest = NULL;\n \tstruct ref **sought = NULL;\n+\tstruct ref **sought_to_free = NULL;\n \tint nr_sought = 0, alloc_sought = 0;\n \tint fd[2];\n \tstruct string_list pack_lockfiles = STRING_LIST_INIT_DUP;\n@@ -243,6 +244,13 @@ int cmd_fetch_pack(int argc,\n \t\tBUG(\"unknown protocol version\");\n \t}\n \n+\t/*\n+\t * Create a shallow copy of `sought` so that we can free all of its entries.\n+\t * This is because `fetch_pack()` will modify the array to evict some\n+\t * entries, but won't free those.\n+\t */\n+\tDUP_ARRAY(sought_to_free, sought, nr_sought);\n+\n \tfetched_refs = fetch_pack(&args, fd, remote_refs, sought, nr_sought,\n \t\t\t &shallow, pack_lockfiles_ptr, version);\n \n@@ -280,7 +288,8 @@ int cmd_fetch_pack(int argc,\n \t\t oid_to_hex(&ref->old_oid), ref->name);\n \n \tfor (size_t i = 0; i < nr_sought; i++)\n-\t\tfree_one_ref(sought[i]);\n+\t\tfree_one_ref(sought_to_free[i]);\n+\tfree(sought_to_free);\n \tfree(sought);\n \tfree_refs(fetched_refs);\n \tfree_refs(remote_refs);\ndiff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\nindex a73b4d4ff6..985e04d06e 100755\n--- a/t/t5700-protocol-v1.sh\n+++ b/t/t5700-protocol-v1.sh\n@@ -11,6 +11,7 @@ export GIT_TEST_PROTOCOL_VERSION\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # Test protocol v1 with 'git://' transport\ndiff --git a/transport.c b/transport.c\nindex 3c4714581f..1098bbd60e 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -414,7 +414,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \tstruct git_transport_data *data = transport->data;\n \tstruct ref *refs = NULL;\n \tstruct fetch_pack_args args;\n-\tstruct ref *refs_tmp = NULL;\n+\tstruct ref *refs_tmp = NULL, **to_fetch_dup = NULL;\n \n \tmemset(&args, 0, sizeof(args));\n \targs.uploadpack = data->options.uploadpack;\n@@ -477,6 +477,14 @@ static int fetch_refs_via_pack(struct transport *transport,\n \t\tgoto cleanup;\n \t}\n \n+\t/*\n+\t * Create a shallow copy of `sought` so that we can free all of its entries.\n+\t * This is because `fetch_pack()` will modify the array to evict some\n+\t * entries, but won't free those.\n+\t */\n+\tDUP_ARRAY(to_fetch_dup, to_fetch, nr_heads);\n+\tto_fetch = to_fetch_dup;\n+\n \trefs = fetch_pack(&args, data->fd,\n \t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n \t\t\t to_fetch, nr_heads, &data->shallow,\n@@ -500,6 +508,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \t\tret = -1;\n \tdata->conn = NULL;\n \n+\tfree(to_fetch_dup);\n \tfree_refs(refs_tmp);\n \tfree_refs(refs);\n \tlist_objects_filter_release(&args.filter_options);\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503411","messageId":"20240924215124.GD1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 04/28] connect: clear child process before freeing in diagnostic mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T21:51:24Z","receivedAt":"2024-09-24T21:51:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The git_connect() function has a special CONNECT_DIAG_URL mode, where we\nstop short of actually connecting to the other side and just print some\nparsing details. For URLs that require a child process (like ssh), we\nfree() the child_process struct but forget to clear it, leaking the\nstrings we stuffed into its \"env\" list.\n\nThis leak is triggered many times in t5500, which uses \"fetch-pack\n--diag-url\", but we're not yet ready to mark it as leak-free.\n\nSigned-off-by: Jeff King \n---\n connect.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/connect.c b/connect.c\nindex 6829ab3974..58f53d8dcb 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -1485,6 +1485,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \n \t\t\t\tfree(hostandport);\n \t\t\t\tfree(path);\n+\t\t\t\tchild_process_clear(conn);\n \t\t\t\tfree(conn);\n \t\t\t\tstrbuf_release(&cmd);\n \t\t\t\treturn NULL;\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503412","messageId":"20240924215207.GE1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 05/28] fetch-pack: free object filter before exiting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T21:52:07Z","receivedAt":"2024-09-24T21:52:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our fetch_pack_args holds a filter_options struct that may be populated\nwith allocated strings by the by the \"--filter\" command-line option. We\nmust free it before exiting to avoid a leak when the program exits.\n\nThe usual fetch code paths that use transport.c don't have the same\nleak, because we do the cleanup in disconnect_git().\n\nFixing this leak lets us mark t5500 as leak-free.\n\nSigned-off-by: Jeff King \n---\n builtin/fetch-pack.c | 1 +\n t/t5500-fetch-pack.sh | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex c5319232a5..cfc6951d23 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -293,5 +293,6 @@ int cmd_fetch_pack(int argc,\n \tfree(sought);\n \tfree_refs(fetched_refs);\n \tfree_refs(remote_refs);\n+\tlist_objects_filter_release(&args.filter_options);\n \treturn ret;\n }\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 585ea0ee16..605f17240c 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -8,6 +8,7 @@ test_description='Testing multi_ack pack fetching'\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 fetch-pack/upload-pack pair.\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503413","messageId":"20240924215225.GF1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 06/28] fetch-pack, send-pack: clean up shallow oid array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T21:52:25Z","receivedAt":"2024-09-24T21:52:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we call get_remote_heads() for protocol v0, that may populate the\n\"shallow\" oid_array, which must be cleaned up to avoid a leak at the\nprogram exit. The same problem exists for both fetch-pack and send-pack,\nbut not for the usual transport.c code paths, since we already do this\ncleanup in disconnect_git().\n\nFixing this lets us mark t5542 as leak-free for the send-pack side, but\nfetch-pack will need some more fixes before we can do the same for\nt5539.\n\nSigned-off-by: Jeff King \n---\n builtin/fetch-pack.c | 1 +\n builtin/send-pack.c | 1 +\n t/t5542-push-http-shallow.sh | 1 +\n 3 files changed, 3 insertions(+)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex cfc6951d23..ef4143eef3 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -294,5 +294,6 @@ int cmd_fetch_pack(int argc,\n \tfree_refs(fetched_refs);\n \tfree_refs(remote_refs);\n \tlist_objects_filter_release(&args.filter_options);\n+\toid_array_clear(&shallow);\n \treturn ret;\n }\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 81fc96d423..c49fe6c53c 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -343,5 +343,6 @@ int cmd_send_pack(int argc,\n \tfree_refs(remote_refs);\n \tfree_refs(local_refs);\n \trefspec_clear(&rs);\n+\toid_array_clear(&shallow);\n \treturn ret;\n }\ndiff --git a/t/t5542-push-http-shallow.sh b/t/t5542-push-http-shallow.sh\nindex c2cc83182f..07624a1d7f 100755\n--- a/t/t5542-push-http-shallow.sh\n+++ b/t/t5542-push-http-shallow.sh\n@@ -5,6 +5,7 @@ test_description='push from/to a shallow clone over http'\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.2.1011.gf1f9323e02\n\n"},{"id":"503415","messageId":"20240924215434.GG1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 07/28] commit: avoid leaking already-saved buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T21:54:34Z","receivedAt":"2024-09-24T21:54:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we parse a commit via repo_parse_commit_internal(), if\nsave_commit_buffer is set we'll stuff the buffer of the object contents\ninto a cache, overwriting any previous value.\n\nThis can result in a leak of that previously cached value, though it's\nrare in practice. If we have a value in the cache it would have come\nfrom a previous parse, and during that parse we'd set the object.parsed\nflag, causing any subsequent parse attempts to exit without doing any\nwork.\n\nBut it's possible to \"unparse\" a commit, which we do when registering a\ncommit graft. And since shallow fetches are implemented using grafts,\nthe leak is triggered in practice by t5539.\n\nThere are a number of possible ways to address this:\n\n 1. the unparsing function could clear the cached commit buffer, too. I\n think this would work for the case I found, but I'm not sure if\n there are other ways to end up in the same state (an unparsed\n commit with an entry in the commit buffer cache).\n\n 2. when we parse, we could check the buffer cache and prefer it to\n reading the contents from the object database. In theory the\n contents of a particular sha1 are immutable, but the code in\n question is violating the immutability with grafts. So this\n approach makes me a bit nervous, although I think it would work in\n practice (the grafts are applied to what we parse, but we still\n retain the original contents).\n\n 3. We could realize the cache is already populated and discard its\n contents before overwriting. It's possible some other code could be\n holding on to a pointer to the old cache entry (and we'd introduce\n a use-after-free), but I think the risk of that is relatively low.\n\n 4. The reverse of (3): when the cache is populated, don't bother\n saving our new copy. This is perhaps a little weird, since we'll\n have just populated the commit struct based on a different buffer.\n But the two buffers should be the same, even in the presence of\n grafts (as in (2) above).\n\nI went with option 4. It addresses the leak directly and doesn't carry\nany risk of breaking other assumptions. And it's the same technique used\nby parse_object_buffer() for this situation, though I'm not sure when it\nwould even come up there. The extra safety has been there since\nbd1e17e245 (Make \"parse_object()\" also fill in commit message buffer\ndata., 2005-05-25).\n\nThis lets us mark t5539 as leak-free.\n\nSigned-off-by: Jeff King \n---\n commit.c | 3 ++-\n t/t5539-fetch-http-shallow.sh | 1 +\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/commit.c b/commit.c\nindex 3a54e4db0d..cc03a93036 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -595,7 +595,8 @@ int repo_parse_commit_internal(struct repository *r,\n \t}\n \n \tret = parse_commit_buffer(r, item, buffer, size, 0);\n-\tif (save_commit_buffer && !ret) {\n+\tif (save_commit_buffer && !ret &&\n+\t !get_cached_commit_buffer(r, item, NULL)) {\n \t\tset_commit_buffer(r, item, buffer, size);\n \t\treturn 0;\n \t}\ndiff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh\nindex 3ea75d34ca..82fe09d0a9 100755\n--- a/t/t5539-fetch-http-shallow.sh\n+++ b/t/t5539-fetch-http-shallow.sh\n@@ -5,6 +5,7 @@ test_description='fetch/clone from a shallow clone over http'\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.2.1011.gf1f9323e02\n\n"},{"id":"503416","messageId":"20240924215539.GH1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 08/28] send-pack: free cas options before exit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T21:55:39Z","receivedAt":"2024-09-24T21:55:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The send-pack --force-with-lease option populates a push_cas_option\nstruct with allocated strings. Exiting without cleaning this up will\ncause leak-checkers to complain.\n\nWe can fix this by calling clear_cas_option(), after making it publicly\navailable. Previously it was used only for resetting the list when we\nsaw --no-force-with-lease.\n\nThe git-push command has the same \"leak\", though in this case it won't\ntrigger a leak-checker since it stores the push_cas_option struct as a\nglobal rather than on the stack (and is thus reachable even after main()\nexits). I've added cleanup for it here anyway, though, as\nfuture-proofing.\n\nThe leak is triggered by t5541 (it tests --force-with-lease over http,\nwhich requires a separate send-pack process under the hood), but we\ncan't mark it as leak-free yet.\n\nSigned-off-by: Jeff King \n---\n builtin/push.c | 1 +\n builtin/send-pack.c | 1 +\n remote.c | 2 +-\n remote.h | 1 +\n 4 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex e6f48969b8..59d4485603 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -669,6 +669,7 @@ int cmd_push(int argc,\n \trc = do_push(flags, push_options, remote);\n \tstring_list_clear(&push_options_cmdline, 0);\n \tstring_list_clear(&push_options_config, 0);\n+\tclear_cas_option(&cas);\n \tif (rc == -1)\n \t\tusage_with_options(push_usage, options);\n \telse\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex c49fe6c53c..8b1d46e79a 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -344,5 +344,6 @@ int cmd_send_pack(int argc,\n \tfree_refs(local_refs);\n \trefspec_clear(&rs);\n \toid_array_clear(&shallow);\n+\tclear_cas_option(&cas);\n \treturn ret;\n }\ndiff --git a/remote.c b/remote.c\nindex 390a03c264..e291e8ff5c 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -2544,7 +2544,7 @@ struct ref *get_stale_heads(struct refspec *rs, struct ref *fetch_map)\n /*\n * Compare-and-swap\n */\n-static void clear_cas_option(struct push_cas_option *cas)\n+void clear_cas_option(struct push_cas_option *cas)\n {\n \tint i;\n \ndiff --git a/remote.h b/remote.h\nindex a58713f20a..ad4513f639 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -409,6 +409,7 @@ struct push_cas_option {\n };\n \n int parseopt_push_cas_option(const struct option *, const char *arg, int unset);\n+void clear_cas_option(struct push_cas_option *);\n \n int is_empty_cas(const struct push_cas_option *);\n void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503417","messageId":"20240924215634.GI1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 09/28] transport-helper: fix strbuf leak in push_refs_with_push()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T21:56:34Z","receivedAt":"2024-09-24T21:56:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We loop over the refs to push, building up a strbuf with the set of\n\"push\" directives to send to the remote helper. But if the atomic-push\nflag is set and we hit a rejected ref, we'll bail from the function\nearly. We clean up most things, but forgot to release the strbuf.\n\nFixing this lets us mark t5541 as leak-free.\n\nSigned-off-by: Jeff King \n---\nArguably this whole function could benefit from a cleanup goto, but it's\na little non-trivial. So I stuck with simple-and-stupid.\n\n t/t5541-http-push-smart.sh | 1 +\n transport-helper.c | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex 71428f3d5c..3ad514bbd4 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -7,6 +7,7 @@ test_description='test smart pushing over http via http-backend'\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 ROOT_PATH=\"$PWD\"\ndiff --git a/transport-helper.c b/transport-helper.c\nindex c688967b8c..9c8abd8eca 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1023,6 +1023,7 @@ static int push_refs_with_push(struct transport *transport,\n \t\t\tif (atomic) {\n \t\t\t\treject_atomic_push(remote_refs, mirror);\n \t\t\t\tstring_list_clear(&cas_options, 0);\n+\t\t\t\tstrbuf_release(&buf);\n \t\t\t\treturn 0;\n \t\t\t} else\n \t\t\t\tcontinue;\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503418","messageId":"20240924215740.GJ1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 10/28] fetch: free \"raw\" string when shrinking refspec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T21:57:40Z","receivedAt":"2024-09-24T21:57:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"--prefetch\" option to git-fetch modifies the default refspec,\nincluding eliminating some entries entirely. When we drop an entry we\nfree the strings in the refspec_item, but we forgot to free the matching\nstring in the \"raw\" array of the refspec struct. There's no behavioral\nbug here (since we correctly shrink the raw array, too), but we're\nleaking the allocated string.\n\nLet's add in the leak-fix, and while we're at it drop \"const\" from\nthe type of the raw string array. These strings are always allocated by\nrefspec_append(), etc, and this makes the memory ownership more clear.\n\nThis is all a bit more intimate with the refspec code than I'd like, and\nI suspect it would be better if each refspec_item held on to its own raw\nstring, we had a single array, and we could use refspec_item_clear() to\nclean up everything. But that's a non-trivial refactoring, since\nrefspec_item structs can be held outside of a \"struct refspec\", without\nhaving a matching raw string at all. So let's leave that for now and\njust fix the leak in the most immediate way.\n\nThis lets us mark t5582 as leak-free.\n\nSigned-off-by: Jeff King \n---\n builtin/fetch.c | 1 +\n refspec.c | 2 +-\n refspec.h | 2 +-\n t/t5582-fetch-negative-refspec.sh | 1 +\n 4 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex c900f57721..80a64d0d26 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -456,6 +456,7 @@ static void filter_prefetch_refspec(struct refspec *rs)\n \n \t\t\tfree(rs->items[i].src);\n \t\t\tfree(rs->items[i].dst);\n+\t\t\tfree(rs->raw[i]);\n \n \t\t\tfor (j = i + 1; j < rs->nr; j++) {\n \t\t\t\trs->items[j - 1] = rs->items[j];\ndiff --git a/refspec.c b/refspec.c\nindex ec90ab349a..c3cf003443 100644\n--- a/refspec.c\n+++ b/refspec.c\n@@ -225,7 +225,7 @@ void refspec_clear(struct refspec *rs)\n \trs->nr = 0;\n \n \tfor (i = 0; i < rs->raw_nr; i++)\n-\t\tfree((char *)rs->raw[i]);\n+\t\tfree(rs->raw[i]);\n \tFREE_AND_NULL(rs->raw);\n \trs->raw_alloc = 0;\n \trs->raw_nr = 0;\ndiff --git a/refspec.h b/refspec.h\nindex 754be45cee..3760fdaf2b 100644\n--- a/refspec.h\n+++ b/refspec.h\n@@ -43,7 +43,7 @@ struct refspec {\n \tint alloc;\n \tint nr;\n \n-\tconst char **raw;\n+\tchar **raw;\n \tint raw_alloc;\n \tint raw_nr;\n \ndiff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh\nindex 7a80e47c2b..7fa54a4029 100755\n--- a/t/t5582-fetch-negative-refspec.sh\n+++ b/t/t5582-fetch-negative-refspec.sh\n@@ -8,6 +8,7 @@ test_description='\"git fetch\" with negative refspecs.\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success setup '\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503419","messageId":"20240924215800.GK1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 11/28] fetch-pack: clear pack lockfiles list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T21:58:00Z","receivedAt":"2024-09-24T21:58:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If the --lock-pack option is passed (which it typically is when\nfetch-pack is used under the hood by smart-http), then we may end up\nwith entries in our pack_lockfiles string_list. We need to clear them\nbefore returning to avoid a leak.\n\nIn git-fetch this isn't a problem, since the same cleanup happens via\ntransport_unlock_pack(). But the leak is detectable in t5551, which does\nhttp fetches.\n\nSigned-off-by: Jeff King \n---\n builtin/fetch-pack.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex ef4143eef3..62e8c3aa6b 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -295,5 +295,6 @@ int cmd_fetch_pack(int argc,\n \tfree_refs(remote_refs);\n \tlist_objects_filter_release(&args.filter_options);\n \toid_array_clear(&shallow);\n+\tstring_list_clear(&pack_lockfiles, 0);\n \treturn ret;\n }\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503420","messageId":"20240924215847.GL1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 12/28] transport-helper: fix leak of dummy refs_list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T21:58:47Z","receivedAt":"2024-09-24T21:58:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When using a remote-helper, the fetch_refs() function will issue a\n\"list\" command if we haven't already done so. We don't care about the\nresult, but this is just to maintain compatibility as explained in\nac3fda82bf (transport-helper: skip ls-refs if unnecessary, 2019-08-21).\n\nBut get_refs_list_using_list(), the function we call to issue the\ncommand, does parse and return the resulting ref list, which we simply\nleak. We should record the return value and free it immediately (another\napproach would be to teach it to avoid allocating at all, but it does\nnot seem worth the trouble to micro-optimize this mostly historical\ncase).\n\nTriggering this requires the v0 protocol (since in v2 we use stateless\nconnect to take over the connection). You can see it in t5551.37, \"fetch\nby SHA-1 without tag following\", as it explicitly enables v0.\n\nSigned-off-by: Jeff King \n---\n transport-helper.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 9c8abd8eca..013ec79dc9 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -717,8 +717,14 @@ static int fetch_refs(struct transport *transport,\n \t\treturn -1;\n \t}\n \n-\tif (!data->get_refs_list_called)\n-\t\tget_refs_list_using_list(transport, 0);\n+\tif (!data->get_refs_list_called) {\n+\t\t/*\n+\t\t * We do not care about the list of refs returned, but only\n+\t\t * that the \"list\" command was sent.\n+\t\t */\n+\t\tstruct ref *dummy = get_refs_list_using_list(transport, 0);\n+\t\tfree_refs(dummy);\n+\t}\n \n \tcount = 0;\n \tfor (i = 0; i < nr_heads; i++)\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503421","messageId":"20240924215914.GM1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 13/28] http: fix leak when redacting cookies from curl trace","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T21:59:14Z","receivedAt":"2024-09-24T21:59:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When redacting headers for GIT_TRACE_CURL, we build up a redacted cookie\nheader in a local strbuf, and then copy it into the output. But we\nforget to release the temporary strbuf, leaking it for every cookie\nheader we show.\n\nThe other redacted headers don't run into this problem, since they're\nable to work in-place in the output buffer. But the cookie parsing is\ntoo complicated for that, since we redact the cookies individually.\n\nThis leak is triggered by the cookie tests in t5551.\n\nSigned-off-by: Jeff King \n---\n http.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/http.c b/http.c\nindex 6c6cc5c822..cc136408c0 100644\n--- a/http.c\n+++ b/http.c\n@@ -800,6 +800,7 @@ static int redact_sensitive_header(struct strbuf *header, size_t offset)\n \n \t\tstrbuf_setlen(header, sensitive_header - header->buf);\n \t\tstrbuf_addbuf(header, &redacted_header);\n+\t\tstrbuf_release(&redacted_header);\n \t\tret = 1;\n \t}\n \treturn ret;\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503423","messageId":"20240924220109.GN1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 14/28] http: fix leak of http_object_request struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T22:01:09Z","receivedAt":"2024-09-24T22:01:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The new_http_object_request() function allocates a struct on the heap,\nalong with some fields inside the struct. But the matching function to\nclean it up, release_http_object_request(), only frees the interior\nfields without freeing the struct itself, causing a leak.\n\nThe related http_pack_request new/release pair gets this right, and at\nfirst glance we should be able to do the same thing and just add a\nsingle free() call. But there's a catch.\n\nThese http_object_request structs are typically embedded in the\nobject_request struct of http-walker.c. And when we clean up that parent\nstruct, it sanity-checks the embedded struct to make sure we are not\nleaking descriptors. Which means a use-after-free if we simply free()\nthe embedded struct.\n\nI have no idea how valuable that sanity-check is, or whether it can\nsimply be deleted. This all goes back to 5424bc557f (http*: add helper\nmethods for fetching objects (loose), 2009-06-06). But the obvious way\nto make it all work is to be sure we set the pointer to NULL after\nfreeing it (and our freeing process closes the descriptor, so we know\nthere is no leak).\n\nTo make sure we do that consistently, we'll switch the pointer we take\nin release_http_object_request() to a pointer-to-pointer, and we'll set\nit to NULL ourselves. And then the compiler can help us find each caller\nwhich needs to be updated.\n\nMost cases will just pass \"&obj_req->req\", which will obviously do the\nright thing. In a few cases, like http-push's finish_request(), we are\nworking with a copy of the pointer, so we don't NULL the original. But\nit's OK because the next step is to free the struct containing the\noriginal pointer anyway.\n\nThis lets us mark t5551 as leak-free. Ironically this is the \"smart\"\nhttp test, and the leak here only affects dumb http. But there's a\nsingle dumb-http invocation in there. The full dumb tests are in t5550,\nwhich still has some more leaks.\n\nThis also makes t5559 leak-free, as it's just an HTTP/2 variant of\nt5551. But we don't need to mark it as such, since it inherits the flag\nfrom t5551.\n\nSigned-off-by: Jeff King \n---\n http-push.c | 4 ++--\n http-walker.c | 8 ++++----\n http.c | 11 ++++++++---\n http.h | 4 ++--\n t/t5551-http-fetch-smart.sh | 1 +\n 5 files changed, 17 insertions(+), 11 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 7315a694aa..7196ffa525 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -275,7 +275,7 @@ static void start_fetch_loose(struct transfer_request *request)\n \tif (!start_active_slot(slot)) {\n \t\tfprintf(stderr, \"Unable to start GET request\\n\");\n \t\trepo->can_update_info_refs = 0;\n-\t\trelease_http_object_request(obj_req);\n+\t\trelease_http_object_request(&obj_req);\n \t\trelease_request(request);\n \t}\n }\n@@ -580,7 +580,7 @@ static void finish_request(struct transfer_request *request)\n \n \t\t/* Try fetching packed if necessary */\n \t\tif (request->obj->flags & LOCAL) {\n-\t\t\trelease_http_object_request(obj_req);\n+\t\t\trelease_http_object_request(&obj_req);\n \t\t\trelease_request(request);\n \t\t} else\n \t\t\tstart_fetch_packed(request);\ndiff --git a/http-walker.c b/http-walker.c\nindex e417a7f51c..9c1e5c37e6 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -74,7 +74,7 @@ static void start_object_request(struct object_request *obj_req)\n \tobj_req->state = ACTIVE;\n \tif (!start_active_slot(slot)) {\n \t\tobj_req->state = ABORTED;\n-\t\trelease_http_object_request(req);\n+\t\trelease_http_object_request(&req);\n \t\treturn;\n \t}\n }\n@@ -110,7 +110,7 @@ static void process_object_response(void *callback_data)\n \t\tif (obj_req->repo->next) {\n \t\t\tobj_req->repo =\n \t\t\t\tobj_req->repo->next;\n-\t\t\trelease_http_object_request(obj_req->req);\n+\t\t\trelease_http_object_request(&obj_req->req);\n \t\t\tstart_object_request(obj_req);\n \t\t\treturn;\n \t\t}\n@@ -495,7 +495,7 @@ static int fetch_object(struct walker *walker, unsigned char *hash)\n \n \tif (repo_has_object_file(the_repository, &obj_req->oid)) {\n \t\tif (obj_req->req)\n-\t\t\tabort_http_object_request(obj_req->req);\n+\t\t\tabort_http_object_request(&obj_req->req);\n \t\tabort_object_request(obj_req);\n \t\treturn 0;\n \t}\n@@ -543,7 +543,7 @@ static int fetch_object(struct walker *walker, unsigned char *hash)\n \t\tstrbuf_release(&buf);\n \t}\n \n-\trelease_http_object_request(req);\n+\trelease_http_object_request(&obj_req->req);\n \trelease_object_request(obj_req);\n \treturn ret;\n }\ndiff --git a/http.c b/http.c\nindex cc136408c0..d0242ffb50 100644\n--- a/http.c\n+++ b/http.c\n@@ -2816,15 +2816,17 @@ int finish_http_object_request(struct http_object_request *freq)\n \treturn freq->rename;\n }\n \n-void abort_http_object_request(struct http_object_request *freq)\n+void abort_http_object_request(struct http_object_request **freq_p)\n {\n+\tstruct http_object_request *freq = *freq_p;\n \tunlink_or_warn(freq->tmpfile.buf);\n \n-\trelease_http_object_request(freq);\n+\trelease_http_object_request(freq_p);\n }\n \n-void release_http_object_request(struct http_object_request *freq)\n+void release_http_object_request(struct http_object_request **freq_p)\n {\n+\tstruct http_object_request *freq = *freq_p;\n \tif (freq->localfile != -1) {\n \t\tclose(freq->localfile);\n \t\tfreq->localfile = -1;\n@@ -2838,4 +2840,7 @@ void release_http_object_request(struct http_object_request *freq)\n \t}\n \tcurl_slist_free_all(freq->headers);\n \tstrbuf_release(&freq->tmpfile);\n+\n+\tfree(freq);\n+\t*freq_p = NULL;\n }\ndiff --git a/http.h b/http.h\nindex a516ca4a9a..46e334c2c2 100644\n--- a/http.h\n+++ b/http.h\n@@ -240,8 +240,8 @@ struct http_object_request *new_http_object_request(\n \tconst char *base_url, const struct object_id *oid);\n void process_http_object_request(struct http_object_request *freq);\n int finish_http_object_request(struct http_object_request *freq);\n-void abort_http_object_request(struct http_object_request *freq);\n-void release_http_object_request(struct http_object_request *freq);\n+void abort_http_object_request(struct http_object_request **freq);\n+void release_http_object_request(struct http_object_request **freq);\n \n /*\n * Instead of using environment variables to determine if curl tracing happens,\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 7b5ab0eae1..e36dfde17e 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -5,6 +5,7 @@ test_description=\"test smart fetching over http via http-backend ($HTTP_PROTO)\"\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 test \"$HTTP_PROTO\" = \"HTTP/2\" && enable_http2\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503424","messageId":"20240924220213.GO1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 15/28] http: call git_inflate_end() when releasing http_object_request","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T22:02:13Z","receivedAt":"2024-09-24T22:02:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In new_http_object_request(), we initialize the zlib stream with\ngit_inflate_init(). We must have a matching git_inflate_end() to avoid\nleaking any memory allocated by zlib.\n\nIn most cases this happens in finish_http_object_request(), but we don't\nalways get there. If we abort a request mid-stream, then we may clean it\nup without hitting that function.\n\nWe can't just add a git_inflate_end() call to the release function,\nthough. That would double-free the cases that did actually finish.\nInstead, we'll move the call from the finish function to the release\nfunction. This does delay it for the cases that do finish, but I don't\nthink it matters. We should have already reached Z_STREAM_END (and\ncomplain if we didn't), and we do not record any status code from\ngit_inflate_end().\n\nThis leak is triggered by t5550 at least (and probably other dumb-http\ntests).\n\nI did find one other related spot of interest. If we try to read a\npreviously downloaded file and fail, we reset the stream by calling\nmemset() followed by a fresh git_inflate_init(). I don't think this case\nis triggered in the test suite, but it seemed like an obvious leak, so I\nadded the appropriate git_inflate_end() before the memset() there.\n\nSigned-off-by: Jeff King \n---\n http.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/http.c b/http.c\nindex d0242ffb50..4d841becca 100644\n--- a/http.c\n+++ b/http.c\n@@ -2726,6 +2726,7 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \t * file; also rewind to the beginning of the local file.\n \t */\n \tif (prev_read == -1) {\n+\t\tgit_inflate_end(&freq->stream);\n \t\tmemset(&freq->stream, 0, sizeof(freq->stream));\n \t\tgit_inflate_init(&freq->stream);\n \t\tthe_hash_algo->init_fn(&freq->c);\n@@ -2799,7 +2800,6 @@ int finish_http_object_request(struct http_object_request *freq)\n \t\treturn -1;\n \t}\n \n-\tgit_inflate_end(&freq->stream);\n \tthe_hash_algo->final_oid_fn(&freq->real_oid, &freq->c);\n \tif (freq->zret != Z_STREAM_END) {\n \t\tunlink_or_warn(freq->tmpfile.buf);\n@@ -2840,6 +2840,7 @@ void release_http_object_request(struct http_object_request **freq_p)\n \t}\n \tcurl_slist_free_all(freq->headers);\n \tstrbuf_release(&freq->tmpfile);\n+\tgit_inflate_end(&freq->stream);\n \n \tfree(freq);\n \t*freq_p = NULL;\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503425","messageId":"20240924220227.GP1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 16/28] http: stop leaking buffer in http_get_info_packs()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T22:02:27Z","receivedAt":"2024-09-24T22:02:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We use http_get_strbuf() to fetch the remote info/packs content into a\nstrbuf, but never free it, causing a leak. There's no need to hold onto\nit, as we've already parsed it completely.\n\nThis lets us mark t5619 as leak-free.\n\nSigned-off-by: Jeff King \n---\n http.c | 1 +\n t/t5619-clone-local-ambiguous-transport.sh | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex 4d841becca..54463770b4 100644\n--- a/http.c\n+++ b/http.c\n@@ -2475,6 +2475,7 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head)\n \n cleanup:\n \tfree(url);\n+\tstrbuf_release(&buf);\n \treturn ret;\n }\n \ndiff --git a/t/t5619-clone-local-ambiguous-transport.sh b/t/t5619-clone-local-ambiguous-transport.sh\nindex cce62bf78d..1d4efe414d 100755\n--- a/t/t5619-clone-local-ambiguous-transport.sh\n+++ b/t/t5619-clone-local-ambiguous-transport.sh\n@@ -2,6 +2,7 @@\n \n test_description='test local clone with ambiguous transport'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY/lib-httpd.sh\"\n \n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503427","messageId":"20240924220310.GQ1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 17/28] remote-curl: free HEAD ref with free_one_ref()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T22:03:10Z","receivedAt":"2024-09-24T22:03:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After dumb-http downloads the remote info/refs file, it adds an extra\nHEAD ref struct to our list by downloading the remote symref and finding\nthe matching ref within our list. If either of those fails, we throw\naway the ref struct. But we do so with free(), when we should use\nfree_one_ref() to catch any embedded allocations (in particular, if\nfetching the remote HEAD succeeded but the branch is unborn, its\nref->symref field will be populated but we'll still throw it all away).\n\nThis leak is triggered by t5550 (but we still have a little more work to\nmark it leak-free).\n\nSigned-off-by: Jeff King \n---\n remote-curl.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 4adcf25ed6..9a71e04301 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -347,7 +347,7 @@ static struct ref *parse_info_refs(struct discovery *heads)\n \t\tref->next = refs;\n \t\trefs = ref;\n \t} else {\n-\t\tfree(ref);\n+\t\tfree_one_ref(ref);\n \t}\n \n \treturn refs;\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503430","messageId":"20240924220412.GR1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 18/28] http-walker: free fake packed_git list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T22:04:12Z","receivedAt":"2024-09-24T22:04:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The dumb-http walker code creates a \"fake\" packed_git list representing\npacks we've downloaded from the remote (I call it \"fake\" because\ngenerally that struct is only used and managed by the local repository\nstruct). But during our cleanup phase we don't touch those at all,\ncausing a leak.\n\nThere's no support here from the rest of the object-database API, as\nthese structs are not meant to be freed, except when closing the object\nstore completely. But we can see that raw_object_store_clear() just\ncalls free() on them, and that's enough here to fix the leak.\n\nI also added a call to close_pack() before each. In the regular code\nthis happens via close_object_store(), which we do as part of\nraw_object_store_clear(). This is necessary to prevent leaking mmap'd\ndata (like the pack idx) or descriptors. The leak-checker won't catch\neither of these itself, but I did confirm with some hacky warning()\ncalls and running t5550 that it's easy to leak at least index data.\n\nThis is all much more intimate with the packed_git struct than I'd like,\nbut I think fixing it would be a pretty big refactor. And it's just not\nworth it for dumb-http code which is rarely used these days. If we can\nsilence the leak-checker without creating too much hassle, we should\njust do that.\n\nThis lets us mark t5550 as leak-free.\n\nSigned-off-by: Jeff King \n---\n http-walker.c | 10 ++++++++++\n t/t5550-http-fetch-dumb.sh | 1 +\n 2 files changed, 11 insertions(+)\n\ndiff --git a/http-walker.c b/http-walker.c\nindex 9c1e5c37e6..fb2d86d5e7 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -579,8 +579,18 @@ static void cleanup(struct walker *walker)\n \tif (data) {\n \t\talt = data->alt;\n \t\twhile (alt) {\n+\t\t\tstruct packed_git *pack;\n+\n \t\t\talt_next = alt->next;\n \n+\t\t\tpack = alt->packs;\n+\t\t\twhile (pack) {\n+\t\t\t\tstruct packed_git *pack_next = pack->next;\n+\t\t\t\tclose_pack(pack);\n+\t\t\t\tfree(pack);\n+\t\t\t\tpack = pack_next;\n+\t\t\t}\n+\n \t\t\tfree(alt->base);\n \t\t\tfree(alt);\n \ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex ea8e48f627..58189c9f7d 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -4,6 +4,7 @@ test_description='test dumb fetching over http via static file'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n if test_have_prereq !REFFILES\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503431","messageId":"20240924220430.GS1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 19/28] http-push: clear refspecs before exiting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T22:04:30Z","receivedAt":"2024-09-24T22:04:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We parse the command-line arguments into a refspec struct, but we never\nfree them. We should do so before exiting to avoid triggering the\nleak-checker.\n\nThis triggers in t5540 many times (basically every invocation of\nhttp-push).\n\nSigned-off-by: Jeff King \n---\n http-push.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/http-push.c b/http-push.c\nindex 7196ffa525..f60b2ceba5 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1983,5 +1983,7 @@ int cmd_main(int argc, const char **argv)\n \t\trequest = next_request;\n \t}\n \n+\trefspec_clear(&rs);\n+\n \treturn rc;\n }\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503432","messageId":"20240924220446.GT1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 20/28] http-push: free repo->url string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T22:04:46Z","receivedAt":"2024-09-24T22:04:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our repo->url string comes from str_end_url_with_slash(), which always\nallocates its output buffer. We should free it before exiting to avoid\ntriggering the leak-checker.\n\nThis can be seen by leak-checking t5540.\n\nSigned-off-by: Jeff King \n---\n http-push.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/http-push.c b/http-push.c\nindex f60b2ceba5..52c53928a9 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1972,6 +1972,7 @@ int cmd_main(int argc, const char **argv)\n cleanup:\n \tif (info_ref_lock)\n \t\tunlock_remote(info_ref_lock);\n+\tfree(repo->url);\n \tfree(repo);\n \n \thttp_cleanup();\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503433","messageId":"20240924220550.GU1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 21/28] http-push: free curl header lists","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T22:05:50Z","receivedAt":"2024-09-24T22:05:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"To pass headers to curl, we have to allocate a curl_slist linked list\nand then feed it to curl_easy_setopt(). But the header list is not\ncopied by curl, and must remain valid until we are finished with the\nrequest.\n\nA few spots in http-push get this right, freeing the list after\nfinishing the request, but many do not. In most cases the fix is simple:\nwe set up the curl slot, start it, and then use run_active_slot() to\ntake it to completion. After that, we don't need the headers anymore and\ncan call curl_slist_free_all().\n\nBut one case is trickier: when we do a MOVE request, we start the\nrequest but don't immediately finish it. It's possible we could change\nthis to be more like the other requests, but I didn't want to get into\nrisky refactoring of this code. So we need to stick the header list into\nthe request struct and remember to free it later.\n\nCuriously, the struct already has a headers field for this purpose! It\ngoes all the way back to 58e60dd203 (Add support for pushing to a remote\nrepository using HTTP/DAV, 2005-11-02), but it doesn't look like it was\never used. We can make use of it just by assigning our headers to it,\nand there is already code in finish_request() to clean it up.\n\nThis fixes several leaks triggered by t5540.\n\nSigned-off-by: Jeff King \n---\n http-push.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/http-push.c b/http-push.c\nindex 52c53928a9..451f7d14bb 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -437,9 +437,11 @@ static void start_move(struct transfer_request *request)\n \tif (start_active_slot(slot)) {\n \t\trequest->slot = slot;\n \t\trequest->state = RUN_MOVE;\n+\t\trequest->headers = dav_headers;\n \t} else {\n \t\trequest->state = ABORTED;\n \t\tFREE_AND_NULL(request->url);\n+\t\tcurl_slist_free_all(dav_headers);\n \t}\n }\n \n@@ -1398,6 +1400,7 @@ static int update_remote(const struct object_id *oid, struct remote_lock *lock)\n \tif (start_active_slot(slot)) {\n \t\trun_active_slot(slot);\n \t\tstrbuf_release(&out_buffer.buf);\n+\t\tcurl_slist_free_all(dav_headers);\n \t\tif (results.curl_result != CURLE_OK) {\n \t\t\tfprintf(stderr,\n \t\t\t\t\"PUT error: curl result=%d, HTTP code=%ld\\n\",\n@@ -1407,6 +1410,7 @@ static int update_remote(const struct object_id *oid, struct remote_lock *lock)\n \t\t}\n \t} else {\n \t\tstrbuf_release(&out_buffer.buf);\n+\t\tcurl_slist_free_all(dav_headers);\n \t\tfprintf(stderr, \"Unable to start PUT request\\n\");\n \t\treturn 0;\n \t}\n@@ -1516,6 +1520,7 @@ static void update_remote_info_refs(struct remote_lock *lock)\n \t\t\t\t\tresults.curl_result, results.http_code);\n \t\t\t}\n \t\t}\n+\t\tcurl_slist_free_all(dav_headers);\n \t}\n \tstrbuf_release(&buffer.buf);\n }\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503434","messageId":"20240924220638.GV1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 22/28] http-push: free transfer_request dest field","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T22:06:38Z","receivedAt":"2024-09-24T22:06:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we issue a PUT request, we store the destination in the \"dest\"\nfield by detaching from a strbuf. But we never free the result, causing\na leak.\n\nWe can address this in the release_request() function. But note that we\nalso need to initialize it to NULL, as most other request types do not\nset it at all.\n\nCuriously there are _two_ functions to initialize a transfer_request\nstruct. Adding the initialization only to add_fetch_request() seems to\nbe enough for t5540, but I won't pretend to understand why. Rather than\njust adding \"request->dest = NULL\" in both spots, let's zero the whole\nstruct. That addresses this problem, as well as any future ones (and it\ncan't possibly hurt, as by definition we'd be hitting uninitialized\nmemory previously).\n\nThis fixes several leaks noticed by t5540.\n\nSigned-off-by: Jeff King \n---\n http-push.c | 10 +++-------\n 1 file changed, 3 insertions(+), 7 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 451f7d14bb..9aa4d11ccd 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -514,6 +514,7 @@ static void release_request(struct transfer_request *request)\n \t}\n \n \tfree(request->url);\n+\tfree(request->dest);\n \tfree(request);\n }\n \n@@ -651,11 +652,8 @@ static void add_fetch_request(struct object *obj)\n \t\treturn;\n \n \tobj->flags |= FETCHING;\n-\trequest = xmalloc(sizeof(*request));\n+\tCALLOC_ARRAY(request, 1);\n \trequest->obj = obj;\n-\trequest->url = NULL;\n-\trequest->lock = NULL;\n-\trequest->headers = NULL;\n \trequest->state = NEED_FETCH;\n \trequest->next = request_queue_head;\n \trequest_queue_head = request;\n@@ -687,11 +685,9 @@ static int add_send_request(struct object *obj, struct remote_lock *lock)\n \t}\n \n \tobj->flags |= PUSHING;\n-\trequest = xmalloc(sizeof(*request));\n+\tCALLOC_ARRAY(request, 1);\n \trequest->obj = obj;\n-\trequest->url = NULL;\n \trequest->lock = lock;\n-\trequest->headers = NULL;\n \trequest->state = NEED_PUSH;\n \trequest->next = request_queue_head;\n \trequest_queue_head = request;\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503437","messageId":"20240924220849.GW1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 23/28] http-push: free transfer_request strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T22:08:49Z","receivedAt":"2024-09-24T22:08:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we issue a PUT, we initialize and fill a strbuf embedded in the\ntransfer_request struct. But we never release this buffer, causing a\nleak.\n\nWe can fix this by adding a strbuf_release() call to release_request().\nIf we stopped there, then non-PUT requests would try to release a\nzero-initialized strbuf. This works OK in practice, but we should try to\nfollow the strbuf API more closely. So instead, we'll always initialize\nthe strbuf when we create the transfer_request struct.\n\nThat in turn means switching the strbuf_init() call in start_put() to a\nsimple strbuf_grow().\n\nThis leak is triggered in t5540.\n\nSigned-off-by: Jeff King \n---\n http-push.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 9aa4d11ccd..8acdb3f265 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -375,7 +375,7 @@ static void start_put(struct transfer_request *request)\n \t/* Set it up */\n \tgit_deflate_init(&stream, zlib_compression_level);\n \tsize = git_deflate_bound(&stream, len + hdrlen);\n-\tstrbuf_init(&request->buffer.buf, size);\n+\tstrbuf_grow(&request->buffer.buf, size);\n \trequest->buffer.posn = 0;\n \n \t/* Compress it */\n@@ -515,6 +515,7 @@ static void release_request(struct transfer_request *request)\n \n \tfree(request->url);\n \tfree(request->dest);\n+\tstrbuf_release(&request->buffer.buf);\n \tfree(request);\n }\n \n@@ -655,6 +656,7 @@ static void add_fetch_request(struct object *obj)\n \tCALLOC_ARRAY(request, 1);\n \trequest->obj = obj;\n \trequest->state = NEED_FETCH;\n+\tstrbuf_init(&request->buffer.buf, 0);\n \trequest->next = request_queue_head;\n \trequest_queue_head = request;\n \n@@ -689,6 +691,7 @@ static int add_send_request(struct object *obj, struct remote_lock *lock)\n \trequest->obj = obj;\n \trequest->lock = lock;\n \trequest->state = NEED_PUSH;\n+\tstrbuf_init(&request->buffer.buf, 0);\n \trequest->next = request_queue_head;\n \trequest_queue_head = request;\n \n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503438","messageId":"20240924220937.GX1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 24/28] http-push: free remote_ls_ctx.dentry_name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T22:09:37Z","receivedAt":"2024-09-24T22:09:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The remote_ls_ctx struct has dentry_name string, which is filled in with\na heap allocation in the handle_remote_ls_ctx() XML callback. After the\nXML parse is done in remote_ls(), we should free the string to avoid a\nleak.\n\nThis fixes several leaks found by running t5540.\n\nSigned-off-by: Jeff King \n---\n http-push.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/http-push.c b/http-push.c\nindex 8acdb3f265..2e1c6851bb 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1183,6 +1183,7 @@ static void remote_ls(const char *path, int flags,\n \t}\n \n \tfree(ls.path);\n+\tfree(ls.dentry_name);\n \tfree(url);\n \tstrbuf_release(&out_buffer.buf);\n \tstrbuf_release(&in_buffer);\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503439","messageId":"20240924220954.GY1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 25/28] http-push: free xml_ctx.cdata after use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T22:09:54Z","receivedAt":"2024-09-24T22:09:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we ask libexpat to parse XML data, we sometimes set xml_cdata as a\nCharacterDataHandler callback. This fills in an allocated string in the\nxml_ctx struct which we never free, causing a leak.\n\nI won't pretend to understand the purpose of the field, but it looks\nlike it is used by other callbacks during the parse. At any rate, we\nnever look at it again after XML_Parse() returns, so we should be OK to\nfree() it then.\n\nThis fixes several leaks triggered by t5540.\n\nSigned-off-by: Jeff King \n---\n http-push.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/http-push.c b/http-push.c\nindex 2e1c6851bb..1146d7c6fe 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -913,6 +913,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)\n \t\t\tresult = XML_Parse(parser, in_buffer.buf,\n \t\t\t\t\t in_buffer.len, 1);\n \t\t\tfree(ctx.name);\n+\t\t\tfree(ctx.cdata);\n \t\t\tif (result != XML_STATUS_OK) {\n \t\t\t\tfprintf(stderr, \"XML error: %s\\n\",\n \t\t\t\t\tXML_ErrorString(\n@@ -1170,6 +1171,7 @@ static void remote_ls(const char *path, int flags,\n \t\t\tresult = XML_Parse(parser, in_buffer.buf,\n \t\t\t\t\t in_buffer.len, 1);\n \t\t\tfree(ctx.name);\n+\t\t\tfree(ctx.cdata);\n \n \t\t\tif (result != XML_STATUS_OK) {\n \t\t\t\tfprintf(stderr, \"XML error: %s\\n\",\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503440","messageId":"20240924221038.GZ1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 26/28] http-push: clean up objects list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T22:10:38Z","receivedAt":"2024-09-24T22:10:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In http-push's get_delta(), we generate a list of pending objects by\nrecursively processing trees and blobs, adding them to a linked list.\nAnd then we iterate over the list, adding a new request for each\nelement.\n\nBut since we iterate using the list head pointer, at the end it is NULL\nand all of the actual list structs have been leaked.\n\nWe can fix this either by using a separate iterator and then calling\nobject_list_free(), or by just freeing as we go. I picked the latter,\njust because it means we continue to shrink the list as we go, though\nI'm not sure it matters in practice (we call add_send_request() in the\nloop, but I don't think it ever looks at the global objects list\nitself).\n\nThis fixes several leaks noticed in t5540.\n\nSigned-off-by: Jeff King \n---\n http-push.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 1146d7c6fe..1cddd2fb37 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1374,9 +1374,13 @@ static int get_delta(struct rev_info *revs, struct remote_lock *lock)\n \t}\n \n \twhile (objects) {\n+\t\tstruct object_list *next = objects->next;\n+\n \t\tif (!(objects->item->flags & UNINTERESTING))\n \t\t\tcount += add_send_request(objects->item, lock);\n-\t\tobjects = objects->next;\n+\n+\t\tfree(objects);\n+\t\tobjects = next;\n \t}\n \n \treturn count;\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503441","messageId":"20240924221113.GA1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 27/28] http-push: clean up loose request when falling back to packed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T22:11:13Z","receivedAt":"2024-09-24T22:11:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In http-push's finish_request(), if we fail a loose object request we\nmay fall back to trying a packed request. But if we do so, we leave the\nhttp_loose_object_request in place, leaking it.\n\nWe can fix this by always cleaning it up. Note that the obj_req pointer\nhere (which we'll set to NULL) is a copy of the request->userData\npointer, which will now point to freed memory. But that's OK. We'll\neither release the parent request struct entirely, or we'll convert it\ninto a packed request, which will overwrite userData itself.\n\nThis leak is found by t5540, but it's not quite leak-free yet.\n\nSigned-off-by: Jeff King \n---\n http-push.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 1cddd2fb37..b36b1f9e35 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -582,9 +582,10 @@ static void finish_request(struct transfer_request *request)\n \t\t\tif (obj_req->rename == 0)\n \t\t\t\trequest->obj->flags |= (LOCAL | REMOTE);\n \n+\t\trelease_http_object_request(&obj_req);\n+\n \t\t/* Try fetching packed if necessary */\n \t\tif (request->obj->flags & LOCAL) {\n-\t\t\trelease_http_object_request(&obj_req);\n \t\t\trelease_request(request);\n \t\t} else\n \t\t\tstart_fetch_packed(request);\n-- \n2.46.2.1011.gf1f9323e02\n\n"},{"id":"503442","messageId":"20240924221239.GB1143820@coredump.intra.peff.net","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"[PATCH 28/28] http-push: clean up local_refs at exit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-24T22:12:39Z","receivedAt":"2024-09-24T22:12:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We allocate a list of ref structs from get_local_heads() but never clean\nit up. We should do so before exiting to avoid complaints from the\nleak-checker. Note that we have to initialize it to NULL, because\nthere's one code path that can jump to the cleanup label before we\nassign to it.\n\nFixing this lets us mark t5540 as leak-free.\n\nCuriously building with SANITIZE=leak and gcc does not seem to find this\nproblem, but switching to clang does. It seems like a fairly obvious\nleak, though.\n\nI was curious that the matching remote_refs did not have the same leak.\nBut that is because we store the list in a global variable, so it's\nstill reachable after we exit. Arguably we could treat it the same as\nfuture-proofing, but I didn't bother (now that the script is marked\nleak-free, anybody moving it to a stack variable will notice).\n\nSigned-off-by: Jeff King \n---\n http-push.c | 3 ++-\n t/t5540-http-push-webdav.sh | 1 +\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex b36b1f9e35..aad89f2eab 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1719,7 +1719,7 @@ int cmd_main(int argc, const char **argv)\n \tint rc = 0;\n \tint i;\n \tint new_refs;\n-\tstruct ref *ref, *local_refs;\n+\tstruct ref *ref, *local_refs = NULL;\n \n \tCALLOC_ARRAY(repo, 1);\n \n@@ -1997,6 +1997,7 @@ int cmd_main(int argc, const char **argv)\n \t}\n \n \trefspec_clear(&rs);\n+\tfree_refs(local_refs);\n \n \treturn rc;\n }\ndiff --git a/t/t5540-http-push-webdav.sh b/t/t5540-http-push-webdav.sh\nindex 37db3dec0c..27389b0908 100755\n--- a/t/t5540-http-push-webdav.sh\n+++ b/t/t5540-http-push-webdav.sh\n@@ -10,6 +10,7 @@ This test runs various sanity checks on http-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 \n if git http-push > /dev/null 2>&1 || [ $? -eq 128 ]\n-- \n2.46.2.1011.gf1f9323e02\n"},{"id":"503483","messageId":"5914d021-722c-4188-806c-1633bba3d3ab@web.de","threadId":"62187","inReplyTo":"20240924215103.GC1143820@coredump.intra.peff.net","subject":"Re: [PATCH 03/28] fetch-pack: fix leaking sought refs","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-09-25T17:17:24Z","receivedAt":"2024-09-25T17:17:39Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 24.09.24 um 23:51 schrieb Jeff King:\n> From: Patrick Steinhardt \n>\n> When calling `fetch_pack()` the caller is expected to pass in a set of\n> sought-after refs that they want to fetch. This array gets massaged to\n> not contain duplicate entries, which is done by replacing duplicate refs\n> with `NULL` pointers. This modifies the caller-provided array, and in\n> case we do unset any pointers the caller now loses track of that ref and\n> cannot free it anymore.\n>\n> Now the obvious fix would be to not only unset these pointers, but to\n> also free their contents. But this doesn't work because callers continue\n> to use those refs. Another potential solution would be to copy the array\n> in `fetch_pack()` so that we dont modify the caller-provided one. But\n> that doesn't work either because the NULL-ness of those entries is used\n> by callers to skip over ref entries that we didn't even try to fetch in\n> `report_unmatched_refs()`.\n>\n> Instead, we make it the responsibility of our callers to duplicate these\n> arrays as needed. It ain't pretty, but it works to plug the memory leak.\n>\n> Signed-off-by: Patrick Steinhardt \n> Signed-off-by: Jeff King \n> ---\n> builtin/fetch-pack.c | 11 ++++++++++-\n> t/t5700-protocol-v1.sh | 1 +\n> transport.c | 11 ++++++++++-\n> 3 files changed, 21 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n> index 49222a36fa..c5319232a5 100644\n> --- a/builtin/fetch-pack.c\n> +++ b/builtin/fetch-pack.c\n> @@ -53,6 +53,7 @@ int cmd_fetch_pack(int argc,\n> \tstruct ref *fetched_refs = NULL, *remote_refs = NULL;\n> \tconst char *dest = NULL;\n> \tstruct ref **sought = NULL;\n> +\tstruct ref **sought_to_free = NULL;\n> \tint nr_sought = 0, alloc_sought = 0;\n> \tint fd[2];\n> \tstruct string_list pack_lockfiles = STRING_LIST_INIT_DUP;\n> @@ -243,6 +244,13 @@ int cmd_fetch_pack(int argc,\n> \t\tBUG(\"unknown protocol version\");\n> \t}\n>\n> +\t/*\n> +\t * Create a shallow copy of `sought` so that we can free all of its entries.\n> +\t * This is because `fetch_pack()` will modify the array to evict some\n> +\t * entries, but won't free those.\n> +\t */\n> +\tDUP_ARRAY(sought_to_free, sought, nr_sought);\n> +\n> \tfetched_refs = fetch_pack(&args, fd, remote_refs, sought, nr_sought,\n> \t\t\t &shallow, pack_lockfiles_ptr, version);\n>\n> @@ -280,7 +288,8 @@ int cmd_fetch_pack(int argc,\n> \t\t oid_to_hex(&ref->old_oid), ref->name);\n>\n> \tfor (size_t i = 0; i < nr_sought; i++)\n> -\t\tfree_one_ref(sought[i]);\n> +\t\tfree_one_ref(sought_to_free[i]);\n> +\tfree(sought_to_free);\n\nOK.\n\n> \tfree(sought);\n> \tfree_refs(fetched_refs);\n> \tfree_refs(remote_refs);\n> diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\n> index a73b4d4ff6..985e04d06e 100755\n> --- a/t/t5700-protocol-v1.sh\n> +++ b/t/t5700-protocol-v1.sh\n> @@ -11,6 +11,7 @@ export GIT_TEST_PROTOCOL_VERSION\n> GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n>\n> +TEST_PASSES_SANITIZE_LEAK=true\n> . ./test-lib.sh\n>\n> # Test protocol v1 with 'git://' transport\n> diff --git a/transport.c b/transport.c\n> index 3c4714581f..1098bbd60e 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -414,7 +414,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n> \tstruct git_transport_data *data = transport->data;\n> \tstruct ref *refs = NULL;\n> \tstruct fetch_pack_args args;\n> -\tstruct ref *refs_tmp = NULL;\n> +\tstruct ref *refs_tmp = NULL, **to_fetch_dup = NULL;\n>\n> \tmemset(&args, 0, sizeof(args));\n> \targs.uploadpack = data->options.uploadpack;\n> @@ -477,6 +477,14 @@ static int fetch_refs_via_pack(struct transport *transport,\n> \t\tgoto cleanup;\n> \t}\n>\n> +\t/*\n> +\t * Create a shallow copy of `sought` so that we can free all of its entries.\n> +\t * This is because `fetch_pack()` will modify the array to evict some\n> +\t * entries, but won't free those.\n> +\t */\n> +\tDUP_ARRAY(to_fetch_dup, to_fetch, nr_heads);\n> +\tto_fetch = to_fetch_dup;\n> +\n> \trefs = fetch_pack(&args, data->fd,\n> \t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n> \t\t\t to_fetch, nr_heads, &data->shallow,\n> @@ -500,6 +508,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n> \t\tret = -1;\n> \tdata->conn = NULL;\n>\n> +\tfree(to_fetch_dup);\n\nThis makes a shallow copy and passes it to fetch_pack() and later to\nreport_unmatched_refs(). It shields callers of fetch_refs_via_pack()\nfrom the effect of fetch_pack()'s NULLification. This is what the\ncommit message says would not work. What am I missing?\n\n> \tfree_refs(refs_tmp);\n> \tfree_refs(refs);\n> \tlist_objects_filter_release(&args.filter_options);\n\n"},{"id":"503536","messageId":"ZvVK2gEYaMosdLH8@pks.im","threadId":"62187","inReplyTo":"5914d021-722c-4188-806c-1633bba3d3ab@web.de","subject":"Re: [PATCH 03/28] fetch-pack: fix leaking sought refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:52:04Z","receivedAt":"2024-09-26T11:52:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Sep 25, 2024 at 07:17:24PM +0200, René Scharfe wrote:\n> Am 24.09.24 um 23:51 schrieb Jeff King:\n> > From: Patrick Steinhardt \n> > diff --git a/transport.c b/transport.c\n> > index 3c4714581f..1098bbd60e 100644\n> > --- a/transport.c\n> > +++ b/transport.c\n> > @@ -414,7 +414,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n> > \tstruct git_transport_data *data = transport->data;\n> > \tstruct ref *refs = NULL;\n> > \tstruct fetch_pack_args args;\n> > -\tstruct ref *refs_tmp = NULL;\n> > +\tstruct ref *refs_tmp = NULL, **to_fetch_dup = NULL;\n> >\n> > \tmemset(&args, 0, sizeof(args));\n> > \targs.uploadpack = data->options.uploadpack;\n> > @@ -477,6 +477,14 @@ static int fetch_refs_via_pack(struct transport *transport,\n> > \t\tgoto cleanup;\n> > \t}\n> >\n> > +\t/*\n> > +\t * Create a shallow copy of `sought` so that we can free all of its entries.\n> > +\t * This is because `fetch_pack()` will modify the array to evict some\n> > +\t * entries, but won't free those.\n> > +\t */\n> > +\tDUP_ARRAY(to_fetch_dup, to_fetch, nr_heads);\n> > +\tto_fetch = to_fetch_dup;\n> > +\n> > \trefs = fetch_pack(&args, data->fd,\n> > \t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n> > \t\t\t to_fetch, nr_heads, &data->shallow,\n> > @@ -500,6 +508,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n> > \t\tret = -1;\n> > \tdata->conn = NULL;\n> >\n> > +\tfree(to_fetch_dup);\n> \n> This makes a shallow copy and passes it to fetch_pack() and later to\n> report_unmatched_refs(). It shields callers of fetch_refs_via_pack()\n> from the effect of fetch_pack()'s NULLification. This is what the\n> commit message says would not work. What am I missing?\n\n`fetch_refs_via_pack()` is called via `transport_fetch_refs()`, which\nconstructs the array of refs to fetch ad-hoc and then discards it\nimmediately. So it doesn't ever inspect the modified array in the first\nplace, and consequently we can guard against the NULLification here.\n\nThis code is quite intricate :/\n\nPatrick\n"},{"id":"503542","messageId":"ZvVmeygAJ3GhhQoC@pks.im","threadId":"62187","inReplyTo":"20240924215124.GD1143820@coredump.intra.peff.net","subject":"Re: [PATCH 04/28] connect: clear child process before freeing in diagnostic mode","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T13:49:55Z","receivedAt":"2024-09-26T13:50:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 24, 2024 at 05:51:24PM -0400, Jeff King wrote:\n> The git_connect() function has a special CONNECT_DIAG_URL mode, where we\n> stop short of actually connecting to the other side and just print some\n> parsing details. For URLs that require a child process (like ssh), we\n> free() the child_process struct but forget to clear it, leaking the\n> strings we stuffed into its \"env\" list.\n> \n> This leak is triggered many times in t5500, which uses \"fetch-pack\n> --diag-url\", but we're not yet ready to mark it as leak-free.\n> \n> Signed-off-by: Jeff King \n> ---\n> connect.c | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/connect.c b/connect.c\n> index 6829ab3974..58f53d8dcb 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -1485,6 +1485,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n> \n> \t\t\t\tfree(hostandport);\n> \t\t\t\tfree(path);\n> +\t\t\t\tchild_process_clear(conn);\n> \t\t\t\tfree(conn);\n> \t\t\t\tstrbuf_release(&cmd);\n> \t\t\t\treturn NULL;\n\nThere's only a single exit path in this function that ends up discarding\nthe `struct child_process`, so this looks good to me.\n\nPatrick\n"},{"id":"503543","messageId":"ZvVmhxWGJEWQiWYY@pks.im","threadId":"62187","inReplyTo":"20240924215207.GE1143820@coredump.intra.peff.net","subject":"Re: [PATCH 05/28] fetch-pack: free object filter before exiting","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T13:49:59Z","receivedAt":"2024-09-26T13:50:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 24, 2024 at 05:52:07PM -0400, Jeff King wrote:\n> Our fetch_pack_args holds a filter_options struct that may be populated\n> with allocated strings by the by the \"--filter\" command-line option. We\n\ns/by the by the/by the/\n\n> must free it before exiting to avoid a leak when the program exits.\n> \n> The usual fetch code paths that use transport.c don't have the same\n> leak, because we do the cleanup in disconnect_git().\n> \n> Fixing this leak lets us mark t5500 as leak-free.\n> \n> Signed-off-by: Jeff King \n> ---\n> builtin/fetch-pack.c | 1 +\n> t/t5500-fetch-pack.sh | 1 +\n> 2 files changed, 2 insertions(+)\n> \n> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n> index c5319232a5..cfc6951d23 100644\n> --- a/builtin/fetch-pack.c\n> +++ b/builtin/fetch-pack.c\n> @@ -293,5 +293,6 @@ int cmd_fetch_pack(int argc,\n> \tfree(sought);\n> \tfree_refs(fetched_refs);\n> \tfree_refs(remote_refs);\n> +\tlist_objects_filter_release(&args.filter_options);\n> \treturn ret;\n> }\n\nThis fix seems familiar to me, I think I also had it at one point in\ntime. Cannot find it in any of my series though, so... *shrug* Anyway,\nthis looks good to me.\n\nPatrick\n"},{"id":"503544","messageId":"ZvVmitkT-QUneqvb@pks.im","threadId":"62187","inReplyTo":"20240924215225.GF1143820@coredump.intra.peff.net","subject":"Re: [PATCH 06/28] fetch-pack, send-pack: clean up shallow oid array","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T13:50:02Z","receivedAt":"2024-09-26T13:50:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 24, 2024 at 05:52:25PM -0400, Jeff King wrote:\n> When we call get_remote_heads() for protocol v0, that may populate the\n> \"shallow\" oid_array, which must be cleaned up to avoid a leak at the\n> program exit. The same problem exists for both fetch-pack and send-pack,\n> but not for the usual transport.c code paths, since we already do this\n> cleanup in disconnect_git().\n> \n> Fixing this lets us mark t5542 as leak-free for the send-pack side, but\n> fetch-pack will need some more fixes before we can do the same for\n> t5539.\n> \n> Signed-off-by: Jeff King \n> ---\n> builtin/fetch-pack.c | 1 +\n> builtin/send-pack.c | 1 +\n> t/t5542-push-http-shallow.sh | 1 +\n> 3 files changed, 3 insertions(+)\n> \n> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n> index cfc6951d23..ef4143eef3 100644\n> --- a/builtin/fetch-pack.c\n> +++ b/builtin/fetch-pack.c\n> @@ -294,5 +294,6 @@ int cmd_fetch_pack(int argc,\n> \tfree_refs(fetched_refs);\n> \tfree_refs(remote_refs);\n> \tlist_objects_filter_release(&args.filter_options);\n> +\toid_array_clear(&shallow);\n> \treturn ret;\n> }\n\nI wonder about the early exit path we have when `finish_connect()`\nreturns non-zero. Should we make it go via the common cleanup path as\nwell, or do we not care in the error case?\n\n> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> index 81fc96d423..c49fe6c53c 100644\n> --- a/builtin/send-pack.c\n> +++ b/builtin/send-pack.c\n> @@ -343,5 +343,6 @@ int cmd_send_pack(int argc,\n> \tfree_refs(remote_refs);\n> \tfree_refs(local_refs);\n> \trefspec_clear(&rs);\n> +\toid_array_clear(&shallow);\n> \treturn ret;\n> }\n\nWe also have an early exit in this function when `match_push_refs()`\nreturns non-zero.\n\nPatrick\n"},{"id":"503545","messageId":"ZvVmjbMujO1h2sQp@pks.im","threadId":"62187","inReplyTo":"20240924215434.GG1143820@coredump.intra.peff.net","subject":"Re: [PATCH 07/28] commit: avoid leaking already-saved buffer","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T13:50:05Z","receivedAt":"2024-09-26T13:50:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 24, 2024 at 05:54:34PM -0400, Jeff King wrote:\n> When we parse a commit via repo_parse_commit_internal(), if\n> save_commit_buffer is set we'll stuff the buffer of the object contents\n> into a cache, overwriting any previous value.\n\nInteresting. I saw some cases that I think could be caused by this, but\ncouldn't make much sense of them.\n\n> This can result in a leak of that previously cached value, though it's\n> rare in practice. If we have a value in the cache it would have come\n> from a previous parse, and during that parse we'd set the object.parsed\n> flag, causing any subsequent parse attempts to exit without doing any\n> work.\n> \n> But it's possible to \"unparse\" a commit, which we do when registering a\n> commit graft. And since shallow fetches are implemented using grafts,\n> the leak is triggered in practice by t5539.\n> \n> There are a number of possible ways to address this:\n> \n> 1. the unparsing function could clear the cached commit buffer, too. I\n\ns/the/The/\n\n> think this would work for the case I found, but I'm not sure if\n> there are other ways to end up in the same state (an unparsed\n> commit with an entry in the commit buffer cache).\n> \n> 2. when we parse, we could check the buffer cache and prefer it to\n\ns/when/When/\n\n> reading the contents from the object database. In theory the\n> contents of a particular sha1 are immutable, but the code in\n> question is violating the immutability with grafts. So this\n> approach makes me a bit nervous, although I think it would work in\n> practice (the grafts are applied to what we parse, but we still\n> retain the original contents).\n> \n> 3. We could realize the cache is already populated and discard its\n> contents before overwriting. It's possible some other code could be\n> holding on to a pointer to the old cache entry (and we'd introduce\n> a use-after-free), but I think the risk of that is relatively low.\n> \n> 4. The reverse of (3): when the cache is populated, don't bother\n> saving our new copy. This is perhaps a little weird, since we'll\n> have just populated the commit struct based on a different buffer.\n> But the two buffers should be the same, even in the presence of\n> grafts (as in (2) above).\n> \n> I went with option 4. It addresses the leak directly and doesn't carry\n> any risk of breaking other assumptions. And it's the same technique used\n> by parse_object_buffer() for this situation, though I'm not sure when it\n> would even come up there. The extra safety has been there since\n> bd1e17e245 (Make \"parse_object()\" also fill in commit message buffer\n> data., 2005-05-25).\n\nOkay. This feels a bit weird indeed, but the fact that we already use\nthe same strategy in other places makes me feel way safer.\n\n> This lets us mark t5539 as leak-free.\n> \n> Signed-off-by: Jeff King \n> ---\n> commit.c | 3 ++-\n> t/t5539-fetch-http-shallow.sh | 1 +\n> 2 files changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/commit.c b/commit.c\n> index 3a54e4db0d..cc03a93036 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -595,7 +595,8 @@ int repo_parse_commit_internal(struct repository *r,\n> \t}\n> \n> \tret = parse_commit_buffer(r, item, buffer, size, 0);\n> -\tif (save_commit_buffer && !ret) {\n> +\tif (save_commit_buffer && !ret &&\n> +\t !get_cached_commit_buffer(r, item, NULL)) {\n> \t\tset_commit_buffer(r, item, buffer, size);\n> \t\treturn 0;\n> \t}\n\nAnd the fix is trivial.\n\nPatrick\n"},{"id":"503546","messageId":"ZvVmkXDIyCqpYKZm@pks.im","threadId":"62187","inReplyTo":"20240924215539.GH1143820@coredump.intra.peff.net","subject":"Re: [PATCH 08/28] send-pack: free cas options before exit","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T13:50:09Z","receivedAt":"2024-09-26T13:50:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 24, 2024 at 05:55:39PM -0400, Jeff King wrote:\n> diff --git a/remote.c b/remote.c\n> index 390a03c264..e291e8ff5c 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -2544,7 +2544,7 @@ struct ref *get_stale_heads(struct refspec *rs, struct ref *fetch_map)\n> /*\n> * Compare-and-swap\n> */\n> -static void clear_cas_option(struct push_cas_option *cas)\n> +void clear_cas_option(struct push_cas_option *cas)\n> {\n> \tint i;\n> \n> diff --git a/remote.h b/remote.h\n> index a58713f20a..ad4513f639 100644\n> --- a/remote.h\n> +++ b/remote.h\n> @@ -409,6 +409,7 @@ struct push_cas_option {\n> };\n> \n> int parseopt_push_cas_option(const struct option *, const char *arg, int unset);\n> +void clear_cas_option(struct push_cas_option *);\n\nNit: I was wondering whether we'd also want to fix up this functions\nname to conform to our style guide, which says this should be called\n`push_cas_option_clear()` instead. But I don't mind it much, so please\nfeel free to ignore this nit.\n\nPatrick\n"},{"id":"503547","messageId":"ZvVmllrUvOC3pGJy@pks.im","threadId":"62187","inReplyTo":"20240924215634.GI1143820@coredump.intra.peff.net","subject":"Re: [PATCH 09/28] transport-helper: fix strbuf leak in push_refs_with_push()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T13:50:14Z","receivedAt":"2024-09-26T13:50:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 24, 2024 at 05:56:34PM -0400, Jeff King wrote:\n> diff --git a/transport-helper.c b/transport-helper.c\n> index c688967b8c..9c8abd8eca 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -1023,6 +1023,7 @@ static int push_refs_with_push(struct transport *transport,\n> \t\t\tif (atomic) {\n> \t\t\t\treject_atomic_push(remote_refs, mirror);\n> \t\t\t\tstring_list_clear(&cas_options, 0);\n> +\t\t\t\tstrbuf_release(&buf);\n> \t\t\t\treturn 0;\n> \t\t\t} else\n> \t\t\t\tcontinue;\n\nWhat's not visible here is that a few lines further down we have another\nearly return where we don't clear `buf`. But that exit is conditioned on\n`buf.len == 0`, and given that we never `strbuf_reset()` the buffer we\nknow that `buf.len == 0` only when it hasn't ever be allocated.\n\nSo this LGTM.\n\nPatrick\n"},{"id":"503548","messageId":"ZvVmmV0nYq_cEnj3@pks.im","threadId":"62187","inReplyTo":"20240924220109.GN1143820@coredump.intra.peff.net","subject":"Re: [PATCH 14/28] http: fix leak of http_object_request struct","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T13:50:17Z","receivedAt":"2024-09-26T13:50:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 24, 2024 at 06:01:09PM -0400, Jeff King wrote:\n> The new_http_object_request() function allocates a struct on the heap,\n> along with some fields inside the struct. But the matching function to\n> clean it up, release_http_object_request(), only frees the interior\n> fields without freeing the struct itself, causing a leak.\n\nOh yeah, I remember staring at this code and being completely confused\nas to how this all works.\n\n> diff --git a/http.c b/http.c\n> index cc136408c0..d0242ffb50 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -2816,15 +2816,17 @@ int finish_http_object_request(struct http_object_request *freq)\n> \treturn freq->rename;\n> }\n> \n> -void abort_http_object_request(struct http_object_request *freq)\n> +void abort_http_object_request(struct http_object_request **freq_p)\n> {\n> +\tstruct http_object_request *freq = *freq_p;\n> \tunlink_or_warn(freq->tmpfile.buf);\n> \n> -\trelease_http_object_request(freq);\n> +\trelease_http_object_request(freq_p);\n> }\n> \n> -void release_http_object_request(struct http_object_request *freq)\n> +void release_http_object_request(struct http_object_request **freq_p)\n> {\n> +\tstruct http_object_request *freq = *freq_p;\n> \tif (freq->localfile != -1) {\n> \t\tclose(freq->localfile);\n> \t\tfreq->localfile = -1;\n> @@ -2838,4 +2840,7 @@ void release_http_object_request(struct http_object_request *freq)\n> \t}\n> \tcurl_slist_free_all(freq->headers);\n> \tstrbuf_release(&freq->tmpfile);\n> +\n> +\tfree(freq);\n> +\t*freq_p = NULL;\n> }\n\nOkay, looks simple enough. But I found the whole code in \"http.c\" to be\nquite... elusive, so I had a hard time finding my way.\n\nPatrick\n"},{"id":"503549","messageId":"ZvVmnCJNRY_V0a8a@pks.im","threadId":"62187","inReplyTo":"20240924220213.GO1143820@coredump.intra.peff.net","subject":"Re: [PATCH 15/28] http: call git_inflate_end() when releasing http_object_request","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T13:50:20Z","receivedAt":"2024-09-26T13:50:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 24, 2024 at 06:02:13PM -0400, Jeff King wrote:\n> In new_http_object_request(), we initialize the zlib stream with\n> git_inflate_init(). We must have a matching git_inflate_end() to avoid\n> leaking any memory allocated by zlib.\n> \n> In most cases this happens in finish_http_object_request(), but we don't\n> always get there. If we abort a request mid-stream, then we may clean it\n> up without hitting that function.\n> \n> We can't just add a git_inflate_end() call to the release function,\n> though. That would double-free the cases that did actually finish.\n> Instead, we'll move the call from the finish function to the release\n> function. This does delay it for the cases that do finish, but I don't\n> think it matters. We should have already reached Z_STREAM_END (and\n> complain if we didn't), and we do not record any status code from\n> git_inflate_end().\n\nI had to read this paragraph multiple times to understand it, as I\nwondered why you did end up adding it to `release_http_object_request()`\neven though the paragraph claims that you cannot. But what you say is\nthat you must _move_ the call, not add it, and that's what the patch\ndoes.\n\nSo yeah, that does make sense.\n\nPatrick\n"},{"id":"503550","messageId":"ZvVmoARjOAiAqiIz@pks.im","threadId":"62187","inReplyTo":"20240924220446.GT1143820@coredump.intra.peff.net","subject":"Re: [PATCH 20/28] http-push: free repo->url string","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T13:50:24Z","receivedAt":"2024-09-26T13:50:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 24, 2024 at 06:04:46PM -0400, Jeff King wrote:\n> Our repo->url string comes from str_end_url_with_slash(), which always\n> allocates its output buffer. We should free it before exiting to avoid\n> triggering the leak-checker.\n> \n> This can be seen by leak-checking t5540.\n> \n> Signed-off-by: Jeff King \n> ---\n> http-push.c | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/http-push.c b/http-push.c\n> index f60b2ceba5..52c53928a9 100644\n> --- a/http-push.c\n> +++ b/http-push.c\n> @@ -1972,6 +1972,7 @@ int cmd_main(int argc, const char **argv)\n> cleanup:\n> \tif (info_ref_lock)\n> \t\tunlock_remote(info_ref_lock);\n> +\tfree(repo->url);\n> \tfree(repo);\n> \n> \thttp_cleanup();\n\nI was wondering whether we also need to free `repo->path`, which is a\n`char *` as well. But that is only being assigned pointers into command\nline argument strings, and thus we do not have to free it.\n\nPatrick\n"},{"id":"503551","messageId":"ZvVmoz8StFi95VXR@pks.im","threadId":"62187","inReplyTo":"20240924220550.GU1143820@coredump.intra.peff.net","subject":"Re: [PATCH 21/28] http-push: free curl header lists","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T13:50:27Z","receivedAt":"2024-09-26T13:50:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 24, 2024 at 06:05:50PM -0400, Jeff King wrote:\n> diff --git a/http-push.c b/http-push.c\n> index 52c53928a9..451f7d14bb 100644\n> --- a/http-push.c\n> +++ b/http-push.c\n> @@ -1398,6 +1400,7 @@ static int update_remote(const struct object_id *oid, struct remote_lock *lock)\n> \tif (start_active_slot(slot)) {\n> \t\trun_active_slot(slot);\n> \t\tstrbuf_release(&out_buffer.buf);\n> +\t\tcurl_slist_free_all(dav_headers);\n> \t\tif (results.curl_result != CURLE_OK) {\n> \t\t\tfprintf(stderr,\n> \t\t\t\t\"PUT error: curl result=%d, HTTP code=%ld\\n\",\n> @@ -1407,6 +1410,7 @@ static int update_remote(const struct object_id *oid, struct remote_lock *lock)\n> \t\t}\n> \t} else {\n> \t\tstrbuf_release(&out_buffer.buf);\n> +\t\tcurl_slist_free_all(dav_headers);\n> \t\tfprintf(stderr, \"Unable to start PUT request\\n\");\n> \t\treturn 0;\n> \t}\n\nI was quite confused by the layout of this function, where we had\nanother `return 1` at the end. It took me a second to realize that this\nis the error case for the `if (start_active_slot(slot))` condition\nfurther up. But we do already free the headers in that case, so we're\ngood.\n\nPatric\n"},{"id":"503552","messageId":"ZvVmqpGZkM_m7nXl@pks.im","threadId":"62187","inReplyTo":"20240924214930.GA1143523@coredump.intra.peff.net","subject":"Re: [PATCH 0/28] leak fixes for http fetch/push","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T13:50:34Z","receivedAt":"2024-09-26T13:50:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 24, 2024 at 05:49:30PM -0400, Jeff King wrote:\n> Patrick asked me to take a look at the remaining leaks in the http\n> push/fetch code, especially the dumb variants. So here are enough\n> patches to all of these scripts running leak-free:\n\nThank you for taking a look at this, highly appreciated! The series\nlooks good to me, I've only got a couple of nits regarding typos that\nyou may or may not want to address. Either way is fine with me.\n\nPatrick\n"},{"id":"503592","messageId":"20240927034525.GA567395@coredump.intra.peff.net","threadId":"62187","inReplyTo":"ZvVmitkT-QUneqvb@pks.im","subject":"Re: [PATCH 06/28] fetch-pack, send-pack: clean up shallow oid array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-27T03:45:25Z","receivedAt":"2024-09-27T03:45:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 26, 2024 at 03:50:02PM +0200, Patrick Steinhardt wrote:\n\n> > diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n> > index cfc6951d23..ef4143eef3 100644\n> > --- a/builtin/fetch-pack.c\n> > +++ b/builtin/fetch-pack.c\n> > @@ -294,5 +294,6 @@ int cmd_fetch_pack(int argc,\n> > \tfree_refs(fetched_refs);\n> > \tfree_refs(remote_refs);\n> > \tlist_objects_filter_release(&args.filter_options);\n> > +\toid_array_clear(&shallow);\n> > \treturn ret;\n> > }\n> \n> I wonder about the early exit path we have when `finish_connect()`\n> returns non-zero. Should we make it go via the common cleanup path as\n> well, or do we not care in the error case?\n\nYeah, I think it would technically be a leak if we hit that error case.\nBut it's pretty unlikely in practice (you'd have to finish the fetch and\nthen the \"ssh\" or \"upload-pack\" sub-process returns a non-zero exit code\nanyway).\n\nI'd prefer not to deal with it here for two reasons:\n\n 1. The same leak is true of all the existing cleanup. So it really is\n orthogonal to what this patch is doing, and should be a separate\n patch.\n\n 2. I think cleaning up in top-level builtins like this is mostly\n cosmetic to appease the leak checker. In the real world the memory\n is going back to the OS when we return anyway. Even in a libified\n world, I don't think cmd_fetch_pack() is a reasonable entry point\n (we already have a more lib-ish fetch_pack() that is used by the\n transport code).\n\nSo I don't think it would be wrong to refactor the cleanup to trigger on\nthat other early return. But I also suspect there are many such paths\nlurking (and will continue to lurk, even after we run clean with\nSANITIZE=leak) and I'm not sure how productive it is to hunt them down.\n\n> > diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> > index 81fc96d423..c49fe6c53c 100644\n> > --- a/builtin/send-pack.c\n> > +++ b/builtin/send-pack.c\n> > @@ -343,5 +343,6 @@ int cmd_send_pack(int argc,\n> > \tfree_refs(remote_refs);\n> > \tfree_refs(local_refs);\n> > \trefspec_clear(&rs);\n> > +\toid_array_clear(&shallow);\n> > \treturn ret;\n> > }\n> \n> We also have an early exit in this function when `match_push_refs()`\n> returns non-zero.\n\nAnd I think this is in the same boat.\n\n\n-Peff\n"},{"id":"503593","messageId":"20240927034704.GB567395@coredump.intra.peff.net","threadId":"62187","inReplyTo":"ZvVmkXDIyCqpYKZm@pks.im","subject":"Re: [PATCH 08/28] send-pack: free cas options before exit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-27T03:47:04Z","receivedAt":"2024-09-27T03:47:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 26, 2024 at 03:50:09PM +0200, Patrick Steinhardt wrote:\n\n> On Tue, Sep 24, 2024 at 05:55:39PM -0400, Jeff King wrote:\n> > diff --git a/remote.c b/remote.c\n> > index 390a03c264..e291e8ff5c 100644\n> > --- a/remote.c\n> > +++ b/remote.c\n> > @@ -2544,7 +2544,7 @@ struct ref *get_stale_heads(struct refspec *rs, struct ref *fetch_map)\n> > /*\n> > * Compare-and-swap\n> > */\n> > -static void clear_cas_option(struct push_cas_option *cas)\n> > +void clear_cas_option(struct push_cas_option *cas)\n> > {\n> > \tint i;\n> > \n> > diff --git a/remote.h b/remote.h\n> > index a58713f20a..ad4513f639 100644\n> > --- a/remote.h\n> > +++ b/remote.h\n> > @@ -409,6 +409,7 @@ struct push_cas_option {\n> > };\n> > \n> > int parseopt_push_cas_option(const struct option *, const char *arg, int unset);\n> > +void clear_cas_option(struct push_cas_option *);\n> \n> Nit: I was wondering whether we'd also want to fix up this functions\n> name to conform to our style guide, which says this should be called\n> `push_cas_option_clear()` instead. But I don't mind it much, so please\n> feel free to ignore this nit.\n\nI'd prefer to punt on that for now, as the whole suite of \"methods\" for\nthis struct would need to be renamed to match that style. If we were\nmaking a too-short name into a public symbol, I'd be worried about\naddressing that now, but I think this is purely about style and can\nwait.\n\n-Peff\n"},{"id":"503594","messageId":"20240927034948.GC567395@coredump.intra.peff.net","threadId":"62187","inReplyTo":"ZvVmllrUvOC3pGJy@pks.im","subject":"Re: [PATCH 09/28] transport-helper: fix strbuf leak in push_refs_with_push()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-27T03:49:48Z","receivedAt":"2024-09-27T03:49:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 26, 2024 at 03:50:14PM +0200, Patrick Steinhardt wrote:\n\n> On Tue, Sep 24, 2024 at 05:56:34PM -0400, Jeff King wrote:\n> > diff --git a/transport-helper.c b/transport-helper.c\n> > index c688967b8c..9c8abd8eca 100644\n> > --- a/transport-helper.c\n> > +++ b/transport-helper.c\n> > @@ -1023,6 +1023,7 @@ static int push_refs_with_push(struct transport *transport,\n> > \t\t\tif (atomic) {\n> > \t\t\t\treject_atomic_push(remote_refs, mirror);\n> > \t\t\t\tstring_list_clear(&cas_options, 0);\n> > +\t\t\t\tstrbuf_release(&buf);\n> > \t\t\t\treturn 0;\n> > \t\t\t} else\n> > \t\t\t\tcontinue;\n> \n> What's not visible here is that a few lines further down we have another\n> early return where we don't clear `buf`. But that exit is conditioned on\n> `buf.len == 0`, and given that we never `strbuf_reset()` the buffer we\n> know that `buf.len == 0` only when it hasn't ever be allocated.\n\nYeah, I noticed that, too, and came to the same conclusion. As you note,\nit's _possible_ to have an allocated but zero-length strbuf, but I don't\nthink it happens here. So it's OK in practice.\n\nIf we were going to refactor, I think it would make sense to do it with\na cleanup label to cover both of these early returns, plus the cleanup\nat the end. I was mostly just going for minimal changes where possible.\n\n-Peff\n"},{"id":"503595","messageId":"20240927035005.GD567395@coredump.intra.peff.net","threadId":"62187","inReplyTo":"ZvVmmV0nYq_cEnj3@pks.im","subject":"Re: [PATCH 14/28] http: fix leak of http_object_request struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-27T03:50:05Z","receivedAt":"2024-09-27T03:50:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 26, 2024 at 03:50:17PM +0200, Patrick Steinhardt wrote:\n\n> Okay, looks simple enough. But I found the whole code in \"http.c\" to be\n> quite... elusive, so I had a hard time finding my way.\n\nYou are not the only one. ;)\n\n-Peff\n"},{"id":"503596","messageId":"20240927035123.GE567395@coredump.intra.peff.net","threadId":"62187","inReplyTo":"ZvVmnCJNRY_V0a8a@pks.im","subject":"Re: [PATCH 15/28] http: call git_inflate_end() when releasing http_object_request","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-27T03:51:23Z","receivedAt":"2024-09-27T03:51:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 26, 2024 at 03:50:20PM +0200, Patrick Steinhardt wrote:\n\n> > We can't just add a git_inflate_end() call to the release function,\n> > though. That would double-free the cases that did actually finish.\n> > Instead, we'll move the call from the finish function to the release\n> > function. This does delay it for the cases that do finish, but I don't\n> > think it matters. We should have already reached Z_STREAM_END (and\n> > complain if we didn't), and we do not record any status code from\n> > git_inflate_end().\n> \n> I had to read this paragraph multiple times to understand it, as I\n> wondered why you did end up adding it to `release_http_object_request()`\n> even though the paragraph claims that you cannot. But what you say is\n> that you must _move_ the call, not add it, and that's what the patch\n> does.\n\nYeah, I could see the confusion. It is really \"We can't just add a\ngit_inflate_end() call and be done. We must also...\".\n\nI don't think it's worth a re-roll on its own, but if I do re-roll I'll\ntry to clarify.\n\n-Peff\n"},{"id":"503597","messageId":"20240927035508.GF567395@coredump.intra.peff.net","threadId":"62187","inReplyTo":"ZvVmoARjOAiAqiIz@pks.im","subject":"Re: [PATCH 20/28] http-push: free repo->url string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-27T03:55:08Z","receivedAt":"2024-09-27T03:55:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 26, 2024 at 03:50:24PM +0200, Patrick Steinhardt wrote:\n\n> > diff --git a/http-push.c b/http-push.c\n> > index f60b2ceba5..52c53928a9 100644\n> > --- a/http-push.c\n> > +++ b/http-push.c\n> > @@ -1972,6 +1972,7 @@ int cmd_main(int argc, const char **argv)\n> > cleanup:\n> > \tif (info_ref_lock)\n> > \t\tunlock_remote(info_ref_lock);\n> > +\tfree(repo->url);\n> > \tfree(repo);\n> > \n> > \thttp_cleanup();\n> \n> I was wondering whether we also need to free `repo->path`, which is a\n> `char *` as well. But that is only being assigned pointers into command\n> line argument strings, and thus we do not have to free it.\n\nYeah, I think that \"path\" has the wrong type, and should be \"const char\n*\". I was a little surprised the compiler does not complain, but it is\nthe old C gotcha: we assign to it via strchr(), which launders away the\nconst.\n\nIt would probably be OK to fix, though in general my dream is still that\nwe'd delete all of this code in the not too distant future. Even if we\nkeep dumb-http fetch for its resumability, dumb-http push just seems\nlike a useless holdover from the early days.\n\n-Peff\n"},{"id":"503598","messageId":"20240927035536.GG567395@coredump.intra.peff.net","threadId":"62187","inReplyTo":"ZvVmqpGZkM_m7nXl@pks.im","subject":"Re: [PATCH 0/28] leak fixes for http fetch/push","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-27T03:55:36Z","receivedAt":"2024-09-27T03:55:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 26, 2024 at 03:50:34PM +0200, Patrick Steinhardt wrote:\n\n> On Tue, Sep 24, 2024 at 05:49:30PM -0400, Jeff King wrote:\n> > Patrick asked me to take a look at the remaining leaks in the http\n> > push/fetch code, especially the dumb variants. So here are enough\n> > patches to all of these scripts running leak-free:\n> \n> Thank you for taking a look at this, highly appreciated! The series\n> looks good to me, I've only got a couple of nits regarding typos that\n> you may or may not want to address. Either way is fine with me.\n\nI responded to a few, but I don't think any of them really merits a\nre-roll. Thanks for reviewing!\n\n-Peff\n"}]}