[PATCH] transport-helper: fix TSAN race in transfer_debug()

[PATCH] transport-helper: fix TSAN race in transfer_debug()

[Pushkar Singh]

Currently, transfer_debug() lazily initializes a static variable based
on GIT_TRANSLOOP_DEBUG. Since the function may be called from multiple
worker threads, this initialization is racy and is therefore suppressed
in .tsan-suppressions.

Initialize the variable in bidirectional_transfer_loop() before any
worker threads or processes are created. This patch removes the race and
allows dropping the corresponding TSAN suppression.

Signed-off-by: Pushkar Singh <pushkarkumarsingh1970@gmail.com>
---
 .tsan-suppressions |  1 -
 transport-helper.c | 17 ++++++-----------
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/.tsan-suppressions b/.tsan-suppressions
index 5ba86d6845..d84883bd90 100644
--- a/.tsan-suppressions
+++ b/.tsan-suppressions
@@ -7,7 +7,6 @@
 # A static variable is written to racily, but we always write the same value, so
 # in practice it (hopefully!) doesn't matter.
 race:^want_color$
-race:^transfer_debug$
 
 # A boolean value, which tells whether the replace_map has been initialized or
 # not, is read racily with an update. As this variable is written to only once,
diff --git a/transport-helper.c b/transport-helper.c
index 04d55572a9..95a7fa7d86 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1361,24 +1361,16 @@ int transport_helper_init(struct transport *transport, const char *name)
 /* This should be enough to hold debugging message. */
 #define PBUFFERSIZE 8192
 
+static int transfer_debug_enabled = -1;
+
 /* Print bidirectional transfer loop debug message. */
 __attribute__((format (printf, 1, 2)))
 static void transfer_debug(const char *fmt, ...)
 {
-	/*
-	 * NEEDSWORK: This function is sometimes used from multiple threads, and
-	 * we end up using debug_enabled racily. That "should not matter" since
-	 * we always write the same value, but it's still wrong. This function
-	 * is listed in .tsan-suppressions for the time being.
-	 */
-
 	va_list args;
 	char msgbuf[PBUFFERSIZE];
-	static int debug_enabled = -1;
 
-	if (debug_enabled < 0)
-		debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
-	if (!debug_enabled)
+	if (!transfer_debug_enabled)
 		return;
 
 	va_start(args, fmt);
@@ -1648,6 +1640,9 @@ int bidirectional_transfer_loop(int input, int output)
 {
 	struct bidirectional_transfer_state state;
 
+	if (transfer_debug_enabled < 0)
+		transfer_debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
+
 	/* Fill the state fields. */
 	state.ptg.src = input;
 	state.ptg.dest = 1;
-- 
2.53.0.582.gca1db8a0f7

Re: [PATCH] transport-helper: fix TSAN race in transfer_debug()

[Junio C Hamano]

Pushkar Singh <pushkarkumarsingh1970@gmail.com> writes:

> +static int transfer_debug_enabled = -1;
> ...
> -	if (debug_enabled < 0)
> -		debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
> -	if (!debug_enabled)
> +	if (!transfer_debug_enabled)
>  		return;

Would it be possible that transfer_debug_enabled is still -1 at this
point?  We would proceed in such a case, which is a bit different from
what would have happened in the original.

Perhaps

	if (transfer_debug_enabled <= 0)
		return;

is what you want?  I dunno.

> @@ -1648,6 +1640,9 @@ int bidirectional_transfer_loop(int input, int output)
>  {
>  	struct bidirectional_transfer_state state;
>  
> +	if (transfer_debug_enabled < 0)
> +		transfer_debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
> +
>  	/* Fill the state fields. */
>  	state.ptg.src = input;
>  	state.ptg.dest = 1;

Re: [PATCH] transport-helper: fix TSAN race in transfer_debug()

[Pushkar Singh]

Hi Junio,

On Thu, Jun 4, 2026 at 6:39 AM Junio C Hamano <gitster@pobox.com> wrote:

> Would it be possible that transfer_debug_enabled is still -1 at this
> point?  We would proceed in such a case, which is a bit different from
> what would have happened in the original.
>
> Perhaps
>
>         if (transfer_debug_enabled <= 0)
>                 return;
>
> is what you want?  I dunno.

You're right. The original code would never proceed while the value was still
negative, whereas my change would.

I'll update it to use <= 0 and send a v2.

Thanks,
Pushkar

[PATCH v2] transport-helper: fix TSAN race in transfer_debug()

[Pushkar Singh]

Currently, transfer_debug() lazily initializes a static variable based
on GIT_TRANSLOOP_DEBUG. Since the function may be called from multiple
worker threads, this initialization is racy and is therefore suppressed
in .tsan-suppressions.

Initialize the variable in bidirectional_transfer_loop() before any
worker threads or processes are created. This patch removes the race and
allows dropping the corresponding TSAN suppression.

Signed-off-by: Pushkar Singh <pushkarkumarsingh1970@gmail.com>
---
Changes since v1:
- Treat negative values as disabled by using transfer_debug_enabled <= 0

 .tsan-suppressions |  1 -
 transport-helper.c | 17 ++++++-----------
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/.tsan-suppressions b/.tsan-suppressions
index 5ba86d6845..d84883bd90 100644
--- a/.tsan-suppressions
+++ b/.tsan-suppressions
@@ -7,7 +7,6 @@
 # A static variable is written to racily, but we always write the same value, so
 # in practice it (hopefully!) doesn't matter.
 race:^want_color$
-race:^transfer_debug$
 
 # A boolean value, which tells whether the replace_map has been initialized or
 # not, is read racily with an update. As this variable is written to only once,
diff --git a/transport-helper.c b/transport-helper.c
index 04d55572a9..9e69c67cde 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1361,24 +1361,16 @@ int transport_helper_init(struct transport *transport, const char *name)
 /* This should be enough to hold debugging message. */
 #define PBUFFERSIZE 8192
 
+static int transfer_debug_enabled = -1;
+
 /* Print bidirectional transfer loop debug message. */
 __attribute__((format (printf, 1, 2)))
 static void transfer_debug(const char *fmt, ...)
 {
-	/*
-	 * NEEDSWORK: This function is sometimes used from multiple threads, and
-	 * we end up using debug_enabled racily. That "should not matter" since
-	 * we always write the same value, but it's still wrong. This function
-	 * is listed in .tsan-suppressions for the time being.
-	 */
-
 	va_list args;
 	char msgbuf[PBUFFERSIZE];
-	static int debug_enabled = -1;
 
-	if (debug_enabled < 0)
-		debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
-	if (!debug_enabled)
+	if (transfer_debug_enabled <= 0)
 		return;
 
 	va_start(args, fmt);
@@ -1648,6 +1640,9 @@ int bidirectional_transfer_loop(int input, int output)
 {
 	struct bidirectional_transfer_state state;
 
+	if (transfer_debug_enabled < 0)
+		transfer_debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
+
 	/* Fill the state fields. */
 	state.ptg.src = input;
 	state.ptg.dest = 1;
-- 
2.53.0.582.gca1db8a0f7

Re: [PATCH v2] transport-helper: fix TSAN race in transfer_debug()

[Jeff King]

On Thu, Jun 04, 2026 at 01:23:29PM +0000, Pushkar Singh wrote:

> Currently, transfer_debug() lazily initializes a static variable based
> on GIT_TRANSLOOP_DEBUG. Since the function may be called from multiple
> worker threads, this initialization is racy and is therefore suppressed
> in .tsan-suppressions.
> 
> Initialize the variable in bidirectional_transfer_loop() before any
> worker threads or processes are created. This patch removes the race and
> allows dropping the corresponding TSAN suppression.

OK. I was surprised that this code would use threads at all, but I guess
it all comes from 419f37db4d (Add bidirectional_transfer_loop(),
2010-10-12).

It feels like this could probably be implemented without threads by
using poll(), but that's out of scope for this patch. (I also thought
that run-command's pump_io() might help, but I think it only pumps
to/from in-memory buffers, not between descriptors).

> Changes since v1:
> - Treat negative values as disabled by using transfer_debug_enabled <= 0

I saw Junio's comment on v1, but I wonder if quietly ignoring negative
values is correct. Isn't it a BUG() if the value is negative when we get
here? It means we're ignoring the value of $GIT_TRANSLOOP_DEBUG, which
might have actually been "1".

(Yet another aside: this really ought to use git_env_bool, though that
is a user-facing change).

>  static void transfer_debug(const char *fmt, ...)
> [...]
> -	if (debug_enabled < 0)
> -		debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
> -	if (!debug_enabled)
> +	if (transfer_debug_enabled <= 0)
>  		return;

So I feel like this ought to be:

  if (transfer_debug_enabled < 0)
	BUG("somebody forgot to check GIT_TRANSLOOP_DEBUG!");
  if (!transfer_debug_enabled)
	return;

> @@ -1648,6 +1640,9 @@ int bidirectional_transfer_loop(int input, int output)
>  {
>  	struct bidirectional_transfer_state state;
>  
> +	if (transfer_debug_enabled < 0)
> +		transfer_debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
> +

And then we're pretty confident that the BUG() does not trigger because
all of the users of the flag will run through this function.

For the same reason, we can be pretty confident that your existing code
would be fine in practice, too, of course. ;) But if we do hit this
case, I think a BUG() is the right thing.

-Peff

[PATCH v3] transport-helper: fix TSAN race in transfer_debug()

[Pushkar Singh]

Currently, transfer_debug() lazily initializes a static variable based
on GIT_TRANSLOOP_DEBUG. Since the function may be called from multiple
worker threads, this initialization is racy and is therefore suppressed
in .tsan-suppressions.

Initialize the variable in bidirectional_transfer_loop() before any
worker threads or processes are created. This patch removes the race and
allows dropping the corresponding TSAN suppression.

Signed-off-by: Pushkar Singh <pushkarkumarsingh1970@gmail.com>
---
Changes since v2:
- Treat an uninitialized transfer_debug_enabled as a BUG()
  instead of silently treating it as disabled.
- Follow Jeff King's suggestion to distinguish an
  uninitialized state from a disabled state.

 .tsan-suppressions |  1 -
 transport-helper.c | 19 ++++++++-----------
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/.tsan-suppressions b/.tsan-suppressions
index 5ba86d6845..d84883bd90 100644
--- a/.tsan-suppressions
+++ b/.tsan-suppressions
@@ -7,7 +7,6 @@
 # A static variable is written to racily, but we always write the same value, so
 # in practice it (hopefully!) doesn't matter.
 race:^want_color$
-race:^transfer_debug$
 
 # A boolean value, which tells whether the replace_map has been initialized or
 # not, is read racily with an update. As this variable is written to only once,
diff --git a/transport-helper.c b/transport-helper.c
index 04d55572a9..5b639bff3d 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1361,24 +1361,18 @@ int transport_helper_init(struct transport *transport, const char *name)
 /* This should be enough to hold debugging message. */
 #define PBUFFERSIZE 8192
 
+static int transfer_debug_enabled = -1;
+
 /* Print bidirectional transfer loop debug message. */
 __attribute__((format (printf, 1, 2)))
 static void transfer_debug(const char *fmt, ...)
 {
-	/*
-	 * NEEDSWORK: This function is sometimes used from multiple threads, and
-	 * we end up using debug_enabled racily. That "should not matter" since
-	 * we always write the same value, but it's still wrong. This function
-	 * is listed in .tsan-suppressions for the time being.
-	 */
-
 	va_list args;
 	char msgbuf[PBUFFERSIZE];
-	static int debug_enabled = -1;
 
-	if (debug_enabled < 0)
-		debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
-	if (!debug_enabled)
+	if (transfer_debug_enabled < 0)
+		BUG("somebody forgot to check GIT_TRANSLOOP_DEBUG!");
+	if (!transfer_debug_enabled)
 		return;
 
 	va_start(args, fmt);
@@ -1648,6 +1642,9 @@ int bidirectional_transfer_loop(int input, int output)
 {
 	struct bidirectional_transfer_state state;
 
+	if (transfer_debug_enabled < 0)
+		transfer_debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
+
 	/* Fill the state fields. */
 	state.ptg.src = input;
 	state.ptg.dest = 1;
-- 
2.53.0.582.gca1db8a0f7