{"thread":{"id":"64588","subject":"[PATCH] connect: plug protocol capability leak","startedAt":"2025-12-07T04:40:48Z","lastAt":"2025-12-08T20:18:58Z","messageCount":4,"participants":["Junio C Hamano","Patrick Steinhardt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"531789","messageId":"xmqqfr9mnbu9.fsf@gitster.g","threadId":"64588","inReplyTo":null,"subject":"[PATCH] connect: plug protocol capability leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-12-07T04:40:46Z","receivedAt":"2025-12-07T04:40:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When pushing to a set of remotes using a nickname for the group, the\nclient initializes the connection to each remote, talks to the\nremote and reads and parses capabilities line, and holds the\ncapabilities in a file-scope static variable server_capabilities_v1.\n\nThere are a few other such file-scope static variables, and these\nconnections cannot be parallelized until they are refactored to a\nstructure that keeps track of active connections.\n\nWhich is *not* the theme of this patch ;-)\n\nFor a single connection, the server_capabilities_v1 variable is\ninitialized to NULL (at the program initialization), populated when\nwe talk to the other side, used to look up capabilities of the other\nsdie possible multiple times, and the memory is held by the variable\nuntil program exit, without leaking.  When talking to multiple remotes,\nhowever, the server capabilities from the second connection overwrites\nwithout freeing the one from the first connection, which leaks.\n\n    ==1080970==ERROR: LeakSanitizer: detected memory leaks\n\n    Direct leak of 421 byte(s) in 2 object(s) allocated from:\n\t#0 0x5615305f849e in strdup (/home/gitster/g/git-jch/bin/bin/git+0x2b349e) (BuildId: 54d149994c9e85374831958f694bd0aa3b8b1e26)\n\t#1 0x561530e76cc4 in xstrdup /home/gitster/w/build/wrapper.c:43:14\n\t#2 0x5615309cd7fa in process_capabilities /home/gitster/w/build/connect.c:243:27\n\t#3 0x5615309cd502 in get_remote_heads /home/gitster/w/build/connect.c:366:4\n\t#4 0x561530e2cb0b in handshake /home/gitster/w/build/transport.c:372:3\n\t#5 0x561530e29ed7 in get_refs_via_connect /home/gitster/w/build/transport.c:398:9\n\t#6 0x561530e26464 in transport_push /home/gitster/w/build/transport.c:1421:16\n\t#7 0x561530800bec in push_with_options /home/gitster/w/build/builtin/push.c:387:8\n\t#8 0x5615307ffb99 in do_push /home/gitster/w/build/builtin/push.c:442:7\n\t#9 0x5615307fe926 in cmd_push /home/gitster/w/build/builtin/push.c:664:7\n\t#10 0x56153065673f in run_builtin /home/gitster/w/build/git.c:506:11\n\t#11 0x56153065342f in handle_builtin /home/gitster/w/build/git.c:779:9\n\t#12 0x561530655b89 in run_argv /home/gitster/w/build/git.c:862:4\n\t#13 0x561530652cba in cmd_main /home/gitster/w/build/git.c:984:19\n\t#14 0x5615308dda0a in main /home/gitster/w/build/common-main.c:9:11\n\t#15 0x7f051651bca7 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16\n\n    SUMMARY: AddressSanitizer: 421 byte(s) leaked in 2 allocation(s).\n\nFree the capablities data for the previous server before overwriting\nit with the next server to plug this leak.\n\nThe added test fails without the freeing with SANITIZE=leak; I\nsomehow couldn't get it fail reliably with SANITIZE=leak,address\nthough.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n connect.c                |  2 ++\n t/meson.build            |  1 +\n t/t5565-push-multiple.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 3 files changed, 42 insertions(+)\n create mode 100755 t/t5565-push-multiple.sh\n\ndiff --git a/connect.c b/connect.c\nindex 8352b71faf..c6f76e3082 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -240,6 +240,8 @@ static void process_capabilities(struct packet_reader *reader, size_t *linelen)\n \tsize_t nul_location = strlen(line);\n \tif (nul_location == *linelen)\n \t\treturn;\n+\n+\tfree(server_capabilities_v1);\n \tserver_capabilities_v1 = xstrdup(line + nul_location + 1);\n \t*linelen = nul_location;\n \ndiff --git a/t/meson.build b/t/meson.build\nindex d3d0be2822..459c52a489 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -690,6 +690,7 @@ integration_tests = [\n   't5562-http-backend-content-length.sh',\n   't5563-simple-http-auth.sh',\n   't5564-http-proxy.sh',\n+  't5565-push-multiple.sh',\n   't5570-git-daemon.sh',\n   't5571-pre-push-hook.sh',\n   't5572-pull-submodule.sh',\ndiff --git a/t/t5565-push-multiple.sh b/t/t5565-push-multiple.sh\nnew file mode 100755\nindex 0000000000..7e93668566\n--- /dev/null\n+++ b/t/t5565-push-multiple.sh\n@@ -0,0 +1,39 @@\n+#!/bin/sh\n+\n+test_description='push to group'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\tfor i in 1 2 3\n+\tdo\n+\t\tgit init dest-$i &&\n+\t\tgit -C dest-$i symbolic-ref HEAD refs/heads/not-a-branch ||\n+\t\treturn 1\n+\tdone &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m \"initial\" &&\n+\tgit config set --append remote.them.pushurl \"file://$(pwd)/dest-1\" &&\n+\tgit config set --append remote.them.pushurl \"file://$(pwd)/dest-2\" &&\n+\tgit config set --append remote.them.pushurl \"file://$(pwd)/dest-3\" &&\n+\tgit config set --append remote.them.push \"+refs/heads/*:refs/heads/*\"\n+'\n+\n+test_expect_success 'push to group' '\n+\tgit push them &&\n+\tj= &&\n+\tfor i in 1 2 3\n+\tdo\n+\t\tgit -C dest-$i for-each-ref >actual-$i &&\n+\t\tif test -n \"$j\"\n+\t\tthen\n+\t\t\ttest_cmp actual-$j actual-$i\n+\t\telse\n+\t\t\tcat actual-$i\n+\t\tfi &&\n+\t\tj=$i ||\n+\t\treturn 1\n+\tdone\n+'\n+\n+test_done\n-- \n2.52.0-313-gc4a767987d\n\n"},{"id":"531810","messageId":"aTZ9iMPKLAfd-GSt@pks.im","threadId":"64588","inReplyTo":"xmqqfr9mnbu9.fsf@gitster.g","subject":"Re: [PATCH] connect: plug protocol capability leak","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-08T07:26:00Z","receivedAt":"2025-12-08T07:26:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Dec 07, 2025 at 01:40:46PM +0900, Junio C Hamano wrote:\n> When pushing to a set of remotes using a nickname for the group, the\n> client initializes the connection to each remote, talks to the\n> remote and reads and parses capabilities line, and holds the\n> capabilities in a file-scope static variable server_capabilities_v1.\n> \n> There are a few other such file-scope static variables, and these\n> connections cannot be parallelized until they are refactored to a\n> structure that keeps track of active connections.\n> \n> Which is *not* the theme of this patch ;-)\n> \n> For a single connection, the server_capabilities_v1 variable is\n> initialized to NULL (at the program initialization), populated when\n> we talk to the other side, used to look up capabilities of the other\n> sdie possible multiple times, and the memory is held by the variable\n\ns/sdie/side/\n\n> diff --git a/connect.c b/connect.c\n> index 8352b71faf..c6f76e3082 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -240,6 +240,8 @@ static void process_capabilities(struct packet_reader *reader, size_t *linelen)\n>  \tsize_t nul_location = strlen(line);\n>  \tif (nul_location == *linelen)\n>  \t\treturn;\n> +\n> +\tfree(server_capabilities_v1);\n>  \tserver_capabilities_v1 = xstrdup(line + nul_location + 1);\n>  \t*linelen = nul_location;\n\nThis looks obviously correct.\n\n> diff --git a/t/meson.build b/t/meson.build\n> index d3d0be2822..459c52a489 100644\n> --- a/t/meson.build\n> +++ b/t/meson.build\n> @@ -690,6 +690,7 @@ integration_tests = [\n>    't5562-http-backend-content-length.sh',\n>    't5563-simple-http-auth.sh',\n>    't5564-http-proxy.sh',\n> +  't5565-push-multiple.sh',\n>    't5570-git-daemon.sh',\n>    't5571-pre-push-hook.sh',\n>    't5572-pull-submodule.sh',\n> diff --git a/t/t5565-push-multiple.sh b/t/t5565-push-multiple.sh\n> new file mode 100755\n> index 0000000000..7e93668566\n> --- /dev/null\n> +++ b/t/t5565-push-multiple.sh\n\nNit: we have several tests in t5505 that are related to push groups, so\nwe might want to add this new test over there. I don't care too much\nthough, so please feel free to ignore this nit.\n\nThanks!\n\nPatrick\n"},{"id":"531832","messageId":"xmqqecp5ksbi.fsf@gitster.g","threadId":"64588","inReplyTo":"aTZ9iMPKLAfd-GSt@pks.im","subject":"Re: [PATCH] connect: plug protocol capability leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-12-08T13:37:37Z","receivedAt":"2025-12-08T13:37:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> +++ b/t/t5565-push-multiple.sh\n>\n> Nit: we have several tests in t5505 that are related to push groups, so\n> we might want to add this new test over there. I don't care too much\n> though, so please feel free to ignore this nit.\n\nI did notice the one that adds configuration but it did not look\nlike it is actually pushing there.  In fact, I think 5505 is more\nabout \"git remote\" futzing with the configuration that defines\nvarious attributes of remote, not about the push or fetch operations\nthat are carried out using these remote definitions.\n"},{"id":"531858","messageId":"20251208201856.GB216526@coredump.intra.peff.net","threadId":"64588","inReplyTo":"xmqqfr9mnbu9.fsf@gitster.g","subject":"Re: [PATCH] connect: plug protocol capability leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-12-08T20:18:56Z","receivedAt":"2025-12-08T20:18:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 07, 2025 at 01:40:46PM +0900, Junio C Hamano wrote:\n\n> diff --git a/connect.c b/connect.c\n> index 8352b71faf..c6f76e3082 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -240,6 +240,8 @@ static void process_capabilities(struct packet_reader *reader, size_t *linelen)\n>  \tsize_t nul_location = strlen(line);\n>  \tif (nul_location == *linelen)\n>  \t\treturn;\n> +\n> +\tfree(server_capabilities_v1);\n>  \tserver_capabilities_v1 = xstrdup(line + nul_location + 1);\n>  \t*linelen = nul_location;\n>  \n\nThe fix looks obviously correct.\n\nI couldn't help but notice that \"v1\" here is a little confusing, as it\nis really \"v0\". Or I guess if you want to be pedantic, \"v1\" is v0 with\nthe extra useless version string probe that nobody actually sends. So it\ntechnically is also the v1 capabilities string, but I think v0 is more\ndescriptive.\n\nAnyway, way off the topic of your patch, and maybe not even worth fixing\nindependently. I removed a couple of confusing \"v1 protocol\" mentions in\nthe test suite, but I don't know if this one would actually bother\nanyone.\n\n-Peff\n"}]}