[v3,1/2] nptl: pthread_kill, pthread_cancel should not fail after exit (bug 19193)

Message ID a565c298740c6ecfe9768dfe3e5e3bb8265213f2.1630931178.git.fweimer@redhat.com
State New
Headers show
Series
  • Fix pthread_cancel, pthread_kill race (bug 12889)
Related show

Commit Message

H.J. Lu via Libc-alpha Sept. 6, 2021, 12:33 p.m.
This closes one remaining race condition related to bug 12889: if
the thread already exited on the kernel side, returning ESRCH
is not correct because that error is reserved for the thread IDs
(pthread_t values) whose lifetime has ended.  In case of a
kernel-side exit and a valid thread ID, no signal needs to be sent
and cancellation does not have an effect, so just return 0.

sysdeps/pthread/tst-kill4.c triggers undefined behavior and is
removed with this commit.
---
 nptl/pthread_cancel.c                       |  9 ++-
 nptl/pthread_kill.c                         |  7 +-
 sysdeps/pthread/Makefile                    |  5 +-
 sysdeps/pthread/tst-kill4.c                 | 89 ---------------------
 sysdeps/pthread/tst-pthread_cancel-exited.c | 45 +++++++++++
 sysdeps/pthread/tst-pthread_kill-exited.c   | 46 +++++++++++
 6 files changed, 106 insertions(+), 95 deletions(-)
 delete mode 100644 sysdeps/pthread/tst-kill4.c
 create mode 100644 sysdeps/pthread/tst-pthread_cancel-exited.c
 create mode 100644 sysdeps/pthread/tst-pthread_kill-exited.c

-- 
2.31.1

Comments

H.J. Lu via Libc-alpha Sept. 7, 2021, 12:42 p.m. | #1
On 06/09/2021 09:33, Florian Weimer via Libc-alpha wrote:
> This closes one remaining race condition related to bug 12889: if

> the thread already exited on the kernel side, returning ESRCH

> is not correct because that error is reserved for the thread IDs

> (pthread_t values) whose lifetime has ended.  In case of a

> kernel-side exit and a valid thread ID, no signal needs to be sent

> and cancellation does not have an effect, so just return 0.

> 

> sysdeps/pthread/tst-kill4.c triggers undefined behavior and is

> removed with this commit.


LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>


> ---

>  nptl/pthread_cancel.c                       |  9 ++-

>  nptl/pthread_kill.c                         |  7 +-

>  sysdeps/pthread/Makefile                    |  5 +-

>  sysdeps/pthread/tst-kill4.c                 | 89 ---------------------

>  sysdeps/pthread/tst-pthread_cancel-exited.c | 45 +++++++++++

>  sysdeps/pthread/tst-pthread_kill-exited.c   | 46 +++++++++++

>  6 files changed, 106 insertions(+), 95 deletions(-)

>  delete mode 100644 sysdeps/pthread/tst-kill4.c

>  create mode 100644 sysdeps/pthread/tst-pthread_cancel-exited.c

>  create mode 100644 sysdeps/pthread/tst-pthread_kill-exited.c

> 

> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c

> index 2bb523c0ec..a8aa3b3d15 100644

> --- a/nptl/pthread_cancel.c

> +++ b/nptl/pthread_cancel.c

> @@ -61,10 +61,11 @@ __pthread_cancel (pthread_t th)

>  {

>    volatile struct pthread *pd = (volatile struct pthread *) th;

>  

> -  /* Make sure the descriptor is valid.  */

> -  if (INVALID_TD_P (pd))

> -    /* Not a valid thread handle.  */

> -    return ESRCH;

> +  if (pd->tid == 0)

> +    /* The thread has already exited on the kernel side.  Its outcome

> +       (regular exit, other cancelation) has already been

> +       determined.  */

> +    return 0;

>  

>    static int init_sigcancel = 0;

>    if (atomic_load_relaxed (&init_sigcancel) == 0)


Ok.

> diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c

> index f79a2b26fc..5d4c86f920 100644

> --- a/nptl/pthread_kill.c

> +++ b/nptl/pthread_kill.c

> @@ -46,7 +46,12 @@ __pthread_kill_internal (pthread_t threadid, int signo)

>  	    ? INTERNAL_SYSCALL_ERRNO (val) : 0);

>      }

>    else

> -    val = ESRCH;

> +    /* The kernel reports that the thread has exited.  POSIX specifies

> +       the ESRCH error only for the case when the lifetime of a thread

> +       ID has ended, but calling pthread_kill on such a thread ID is

> +       undefined in glibc.  Therefore, do not treat kernel thread exit

> +       as an error.  */

> +    val = 0;

>  

>    return val;

>  }


Ok.

> diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile

> index 42f9fc5072..dedfa0d290 100644

> --- a/sysdeps/pthread/Makefile

> +++ b/sysdeps/pthread/Makefile

> @@ -89,7 +89,7 @@ tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \

>  	 tst-join8 tst-join9 tst-join10 tst-join11 tst-join12 tst-join13 \

>  	 tst-join14 tst-join15 \

>  	 tst-key1 tst-key2 tst-key3 tst-key4 \

> -	 tst-kill1 tst-kill2 tst-kill3 tst-kill4 tst-kill5 tst-kill6 \

> +	 tst-kill1 tst-kill2 tst-kill3 tst-kill5 tst-kill6 \

>  	 tst-locale1 tst-locale2 \

>  	 tst-memstream \

>  	 tst-mutex-errorcheck tst-mutex1 tst-mutex2 tst-mutex3 tst-mutex4 \

> @@ -118,6 +118,9 @@ tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \

>  	 tst-unload \

>  	 tst-unwind-thread \

>  	 tst-pt-vfork1 tst-pt-vfork2 tst-vfork1x tst-vfork2x \

> +	 tst-pthread_cancel-exited \

> +	 tst-pthread_kill-exited \

> +	 # tests

>  

>  tests-time64 := \

>    tst-abstime-time64 \


Ok.

> diff --git a/sysdeps/pthread/tst-kill4.c b/sysdeps/pthread/tst-kill4.c

> deleted file mode 100644

> index 222ceb724c..0000000000

> --- a/sysdeps/pthread/tst-kill4.c

> +++ /dev/null

> @@ -1,89 +0,0 @@

> -/* Copyright (C) 2003-2021 Free Software Foundation, Inc.

> -   This file is part of the GNU C Library.

> -

> -   The GNU C Library is free software; you can redistribute it and/or

> -   modify it under the terms of the GNU Lesser General Public

> -   License as published by the Free Software Foundation; either

> -   version 2.1 of the License, or (at your option) any later version.

> -

> -   The GNU C Library is distributed in the hope that it will be useful,

> -   but WITHOUT ANY WARRANTY; without even the implied warranty of

> -   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU

> -   Lesser General Public License for more details.

> -

> -   You should have received a copy of the GNU Lesser General Public

> -   License along with the GNU C Library; if not, see

> -   <https://www.gnu.org/licenses/>.  */

> -

> -#include <errno.h>

> -#include <pthread.h>

> -#include <signal.h>

> -#include <stdio.h>

> -#include <stdlib.h>

> -#include <unistd.h>

> -

> -

> -static void *

> -tf (void *a)

> -{

> -  return NULL;

> -}

> -

> -

> -int

> -do_test (void)

> -{

> -  pthread_attr_t at;

> -  if (pthread_attr_init (&at) != 0)

> -    {

> -      puts ("attr_create failed");

> -      exit (1);

> -    }

> -

> -  /* Limit thread stack size, because if it is too large, pthread_join

> -     will free it immediately rather than put it into stack cache.  */

> -  if (pthread_attr_setstacksize (&at, 2 * 1024 * 1024) != 0)

> -    {

> -      puts ("setstacksize failed");

> -      exit (1);

> -    }

> -

> -  pthread_t th;

> -  if (pthread_create (&th, &at, tf, NULL) != 0)

> -    {

> -      puts ("create failed");

> -      exit (1);

> -    }

> -

> -  pthread_attr_destroy (&at);

> -

> -  if (pthread_join (th, NULL) != 0)

> -    {

> -      puts ("join failed");

> -      exit (1);

> -    }

> -

> -  /* The following only works because we assume here something about

> -     the implementation.  Namely, that the memory allocated for the

> -     thread descriptor is not going away, that the TID field is

> -     cleared and therefore the signal is sent to process 0, and that

> -     we can savely assume there is no other process with this ID at

> -     that time.  */

> -  int e = pthread_kill (th, 0);

> -  if (e == 0)

> -    {

> -      puts ("pthread_kill succeeded");

> -      exit (1);

> -    }

> -  if (e != ESRCH)

> -    {

> -      puts ("pthread_kill didn't return ESRCH");

> -      exit (1);

> -    }

> -

> -  return 0;

> -}

> -

> -

> -#define TEST_FUNCTION do_test ()

> -#include "../test-skeleton.c"


Ok.

> diff --git a/sysdeps/pthread/tst-pthread_cancel-exited.c b/sysdeps/pthread/tst-pthread_cancel-exited.c

> new file mode 100644

> index 0000000000..811c9bee07

> --- /dev/null

> +++ b/sysdeps/pthread/tst-pthread_cancel-exited.c

> @@ -0,0 +1,45 @@

> +/* Test that pthread_kill succeeds for an exited thread.

> +   Copyright (C) 2021 Free Software Foundation, Inc.

> +   This file is part of the GNU C Library.

> +

> +   The GNU C Library is free software; you can redistribute it and/or

> +   modify it under the terms of the GNU Lesser General Public

> +   License as published by the Free Software Foundation; either

> +   version 2.1 of the License, or (at your option) any later version.

> +

> +   The GNU C Library is distributed in the hope that it will be useful,

> +   but WITHOUT ANY WARRANTY; without even the implied warranty of

> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU

> +   Lesser General Public License for more details.

> +

> +   You should have received a copy of the GNU Lesser General Public

> +   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.  */

> +

> +#include <stddef.h>

> +#include <support/support.h>

> +#include <support/xthread.h>

> +

> +static void *

> +noop_thread (void *closure)

> +{

> +  return NULL;

> +}

> +

> +static int

> +do_test (void)

> +{

> +  pthread_t thr = xpthread_create (NULL, noop_thread, NULL);

> +

> +  support_wait_for_thread_exit ();

> +

> +  xpthread_cancel (thr);

> +  xpthread_join (thr);

> +

> +  return 0;

> +}

> +

> +#include <support/test-driver.c>


Ok.

> diff --git a/sysdeps/pthread/tst-pthread_kill-exited.c b/sysdeps/pthread/tst-pthread_kill-exited.c

> new file mode 100644

> index 0000000000..7575fb6d58

> --- /dev/null

> +++ b/sysdeps/pthread/tst-pthread_kill-exited.c

> @@ -0,0 +1,46 @@

> +/* Test that pthread_kill succeeds for an exited thread.

> +   Copyright (C) 2021 Free Software Foundation, Inc.

> +   This file is part of the GNU C Library.

> +

> +   The GNU C Library is free software; you can redistribute it and/or

> +   modify it under the terms of the GNU Lesser General Public

> +   License as published by the Free Software Foundation; either

> +   version 2.1 of the License, or (at your option) any later version.

> +

> +   The GNU C Library is distributed in the hope that it will be useful,

> +   but WITHOUT ANY WARRANTY; without even the implied warranty of

> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU

> +   Lesser General Public License for more details.

> +

> +   You should have received a copy of the GNU Lesser General Public

> +   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.  */

> +

> +#include <signal.h>

> +#include <stddef.h>

> +#include <support/support.h>

> +#include <support/xthread.h>

> +

> +static void *

> +noop_thread (void *closure)

> +{

> +  return NULL;

> +}

> +

> +static int

> +do_test (void)

> +{

> +  pthread_t thr = xpthread_create (NULL, noop_thread, NULL);

> +

> +  support_wait_for_thread_exit ();

> +

> +  xpthread_kill (thr, SIGUSR1);

> +  xpthread_join (thr);

> +

> +  return 0;

> +}

> +

> +#include <support/test-driver.c>

> 


Ok.

Patch

diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index 2bb523c0ec..a8aa3b3d15 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -61,10 +61,11 @@  __pthread_cancel (pthread_t th)
 {
   volatile struct pthread *pd = (volatile struct pthread *) th;
 
-  /* Make sure the descriptor is valid.  */
-  if (INVALID_TD_P (pd))
-    /* Not a valid thread handle.  */
-    return ESRCH;
+  if (pd->tid == 0)
+    /* The thread has already exited on the kernel side.  Its outcome
+       (regular exit, other cancelation) has already been
+       determined.  */
+    return 0;
 
   static int init_sigcancel = 0;
   if (atomic_load_relaxed (&init_sigcancel) == 0)
diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c
index f79a2b26fc..5d4c86f920 100644
--- a/nptl/pthread_kill.c
+++ b/nptl/pthread_kill.c
@@ -46,7 +46,12 @@  __pthread_kill_internal (pthread_t threadid, int signo)
 	    ? INTERNAL_SYSCALL_ERRNO (val) : 0);
     }
   else
-    val = ESRCH;
+    /* The kernel reports that the thread has exited.  POSIX specifies
+       the ESRCH error only for the case when the lifetime of a thread
+       ID has ended, but calling pthread_kill on such a thread ID is
+       undefined in glibc.  Therefore, do not treat kernel thread exit
+       as an error.  */
+    val = 0;
 
   return val;
 }
diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
index 42f9fc5072..dedfa0d290 100644
--- a/sysdeps/pthread/Makefile
+++ b/sysdeps/pthread/Makefile
@@ -89,7 +89,7 @@  tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
 	 tst-join8 tst-join9 tst-join10 tst-join11 tst-join12 tst-join13 \
 	 tst-join14 tst-join15 \
 	 tst-key1 tst-key2 tst-key3 tst-key4 \
-	 tst-kill1 tst-kill2 tst-kill3 tst-kill4 tst-kill5 tst-kill6 \
+	 tst-kill1 tst-kill2 tst-kill3 tst-kill5 tst-kill6 \
 	 tst-locale1 tst-locale2 \
 	 tst-memstream \
 	 tst-mutex-errorcheck tst-mutex1 tst-mutex2 tst-mutex3 tst-mutex4 \
@@ -118,6 +118,9 @@  tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
 	 tst-unload \
 	 tst-unwind-thread \
 	 tst-pt-vfork1 tst-pt-vfork2 tst-vfork1x tst-vfork2x \
+	 tst-pthread_cancel-exited \
+	 tst-pthread_kill-exited \
+	 # tests
 
 tests-time64 := \
   tst-abstime-time64 \
diff --git a/sysdeps/pthread/tst-kill4.c b/sysdeps/pthread/tst-kill4.c
deleted file mode 100644
index 222ceb724c..0000000000
--- a/sysdeps/pthread/tst-kill4.c
+++ /dev/null
@@ -1,89 +0,0 @@ 
-/* Copyright (C) 2003-2021 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#include <errno.h>
-#include <pthread.h>
-#include <signal.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <unistd.h>
-
-
-static void *
-tf (void *a)
-{
-  return NULL;
-}
-
-
-int
-do_test (void)
-{
-  pthread_attr_t at;
-  if (pthread_attr_init (&at) != 0)
-    {
-      puts ("attr_create failed");
-      exit (1);
-    }
-
-  /* Limit thread stack size, because if it is too large, pthread_join
-     will free it immediately rather than put it into stack cache.  */
-  if (pthread_attr_setstacksize (&at, 2 * 1024 * 1024) != 0)
-    {
-      puts ("setstacksize failed");
-      exit (1);
-    }
-
-  pthread_t th;
-  if (pthread_create (&th, &at, tf, NULL) != 0)
-    {
-      puts ("create failed");
-      exit (1);
-    }
-
-  pthread_attr_destroy (&at);
-
-  if (pthread_join (th, NULL) != 0)
-    {
-      puts ("join failed");
-      exit (1);
-    }
-
-  /* The following only works because we assume here something about
-     the implementation.  Namely, that the memory allocated for the
-     thread descriptor is not going away, that the TID field is
-     cleared and therefore the signal is sent to process 0, and that
-     we can savely assume there is no other process with this ID at
-     that time.  */
-  int e = pthread_kill (th, 0);
-  if (e == 0)
-    {
-      puts ("pthread_kill succeeded");
-      exit (1);
-    }
-  if (e != ESRCH)
-    {
-      puts ("pthread_kill didn't return ESRCH");
-      exit (1);
-    }
-
-  return 0;
-}
-
-
-#define TEST_FUNCTION do_test ()
-#include "../test-skeleton.c"
diff --git a/sysdeps/pthread/tst-pthread_cancel-exited.c b/sysdeps/pthread/tst-pthread_cancel-exited.c
new file mode 100644
index 0000000000..811c9bee07
--- /dev/null
+++ b/sysdeps/pthread/tst-pthread_cancel-exited.c
@@ -0,0 +1,45 @@ 
+/* Test that pthread_kill succeeds for an exited thread.
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   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.  */
+
+#include <stddef.h>
+#include <support/support.h>
+#include <support/xthread.h>
+
+static void *
+noop_thread (void *closure)
+{
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  pthread_t thr = xpthread_create (NULL, noop_thread, NULL);
+
+  support_wait_for_thread_exit ();
+
+  xpthread_cancel (thr);
+  xpthread_join (thr);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/sysdeps/pthread/tst-pthread_kill-exited.c b/sysdeps/pthread/tst-pthread_kill-exited.c
new file mode 100644
index 0000000000..7575fb6d58
--- /dev/null
+++ b/sysdeps/pthread/tst-pthread_kill-exited.c
@@ -0,0 +1,46 @@ 
+/* Test that pthread_kill succeeds for an exited thread.
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   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.  */
+
+#include <signal.h>
+#include <stddef.h>
+#include <support/support.h>
+#include <support/xthread.h>
+
+static void *
+noop_thread (void *closure)
+{
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  pthread_t thr = xpthread_create (NULL, noop_thread, NULL);
+
+  support_wait_for_thread_exit ();
+
+  xpthread_kill (thr, SIGUSR1);
+  xpthread_join (thr);
+
+  return 0;
+}
+
+#include <support/test-driver.c>