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

[Jeff King <peff@peff.net>]

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