Message ID | 877dffcpwd.fsf@oldenburg.str.redhat.com |
---|---|
State | New |
Headers | show |
Series | [v2] nptl: pthread_kill needs to return ESRCH for old programs (bug 19193) | expand |
On 9/17/21 09:06, Florian Weimer wrote: > The fix for bug 19193 breaks some old applications which appear > to use pthread_kill to probe if a thread is still running, something > that is not supported by POSIX. Initial patch didn't build for hurd, but bmg caught the issue, and this one uses PTHREAD_IN_LIBC to fix those issues. So that looks good. The overall refactor looks fine, we split the implementation into two pieces and use no_tid to control the behaviour of the generic code. LGTM. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > --- > v2: Fix test build on Hurd by disabling the compat testing > > nptl/pthread_kill.c | 37 ++++++++++++++++++++++++------- > sysdeps/pthread/tst-pthread_kill-exited.c | 21 ++++++++++++++++-- > 2 files changed, 48 insertions(+), 10 deletions(-) > > diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c > index fb7862eff7..a44dc8f2d9 100644 > --- a/nptl/pthread_kill.c > +++ b/nptl/pthread_kill.c > @@ -21,8 +21,11 @@ > #include <pthreadP.h> > #include <shlib-compat.h> > > -int > -__pthread_kill_internal (pthread_t threadid, int signo) > +/* Sends SIGNO to THREADID. If the thread is about to exit or has > + already exited on the kernel side, return NO_TID. Otherwise return > + 0 or an error code. */ OK. Rename to __pthread_kill_impl*. > +static int > +__pthread_kill_implementation (pthread_t threadid, int signo, int no_tid) > { > struct pthread *pd = (struct pthread *) threadid; > if (pd == THREAD_SELF) > @@ -52,11 +55,8 @@ __pthread_kill_internal (pthread_t threadid, int signo) > signal is either not observable (the target thread has already > blocked signals at this point), or it will fail, or it might be > delivered to a new, unrelated thread that has reused the TID. > - So do not actually send the signal. Do not report an error > - because the threadid argument is still valid (the thread ID > - lifetime has not ended), and ESRCH (for example) would be > - misleading. */ > - ret = 0; > + So do not actually send the signal. */ > + ret = no_tid; OK. So this is the semantic change which is that we *do* lie a little bit here, but from the point of legacy applications we need to do that because of the expected behaviour. > else > { > /* Using tgkill is a safety measure. pd->exit_lock ensures that > @@ -71,6 +71,15 @@ __pthread_kill_internal (pthread_t threadid, int signo) > return ret; > } > > +int > +__pthread_kill_internal (pthread_t threadid, int signo) > +{ > + /* Do not report an error in the no-tid case because the threadid > + argument is still valid (the thread ID lifetime has not ended), > + and ESRCH (for example) would be misleading. */ > + return __pthread_kill_implementation (threadid, signo, 0); OK. no_tid is 0. This is the default for new programs. > +} > + > int > __pthread_kill (pthread_t threadid, int signo) > { > @@ -81,6 +90,7 @@ __pthread_kill (pthread_t threadid, int signo) > > return __pthread_kill_internal (threadid, signo); > } > + > /* Some architectures (for instance arm) might pull raise through libgcc, so > avoid the symbol version if it ends up being used on ld.so. */ > #if !IS_IN(rtld) > @@ -88,6 +98,17 @@ libc_hidden_def (__pthread_kill) > versioned_symbol (libc, __pthread_kill, pthread_kill, GLIBC_2_34); > > # if OTHER_SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_34) > -compat_symbol (libc, __pthread_kill, pthread_kill, GLIBC_2_0); > +/* Variant which returns ESRCH in the no-TID case, for backwards > + compatibility. */ > +int > +attribute_compat_text_section > +__pthread_kill_esrch (pthread_t threadid, int signo) > +{ > + if (__is_internal_signal (signo)) > + return EINVAL; > + > + return __pthread_kill_implementation (threadid, signo, ESRCH); OK. no_tid is ESRCH, this is the default for old programs. > +} > +compat_symbol (libc, __pthread_kill_esrch, pthread_kill, GLIBC_2_0); OK. Use a compat symbol to call __pthread_kill_esrch. > # endif > #endif > diff --git a/sysdeps/pthread/tst-pthread_kill-exited.c b/sysdeps/pthread/tst-pthread_kill-exited.c > index 7575fb6d58..a2fddad526 100644 > --- a/sysdeps/pthread/tst-pthread_kill-exited.c > +++ b/sysdeps/pthread/tst-pthread_kill-exited.c > @@ -16,11 +16,15 @@ > License along with the GNU C Library; if not, see > <https://www.gnu.org/licenses/>. */ > > -/* This test verifies that pthread_kill returns 0 (and not ESRCH) for > - a thread that has exited on the kernel side. */ > +/* This test verifies that the default pthread_kill returns 0 (and not > + ESRCH) for a thread that has exited on the kernel side. */ OK. > > +#include <errno.h> > +#include <pthread.h> > +#include <shlib-compat.h> > #include <signal.h> > #include <stddef.h> > +#include <support/check.h> > #include <support/support.h> > #include <support/xthread.h> > > @@ -30,6 +34,12 @@ noop_thread (void *closure) > return NULL; > } > > +#if TEST_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_34) && PTHREAD_IN_LIBC > +extern __typeof (pthread_kill) compat_pthread_kill; > +compat_symbol_reference (libpthread, compat_pthread_kill, pthread_kill, > + GLIBC_2_0); OK. Use old compat symbol. > +#endif > + > static int > do_test (void) > { > @@ -37,7 +47,14 @@ do_test (void) > > support_wait_for_thread_exit (); > > + /* NB: Always uses the default symbol due to separate compilation. */ > xpthread_kill (thr, SIGUSR1); > + > +#if TEST_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_34) && PTHREAD_IN_LIBC > + /* Old binaries need the non-conforming ESRCH error code. */ > + TEST_COMPARE (compat_pthread_kill (thr, SIGUSR1), ESRCH); OK. > +#endif > + > xpthread_join (thr); > > return 0; >
diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c index fb7862eff7..a44dc8f2d9 100644 --- a/nptl/pthread_kill.c +++ b/nptl/pthread_kill.c @@ -21,8 +21,11 @@ #include <pthreadP.h> #include <shlib-compat.h> -int -__pthread_kill_internal (pthread_t threadid, int signo) +/* Sends SIGNO to THREADID. If the thread is about to exit or has + already exited on the kernel side, return NO_TID. Otherwise return + 0 or an error code. */ +static int +__pthread_kill_implementation (pthread_t threadid, int signo, int no_tid) { struct pthread *pd = (struct pthread *) threadid; if (pd == THREAD_SELF) @@ -52,11 +55,8 @@ __pthread_kill_internal (pthread_t threadid, int signo) signal is either not observable (the target thread has already blocked signals at this point), or it will fail, or it might be delivered to a new, unrelated thread that has reused the TID. - So do not actually send the signal. Do not report an error - because the threadid argument is still valid (the thread ID - lifetime has not ended), and ESRCH (for example) would be - misleading. */ - ret = 0; + So do not actually send the signal. */ + ret = no_tid; else { /* Using tgkill is a safety measure. pd->exit_lock ensures that @@ -71,6 +71,15 @@ __pthread_kill_internal (pthread_t threadid, int signo) return ret; } +int +__pthread_kill_internal (pthread_t threadid, int signo) +{ + /* Do not report an error in the no-tid case because the threadid + argument is still valid (the thread ID lifetime has not ended), + and ESRCH (for example) would be misleading. */ + return __pthread_kill_implementation (threadid, signo, 0); +} + int __pthread_kill (pthread_t threadid, int signo) { @@ -81,6 +90,7 @@ __pthread_kill (pthread_t threadid, int signo) return __pthread_kill_internal (threadid, signo); } + /* Some architectures (for instance arm) might pull raise through libgcc, so avoid the symbol version if it ends up being used on ld.so. */ #if !IS_IN(rtld) @@ -88,6 +98,17 @@ libc_hidden_def (__pthread_kill) versioned_symbol (libc, __pthread_kill, pthread_kill, GLIBC_2_34); # if OTHER_SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_34) -compat_symbol (libc, __pthread_kill, pthread_kill, GLIBC_2_0); +/* Variant which returns ESRCH in the no-TID case, for backwards + compatibility. */ +int +attribute_compat_text_section +__pthread_kill_esrch (pthread_t threadid, int signo) +{ + if (__is_internal_signal (signo)) + return EINVAL; + + return __pthread_kill_implementation (threadid, signo, ESRCH); +} +compat_symbol (libc, __pthread_kill_esrch, pthread_kill, GLIBC_2_0); # endif #endif diff --git a/sysdeps/pthread/tst-pthread_kill-exited.c b/sysdeps/pthread/tst-pthread_kill-exited.c index 7575fb6d58..a2fddad526 100644 --- a/sysdeps/pthread/tst-pthread_kill-exited.c +++ b/sysdeps/pthread/tst-pthread_kill-exited.c @@ -16,11 +16,15 @@ License along with the GNU C Library; if not, see <https://www.gnu.org/licenses/>. */ -/* This test verifies that pthread_kill returns 0 (and not ESRCH) for - a thread that has exited on the kernel side. */ +/* This test verifies that the default pthread_kill returns 0 (and not + ESRCH) for a thread that has exited on the kernel side. */ +#include <errno.h> +#include <pthread.h> +#include <shlib-compat.h> #include <signal.h> #include <stddef.h> +#include <support/check.h> #include <support/support.h> #include <support/xthread.h> @@ -30,6 +34,12 @@ noop_thread (void *closure) return NULL; } +#if TEST_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_34) && PTHREAD_IN_LIBC +extern __typeof (pthread_kill) compat_pthread_kill; +compat_symbol_reference (libpthread, compat_pthread_kill, pthread_kill, + GLIBC_2_0); +#endif + static int do_test (void) { @@ -37,7 +47,14 @@ do_test (void) support_wait_for_thread_exit (); + /* NB: Always uses the default symbol due to separate compilation. */ xpthread_kill (thr, SIGUSR1); + +#if TEST_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_34) && PTHREAD_IN_LIBC + /* Old binaries need the non-conforming ESRCH error code. */ + TEST_COMPARE (compat_pthread_kill (thr, SIGUSR1), ESRCH); +#endif + xpthread_join (thr); return 0;