[v2] nptl: pthread_kill needs to return ESRCH for old programs (bug 19193)
Checks
Context |
Check |
Description |
dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
dj/TryBot-32bit |
success
|
Build for i686
|
Commit Message
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.
---
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(-)
Comments
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;
>
@@ -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
@@ -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;