Re: [PATCH v4] transport-helper, connect: use clean_on_exit to reap children on abnormal exit

From: Jeff King <peff@peff.net>
Date: Mar 14, 2026, 16:08 UTC
Message-ID: <20260314160814.GA918806@coredump.intra.peff.net>
In-Reply-To: <xmqqsea4aen2.fsf@gitster.g>

On Thu, Mar 12, 2026 at 03:04:17PM -0700, Junio C Hamano wrote:

> Thanks, queued.

Curiously this patch seems to cause a failure in one of the CI leak jobs, but I don't think it's the culprit. See below for a fix and explanation.

I don't know if you want to apply it separately (since it's really a totally different topic) or on top (since it is only the application of Andrew's patch which lets us find the problem).

-- >8 --
Subject: [PATCH] transport: plug leaks in transport_color_config()

We retrieve config values with repo_config_get_string(), which will allocate a new copy of the string for us. But we don't hold on to those strings, since they are just fed to git_config_colorbool() and color_parse(). But nor do we free them, which means they leak.

We can fix this by using the "_tmp" form of repo_config_get_string(), which just hands us a pointer directly to the internal storage. This is OK for our purposes, since we don't need it to last for longer than our parsing calls.

Two interesting side notes here:

  1. Many types already have a repo_config_get_X() variant that handles
     this for us (e.g., repo_config_get_bool()). But neither colorbools
     nor colors themselves have such helpers. We might think about
     adding them, but converting all callers is a larger task, and out
     of scope for this fix.

  2. As far as I can tell, this leak has been there since 960786e761
     (push: colorize errors, 2018-04-21), but wasn't detected by LSan in
     our test suite. It started triggering when we applied dd3693eb08
     (transport-helper, connect: use clean_on_exit to reap children on
     abnormal exit, 2026-03-12) which is mostly unrelated.

     Even weirder, it seems to trigger only with clang (and not gcc),
     and only with GIT_TEST_DEFAULT_REF_FORMAT=reftable. So I think this
     is another odd case where the pointers happened to be hanging
     around in stack memory, but changing the pattern of function calls
     in nearby code was enough for them to be incidentally overwritten.

Signed-off-by: Jeff King <peff@peff.net>
---
 transport.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/transport.c b/transport.c
index 107f4fa5dc..2fb4767821 100644
--- a/transport.c
+++ b/transport.c
@@ -54,14 +54,14 @@ static int transport_color_config(void)
 		return 0;
 	initialized = 1;
 
-	if (!repo_config_get_string(the_repository, key, &value))
+	if (!repo_config_get_string_tmp(the_repository, key, &value))
 		transport_use_color = git_config_colorbool(key, value);
 
 	if (!want_color_stderr(transport_use_color))
 		return 0;
 
 	for (size_t i = 0; i < ARRAY_SIZE(keys); i++)
-		if (!repo_config_get_string(the_repository, keys[i], &value)) {
+		if (!repo_config_get_string_tmp(the_repository, keys[i], &value)) {
 			if (!value)
 				return config_error_nonbool(keys[i]);
 			if (color_parse(value, transport_colors[i]) < 0)

-- 
2.53.0.887.g3d5d06adec