# [PATCH] transport-helper: fix TSAN race in transfer_debug() 7 messages from 2026-06-02 to 2026-06-11. Participants: Pushkar Singh, Junio C Hamano, Jeff King. Thread: https://gitlist.dev/t/65741 ## Pushkar Singh, 2026-06-02 20:13 Subject: [PATCH] transport-helper: fix TSAN race in transfer_debug() Message-ID: <20260602201309.38434-2-pushkarkumarsingh1970@gmail.com> ``` 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 --- .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 ``` ## Junio C Hamano, 2026-06-04 01:09 Subject: Re: [PATCH] transport-helper: fix TSAN race in transfer_debug() Message-ID: In-Reply-To: <20260602201309.38434-2-pushkarkumarsingh1970@gmail.com> ``` Pushkar Singh 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; ``` ## Pushkar Singh, 2026-06-04 10:19 Subject: Re: [PATCH] transport-helper: fix TSAN race in transfer_debug() Message-ID: In-Reply-To: ``` Hi Junio, On Thu, Jun 4, 2026 at 6:39 AM Junio C Hamano 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 ``` ## Pushkar Singh, 2026-06-04 13:23 Subject: [PATCH v2] transport-helper: fix TSAN race in transfer_debug() Message-ID: <20260604132327.277693-3-pushkarkumarsingh1970@gmail.com> In-Reply-To: <20260602201309.38434-2-pushkarkumarsingh1970@gmail.com> ``` 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 --- 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 ``` ## Jeff King, 2026-06-09 00:28 Subject: Re: [PATCH v2] transport-helper: fix TSAN race in transfer_debug() 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: > 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 ``` ## Pushkar Singh, 2026-06-09 13:47 Subject: [PATCH v3] transport-helper: fix TSAN race in transfer_debug() Message-ID: <20260609134741.4727-2-pushkarkumarsingh1970@gmail.com> In-Reply-To: <20260604132327.277693-3-pushkarkumarsingh1970@gmail.com> ``` 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 --- 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 ``` ## Jeff King, 2026-06-11 08:33 Subject: Re: [PATCH v3] transport-helper: fix TSAN race in transfer_debug() Message-ID: <20260611083320.GI2191159@coredump.intra.peff.net> In-Reply-To: <20260609134741.4727-2-pushkarkumarsingh1970@gmail.com> ``` On Tue, Jun 09, 2026 at 01:47:42PM +0000, Pushkar Singh wrote: > 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 ```