From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 08 Dec 2025 07:26:00 GMT
Subject: Re: [PATCH] connect: plug protocol capability leak
Message-ID: <aTZ9iMPKLAfd-GSt@pks.im>
In-Reply-To: <xmqqfr9mnbu9.fsf@gitster.g>

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