| Message ID | pull.1072.git.1635990465854.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | async_die_is_recursing: fix use of pthread_getspecific for Fedora | expand |
On Wed, Nov 3, 2021 at 6:48 PM Victoria Dye via GitGit > The fix imitates a workaround added in SELinux [2] by using the pointer to > `ret` as the second argument to `pthread_getspecific`. the SELinux workaround uses a valid global pointer though; while this won't > 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? If neither, would MAP_FAILED also trigger the warning? Carlo
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. Keeping an actual counter variable would be the least-confusing thing, IMHO, but that implies allocating per-thread storage (which means having to clean it up). And we really only care about counting up to "1", so the boolean "do we have a pointer" is fine. The static variable you suggest might be a good middle ground there, and we could even use it for the comparison to make things more clear. Something like: static int async_die_is_recursing(void) { static int async_recursing_flag; void *ret = pthread_getspecific(async_die_counter); pthread_setspecific(async_die_counter, &async_recursing_flag); return ret == &async_recursing_flag; } But I don't feel that strongly either way. -Peff
On Wed, Nov 3, 2021 at 7:37 PM Jeff King <peff@peff.net> 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
On Wed, Nov 03, 2021 at 08:20:56PM -0700, Carlo Arenas wrote: > 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). It's in scope when we call the function. The potential for bugs only happens because we know pthread_setspecific() is going to hold on to it. So the compiler complaining would imply that it knows the semantics of the pthread function. -Peff
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 */ return ret != NULL; }