Re: racy leak sanitizer builds, was Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups

From: Jeff King <peff@peff.net>
Date: Dec 28, 2024, 19:23 UTC
Message-ID: <20241228192307.GC815586@coredump.intra.peff.net>
In-Reply-To: <20241228190541.GA815586@coredump.intra.peff.net>

On Sat, Dec 28, 2024 at 02:05:41PM -0500, Jeff King wrote:

Show 12 quoted lines

> So I suspect the race is actually trickier, and that the "weird state"
> is not something that happens just while pthread_create() is being
> called, but is actually running _in the thread itself_. So even though
> pthread_create() has returned for each thread, they are still setting
> themselves up before running.
> [...]
> So a full fix would actually require synchronization where we spawn each
> thread, then wait for all of them to hit the barrier to declare
> themselves ready, and then let them all start running. There is a
> pthread_barrier type that would help with this, but we've never used it
> before (so we'd probably need to at least provide a Windows compat
> layer).

So here's a fix that seems to work, but doesn't address the portability issues. One tweak is that the exit() call is actually not in a sub-thread, but in the main thread itself. I don't think that really changes the analysis too much, but it does mean we need to include the main thread in the barrier wait.

diff --git a/builtin/grep.c b/builtin/grep.c
index d00ee76f24..933b4503b8 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -101,6 +101,9 @@ static pthread_cond_t cond_write;
 /* Signalled when we are finished with everything. */
 static pthread_cond_t cond_result;
 
+/* Synchronize the start of all threads */
+static pthread_barrier_t start_barrier;
+
 static int skip_first_line;
 
 static void add_work(struct grep_opt *opt, struct grep_source *gs)
@@ -198,6 +201,8 @@ static void *run(void *arg)
 	int hit = 0;
 	struct grep_opt *opt = arg;
 
+	pthread_barrier_wait(&start_barrier);
+
 	while (1) {
 		struct work_item *w = get_work();
 		if (!w)
@@ -229,6 +234,7 @@ static void start_threads(struct grep_opt *opt)
 	pthread_cond_init(&cond_add, NULL);
 	pthread_cond_init(&cond_write, NULL);
 	pthread_cond_init(&cond_result, NULL);
+	pthread_barrier_init(&start_barrier, NULL, num_threads + 1);
 	grep_use_locks = 1;
 	enable_obj_read_lock();
 
@@ -248,6 +254,7 @@ static void start_threads(struct grep_opt *opt)
 			die(_("grep: failed to create thread: %s"),
 			    strerror(err));
 	}
+	pthread_barrier_wait(&start_barrier);
 }
 
 static int wait_all(void)
@@ -284,6 +291,7 @@ static int wait_all(void)
 	pthread_cond_destroy(&cond_add);
 	pthread_cond_destroy(&cond_write);
 	pthread_cond_destroy(&cond_result);
+	pthread_barrier_destroy(&start_barrier);
 	grep_use_locks = 0;
 	disable_obj_read_lock();
 

It would probably be pretty easy to do something similar for index-pack,
though really this is something I guess we'd need to do manually
anywhere we spawn worker threads.

-Peff