[PATCH v2] transport-helper: fix TSAN race in transfer_debug()
- From
Pushkar Singh <pushkarkumarsingh1970@gmail.com>
- Date
- Jun 4, 2026, 13:23 UTC
- 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 <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