{"thread":{"id":"63704","subject":"[PATCH] send-pack: clean up extra_have oid array","startedAt":"2025-06-27T22:09:46Z","lastAt":"2025-07-03T16:26:06Z","messageCount":9,"participants":["Jacob Keller","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"520828","messageId":"20250627-jk-fix-leak-send-pack-v1-1-aadcf0ed8a4b@gmail.com","threadId":"63704","inReplyTo":null,"subject":"[PATCH] send-pack: clean up extra_have oid array","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-27T22:09:04Z","receivedAt":"2025-06-27T22:09:46Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nCommit c8009635785e (\"fetch-pack, send-pack: clean up shallow oid\narray\", 2024-09-25) cleaned up the shallow oid array in cmd_send_pack,\nbut didn't clean up extra_have, which is still leaked at program exit.\nI suspect the particular tests in t5539 don't trigger any additions to\nthe extra_have array, which explains why the tests can pass leak free\ndespite this gap.\n\nSigned-off-by: Jacob Keller \n---\nI didn't check to see why the t5539 tests don't leak. This leak occured for\nme in a day-to-day run with my local git build that happened to still have\nsanitizers enabled:\n\n=================================================================\n==2930359==ERROR: LeakSanitizer: detected memory leaks\n\nDirect leak of 2160 byte(s) in 1 object(s) allocated from:\n #0 0x7f51af6e5e2b in realloc.part.0 (/lib64/libasan.so.8+0xe5e2b) (BuildId: 7f1aa7e2e600e8c9d54ce6e3d36f3d31bfe7949a)\n #1 0x0000010dfc26 in xrealloc ../wrapper.c:140\n #2 0x000000c5d231 in oid_array_append ../oid-array.c:9\n #3 0x00000096036a in process_ref ../connect.c:296\n #4 0x00000096036a in get_remote_heads ../connect.c:374\n #5 0x00000072f8fc in cmd_send_pack ../builtin/send-pack.c:290\n #6 0x0000007d74d4 in run_builtin ../git.c:480\n #7 0x0000007d74d4 in handle_builtin ../git.c:746\n #8 0x0000007dbeb5 in run_argv ../git.c:813\n #9 0x0000007dbeb5 in cmd_main ../git.c:953\n #10 0x000000441dbf in main ../common-main.c:9\n #11 0x7f51aec115f4 in __libc_start_call_main (/lib64/libc.so.6+0x35f4) (BuildId: 2b3c02fe7e4d3811767175b6f323692a10a4e116)\n #12 0x7f51aec116a7 in __libc_start_main@@GLIBC_2.34 (/lib64/libc.so.6+0x36a7) (BuildId: 2b3c02fe7e4d3811767175b6f323692a10a4e116)\n #13 0x0000004440b4 in _start (/home/jekeller/libexec/git-core/git+0x4440b4) (BuildId: 6cd37a01505f2d67a4e7d39fd9f813b683be0300)\n\nSUMMARY: AddressSanitizer: 2160 byte(s) leaked in 1 allocation(s)\n---\n builtin/send-pack.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex c6e0e9d05186..61486e378cab 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -343,6 +343,7 @@ int cmd_send_pack(int argc,\n \tfree_refs(remote_refs);\n \tfree_refs(local_refs);\n \trefspec_clear(&rs);\n+\toid_array_clear(&extra_have);\n \toid_array_clear(&shallow);\n \tclear_cas_option(&cas);\n \treturn ret;\n\n---\nbase-commit: 16bd9f20a403117f2e0d9bcda6c6e621d3763e77\nchange-id: 20250627-jk-fix-leak-send-pack-e4787600cf60\n\nBest regards,\n-- \nJacob Keller \n\n"},{"id":"520923","messageId":"xmqqfrfh5mis.fsf@gitster.g","threadId":"63704","inReplyTo":"20250627-jk-fix-leak-send-pack-v1-1-aadcf0ed8a4b@gmail.com","subject":"Re: [PATCH] send-pack: clean up extra_have oid array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-30T14:31:23Z","receivedAt":"2025-06-30T14:31:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> From: Jacob Keller \n>\n> Commit c8009635785e (\"fetch-pack, send-pack: clean up shallow oid\n> array\", 2024-09-25) cleaned up the shallow oid array in cmd_send_pack,\n> but didn't clean up extra_have, which is still leaked at program exit.\n> I suspect the particular tests in t5539 don't trigger any additions to\n> the extra_have array, which explains why the tests can pass leak free\n> despite this gap.\n>\n> Signed-off-by: Jacob Keller \n> ---\n> I didn't check to see why the t5539 tests don't leak. This leak occured for\n> me in a day-to-day run with my local git build that happened to still have\n> sanitizers enabled:\n\nThe other side may tell you about objects you _cannot_ fetch from\nthem, but if you have them, these objects can participate in the\ncommon ancestor discovery and reduce the size of the transfer.\n\nIf the repository A you are pushing into use an alternate object\nstore B (i.e., created by \"git clone --reference B $URL A\" to make A\nborrow from another local repository B) for example, the refs in\nthat alternate B that point at objects not in the repository A are\nshown as \"extra\" objects.\n\nPerhaps we can have these tests push into such a repository?\n\n> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> index c6e0e9d05186..61486e378cab 100644\n> --- a/builtin/send-pack.c\n> +++ b/builtin/send-pack.c\n> @@ -343,6 +343,7 @@ int cmd_send_pack(int argc,\n> \tfree_refs(remote_refs);\n> \tfree_refs(local_refs);\n> \trefspec_clear(&rs);\n> +\toid_array_clear(&extra_have);\n> \toid_array_clear(&shallow);\n> \tclear_cas_option(&cas);\n> \treturn ret;\n\nThe change looks obviously correct.\n\nThanks.\n"},{"id":"520975","messageId":"CA+P7+xo29ibqM7uXNuWyNy39G1Lx=8+6p4BxvQw7=PR7PjVW8w@mail.gmail.com","threadId":"63704","inReplyTo":"xmqqfrfh5mis.fsf@gitster.g","subject":"Re: [PATCH] send-pack: clean up extra_have oid array","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2025-06-30T22:14:43Z","receivedAt":"2025-06-30T22:14:53Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Jun 30, 2025 at 7:31 AM Junio C Hamano wrote:\n>\n> Jacob Keller writes:\n>\n> > From: Jacob Keller \n> >\n> > Commit c8009635785e (\"fetch-pack, send-pack: clean up shallow oid\n> > array\", 2024-09-25) cleaned up the shallow oid array in cmd_send_pack,\n> > but didn't clean up extra_have, which is still leaked at program exit.\n> > I suspect the particular tests in t5539 don't trigger any additions to\n> > the extra_have array, which explains why the tests can pass leak free\n> > despite this gap.\n> >\n> > Signed-off-by: Jacob Keller \n> > ---\n> > I didn't check to see why the t5539 tests don't leak. This leak occured for\n> > me in a day-to-day run with my local git build that happened to still have\n> > sanitizers enabled:\n>\n> The other side may tell you about objects you _cannot_ fetch from\n> them, but if you have them, these objects can participate in the\n> common ancestor discovery and reduce the size of the transfer.\n>\n> If the repository A you are pushing into use an alternate object\n> store B (i.e., created by \"git clone --reference B $URL A\" to make A\n> borrow from another local repository B) for example, the refs in\n> that alternate B that point at objects not in the repository A are\n> shown as \"extra\" objects.\n>\n> Perhaps we can have these tests push into such a repository?\n>\n\nI probably won't personally have time to work on extending these tests.\n\nThanks,\nJake\n\n> > diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> > index c6e0e9d05186..61486e378cab 100644\n> > --- a/builtin/send-pack.c\n> > +++ b/builtin/send-pack.c\n> > @@ -343,6 +343,7 @@ int cmd_send_pack(int argc,\n> > free_refs(remote_refs);\n> > free_refs(local_refs);\n> > refspec_clear(&rs);\n> > + oid_array_clear(&extra_have);\n> > oid_array_clear(&shallow);\n> > clear_cas_option(&cas);\n> > return ret;\n>\n> The change looks obviously correct.\n>\n> Thanks.\n"},{"id":"521092","messageId":"xmqqzfdnkdx6.fsf@gitster.g","threadId":"63704","inReplyTo":"20250627-jk-fix-leak-send-pack-v1-1-aadcf0ed8a4b@gmail.com","subject":"Re: [PATCH] send-pack: clean up extra_have oid array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-01T17:40:21Z","receivedAt":"2025-07-01T17:40:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> index c6e0e9d05186..61486e378cab 100644\n> --- a/builtin/send-pack.c\n> +++ b/builtin/send-pack.c\n> @@ -343,6 +343,7 @@ int cmd_send_pack(int argc,\n> \tfree_refs(remote_refs);\n> \tfree_refs(local_refs);\n> \trefspec_clear(&rs);\n> +\toid_array_clear(&extra_have);\n> \toid_array_clear(&shallow);\n> \tclear_cas_option(&cas);\n> \treturn ret;\n\nThere is an early exit from the function that would bypass these\nclean-up. Perhaps something like this on top?\n\n builtin/send-pack.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git c/builtin/send-pack.c w/builtin/send-pack.c\nindex b28da7ddd7..6ce9f6665a 100644\n--- c/builtin/send-pack.c\n+++ w/builtin/send-pack.c\n@@ -305,9 +305,10 @@ int cmd_send_pack(int argc,\n \t\tflags |= MATCH_REFS_MIRROR;\n \n \t/* match them up */\n-\tif (match_push_refs(local_refs, &remote_refs, &rs, flags))\n-\t\treturn -1;\n-\n+\tif (match_push_refs(local_refs, &remote_refs, &rs, flags)) {\n+\t\tret = -1;\n+\t\tgoto cleanup;\n+\t}\n \tif (!is_empty_cas(&cas))\n \t\tapply_push_cas(&cas, remote, remote_refs);\n \n@@ -340,6 +341,7 @@ int cmd_send_pack(int argc,\n \t\t/* stable plumbing output; do not modify or localize */\n \t\tfprintf(stderr, \"Everything up-to-date\\n\");\n \n+cleanup:\n \tstring_list_clear(&push_options, 0);\n \tfree_refs(remote_refs);\n \tfree_refs(local_refs);\n"},{"id":"521104","messageId":"753b6548-9b7c-411a-ab39-adbf769f83bd@intel.com","threadId":"63704","inReplyTo":"xmqqzfdnkdx6.fsf@gitster.g","subject":"Re: [PATCH] send-pack: clean up extra_have oid array","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-07-01T20:36:29Z","receivedAt":"2025-07-01T20:36:39Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn 7/1/2025 10:40 AM, Junio C Hamano wrote:\n> Jacob Keller writes:\n> \n>> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n>> index c6e0e9d05186..61486e378cab 100644\n>> --- a/builtin/send-pack.c\n>> +++ b/builtin/send-pack.c\n>> @@ -343,6 +343,7 @@ int cmd_send_pack(int argc,\n>> \tfree_refs(remote_refs);\n>> \tfree_refs(local_refs);\n>> \trefspec_clear(&rs);\n>> +\toid_array_clear(&extra_have);\n>> \toid_array_clear(&shallow);\n>> \tclear_cas_option(&cas);\n>> \treturn ret;\n> \n> There is an early exit from the function that would bypass these\n> clean-up. Perhaps something like this on top?\n> \n> builtin/send-pack.c | 8 +++++---\n> 1 file changed, 5 insertions(+), 3 deletions(-)\n> \n> diff --git c/builtin/send-pack.c w/builtin/send-pack.c\n> index b28da7ddd7..6ce9f6665a 100644\n> --- c/builtin/send-pack.c\n> +++ w/builtin/send-pack.c\n> @@ -305,9 +305,10 @@ int cmd_send_pack(int argc,\n> \t\tflags |= MATCH_REFS_MIRROR;\n> \n> \t/* match them up */\n> -\tif (match_push_refs(local_refs, &remote_refs, &rs, flags))\n> -\t\treturn -1;\n> -\n> +\tif (match_push_refs(local_refs, &remote_refs, &rs, flags)) {\n> +\t\tret = -1;\n> +\t\tgoto cleanup;\n> +\t}\n> \tif (!is_empty_cas(&cas))\n> \t\tapply_push_cas(&cas, remote, remote_refs);\n> \n> @@ -340,6 +341,7 @@ int cmd_send_pack(int argc,\n> \t\t/* stable plumbing output; do not modify or localize */\n> \t\tfprintf(stderr, \"Everything up-to-date\\n\");\n> \n> +cleanup:\n> \tstring_list_clear(&push_options, 0);\n> \tfree_refs(remote_refs);\n> \tfree_refs(local_refs);\n\nThis addition looks good to me.\n\nThanks,\nJake\n"},{"id":"521109","messageId":"xmqqwm8ripii.fsf@gitster.g","threadId":"63704","inReplyTo":"753b6548-9b7c-411a-ab39-adbf769f83bd@intel.com","subject":"Re: [PATCH] send-pack: clean up extra_have oid array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-01T21:12:53Z","receivedAt":"2025-07-01T21:12:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> On 7/1/2025 10:40 AM, Junio C Hamano wrote:\n>> Jacob Keller writes:\n>> \n>>> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n>>> index c6e0e9d05186..61486e378cab 100644\n>>> --- a/builtin/send-pack.c\n>>> +++ b/builtin/send-pack.c\n>>> @@ -343,6 +343,7 @@ int cmd_send_pack(int argc,\n>>> \tfree_refs(remote_refs);\n>>> \tfree_refs(local_refs);\n>>> \trefspec_clear(&rs);\n>>> +\toid_array_clear(&extra_have);\n>>> \toid_array_clear(&shallow);\n>>> \tclear_cas_option(&cas);\n>>> \treturn ret;\n>> \n>> There is an early exit from the function that would bypass these\n>> clean-up. Perhaps something like this on top?\n>> \n>> builtin/send-pack.c | 8 +++++---\n>> 1 file changed, 5 insertions(+), 3 deletions(-)\n>> \n>> diff --git c/builtin/send-pack.c w/builtin/send-pack.c\n>> index b28da7ddd7..6ce9f6665a 100644\n>> --- c/builtin/send-pack.c\n>> +++ w/builtin/send-pack.c\n>> @@ -305,9 +305,10 @@ int cmd_send_pack(int argc,\n>> \t\tflags |= MATCH_REFS_MIRROR;\n>> \n>> \t/* match them up */\n>> -\tif (match_push_refs(local_refs, &remote_refs, &rs, flags))\n>> -\t\treturn -1;\n>> -\n>> +\tif (match_push_refs(local_refs, &remote_refs, &rs, flags)) {\n>> +\t\tret = -1;\n>> +\t\tgoto cleanup;\n>> +\t}\n>> \tif (!is_empty_cas(&cas))\n>> \t\tapply_push_cas(&cas, remote, remote_refs);\n>> \n>> @@ -340,6 +341,7 @@ int cmd_send_pack(int argc,\n>> \t\t/* stable plumbing output; do not modify or localize */\n>> \t\tfprintf(stderr, \"Everything up-to-date\\n\");\n>> \n>> +cleanup:\n>> \tstring_list_clear(&push_options, 0);\n>> \tfree_refs(remote_refs);\n>> \tfree_refs(local_refs);\n>\n> This addition looks good to me.\n\nThanks for a quick sanity check. I'll queue it on top of yours,\nthen.\n\n"},{"id":"521280","messageId":"20250703153853.GC1309870@coredump.intra.peff.net","threadId":"63704","inReplyTo":"20250627-jk-fix-leak-send-pack-v1-1-aadcf0ed8a4b@gmail.com","subject":"Re: [PATCH] send-pack: clean up extra_have oid array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-03T15:38:53Z","receivedAt":"2025-07-03T15:38:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 27, 2025 at 03:09:04PM -0700, Jacob Keller wrote:\n\n> From: Jacob Keller \n> \n> Commit c8009635785e (\"fetch-pack, send-pack: clean up shallow oid\n> array\", 2024-09-25) cleaned up the shallow oid array in cmd_send_pack,\n> but didn't clean up extra_have, which is still leaked at program exit.\n> I suspect the particular tests in t5539 don't trigger any additions to\n> the extra_have array, which explains why the tests can pass leak free\n> despite this gap.\n\nThanks, this looks good. At the time I did that other commit, I was\nfocused on just bug-hunting the leaks reported by the tests. So I missed\nthis one.\n\n> I didn't check to see why the t5539 tests don't leak. This leak occured for\n> me in a day-to-day run with my local git build that happened to still have\n> sanitizers enabled:\n\nThe tests are leak-free now, but I suspect we have a lot of\nslightly-exotic command invocations like this that still leak. It might\nbe nice to beef up the test coverage, but I'm OK with just fixing them,\ntoo.\n\n-Peff\n"},{"id":"521281","messageId":"20250703154047.GD1309870@coredump.intra.peff.net","threadId":"63704","inReplyTo":"xmqqzfdnkdx6.fsf@gitster.g","subject":"Re: [PATCH] send-pack: clean up extra_have oid array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-03T15:40:47Z","receivedAt":"2025-07-03T15:40:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 01, 2025 at 10:40:21AM -0700, Junio C Hamano wrote:\n\n> There is an early exit from the function that would bypass these\n> clean-up. Perhaps something like this on top?\n> \n> builtin/send-pack.c | 8 +++++---\n> 1 file changed, 5 insertions(+), 3 deletions(-)\n> \n> diff --git c/builtin/send-pack.c w/builtin/send-pack.c\n> index b28da7ddd7..6ce9f6665a 100644\n> --- c/builtin/send-pack.c\n> +++ w/builtin/send-pack.c\n> @@ -305,9 +305,10 @@ int cmd_send_pack(int argc,\n> \t\tflags |= MATCH_REFS_MIRROR;\n> \n> \t/* match them up */\n> -\tif (match_push_refs(local_refs, &remote_refs, &rs, flags))\n> -\t\treturn -1;\n> -\n> +\tif (match_push_refs(local_refs, &remote_refs, &rs, flags)) {\n> +\t\tret = -1;\n> +\t\tgoto cleanup;\n> +\t}\n> \tif (!is_empty_cas(&cas))\n> \t\tapply_push_cas(&cas, remote, remote_refs);\n> \n> @@ -340,6 +341,7 @@ int cmd_send_pack(int argc,\n> \t\t/* stable plumbing output; do not modify or localize */\n> \t\tfprintf(stderr, \"Everything up-to-date\\n\");\n> \n> +cleanup:\n> \tstring_list_clear(&push_options, 0);\n> \tfree_refs(remote_refs);\n> \tfree_refs(local_refs);\n\nThis made me wonder if the remote_refs out-parameter is valid after\nmatch_push_refs() returns failure (especially since we do not initialize\nit at the top of the function).\n\nI think the answer is \"yes\"; it is both an in-parameter and an\nout-parameter, and will have been earlier set up via get_remote_heads().\nSo even on the failure case, match_push_refs() will leave it untouched\nand it is still valid (and needs to be cleaned up).\n\n-Peff\n"},{"id":"521283","messageId":"72331edd-8e03-4415-ada2-2be8fbee922b@intel.com","threadId":"63704","inReplyTo":"20250703154047.GD1309870@coredump.intra.peff.net","subject":"Re: [PATCH] send-pack: clean up extra_have oid array","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-07-03T16:26:01Z","receivedAt":"2025-07-03T16:26:06Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn 7/3/2025 8:40 AM, Jeff King wrote:\n> On Tue, Jul 01, 2025 at 10:40:21AM -0700, Junio C Hamano wrote:\n> \n>> There is an early exit from the function that would bypass these\n>> clean-up. Perhaps something like this on top?\n>>\n>> builtin/send-pack.c | 8 +++++---\n>> 1 file changed, 5 insertions(+), 3 deletions(-)\n>>\n>> diff --git c/builtin/send-pack.c w/builtin/send-pack.c\n>> index b28da7ddd7..6ce9f6665a 100644\n>> --- c/builtin/send-pack.c\n>> +++ w/builtin/send-pack.c\n>> @@ -305,9 +305,10 @@ int cmd_send_pack(int argc,\n>> \t\tflags |= MATCH_REFS_MIRROR;\n>> \n>> \t/* match them up */\n>> -\tif (match_push_refs(local_refs, &remote_refs, &rs, flags))\n>> -\t\treturn -1;\n>> -\n>> +\tif (match_push_refs(local_refs, &remote_refs, &rs, flags)) {\n>> +\t\tret = -1;\n>> +\t\tgoto cleanup;\n>> +\t}\n>> \tif (!is_empty_cas(&cas))\n>> \t\tapply_push_cas(&cas, remote, remote_refs);\n>> \n>> @@ -340,6 +341,7 @@ int cmd_send_pack(int argc,\n>> \t\t/* stable plumbing output; do not modify or localize */\n>> \t\tfprintf(stderr, \"Everything up-to-date\\n\");\n>> \n>> +cleanup:\n>> \tstring_list_clear(&push_options, 0);\n>> \tfree_refs(remote_refs);\n>> \tfree_refs(local_refs);\n> \n> This made me wonder if the remote_refs out-parameter is valid after\n> match_push_refs() returns failure (especially since we do not initialize\n> it at the top of the function).\n> \n> I think the answer is \"yes\"; it is both an in-parameter and an\n> out-parameter, and will have been earlier set up via get_remote_heads().\n> So even on the failure case, match_push_refs() will leave it untouched\n> and it is still valid (and needs to be cleaned up).\n> \n\nThis was my assumption too.\n"}]}