[PATCH] connect: plug protocol capability leak

[PATCH] connect: plug protocol capability leak

[Junio C Hamano]

When pushing to a set of remotes using a nickname for the group, the
client initializes the connection to each remote, talks to the
remote and reads and parses capabilities line, and holds the
capabilities in a file-scope static variable server_capabilities_v1.

There are a few other such file-scope static variables, and these
connections cannot be parallelized until they are refactored to a
structure that keeps track of active connections.

Which is *not* the theme of this patch ;-)

For a single connection, the server_capabilities_v1 variable is
initialized to NULL (at the program initialization), populated when
we talk to the other side, used to look up capabilities of the other
sdie possible multiple times, and the memory is held by the variable
until program exit, without leaking.  When talking to multiple remotes,
however, the server capabilities from the second connection overwrites
without freeing the one from the first connection, which leaks.

    ==1080970==ERROR: LeakSanitizer: detected memory leaks

    Direct leak of 421 byte(s) in 2 object(s) allocated from:
	#0 0x5615305f849e in strdup (/home/gitster/g/git-jch/bin/bin/git+0x2b349e) (BuildId: 54d149994c9e85374831958f694bd0aa3b8b1e26)
	#1 0x561530e76cc4 in xstrdup /home/gitster/w/build/wrapper.c:43:14
	#2 0x5615309cd7fa in process_capabilities /home/gitster/w/build/connect.c:243:27
	#3 0x5615309cd502 in get_remote_heads /home/gitster/w/build/connect.c:366:4
	#4 0x561530e2cb0b in handshake /home/gitster/w/build/transport.c:372:3
	#5 0x561530e29ed7 in get_refs_via_connect /home/gitster/w/build/transport.c:398:9
	#6 0x561530e26464 in transport_push /home/gitster/w/build/transport.c:1421:16
	#7 0x561530800bec in push_with_options /home/gitster/w/build/builtin/push.c:387:8
	#8 0x5615307ffb99 in do_push /home/gitster/w/build/builtin/push.c:442:7
	#9 0x5615307fe926 in cmd_push /home/gitster/w/build/builtin/push.c:664:7
	#10 0x56153065673f in run_builtin /home/gitster/w/build/git.c:506:11
	#11 0x56153065342f in handle_builtin /home/gitster/w/build/git.c:779:9
	#12 0x561530655b89 in run_argv /home/gitster/w/build/git.c:862:4
	#13 0x561530652cba in cmd_main /home/gitster/w/build/git.c:984:19
	#14 0x5615308dda0a in main /home/gitster/w/build/common-main.c:9:11
	#15 0x7f051651bca7 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

    SUMMARY: AddressSanitizer: 421 byte(s) leaked in 2 allocation(s).

Free the capablities data for the previous server before overwriting
it with the next server to plug this leak.

The added test fails without the freeing with SANITIZE=leak; I
somehow couldn't get it fail reliably with SANITIZE=leak,address
though.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 connect.c                |  2 ++
 t/meson.build            |  1 +
 t/t5565-push-multiple.sh | 39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+)
 create mode 100755 t/t5565-push-multiple.sh

diff --git a/connect.c b/connect.c
index 8352b71faf..c6f76e3082 100644
--- a/connect.c
+++ b/connect.c
@@ -240,6 +240,8 @@ static void process_capabilities(struct packet_reader *reader, size_t *linelen)
 	size_t nul_location = strlen(line);
 	if (nul_location == *linelen)
 		return;
+
+	free(server_capabilities_v1);
 	server_capabilities_v1 = xstrdup(line + nul_location + 1);
 	*linelen = nul_location;
 
diff --git a/t/meson.build b/t/meson.build
index d3d0be2822..459c52a489 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -690,6 +690,7 @@ integration_tests = [
   't5562-http-backend-content-length.sh',
   't5563-simple-http-auth.sh',
   't5564-http-proxy.sh',
+  't5565-push-multiple.sh',
   't5570-git-daemon.sh',
   't5571-pre-push-hook.sh',
   't5572-pull-submodule.sh',
diff --git a/t/t5565-push-multiple.sh b/t/t5565-push-multiple.sh
new file mode 100755
index 0000000000..7e93668566
--- /dev/null
+++ b/t/t5565-push-multiple.sh
@@ -0,0 +1,39 @@
+#!/bin/sh
+
+test_description='push to group'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	for i in 1 2 3
+	do
+		git init dest-$i &&
+		git -C dest-$i symbolic-ref HEAD refs/heads/not-a-branch ||
+		return 1
+	done &&
+	test_tick &&
+	git commit --allow-empty -m "initial" &&
+	git config set --append remote.them.pushurl "file://$(pwd)/dest-1" &&
+	git config set --append remote.them.pushurl "file://$(pwd)/dest-2" &&
+	git config set --append remote.them.pushurl "file://$(pwd)/dest-3" &&
+	git config set --append remote.them.push "+refs/heads/*:refs/heads/*"
+'
+
+test_expect_success 'push to group' '
+	git push them &&
+	j= &&
+	for i in 1 2 3
+	do
+		git -C dest-$i for-each-ref >actual-$i &&
+		if test -n "$j"
+		then
+			test_cmp actual-$j actual-$i
+		else
+			cat actual-$i
+		fi &&
+		j=$i ||
+		return 1
+	done
+'
+
+test_done
-- 
2.52.0-313-gc4a767987d

Re: [PATCH] connect: plug protocol capability leak

[Patrick Steinhardt]

On Sun, Dec 07, 2025 at 01:40:46PM +0900, Junio C Hamano wrote:
> When pushing to a set of remotes using a nickname for the group, the
> client initializes the connection to each remote, talks to the
> remote and reads and parses capabilities line, and holds the
> capabilities in a file-scope static variable server_capabilities_v1.
> 
> There are a few other such file-scope static variables, and these
> connections cannot be parallelized until they are refactored to a
> structure that keeps track of active connections.
> 
> Which is *not* the theme of this patch ;-)
> 
> For a single connection, the server_capabilities_v1 variable is
> initialized to NULL (at the program initialization), populated when
> we talk to the other side, used to look up capabilities of the other
> sdie possible multiple times, and the memory is held by the variable

s/sdie/side/

> diff --git a/connect.c b/connect.c
> index 8352b71faf..c6f76e3082 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -240,6 +240,8 @@ static void process_capabilities(struct packet_reader *reader, size_t *linelen)
>  	size_t nul_location = strlen(line);
>  	if (nul_location == *linelen)
>  		return;
> +
> +	free(server_capabilities_v1);
>  	server_capabilities_v1 = xstrdup(line + nul_location + 1);
>  	*linelen = nul_location;

This looks obviously correct.

> diff --git a/t/meson.build b/t/meson.build
> index d3d0be2822..459c52a489 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -690,6 +690,7 @@ integration_tests = [
>    't5562-http-backend-content-length.sh',
>    't5563-simple-http-auth.sh',
>    't5564-http-proxy.sh',
> +  't5565-push-multiple.sh',
>    't5570-git-daemon.sh',
>    't5571-pre-push-hook.sh',
>    't5572-pull-submodule.sh',
> diff --git a/t/t5565-push-multiple.sh b/t/t5565-push-multiple.sh
> new file mode 100755
> index 0000000000..7e93668566
> --- /dev/null
> +++ b/t/t5565-push-multiple.sh

Nit: we have several tests in t5505 that are related to push groups, so
we might want to add this new test over there. I don't care too much
though, so please feel free to ignore this nit.

Thanks!

Patrick

Re: [PATCH] connect: plug protocol capability leak

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> +++ b/t/t5565-push-multiple.sh
>
> Nit: we have several tests in t5505 that are related to push groups, so
> we might want to add this new test over there. I don't care too much
> though, so please feel free to ignore this nit.

I did notice the one that adds configuration but it did not look
like it is actually pushing there.  In fact, I think 5505 is more
about "git remote" futzing with the configuration that defines
various attributes of remote, not about the push or fetch operations
that are carried out using these remote definitions.

Re: [PATCH] connect: plug protocol capability leak

[Jeff King]

On Sun, Dec 07, 2025 at 01:40:46PM +0900, Junio C Hamano wrote:

> diff --git a/connect.c b/connect.c
> index 8352b71faf..c6f76e3082 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -240,6 +240,8 @@ static void process_capabilities(struct packet_reader *reader, size_t *linelen)
>  	size_t nul_location = strlen(line);
>  	if (nul_location == *linelen)
>  		return;
> +
> +	free(server_capabilities_v1);
>  	server_capabilities_v1 = xstrdup(line + nul_location + 1);
>  	*linelen = nul_location;
>  

The fix looks obviously correct.

I couldn't help but notice that "v1" here is a little confusing, as it
is really "v0". Or I guess if you want to be pedantic, "v1" is v0 with
the extra useless version string probe that nobody actually sends. So it
technically is also the v1 capabilities string, but I think v0 is more
descriptive.

Anyway, way off the topic of your patch, and maybe not even worth fixing
independently. I removed a couple of confusing "v1 protocol" mentions in
the test suite, but I don't know if this one would actually bother
anyone.

-Peff