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