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

From: Jeff King <peff@peff.net>
Date: Jun 9, 2026, 00:28 UTC
Message-ID: <20260609002833.GE358144@coredump.intra.peff.net>
In-Reply-To: <20260604132327.277693-3-pushkarkumarsingh1970@gmail.com>

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

Show 8 quoted lines

> 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).

Show 7 quoted lines

>  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;

Show 7 quoted lines

> @@ -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