On Wed, Nov 3, 2021 at 7:37 PM Jeff King <peff@xxxxxxxx> wrote:
> On Wed, Nov 03, 2021 at 07:23:23PM -0700, Carlo Arenas wrote:
> > > diff --git a/run-command.c b/run-command.c
> > > index 7ef5cc712a9..a82cf69e7d3 100644
> > > --- a/run-command.c
> > > +++ b/run-command.c
> > > @@ -1099,7 +1099,7 @@ static NORETURN void die_async(const char *err, va_list params)
> > > static int async_die_is_recursing(void)
> > > {
> > > void *ret = pthread_getspecific(async_die_counter);
> > > - pthread_setspecific(async_die_counter, (void *)1);
> > > + pthread_setspecific(async_die_counter, &ret); /* set to any non-NULL valid pointer */
> >
> > I guess this would work, since the pointer is never dereferenced, but
> > the use of (void *)1 was hacky, and this warning seems like the right
> > time to make it less so.
> >
> > Would a dynamically allocated pthread_local variable be a better
> > option, or even a static global, since we don't care about its value
> > so no need to worry about any races?
>
> Yeah, I had the same thought. I think what's in the patch above is OK in
> practice, but it sure _feels_ wrong to store the address of an auto
> variable that goes out of scope.
>
> I'm OK with it as a minimal fix, though, to get things unstuck. The
> commit message nicely explains what's going on, and the original (which
> it looks like blames to me ;) ) is pretty gross, too.
Agree it is OK as a minimal fix, but also AFAIK nothing is really
stuck either, so something as simple as :
s/&ret/&async_die_counter/g
Would make it as minimal, and less likely to trigger something else in
the future (I am surprised nothing warns about local variables being
used out of scope).
Carlo
