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

On Sat, Dec 28, 2024 at 09:27:41AM +0100, Patrick Steinhardt wrote:

I did a bit of digging on this last week, but didn't come up with a very satisfactory solution. You can reproduce easily by building with SANITIZE=leak and running t0003 with --stress.

It's the same issue we tried to address in 993d38a066 (index-pack: spawn threads atomically, 2024-01-05): one thread calls exit() and takes down the process while other threads are still being spawned (so the leak checker runs while those other threads are in a weird state where lsan has allocated some memory but not yet set up the thread stack as a place to look for reachable memory).

There we dealt with it by making sure no thread started work (and thus hit the exit call) until all of them were sanitized. I tried doing something similar here, like:

  diff --git a/builtin/grep.c b/builtin/grep.c
  index 98b85c7fca..866645e6f8 100644
  --- a/builtin/grep.c
  +++ b/builtin/grep.c
  @@ -233,18 +233,20 @@ static void start_threads(struct grep_opt *opt)
                  strbuf_init(&todo[i].out, 0);
          }

  +       grep_lock();
          CALLOC_ARRAY(threads, num_threads);
          for (i = 0; i < num_threads; i++) {
                  int err;
                  struct grep_opt *o = grep_opt_dup(opt);
                  o->output = strbuf_out;
                  compile_grep_patterns(o);
                  err = pthread_create(&threads[i], NULL, run, o);

                  if (err)
                          die(_("grep: failed to create thread: %s"),
                              strerror(err));
          }
  +       grep_unlock();
   }

   static int wait_all(void)

but it doesn't work. In fact, this does nothing at all because each thread will start by looking for work to do via get_work(), and we do not call add_work() to give them anything to do until all threads are spawned.

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.

It mostly worked in 993d38a066 because index-pack has to do more work to get to the exit() call. So delaying the start of the threads was enough to usually win the race. But here, the individual grep threads get to the exit() call very quickly, and it's not enough.

Which would mean that 993d38a066 is not actually a full fix, either. And indeed, I can get t5309 to fail even with that patch (which is weird, because that was how I tested it originally; I wonder if anything on the LSan side changed?).

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).

One quick workaround is this: