diff mbox series

[v2] nptl: pthread_kill needs to return ESRCH for old programs (bug 19193)

Message ID 877dffcpwd.fsf@oldenburg.str.redhat.com
State Committed
Commit 95dba35bf05e4a5d69dfae5e9c9d4df3646a7f93
Headers show
Series [v2] nptl: pthread_kill needs to return ESRCH for old programs (bug 19193) | expand

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

Florian Weimer Sept. 17, 2021, 1:06 p.m. UTC
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

Carlos O'Donell Sept. 20, 2021, 12:52 p.m. UTC | #1
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 mbox series

Patch

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;