public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH v2 0/4]  Fix ESRCH issues in pthread_cancel, pthread_kill
@ 2021-08-20 16:11 Florian Weimer
  2021-08-20 16:12 ` [PATCH v2 1/4] support: Add support_wait_for_thread_exit Florian Weimer
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Florian Weimer @ 2021-08-20 16:11 UTC (permalink / raw)
  To: libc-alpha

The new version fixes the support/Makefile update and puts the generic
“give me a non-reusable TID” functionality into a generic helper
function.

Tested on i686-linux-gnu and x86_64-linux-gnu.  Built with
build-many-glibcs.py.

Thanks,
Florian

Florian Weimer (4):
  support: Add support_wait_for_thread_exit
  nptl: pthread_kill, pthread_cancel should fail after exit (bug 19193)
  nptl: Add internal __pthread_call_with_tid function
  nptl: Fix race between pthread_kill and thread exit (bug 12889)

 nptl/Makefile                                 |   5 +-
 nptl/allocatestack.c                          |   1 +
 nptl/descr.h                                  |  11 ++
 nptl/pthread_call_with_tid.c                  |  66 +++++++
 nptl/pthread_cancel.c                         |   9 +-
 nptl/pthread_create.c                         |  18 ++
 nptl/pthread_kill.c                           |  44 +++--
 nptl/tst-pthread_call_with_tid.c              | 165 ++++++++++++++++++
 support/Makefile                              |   3 +-
 support/support.h                             |   4 +
 support/support_wait_for_thread_exit.c        |  72 ++++++++
 sysdeps/nptl/pthreadP.h                       |  20 +++
 sysdeps/pthread/Makefile                      |   7 +-
 sysdeps/pthread/tst-kill4.c                   |  90 ----------
 sysdeps/pthread/tst-pthread_cancel-exited.c   |  45 +++++
 .../pthread/tst-pthread_cancel-select-loop.c  |  87 +++++++++
 sysdeps/pthread/tst-pthread_kill-exited.c     |  46 +++++
 sysdeps/pthread/tst-pthread_kill-exiting.c    | 110 ++++++++++++
 18 files changed, 682 insertions(+), 121 deletions(-)
 create mode 100644 nptl/pthread_call_with_tid.c
 create mode 100644 nptl/tst-pthread_call_with_tid.c
 create mode 100644 support/support_wait_for_thread_exit.c
 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_cancel-select-loop.c
 create mode 100644 sysdeps/pthread/tst-pthread_kill-exited.c
 create mode 100644 sysdeps/pthread/tst-pthread_kill-exiting.c

-- 
2.31.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/4] support: Add support_wait_for_thread_exit
  2021-08-20 16:11 [PATCH v2 0/4] Fix ESRCH issues in pthread_cancel, pthread_kill Florian Weimer
@ 2021-08-20 16:12 ` Florian Weimer
  2021-09-29 20:27   ` Florian Weimer
  2021-08-20 16:13 ` [PATCH v2 2/4] nptl: pthread_kill, pthread_cancel should fail after exit (bug 19193) Florian Weimer
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Florian Weimer @ 2021-08-20 16:12 UTC (permalink / raw)
  To: libc-alpha

---
v2: Makefile update, make it clearer where the wait operation with
    pidfd_open would be put (it would replace the short usleep call).

 support/Makefile                       |  3 +-
 support/support.h                      |  4 ++
 support/support_wait_for_thread_exit.c | 72 ++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 1 deletion(-)
 create mode 100644 support/support_wait_for_thread_exit.c

diff --git a/support/Makefile b/support/Makefile
index a462781718..ef2b1a980a 100644
--- a/support/Makefile
+++ b/support/Makefile
@@ -82,9 +82,10 @@ libsupport-routines = \
   support_test_compare_blob \
   support_test_compare_failure \
   support_test_compare_string \
-  support_write_file_string \
   support_test_main \
   support_test_verify_impl \
+  support_wait_for_thread_exit \
+  support_write_file_string \
   temp_file \
   timespec \
   timespec-time64 \
diff --git a/support/support.h b/support/support.h
index 834dba9097..a5978b939a 100644
--- a/support/support.h
+++ b/support/support.h
@@ -174,6 +174,10 @@ timer_t support_create_timer (uint64_t sec, long int nsec, bool repeat,
 /* Disable the timer TIMER.  */
 void support_delete_timer (timer_t timer);
 
+/* Wait until all threads except the current thread have exited (as
+   far as the kernel is concerned).  */
+void support_wait_for_thread_exit (void);
+
 struct support_stack
 {
   void *stack;
diff --git a/support/support_wait_for_thread_exit.c b/support/support_wait_for_thread_exit.c
new file mode 100644
index 0000000000..658a813810
--- /dev/null
+++ b/support/support_wait_for_thread_exit.c
@@ -0,0 +1,72 @@
+/* Wait until all threads except the current thread has exited.
+   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/>.  */
+
+#include <dirent.h>
+#include <errno.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <unistd.h>
+
+void
+support_wait_for_thread_exit (void)
+{
+#ifdef __linux__
+  DIR *proc_self_task = opendir ("/proc/self/task");
+  TEST_VERIFY_EXIT (proc_self_task != NULL);
+
+  while (true)
+    {
+      errno = 0;
+      struct dirent *e = readdir (proc_self_task);
+      if (e == NULL && errno != 0)
+        FAIL_EXIT1 ("readdir: %m");
+      if (e == NULL)
+        {
+          /* Only the main thread remains.  Testing may continue.  */
+          closedir (proc_self_task);
+          return;
+        }
+
+      if (strcmp (e->d_name, ".") == 0 || strcmp (e->d_name, "..") == 0)
+        continue;
+
+      int task_tid = atoi (e->d_name);
+      if (task_tid <= 0)
+        FAIL_EXIT1 ("Invalid /proc/self/task entry: %s", e->d_name);
+
+      if (task_tid == gettid ())
+        /* The current thread.  Keep scanning for other
+           threads.  */
+        continue;
+
+      /* task_tid does not refer to this thread here, i.e., there is
+         another running thread.  */
+
+      /* Small timeout to give the thread a chance to exit.  */
+      usleep (50 * 1000);
+
+      /* Start scanning the directory from the start.  */
+      rewinddir (proc_self_task);
+    }
+#else
+  /* Use a large timeout because we cannot verify that the thread has
+     exited.  */
+  usleep (5 * 1000 * 1000);
+#endif
+}
-- 
2.31.1



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 2/4] nptl: pthread_kill, pthread_cancel should fail after exit (bug 19193)
  2021-08-20 16:11 [PATCH v2 0/4] Fix ESRCH issues in pthread_cancel, pthread_kill Florian Weimer
  2021-08-20 16:12 ` [PATCH v2 1/4] support: Add support_wait_for_thread_exit Florian Weimer
@ 2021-08-20 16:13 ` Florian Weimer
  2021-08-20 16:13 ` [PATCH v2 3/4] nptl: Add internal __pthread_call_with_tid function Florian Weimer
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: Florian Weimer @ 2021-08-20 16:13 UTC (permalink / raw)
  To: libc-alpha

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.
---
v2: Actually unchanged.

 nptl/pthread_cancel.c                       |  9 ++--
 nptl/pthread_kill.c                         |  7 +++-
 sysdeps/pthread/Makefile                    |  5 ++-
 sysdeps/pthread/tst-pthread_cancel-exited.c | 45 ++++++++++++++++++++
 sysdeps/pthread/tst-pthread_kill-exited.c   | 46 +++++++++++++++++++++
 5 files changed, 106 insertions(+), 6 deletions(-)
 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 cc25ff21f3..9bac6e3b76 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -62,10 +62,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-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>
-- 
2.31.1



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 3/4] nptl: Add internal __pthread_call_with_tid function
  2021-08-20 16:11 [PATCH v2 0/4] Fix ESRCH issues in pthread_cancel, pthread_kill Florian Weimer
  2021-08-20 16:12 ` [PATCH v2 1/4] support: Add support_wait_for_thread_exit Florian Weimer
  2021-08-20 16:13 ` [PATCH v2 2/4] nptl: pthread_kill, pthread_cancel should fail after exit (bug 19193) Florian Weimer
@ 2021-08-20 16:13 ` Florian Weimer
  2021-08-20 16:14 ` [PATCH v2 4/4] nptl: Fix race between pthread_kill and thread exit (bug 12889) Florian Weimer
  2021-08-20 18:09 ` [PATCH v2 0/4] Fix ESRCH issues in pthread_cancel, pthread_kill Adhemerval Zanella
  4 siblings, 0 replies; 12+ messages in thread
From: Florian Weimer @ 2021-08-20 16:13 UTC (permalink / raw)
  To: libc-alpha

It will be used to address race conditions during thread exit
due to TID reuse.

nptl/tst-pthread_call_with_tid has to be a static test because
__pthread_call_with_tid is not exported from libc.so.
---
v2: New patch, extracted from the previous 3/3 patch.  Added signal
    blocking (but not for the self-thread case), which is required for
    correctness.

 nptl/Makefile                    |   5 +-
 nptl/allocatestack.c             |   1 +
 nptl/descr.h                     |  11 +++
 nptl/pthread_call_with_tid.c     |  66 +++++++++++++
 nptl/pthread_create.c            |  18 ++++
 nptl/tst-pthread_call_with_tid.c | 165 +++++++++++++++++++++++++++++++
 sysdeps/nptl/pthreadP.h          |  20 ++++
 7 files changed, 285 insertions(+), 1 deletion(-)
 create mode 100644 nptl/pthread_call_with_tid.c
 create mode 100644 nptl/tst-pthread_call_with_tid.c

diff --git a/nptl/Makefile b/nptl/Makefile
index ff4d590f11..f6167dcd54 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -93,6 +93,7 @@ routines = \
   pthread_barrierattr_getpshared \
   pthread_barrierattr_init \
   pthread_barrierattr_setpshared \
+  pthread_call_with_tid \
   pthread_cancel \
   pthread_cleanup_upto \
   pthread_clockjoin \
@@ -436,7 +437,9 @@ tests-static += tst-stackguard1-static \
 		tst-mutex8-static tst-mutexpi8-static tst-sem11-static \
 		tst-sem12-static tst-cond11-static \
 		tst-pthread-gdb-attach-static \
-		tst-pthread_exit-nothreads-static
+		tst-pthread_exit-nothreads-static \
+		tst-pthread_call_with_tid \
+		# tests-static
 
 tests += tst-cancel24-static
 
diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index cfe37a3443..dd9e0b2c6b 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -127,6 +127,7 @@ get_cached_stack (size_t *sizep, void **memp)
   /* No pending event.  */
   result->nextevent = NULL;
 
+  result->exit_futex = 0;
   result->tls_state = (struct tls_internal_t) { 0 };
 
   /* Clear the DTV.  */
diff --git a/nptl/descr.h b/nptl/descr.h
index c85778d449..b848b9ecd4 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -396,6 +396,17 @@ struct pthread
      PTHREAD_CANCEL_ASYNCHRONOUS).  */
   unsigned char canceltype;
 
+  /* Futex to prevent thread exit during __pthread_call_with_tid.  The
+     least-significant bit is an exit flag.  The remaining bits count
+     the number of __pthread_call_with_tid calls in progress.
+
+     An exiting thread sets the LSB, and waits until exit_futex is 1.
+
+     If the thread is not yet exiting (as indicated by the LSB),
+     __pthread_call_with_tid atomically adds 2 to the futex variable
+     before the callback, and subtracts 2 after it.  */
+  unsigned int exit_futex;
+
   /* Used on strsignal.  */
   struct tls_internal_t tls_state;
 
diff --git a/nptl/pthread_call_with_tid.c b/nptl/pthread_call_with_tid.c
new file mode 100644
index 0000000000..ea3e801e22
--- /dev/null
+++ b/nptl/pthread_call_with_tid.c
@@ -0,0 +1,66 @@
+/* Invoke a callback with the TID of a specific 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/>.  */
+
+#include <futex-internal.h>
+#include <internal-signals.h>
+#include <pthreadP.h>
+#include <sysdep.h>
+#include <unistd.h>
+
+int
+__pthread_call_with_tid (pthread_t thr,
+                         int (*callback) (struct pthread *thr, pid_t tid,
+                                          void *closure),
+                         void *closure)
+{
+  struct pthread *pd = (struct pthread *) thr;
+
+  if (pd == THREAD_SELF)
+    {
+      /* Use the actual TID from the kernel, so that it refers to the
+         current thread even if called after vfork.  No signal
+         blocking in this case, so that the signal is delivered
+         immediately.  */
+      pid_t tid = INLINE_SYSCALL_CALL (gettid);
+      return callback (pd, tid, closure);
+    }
+
+  /* Block all signals.  The counter increment/decrement is not
+     reentrant.  */
+  sigset_t old_mask;
+  __libc_signal_block_all (&old_mask);
+
+  pid_t tid;
+  if (atomic_fetch_add_acq_rel (&pd->exit_futex, 2) & 1)
+    /* The target thread has exited or is about to.  There is no
+       stable TID value anymore.  */
+    tid = 0;
+  else
+    tid = pd->tid;
+
+  int ret = callback (pd, tid, closure);
+
+  /* A value 3 before the update indicates that the thread is exiting,
+     and this is the last remaining concurrent operation.  */
+  if (atomic_fetch_add_acq_rel (&pd->exit_futex, -2) == 3)
+    futex_wake (&pd->exit_futex, 1, FUTEX_PRIVATE);
+
+  __libc_signal_restore_set (&old_mask);
+
+  return ret;
+}
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index d8ec299cb1..e012d89ecb 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -37,6 +37,7 @@
 #include <sys/single_threaded.h>
 #include <version.h>
 #include <clone_internal.h>
+#include <futex-internal.h>
 
 #include <shlib-compat.h>
 
@@ -485,6 +486,23 @@ start_thread (void *arg)
     /* This was the last thread.  */
     exit (0);
 
+  /* This prevents sending a signal from this thread to itself during
+     its final stages.  This must come after the exit call above
+     because atexit handlers must not run with signals blocked.  */
+  __libc_signal_block_all (NULL);
+
+  /* Delay exiting this thread until there are no concurrent
+     __pthread_call_with_tid operations in progress.  */
+  {
+    /* Indicate that no further signals should be sent.  */
+    atomic_fetch_or_release (&pd->exit_futex, 1);
+
+    /* Wait until no __pthread_call_with_tid operations are left.  */
+    unsigned int val;
+    while ((val = atomic_load_acquire (&pd->exit_futex)) != 1)
+      futex_wait (&pd->exit_futex, val, FUTEX_PRIVATE);
+  }
+
 #ifndef __ASSUME_SET_ROBUST_LIST
   /* If this thread has any robust mutexes locked, handle them now.  */
 # if __PTHREAD_MUTEX_HAVE_PREV
diff --git a/nptl/tst-pthread_call_with_tid.c b/nptl/tst-pthread_call_with_tid.c
new file mode 100644
index 0000000000..07f8e7f0fa
--- /dev/null
+++ b/nptl/tst-pthread_call_with_tid.c
@@ -0,0 +1,165 @@
+/* Whitebox test for __pthread_call_with_tid.
+   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/>.  */
+
+#include <pthreadP.h>
+#include <stddef.h>
+#include <stdint.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/xsignal.h>
+#include <support/xthread.h>
+#include <support/xunistd.h>
+#include <unistd.h>
+
+/* Check if SIGUSR1 is BLOCKED.  */
+static void
+check_sigusr1 (bool blocked)
+{
+  sigset_t signals;
+  xpthread_sigmask (SIG_BLOCK, NULL, &signals);
+  TEST_COMPARE (!sigismember (&signals, SIGUSR1), !blocked);
+}
+
+/* Direct call to this function.  */
+static int
+callback_self_direct (struct pthread *pd, pid_t tid, void *closure)
+{
+  TEST_VERIFY (pd == THREAD_SELF);
+  TEST_COMPARE (getpid (), gettid ()); /* No other threads.  */
+  TEST_COMPARE (tid, gettid());
+  TEST_COMPARE (tid, pd->tid);
+
+  check_sigusr1 (false);
+
+  return *(int *) closure;
+}
+
+/* Callback for call with pthread_self after vfork.  */
+static int
+callback_self_vfork (struct pthread *pd, pid_t tid, void *closure)
+{
+  TEST_VERIFY (pd == THREAD_SELF);
+
+  /* The stored TID refers to the parent process.  */
+  TEST_COMPARE (getppid (), pd->tid);
+
+  TEST_COMPARE (getpid (), gettid ()); /* No other threads.  */
+  TEST_COMPARE (tid, gettid());
+
+  check_sigusr1 (false);
+  return *(int *) closure;
+}
+
+/* Callback for call on other thread that is running.  Returns TID of
+   the other thread.  */
+static int
+callback_other_running (struct pthread *pd, pid_t tid, void *closure)
+{
+  TEST_COMPARE (tid, pd->tid);
+  TEST_VERIFY (closure == NULL);
+  check_sigusr1 (true);
+  return tid;
+}
+
+/* Thread function that goes with callback_other_running.  Returns
+   TID.  */
+static void *
+threadfunc_other_running (void *closure)
+{
+  pthread_barrier_t *barrier = closure;
+
+  /* Do not exit until main thread says so. */
+  xpthread_barrier_wait (barrier);
+
+  return (void *) (intptr_t) gettid ();
+}
+
+/* Callback for call on other thread that has exited.  */
+static int
+callback_other_exited (struct pthread *pd, pid_t tid, void *closure)
+{
+  TEST_COMPARE (tid, 0);        /* No TID available.  */
+  check_sigusr1 (true);
+  return *(int *) closure;
+}
+
+/* Thread function that goes with callback_other_exited.  */
+static void *
+threadfunc_other_exited (void *closure)
+{
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  check_sigusr1 (false);
+
+  /* Same thread, direct call.  */
+  int ret = 5;
+  TEST_COMPARE (5, __pthread_call_with_tid (pthread_self (),
+                                            callback_self_direct, &ret));
+  check_sigusr1 (false);
+
+  /* Same thread, after vfork.  */
+  {
+    pid_t pid = vfork ();
+    if (pid == 0)
+      {
+        ret = 6;
+        TEST_COMPARE (6, __pthread_call_with_tid (pthread_self (),
+                                                  callback_self_vfork, &ret));
+        check_sigusr1 (false);
+        _exit (0);
+      }
+    if (pid < 0)
+      FAIL_EXIT1 ("vfork: %m");
+    int status;
+    xwaitpid (pid, &status, 0);
+    TEST_COMPARE (status, 0);
+  }
+
+  /* Other thread, still running.  */
+  {
+    pthread_barrier_t barrier;
+    xpthread_barrier_init (&barrier, NULL, 2);
+    pthread_t thr = xpthread_create (NULL, threadfunc_other_running, &barrier);
+    pid_t tid1 = __pthread_call_with_tid (thr,
+                                          callback_other_running, NULL);
+    check_sigusr1 (false);
+    xpthread_barrier_wait (&barrier);
+    pid_t tid2 = (intptr_t) xpthread_join (thr);
+    xpthread_barrier_destroy (&barrier);
+    TEST_COMPARE (tid1, tid2);
+  }
+
+  /* Other thread, exited.  */
+  {
+    pthread_t thr = xpthread_create (NULL, threadfunc_other_exited, NULL);
+    support_wait_for_thread_exit ();
+    ret = 7;
+    TEST_COMPARE (7, __pthread_call_with_tid (thr,
+                                              callback_other_exited, &ret));
+    check_sigusr1 (false);
+    xpthread_join (thr);
+  }
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/sysdeps/nptl/pthreadP.h b/sysdeps/nptl/pthreadP.h
index 374657a2fd..fe28389163 100644
--- a/sysdeps/nptl/pthreadP.h
+++ b/sysdeps/nptl/pthreadP.h
@@ -673,6 +673,26 @@ extern void __wait_lookup_done (void) attribute_hidden;
 int __pthread_attr_extension (struct pthread_attr *attr) attribute_hidden
   __attribute_warn_unused_result__;
 
+/* __pthread_call_with_tid CALLBACK (THR, TID, CLOSURE), where TID is
+   either the current TID of THR, or zero if THR has exited or is
+   about to exit.  If TID is not zero, the thread denoted by it is
+   guaranteed not to exit before CALLBACK returns.
+
+   __pthread_call_with_tid returns the result of CALLBACK.
+
+   If THR refers to the current thread, the actual TID is used (to
+   support vfork).  CALLBACK may unwind in this case (e.g., siglongjmp
+   from user-supplied signal handler).
+
+   If THR is not the current thread, then all signals are blocked
+   until CALLBACK returns.  CALLBACK is assumed to return normally (no
+   unwinding).  This makes the __pthread_call_with_tid
+   async-signal-safe as long as CALLBACK is async-signal-safe. */
+int __pthread_call_with_tid (pthread_t thr,
+			     int (*callback) (struct pthread *thr, pid_t tid,
+					      void *closure),
+			     void *closure) attribute_hidden;
+
 #ifdef SHARED
 # define PTHREAD_STATIC_FN_REQUIRE(name)
 #else
-- 
2.31.1



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 4/4] nptl: Fix race between pthread_kill and thread exit (bug 12889)
  2021-08-20 16:11 [PATCH v2 0/4] Fix ESRCH issues in pthread_cancel, pthread_kill Florian Weimer
                   ` (2 preceding siblings ...)
  2021-08-20 16:13 ` [PATCH v2 3/4] nptl: Add internal __pthread_call_with_tid function Florian Weimer
@ 2021-08-20 16:14 ` Florian Weimer
  2021-08-20 18:09 ` [PATCH v2 0/4] Fix ESRCH issues in pthread_cancel, pthread_kill Adhemerval Zanella
  4 siblings, 0 replies; 12+ messages in thread
From: Florian Weimer @ 2021-08-20 16:14 UTC (permalink / raw)
  To: libc-alpha

Thread exit is delayed using __pthread_call_with_tid until all pending
pthread_kill operations on the thread have completed.  This avoids
sending signals to the wrong thread or a spurious ESRCH error.

The test sysdeps/pthread/tst-pthread_cancel-select-loop.c is derived
from a downstream test originally written by Marek Polacek.
---
v2: Use __pthread_call_with_tid.

 nptl/pthread_kill.c                           |  49 ++++----
 sysdeps/pthread/Makefile                      |   2 +
 sysdeps/pthread/tst-kill4.c                   |  90 --------------
 .../pthread/tst-pthread_cancel-select-loop.c  |  87 ++++++++++++++
 sysdeps/pthread/tst-pthread_kill-exiting.c    | 110 ++++++++++++++++++
 5 files changed, 219 insertions(+), 119 deletions(-)
 delete mode 100644 sysdeps/pthread/tst-kill4.c
 create mode 100644 sysdeps/pthread/tst-pthread_cancel-select-loop.c
 create mode 100644 sysdeps/pthread/tst-pthread_kill-exiting.c

diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c
index 5d4c86f920..4bec49a312 100644
--- a/nptl/pthread_kill.c
+++ b/nptl/pthread_kill.c
@@ -20,40 +20,31 @@
 #include <pthreadP.h>
 #include <shlib-compat.h>
 
-int
-__pthread_kill_internal (pthread_t threadid, int signo)
+static int
+tgkill_callback (struct pthread *unused, pid_t tid, void *closure)
 {
-  pid_t tid;
-  struct pthread *pd = (struct pthread *) threadid;
-
-  if (pd == THREAD_SELF)
-    /* It is a special case to handle raise() implementation after a vfork
-       call (which does not update the PD tid field).  */
-    tid = INLINE_SYSCALL_CALL (gettid);
+  int signo = (intptr_t) closure;
+  if (tid == 0)
+    /* Thread is exiting or has exited.  The signal is not observable
+       either way, so do not send it.  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.  */
+    return 0;
   else
-    /* Force load of pd->tid into local variable or register.  Otherwise
-       if a thread exits between ESRCH test and tgkill, we might return
-       EINVAL, because pd->tid would be cleared by the kernel.  */
-    tid = atomic_forced_read (pd->tid);
-
-  int val;
-  if (__glibc_likely (tid > 0))
     {
-      pid_t pid = __getpid ();
-
-      val = INTERNAL_SYSCALL_CALL (tgkill, pid, tid, signo);
-      val = (INTERNAL_SYSCALL_ERROR_P (val)
-	    ? INTERNAL_SYSCALL_ERRNO (val) : 0);
+      /* Using tgkill is a safety measure.  __pthread_call_with_tid
+	 ensures that the TID is still live at the point of the call.  */
+      int ret = INTERNAL_SYSCALL_CALL (tgkill, __getpid (), tid, signo);
+      return INTERNAL_SYSCALL_ERROR_P (ret) ? INTERNAL_SYSCALL_ERRNO (ret) : 0;
     }
-  else
-    /* 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;
+int
+__pthread_kill_internal (pthread_t threadid, int signo)
+{
+  return __pthread_call_with_tid (threadid, tgkill_callback,
+				  (void *) (intptr_t) signo);
 }
 
 int
diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
index dedfa0d290..48dba717a1 100644
--- a/sysdeps/pthread/Makefile
+++ b/sysdeps/pthread/Makefile
@@ -119,7 +119,9 @@ tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
 	 tst-unwind-thread \
 	 tst-pt-vfork1 tst-pt-vfork2 tst-vfork1x tst-vfork2x \
 	 tst-pthread_cancel-exited \
+	 tst-pthread_cancel-select-loop \
 	 tst-pthread_kill-exited \
+	 tst-pthread_kill-exiting \
 	 # tests
 
 tests-time64 := \
diff --git a/sysdeps/pthread/tst-kill4.c b/sysdeps/pthread/tst-kill4.c
deleted file mode 100644
index 9563939792..0000000000
--- a/sysdeps/pthread/tst-kill4.c
+++ /dev/null
@@ -1,90 +0,0 @@
-/* Copyright (C) 2003-2021 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by Ulrich Drepper <drepper@redhat.com>, 2003.
-
-   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-select-loop.c b/sysdeps/pthread/tst-pthread_cancel-select-loop.c
new file mode 100644
index 0000000000..a62087589c
--- /dev/null
+++ b/sysdeps/pthread/tst-pthread_cancel-select-loop.c
@@ -0,0 +1,87 @@
+/* Test that pthread_cancel succeeds during thread exit.
+   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 tries to trigger an internal race condition in
+   pthread_cancel, where the cancellation signal is sent after the
+   thread has begun the cancellation process.  This can result in a
+   spurious ESRCH error.  For the original bug 12889, the window is
+   quite small, so the bug was not reproduced in every run.  */
+
+#include <stdbool.h>
+#include <stddef.h>
+#include <support/check.h>
+#include <support/xthread.h>
+#include <support/xunistd.h>
+#include <sys/select.h>
+#include <unistd.h>
+
+/* Set to true by timeout_thread_function when the test should
+   terminate.  */
+static bool timeout;
+
+static void *
+timeout_thread_function (void *unused)
+{
+  usleep (5 * 1000 * 1000);
+  __atomic_store_n (&timeout, true, __ATOMIC_RELAXED);
+  return NULL;
+}
+
+/* Used for blocking the select function below.  */
+static int pipe_fds[2];
+
+static void *
+canceled_thread_function (void *unused)
+{
+  while (true)
+    {
+      fd_set rfs;
+      fd_set wfs;
+      fd_set efs;
+      FD_ZERO (&rfs);
+      FD_ZERO (&wfs);
+      FD_ZERO (&efs);
+      FD_SET (pipe_fds[0], &rfs);
+
+      /* If the cancellation request is recognized early, the thread
+         begins exiting while the cancellation signal arrives.  */
+      select (FD_SETSIZE, &rfs, &wfs, &efs, NULL);
+    }
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  xpipe (pipe_fds);
+  pthread_t thr_timeout = xpthread_create (NULL, timeout_thread_function, NULL);
+
+  while (!__atomic_load_n (&timeout, __ATOMIC_RELAXED))
+    {
+      pthread_t thr = xpthread_create (NULL, canceled_thread_function, NULL);
+      xpthread_cancel (thr);
+      TEST_VERIFY (xpthread_join (thr) == PTHREAD_CANCELED);
+    }
+
+  xpthread_join (thr_timeout);
+  xclose (pipe_fds[0]);
+  xclose (pipe_fds[1]);
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/sysdeps/pthread/tst-pthread_kill-exiting.c b/sysdeps/pthread/tst-pthread_kill-exiting.c
new file mode 100644
index 0000000000..15550d2310
--- /dev/null
+++ b/sysdeps/pthread/tst-pthread_kill-exiting.c
@@ -0,0 +1,110 @@
+/* Test that pthread_kill succeeds during thread exit.
+   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 for a thread that is exiting
+   succeeds (with or without actually delivering the signal).  */
+
+#include <array_length.h>
+#include <stdbool.h>
+#include <stddef.h>
+#include <support/xsignal.h>
+#include <support/xthread.h>
+#include <unistd.h>
+
+/* Set to true by timeout_thread_function when the test should
+   terminate.  */
+static bool timeout;
+
+static void *
+timeout_thread_function (void *unused)
+{
+  usleep (1000 * 1000);
+  __atomic_store_n (&timeout, true, __ATOMIC_RELAXED);
+  return NULL;
+}
+
+/* Used to synchronize the sending threads with the target thread and
+   main thread.  */
+static pthread_barrier_t barrier_1;
+static pthread_barrier_t barrier_2;
+
+/* The target thread to which signals are to be sent.  */
+static pthread_t target_thread;
+
+static void *
+sender_thread_function (void *unused)
+{
+  while (!__atomic_load_n (&timeout, __ATOMIC_RELAXED))
+    {
+      /* Wait until target_thread has been initialized.  The target
+         thread and main thread participate in this barrier.  */
+      xpthread_barrier_wait (&barrier_1);
+
+      xpthread_kill (target_thread, SIGUSR1);
+
+      /* Communicate that the signal has been sent.  The main thread
+         participates in this barrier.  */
+      xpthread_barrier_wait (&barrier_2);
+    }
+  return NULL;
+}
+
+static void *
+target_thread_function (void *unused)
+{
+  target_thread = pthread_self ();
+  xpthread_barrier_wait (&barrier_1);
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  xsignal (SIGUSR1, SIG_IGN);
+
+  pthread_t thr_timeout = xpthread_create (NULL, timeout_thread_function, NULL);
+
+  pthread_t threads[4];
+  xpthread_barrier_init (&barrier_1, NULL, array_length (threads) + 2);
+  xpthread_barrier_init (&barrier_2, NULL, array_length (threads) + 1);
+
+  for (int i = 0; i < array_length (threads); ++i)
+    threads[i] = xpthread_create (NULL, sender_thread_function, NULL);
+
+  while (!__atomic_load_n (&timeout, __ATOMIC_RELAXED))
+    {
+      xpthread_create (NULL, target_thread_function, NULL);
+
+      /* Wait for the target thread to be set up and signal sending to
+         start.  */
+      xpthread_barrier_wait (&barrier_1);
+
+      /* Wait for signal sending to complete.  */
+      xpthread_barrier_wait (&barrier_2);
+
+      xpthread_join (target_thread);
+    }
+
+  for (int i = 0; i < array_length (threads); ++i)
+    xpthread_join (threads[i]);
+  xpthread_join (thr_timeout);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.31.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/4] Fix ESRCH issues in pthread_cancel, pthread_kill
  2021-08-20 16:11 [PATCH v2 0/4] Fix ESRCH issues in pthread_cancel, pthread_kill Florian Weimer
                   ` (3 preceding siblings ...)
  2021-08-20 16:14 ` [PATCH v2 4/4] nptl: Fix race between pthread_kill and thread exit (bug 12889) Florian Weimer
@ 2021-08-20 18:09 ` Adhemerval Zanella
  2021-08-20 18:48   ` Florian Weimer
  4 siblings, 1 reply; 12+ messages in thread
From: Adhemerval Zanella @ 2021-08-20 18:09 UTC (permalink / raw)
  To: libc-alpha, Florian Weimer



On 20/08/2021 13:11, Florian Weimer via Libc-alpha wrote:
> The new version fixes the support/Makefile update and puts the generic
> “give me a non-reusable TID” functionality into a generic helper
> function.
> 
> Tested on i686-linux-gnu and x86_64-linux-gnu.  Built with
> build-many-glibcs.py.

Sorry, but this approach is even more complex and convoluted than your
previous one.  I have fixed some issues and adapted your patches on top
on my pthread fixes [1].

By using another field instead of pthread::tid to synchronize thread
termination [2] it allows the synchronization required to be *way* more
simple [3]: it basically requires to block all signals (since the lock
is not recursive), lll_lock, lll_unlock (since there is no more kernel
involved on thread termination).

The lll_lock already has the logic to handle multiple waiters, so
there is no need to replicate the idea of more futex/atomic calls.

On the patches I used your tests as-is.

As a side note, now that we are synchronize the tid access, we will
need to handleother pthread functions that also access it as well:

  pthread_getaffinity_np,
  pthread_getcpuclockid
  pthread_getname_np
  pthread_getschedparam
  pthread_setaffinity_np
  pthread_setname_np
  pthread_setschedparam
  pthread_setschedprio
  pthread_sigqueue.

[1] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/azanella/pthread-multiple-fixes
[2] https://sourceware.org/git/?p=glibc.git;a=commit;h=ebae3185ca5529ad2147e6f75978a13e2d459b8e
[3] https://sourceware.org/git/?p=glibc.git;a=commit;h=bb94df8eaba1af636bd6af7ec9142bc3e0e25868


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/4] Fix ESRCH issues in pthread_cancel, pthread_kill
  2021-08-20 18:09 ` [PATCH v2 0/4] Fix ESRCH issues in pthread_cancel, pthread_kill Adhemerval Zanella
@ 2021-08-20 18:48   ` Florian Weimer
  2021-08-23 12:19     ` Adhemerval Zanella
  0 siblings, 1 reply; 12+ messages in thread
From: Florian Weimer @ 2021-08-20 18:48 UTC (permalink / raw)
  To: Adhemerval Zanella; +Cc: libc-alpha

* Adhemerval Zanella:

> On 20/08/2021 13:11, Florian Weimer via Libc-alpha wrote:
>> The new version fixes the support/Makefile update and puts the generic
>> “give me a non-reusable TID” functionality into a generic helper
>> function.
>> 
>> Tested on i686-linux-gnu and x86_64-linux-gnu.  Built with
>> build-many-glibcs.py.
>
> Sorry, but this approach is even more complex and convoluted than your
> previous one.  I have fixed some issues and adapted your patches on top
> on my pthread fixes [1].

> [1] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/azanella/pthread-multiple-fixes

Hmm.  Are you going to repost this series?

Thanks,
Florian


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/4] Fix ESRCH issues in pthread_cancel, pthread_kill
  2021-08-20 18:48   ` Florian Weimer
@ 2021-08-23 12:19     ` Adhemerval Zanella
  2021-08-23 12:21       ` Florian Weimer
  0 siblings, 1 reply; 12+ messages in thread
From: Adhemerval Zanella @ 2021-08-23 12:19 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha



On 20/08/2021 15:48, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 20/08/2021 13:11, Florian Weimer via Libc-alpha wrote:
>>> The new version fixes the support/Makefile update and puts the generic
>>> “give me a non-reusable TID” functionality into a generic helper
>>> function.
>>>
>>> Tested on i686-linux-gnu and x86_64-linux-gnu.  Built with
>>> build-many-glibcs.py.
>>
>> Sorry, but this approach is even more complex and convoluted than your
>> previous one.  I have fixed some issues and adapted your patches on top
>> on my pthread fixes [1].
> 
>> [1] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/azanella/pthread-multiple-fixes
> 
> Hmm.  Are you going to repost this series?

Yes, I will add a patch to a fix for the tid members on the 
functions that uses it and repost it.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/4] Fix ESRCH issues in pthread_cancel, pthread_kill
  2021-08-23 12:19     ` Adhemerval Zanella
@ 2021-08-23 12:21       ` Florian Weimer
  0 siblings, 0 replies; 12+ messages in thread
From: Florian Weimer @ 2021-08-23 12:21 UTC (permalink / raw)
  To: Adhemerval Zanella; +Cc: libc-alpha

* Adhemerval Zanella:

> On 20/08/2021 15:48, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> On 20/08/2021 13:11, Florian Weimer via Libc-alpha wrote:
>>>> The new version fixes the support/Makefile update and puts the generic
>>>> “give me a non-reusable TID” functionality into a generic helper
>>>> function.
>>>>
>>>> Tested on i686-linux-gnu and x86_64-linux-gnu.  Built with
>>>> build-many-glibcs.py.
>>>
>>> Sorry, but this approach is even more complex and convoluted than your
>>> previous one.  I have fixed some issues and adapted your patches on top
>>> on my pthread fixes [1].
>> 
>>> [1] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/azanella/pthread-multiple-fixes
>> 
>> Hmm.  Are you going to repost this series?
>
> Yes, I will add a patch to a fix for the tid members on the 
> functions that uses it and repost it.

Looking forward to it.  I will try to review it.

Florian


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/4] support: Add support_wait_for_thread_exit
  2021-08-20 16:12 ` [PATCH v2 1/4] support: Add support_wait_for_thread_exit Florian Weimer
@ 2021-09-29 20:27   ` Florian Weimer
  2021-09-30 11:48     ` Adhemerval Zanella
  0 siblings, 1 reply; 12+ messages in thread
From: Florian Weimer @ 2021-09-29 20:27 UTC (permalink / raw)
  To: Florian Weimer via Libc-alpha

* Florian Weimer via Libc-alpha:

> +      int task_tid = atoi (e->d_name);
> +      if (task_tid <= 0)
> +        FAIL_EXIT1 ("Invalid /proc/self/task entry: %s", e->d_name);

There's a kernel bug that causes /proc/self/task/0 to be listed
due to concurrent task exit in the kernel.

=====FAIL: nptl/tst-pthread_cancel-exited.out=====
error: support_wait_for_thread_exit.c:51: Invalid /proc/self/task entry: 0
error: 1 test failures
=====FAIL: nptl/tst-pthread_cancel-exited.test-result=====

Should work around this in our test code?

Thanks,
Florian


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/4] support: Add support_wait_for_thread_exit
  2021-09-29 20:27   ` Florian Weimer
@ 2021-09-30 11:48     ` Adhemerval Zanella
  2021-09-30 12:07       ` Florian Weimer
  0 siblings, 1 reply; 12+ messages in thread
From: Adhemerval Zanella @ 2021-09-30 11:48 UTC (permalink / raw)
  To: libc-alpha, Florian Weimer



On 29/09/2021 17:27, Florian Weimer via Libc-alpha wrote:
> * Florian Weimer via Libc-alpha:
> 
>> +      int task_tid = atoi (e->d_name);
>> +      if (task_tid <= 0)
>> +        FAIL_EXIT1 ("Invalid /proc/self/task entry: %s", e->d_name);
> 
> There's a kernel bug that causes /proc/self/task/0 to be listed
> due to concurrent task exit in the kernel.
> 
> =====FAIL: nptl/tst-pthread_cancel-exited.out=====
> error: support_wait_for_thread_exit.c:51: Invalid /proc/self/task entry: 0
> error: 1 test failures
> =====FAIL: nptl/tst-pthread_cancel-exited.test-result=====
> 
> Should work around this in our test code?

I have see this failures in some tests run and I was about to bring
this to you.  Can we safely mitigate it by ignoring the tid '0'?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/4] support: Add support_wait_for_thread_exit
  2021-09-30 11:48     ` Adhemerval Zanella
@ 2021-09-30 12:07       ` Florian Weimer
  0 siblings, 0 replies; 12+ messages in thread
From: Florian Weimer @ 2021-09-30 12:07 UTC (permalink / raw)
  To: Adhemerval Zanella; +Cc: libc-alpha

* Adhemerval Zanella:

> On 29/09/2021 17:27, Florian Weimer via Libc-alpha wrote:
>> * Florian Weimer via Libc-alpha:
>> 
>>> +      int task_tid = atoi (e->d_name);
>>> +      if (task_tid <= 0)
>>> +        FAIL_EXIT1 ("Invalid /proc/self/task entry: %s", e->d_name);
>> 
>> There's a kernel bug that causes /proc/self/task/0 to be listed
>> due to concurrent task exit in the kernel.
>> 
>> =====FAIL: nptl/tst-pthread_cancel-exited.out=====
>> error: support_wait_for_thread_exit.c:51: Invalid /proc/self/task entry: 0
>> error: 1 test failures
>> =====FAIL: nptl/tst-pthread_cancel-exited.test-result=====
>> 
>> Should work around this in our test code?
>
> I have see this failures in some tests run and I was about to bring
> this to you.  Can we safely mitigate it by ignoring the tid '0'?

Right, I have sent a patch:

  [PATCH] support: Add check for TID zero in support_wait_for_thread_exit
  <https://sourceware.org/pipermail/libc-alpha/2021-September/131560.html>

Thanks,
Florian


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-09-30 12:08 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-20 16:11 [PATCH v2 0/4] Fix ESRCH issues in pthread_cancel, pthread_kill Florian Weimer
2021-08-20 16:12 ` [PATCH v2 1/4] support: Add support_wait_for_thread_exit Florian Weimer
2021-09-29 20:27   ` Florian Weimer
2021-09-30 11:48     ` Adhemerval Zanella
2021-09-30 12:07       ` Florian Weimer
2021-08-20 16:13 ` [PATCH v2 2/4] nptl: pthread_kill, pthread_cancel should fail after exit (bug 19193) Florian Weimer
2021-08-20 16:13 ` [PATCH v2 3/4] nptl: Add internal __pthread_call_with_tid function Florian Weimer
2021-08-20 16:14 ` [PATCH v2 4/4] nptl: Fix race between pthread_kill and thread exit (bug 12889) Florian Weimer
2021-08-20 18:09 ` [PATCH v2 0/4] Fix ESRCH issues in pthread_cancel, pthread_kill Adhemerval Zanella
2021-08-20 18:48   ` Florian Weimer
2021-08-23 12:19     ` Adhemerval Zanella
2021-08-23 12:21       ` Florian Weimer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).