Re: [PATCH 0/5] building git with clang/gcc address sanitizer

From: Jeff King <peff@peff.net>
Date: Jul 10, 2017, 13:30 UTC
Message-ID: <20170710133040.yom65mjol3nmf2it@sigill.intra.peff.net>
In-Reply-To: <20170710132418.d6bvzxwvbejretb4@sigill.intra.peff.net>

On Mon, Jul 10, 2017 at 09:24:18AM -0400, Jeff King wrote:

Show 7 quoted lines

> You can also try:
> 
>   make SANITIZE=thread test
> 
> but it's not clean. I poked at some of the errors, and I don't think
> there a problem in practice (though they may be worth cleaning up in the
> name of code hygiene).

Here's an example that I fixed up long ago, but never quite had the stomach to seriously propose.

If it were the last one, it might be worth applying to get a clean run of "make test", but I think it's just one of many.

-- >8 --
Date: Mon, 8 Dec 2014 03:02:34 -0500
Subject: [PATCH] transport-helper: initialize debug flag before starting
 threads

The transport_debug() function uses a static variable to lazily cache the boolean value of $TRANSPORT_DEBUG. If a program uses transport-helper's bidirectional_transfer_loop (as remote-ext and remote-fd do), then two threads may both try to write the variable at the same time.

We can fix this by initializing the variable right before starting the threads.

Noticed by "-fsanitize=thread". This probably isn't a problem in practice, as both threads will write the same value, and we are unlikely to see a torn write on an "int" (i.e., on sane platforms a write to an int is atomic).

Signed-off-by: Jeff King <peff@peff.net>
---
 transport-helper.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 33cff38cc..76f19ddb0 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1113,17 +1113,23 @@ int transport_helper_init(struct transport *transport, const char *name)
 /* This should be enough to hold debugging message. */
 #define PBUFFERSIZE 8192
 
+static int transport_debug_enabled(void)
+{
+	static int debug_enabled = -1;
+
+	if (debug_enabled < 0)
+		debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
+	return debug_enabled;
+}
+
 /* Print bidirectional transfer loop debug message. */
 __attribute__((format (printf, 1, 2)))
 static void transfer_debug(const char *fmt, ...)
 {
 	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 (!transport_debug_enabled())
 		return;
 
 	va_start(args, fmt);
@@ -1292,6 +1298,10 @@ static int tloop_spawnwait_tasks(struct bidirectional_transfer_state *s)
 	pthread_t ptg_thread;
 	int err;
 	int ret = 0;
+
+	/* initialize static global debug flag as a side effect */
+	transport_debug_enabled();
+
 	err = pthread_create(&gtp_thread, NULL, udt_copy_task_routine,
 		&s->gtp);
 	if (err)

-- 
2.13.2.1071.gcd8104b61