{"thread":{"id":"65741","subject":"[PATCH] transport-helper: fix TSAN race in transfer_debug()","startedAt":"2026-06-02T20:15:35Z","lastAt":"2026-06-11T08:33:22Z","messageCount":7,"participants":["Pushkar Singh","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"544553","messageId":"20260602201309.38434-2-pushkarkumarsingh1970@gmail.com","threadId":"65741","inReplyTo":null,"subject":"[PATCH] transport-helper: fix TSAN race in transfer_debug()","fromName":"Pushkar Singh","fromEmail":"pushkarkumarsingh1970@gmail.com","sentAt":"2026-06-02T20:13:10Z","receivedAt":"2026-06-02T20:15:35Z","isPatch":true,"body":"Currently, transfer_debug() lazily initializes a static variable based\non GIT_TRANSLOOP_DEBUG. Since the function may be called from multiple\nworker threads, this initialization is racy and is therefore suppressed\nin .tsan-suppressions.\n\nInitialize the variable in bidirectional_transfer_loop() before any\nworker threads or processes are created. This patch removes the race and\nallows dropping the corresponding TSAN suppression.\n\nSigned-off-by: Pushkar Singh <pushkarkumarsingh1970@gmail.com>\n---\n .tsan-suppressions |  1 -\n transport-helper.c | 17 ++++++-----------\n 2 files changed, 6 insertions(+), 12 deletions(-)\n\ndiff --git a/.tsan-suppressions b/.tsan-suppressions\nindex 5ba86d6845..d84883bd90 100644\n--- a/.tsan-suppressions\n+++ b/.tsan-suppressions\n@@ -7,7 +7,6 @@\n # A static variable is written to racily, but we always write the same value, so\n # in practice it (hopefully!) doesn't matter.\n race:^want_color$\n-race:^transfer_debug$\n \n # A boolean value, which tells whether the replace_map has been initialized or\n # not, is read racily with an update. As this variable is written to only once,\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 04d55572a9..95a7fa7d86 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1361,24 +1361,16 @@ int transport_helper_init(struct transport *transport, const char *name)\n /* This should be enough to hold debugging message. */\n #define PBUFFERSIZE 8192\n \n+static int transfer_debug_enabled = -1;\n+\n /* Print bidirectional transfer loop debug message. */\n __attribute__((format (printf, 1, 2)))\n static void transfer_debug(const char *fmt, ...)\n {\n-\t/*\n-\t * NEEDSWORK: This function is sometimes used from multiple threads, and\n-\t * we end up using debug_enabled racily. That \"should not matter\" since\n-\t * we always write the same value, but it's still wrong. This function\n-\t * is listed in .tsan-suppressions for the time being.\n-\t */\n-\n \tva_list args;\n \tchar msgbuf[PBUFFERSIZE];\n-\tstatic int debug_enabled = -1;\n \n-\tif (debug_enabled < 0)\n-\t\tdebug_enabled = getenv(\"GIT_TRANSLOOP_DEBUG\") ? 1 : 0;\n-\tif (!debug_enabled)\n+\tif (!transfer_debug_enabled)\n \t\treturn;\n \n \tva_start(args, fmt);\n@@ -1648,6 +1640,9 @@ int bidirectional_transfer_loop(int input, int output)\n {\n \tstruct bidirectional_transfer_state state;\n \n+\tif (transfer_debug_enabled < 0)\n+\t\ttransfer_debug_enabled = getenv(\"GIT_TRANSLOOP_DEBUG\") ? 1 : 0;\n+\n \t/* Fill the state fields. */\n \tstate.ptg.src = input;\n \tstate.ptg.dest = 1;\n-- \n2.53.0.582.gca1db8a0f7\n\n"},{"id":"544654","messageId":"xmqqv7bzp0vc.fsf@gitster.g","threadId":"65741","inReplyTo":"20260602201309.38434-2-pushkarkumarsingh1970@gmail.com","subject":"Re: [PATCH] transport-helper: fix TSAN race in transfer_debug()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-04T01:09:43Z","receivedAt":"2026-06-04T01:09:45Z","isPatch":true,"body":"Pushkar Singh <pushkarkumarsingh1970@gmail.com> writes:\n\n> +static int transfer_debug_enabled = -1;\n> ...\n> -\tif (debug_enabled < 0)\n> -\t\tdebug_enabled = getenv(\"GIT_TRANSLOOP_DEBUG\") ? 1 : 0;\n> -\tif (!debug_enabled)\n> +\tif (!transfer_debug_enabled)\n>  \t\treturn;\n\nWould it be possible that transfer_debug_enabled is still -1 at this\npoint?  We would proceed in such a case, which is a bit different from\nwhat would have happened in the original.\n\nPerhaps\n\n\tif (transfer_debug_enabled <= 0)\n\t\treturn;\n\nis what you want?  I dunno.\n\n> @@ -1648,6 +1640,9 @@ int bidirectional_transfer_loop(int input, int output)\n>  {\n>  \tstruct bidirectional_transfer_state state;\n>  \n> +\tif (transfer_debug_enabled < 0)\n> +\t\ttransfer_debug_enabled = getenv(\"GIT_TRANSLOOP_DEBUG\") ? 1 : 0;\n> +\n>  \t/* Fill the state fields. */\n>  \tstate.ptg.src = input;\n>  \tstate.ptg.dest = 1;\n"},{"id":"544715","messageId":"CALE2CrTF_HexShFLdh-Z0zJHcyeB3jOrTxLieZS++1GVqTesjg@mail.gmail.com","threadId":"65741","inReplyTo":"xmqqv7bzp0vc.fsf@gitster.g","subject":"Re: [PATCH] transport-helper: fix TSAN race in transfer_debug()","fromName":"Pushkar Singh","fromEmail":"pushkarkumarsingh1970@gmail.com","sentAt":"2026-06-04T10:19:21Z","receivedAt":"2026-06-04T10:19:34Z","isPatch":true,"body":"Hi Junio,\n\nOn Thu, Jun 4, 2026 at 6:39 AM Junio C Hamano <gitster@pobox.com> wrote:\n\n> Would it be possible that transfer_debug_enabled is still -1 at this\n> point?  We would proceed in such a case, which is a bit different from\n> what would have happened in the original.\n>\n> Perhaps\n>\n>         if (transfer_debug_enabled <= 0)\n>                 return;\n>\n> is what you want?  I dunno.\n\nYou're right. The original code would never proceed while the value was still\nnegative, whereas my change would.\n\nI'll update it to use <= 0 and send a v2.\n\nThanks,\nPushkar\n"},{"id":"544745","messageId":"20260604132327.277693-3-pushkarkumarsingh1970@gmail.com","threadId":"65741","inReplyTo":"20260602201309.38434-2-pushkarkumarsingh1970@gmail.com","subject":"[PATCH v2] transport-helper: fix TSAN race in transfer_debug()","fromName":"Pushkar Singh","fromEmail":"pushkarkumarsingh1970@gmail.com","sentAt":"2026-06-04T13:23:29Z","receivedAt":"2026-06-04T13:25:51Z","isPatch":true,"body":"Currently, transfer_debug() lazily initializes a static variable based\non GIT_TRANSLOOP_DEBUG. Since the function may be called from multiple\nworker threads, this initialization is racy and is therefore suppressed\nin .tsan-suppressions.\n\nInitialize the variable in bidirectional_transfer_loop() before any\nworker threads or processes are created. This patch removes the race and\nallows dropping the corresponding TSAN suppression.\n\nSigned-off-by: Pushkar Singh <pushkarkumarsingh1970@gmail.com>\n---\nChanges since v1:\n- Treat negative values as disabled by using transfer_debug_enabled <= 0\n\n .tsan-suppressions |  1 -\n transport-helper.c | 17 ++++++-----------\n 2 files changed, 6 insertions(+), 12 deletions(-)\n\ndiff --git a/.tsan-suppressions b/.tsan-suppressions\nindex 5ba86d6845..d84883bd90 100644\n--- a/.tsan-suppressions\n+++ b/.tsan-suppressions\n@@ -7,7 +7,6 @@\n # A static variable is written to racily, but we always write the same value, so\n # in practice it (hopefully!) doesn't matter.\n race:^want_color$\n-race:^transfer_debug$\n \n # A boolean value, which tells whether the replace_map has been initialized or\n # not, is read racily with an update. As this variable is written to only once,\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 04d55572a9..9e69c67cde 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1361,24 +1361,16 @@ int transport_helper_init(struct transport *transport, const char *name)\n /* This should be enough to hold debugging message. */\n #define PBUFFERSIZE 8192\n \n+static int transfer_debug_enabled = -1;\n+\n /* Print bidirectional transfer loop debug message. */\n __attribute__((format (printf, 1, 2)))\n static void transfer_debug(const char *fmt, ...)\n {\n-\t/*\n-\t * NEEDSWORK: This function is sometimes used from multiple threads, and\n-\t * we end up using debug_enabled racily. That \"should not matter\" since\n-\t * we always write the same value, but it's still wrong. This function\n-\t * is listed in .tsan-suppressions for the time being.\n-\t */\n-\n \tva_list args;\n \tchar msgbuf[PBUFFERSIZE];\n-\tstatic int debug_enabled = -1;\n \n-\tif (debug_enabled < 0)\n-\t\tdebug_enabled = getenv(\"GIT_TRANSLOOP_DEBUG\") ? 1 : 0;\n-\tif (!debug_enabled)\n+\tif (transfer_debug_enabled <= 0)\n \t\treturn;\n \n \tva_start(args, fmt);\n@@ -1648,6 +1640,9 @@ int bidirectional_transfer_loop(int input, int output)\n {\n \tstruct bidirectional_transfer_state state;\n \n+\tif (transfer_debug_enabled < 0)\n+\t\ttransfer_debug_enabled = getenv(\"GIT_TRANSLOOP_DEBUG\") ? 1 : 0;\n+\n \t/* Fill the state fields. */\n \tstate.ptg.src = input;\n \tstate.ptg.dest = 1;\n-- \n2.53.0.582.gca1db8a0f7\n\n"},{"id":"544990","messageId":"20260609002833.GE358144@coredump.intra.peff.net","threadId":"65741","inReplyTo":"20260604132327.277693-3-pushkarkumarsingh1970@gmail.com","subject":"Re: [PATCH v2] transport-helper: fix TSAN race in transfer_debug()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-06-09T00:28:33Z","receivedAt":"2026-06-09T00:28:34Z","isPatch":true,"body":"On Thu, Jun 04, 2026 at 01:23:29PM +0000, Pushkar Singh wrote:\n\n> Currently, transfer_debug() lazily initializes a static variable based\n> on GIT_TRANSLOOP_DEBUG. Since the function may be called from multiple\n> worker threads, this initialization is racy and is therefore suppressed\n> in .tsan-suppressions.\n> \n> Initialize the variable in bidirectional_transfer_loop() before any\n> worker threads or processes are created. This patch removes the race and\n> allows dropping the corresponding TSAN suppression.\n\nOK. I was surprised that this code would use threads at all, but I guess\nit all comes from 419f37db4d (Add bidirectional_transfer_loop(),\n2010-10-12).\n\nIt feels like this could probably be implemented without threads by\nusing poll(), but that's out of scope for this patch. (I also thought\nthat run-command's pump_io() might help, but I think it only pumps\nto/from in-memory buffers, not between descriptors).\n\n> Changes since v1:\n> - Treat negative values as disabled by using transfer_debug_enabled <= 0\n\nI saw Junio's comment on v1, but I wonder if quietly ignoring negative\nvalues is correct. Isn't it a BUG() if the value is negative when we get\nhere? It means we're ignoring the value of $GIT_TRANSLOOP_DEBUG, which\nmight have actually been \"1\".\n\n(Yet another aside: this really ought to use git_env_bool, though that\nis a user-facing change).\n\n>  static void transfer_debug(const char *fmt, ...)\n> [...]\n> -\tif (debug_enabled < 0)\n> -\t\tdebug_enabled = getenv(\"GIT_TRANSLOOP_DEBUG\") ? 1 : 0;\n> -\tif (!debug_enabled)\n> +\tif (transfer_debug_enabled <= 0)\n>  \t\treturn;\n\nSo I feel like this ought to be:\n\n  if (transfer_debug_enabled < 0)\n\tBUG(\"somebody forgot to check GIT_TRANSLOOP_DEBUG!\");\n  if (!transfer_debug_enabled)\n\treturn;\n\n> @@ -1648,6 +1640,9 @@ int bidirectional_transfer_loop(int input, int output)\n>  {\n>  \tstruct bidirectional_transfer_state state;\n>  \n> +\tif (transfer_debug_enabled < 0)\n> +\t\ttransfer_debug_enabled = getenv(\"GIT_TRANSLOOP_DEBUG\") ? 1 : 0;\n> +\n\nAnd then we're pretty confident that the BUG() does not trigger because\nall of the users of the flag will run through this function.\n\nFor the same reason, we can be pretty confident that your existing code\nwould be fine in practice, too, of course. ;) But if we do hit this\ncase, I think a BUG() is the right thing.\n\n-Peff\n"},{"id":"545076","messageId":"20260609134741.4727-2-pushkarkumarsingh1970@gmail.com","threadId":"65741","inReplyTo":"20260604132327.277693-3-pushkarkumarsingh1970@gmail.com","subject":"[PATCH v3] transport-helper: fix TSAN race in transfer_debug()","fromName":"Pushkar Singh","fromEmail":"pushkarkumarsingh1970@gmail.com","sentAt":"2026-06-09T13:47:42Z","receivedAt":"2026-06-09T13:49:42Z","isPatch":true,"body":"Currently, transfer_debug() lazily initializes a static variable based\non GIT_TRANSLOOP_DEBUG. Since the function may be called from multiple\nworker threads, this initialization is racy and is therefore suppressed\nin .tsan-suppressions.\n\nInitialize the variable in bidirectional_transfer_loop() before any\nworker threads or processes are created. This patch removes the race and\nallows dropping the corresponding TSAN suppression.\n\nSigned-off-by: Pushkar Singh <pushkarkumarsingh1970@gmail.com>\n---\nChanges since v2:\n- Treat an uninitialized transfer_debug_enabled as a BUG()\n  instead of silently treating it as disabled.\n- Follow Jeff King's suggestion to distinguish an\n  uninitialized state from a disabled state.\n\n .tsan-suppressions |  1 -\n transport-helper.c | 19 ++++++++-----------\n 2 files changed, 8 insertions(+), 12 deletions(-)\n\ndiff --git a/.tsan-suppressions b/.tsan-suppressions\nindex 5ba86d6845..d84883bd90 100644\n--- a/.tsan-suppressions\n+++ b/.tsan-suppressions\n@@ -7,7 +7,6 @@\n # A static variable is written to racily, but we always write the same value, so\n # in practice it (hopefully!) doesn't matter.\n race:^want_color$\n-race:^transfer_debug$\n \n # A boolean value, which tells whether the replace_map has been initialized or\n # not, is read racily with an update. As this variable is written to only once,\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 04d55572a9..5b639bff3d 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1361,24 +1361,18 @@ int transport_helper_init(struct transport *transport, const char *name)\n /* This should be enough to hold debugging message. */\n #define PBUFFERSIZE 8192\n \n+static int transfer_debug_enabled = -1;\n+\n /* Print bidirectional transfer loop debug message. */\n __attribute__((format (printf, 1, 2)))\n static void transfer_debug(const char *fmt, ...)\n {\n-\t/*\n-\t * NEEDSWORK: This function is sometimes used from multiple threads, and\n-\t * we end up using debug_enabled racily. That \"should not matter\" since\n-\t * we always write the same value, but it's still wrong. This function\n-\t * is listed in .tsan-suppressions for the time being.\n-\t */\n-\n \tva_list args;\n \tchar msgbuf[PBUFFERSIZE];\n-\tstatic int debug_enabled = -1;\n \n-\tif (debug_enabled < 0)\n-\t\tdebug_enabled = getenv(\"GIT_TRANSLOOP_DEBUG\") ? 1 : 0;\n-\tif (!debug_enabled)\n+\tif (transfer_debug_enabled < 0)\n+\t\tBUG(\"somebody forgot to check GIT_TRANSLOOP_DEBUG!\");\n+\tif (!transfer_debug_enabled)\n \t\treturn;\n \n \tva_start(args, fmt);\n@@ -1648,6 +1642,9 @@ int bidirectional_transfer_loop(int input, int output)\n {\n \tstruct bidirectional_transfer_state state;\n \n+\tif (transfer_debug_enabled < 0)\n+\t\ttransfer_debug_enabled = getenv(\"GIT_TRANSLOOP_DEBUG\") ? 1 : 0;\n+\n \t/* Fill the state fields. */\n \tstate.ptg.src = input;\n \tstate.ptg.dest = 1;\n-- \n2.53.0.582.gca1db8a0f7\n\n"},{"id":"545255","messageId":"20260611083320.GI2191159@coredump.intra.peff.net","threadId":"65741","inReplyTo":"20260609134741.4727-2-pushkarkumarsingh1970@gmail.com","subject":"Re: [PATCH v3] transport-helper: fix TSAN race in transfer_debug()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-06-11T08:33:20Z","receivedAt":"2026-06-11T08:33:22Z","isPatch":true,"body":"On Tue, Jun 09, 2026 at 01:47:42PM +0000, Pushkar Singh wrote:\n\n> Changes since v2:\n> - Treat an uninitialized transfer_debug_enabled as a BUG()\n>   instead of silently treating it as disabled.\n> - Follow Jeff King's suggestion to distinguish an\n>   uninitialized state from a disabled state.\n\nThanks, this looks OK to me.\n\n> +\tif (transfer_debug_enabled < 0)\n> +\t\tBUG(\"somebody forgot to check GIT_TRANSLOOP_DEBUG!\");\n\nThat is a somewhat silly message, but the point is that nobody is\nsupposed to see it ever. And it does lead them to roughly the right\nspot, so I think it's sufficient. (Yes, I know it came from my earlier\nresponse).\n\n-Peff\n"}]}