patchtransport-helper: fix TSAN race in transfer_debug()
7 messages between Jun 2, 2026 and Jun 11, 2026, from Pushkar Singh, Junio C Hamano, Jeff King.
Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.
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(-)
Show changes to 2 files +6 −12
.tsan-suppressions, transport-helper.c
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()
Pushkar Singh <pushkarkumarsingh1970@gmail.com> writes:
Show 7 quoted lines
> +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.
Show 10 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;
> +
> /* Fill the state fields. */
> state.ptg.src = input;
> state.ptg.dest = 1;Re: [PATCH] transport-helper: fix TSAN race in transfer_debug()
Hi Junio,
On Thu, Jun 4, 2026 at 6:39 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> 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()
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(-)
Show changes to 2 files +6 −12
.tsan-suppressions, transport-helper.c
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()
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
[PATCH v3] transport-helper: fix TSAN race in transfer_debug()
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(-)
Show changes to 2 files +8 −12
.tsan-suppressions, transport-helper.c
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
Re: [PATCH v3] transport-helper: fix TSAN race in transfer_debug()
On Tue, Jun 09, 2026 at 01:47:42PM +0000, Pushkar Singh wrote:
Show 5 quoted lines
> 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.
Thanks, this looks OK to me.
> + if (transfer_debug_enabled < 0)
> + BUG("somebody forgot to check GIT_TRANSLOOP_DEBUG!");That is a somewhat silly message, but the point is that nobody is supposed to see it ever. And it does lead them to roughly the right spot, so I think it's sufficient. (Yes, I know it came from my earlier response).
-Peff