improve pthread_exit synchronization with functions targeting tid

if the last thread exited via pthread_exit, the logic that marked it
dead did not account for the possibility of it targeting itself via
atexit handlers. for example, an atexit handler calling
pthread_kill(pthread_self(), SIGKILL) would return success
(previously, ESRCH) rather than causing termination via the signal.

move the release of killlock after the determination is made whether
the exiting thread is the last thread. in the case where it's not,
move the release all the way to the end of the function. this way we
can clear the tid rather than spending storage on a dedicated
dead-flag. clearing the tid is also preferable in that it hardens
against inadvertent use of the value after the thread has terminated
but before it is joined.

diff --git a/src/internal/pthread_impl.h b/src/internal/pthread_impl.h
index f2805b893724e23f25437cdc644e7167f1048982..3b4ad94d61f60191b7b420565ecd7e9fef7c6c0b 100644 (file)
--- a/src/internal/pthread_impl.h
+++ b/src/internal/pthread_impl.h
@@ -34,7 +34,6 @@ struct pthread {
        void *result;
        struct __ptcb *cancelbuf;
        void **tsd;
-       volatile int dead;
        struct {
                volatile void *volatile head;
                long off;

diff --git a/src/thread/pthread_create.c b/src/thread/pthread_create.c
index ac06d7a709d89973855fecf1c52ec3abdc2b0476..a86b5e1ba3f02ab104ea6348d986ad5b140c4ff0 100644 (file)
--- a/src/thread/pthread_create.c
+++ b/src/thread/pthread_create.c
@@ -39,9 +39,11 @@ _Noreturn void __pthread_exit(void *result)
 
        LOCK(self->exitlock);
 
-       /* Mark this thread dead before decrementing count */
+       /* Access to target the exiting thread with syscalls that use
+        * its kernel tid is controlled by killlock. For detached threads,
+        * any use past this point would have undefined behavior, but for
+        * joinable threads it's a valid usage that must be handled. */
        LOCK(self->killlock);
-       self->dead = 1;
 
        /* Block all signals before decrementing the live thread count.
         * This is important to ensure that dynamically allocated TLS
@@ -49,20 +51,14 @@ _Noreturn void __pthread_exit(void *result)
         * reasons as well. */
        __block_all_sigs(&set);
 
-       /* Wait to unlock the kill lock, which governs functions like
-        * pthread_kill which target a thread id, until signals have
-        * been blocked. This precludes observation of the thread id
-        * as a live thread (with application code running in it) after
-        * the thread was reported dead by ESRCH being returned. */
-       UNLOCK(self->killlock);
-
        /* It's impossible to determine whether this is "the last thread"
         * until performing the atomic decrement, since multiple threads
         * could exit at the same time. For the last thread, revert the
-        * decrement and unblock signals to give the atexit handlers and
-        * stdio cleanup code a consistent state. */
+        * decrement, restore the tid, and unblock signals to give the
+        * atexit handlers and stdio cleanup code a consistent state. */
        if (a_fetch_add(&libc.threads_minus_1, -1)==0) {
                libc.threads_minus_1 = 0;
+               UNLOCK(self->killlock);
                __restore_sigs(&set);
                exit(0);
        }
@@ -113,6 +109,12 @@ _Noreturn void __pthread_exit(void *result)
                __unmapself(self->map_base, self->map_size);
        }
 
+       /* After the kernel thread exits, its tid may be reused. Clear it
+        * to prevent inadvertent use and inform functions that would use
+        * it that it's no longer available. */
+       self->tid = 0;
+       UNLOCK(self->killlock);
+
        for (;;) __syscall(SYS_exit, 0);
 }
 

diff --git a/src/thread/pthread_getschedparam.c b/src/thread/pthread_getschedparam.c
index a994b637306a40c1155c1e400bfd2816b888676e..05be42427c4503b5b6d7f1ab7a339a0d186aa641 100644 (file)
--- a/src/thread/pthread_getschedparam.c
+++ b/src/thread/pthread_getschedparam.c
@@ -4,7 +4,7 @@ int pthread_getschedparam(pthread_t t, int *restrict policy, struct sched_param
 {
        int r;
        LOCK(t->killlock);
-       if (t->dead) {
+       if (!t->tid) {
                r = ESRCH;
        } else {
                r = -__syscall(SYS_sched_getparam, t->tid, param);

diff --git a/src/thread/pthread_kill.c b/src/thread/pthread_kill.c
index 0a139231caae5afad1ae11b54f659fd64fecb07f..6d70e626daf40bdf8e60240fc1fbb1b561d377dd 100644 (file)
--- a/src/thread/pthread_kill.c
+++ b/src/thread/pthread_kill.c
@@ -4,8 +4,8 @@ int pthread_kill(pthread_t t, int sig)
 {
        int r;
        LOCK(t->killlock);
-       r = t->dead ? (sig+0U >= _NSIG ? EINVAL : 0)
-               : -__syscall(SYS_tkill, t->tid, sig);
+       r = t->tid ? -__syscall(SYS_tkill, t->tid, sig)
+               : (sig+0U >= _NSIG ? EINVAL : 0);
        UNLOCK(t->killlock);
        return r;
 }

diff --git a/src/thread/pthread_setschedparam.c b/src/thread/pthread_setschedparam.c
index 9e2fa45674ff6f80abd4979dae17f23d51b08dcd..ab45f2ff8ba2e290241db7d0924e1e839bce8301 100644 (file)
--- a/src/thread/pthread_setschedparam.c
+++ b/src/thread/pthread_setschedparam.c
@@ -4,7 +4,7 @@ int pthread_setschedparam(pthread_t t, int policy, const struct sched_param *par
 {
        int r;
        LOCK(t->killlock);
-       r = t->dead ? ESRCH : -__syscall(SYS_sched_setscheduler, t->tid, policy, param);
+       r = !t->tid ? ESRCH : -__syscall(SYS_sched_setscheduler, t->tid, policy, param);
        UNLOCK(t->killlock);
        return r;
 }

diff --git a/src/thread/pthread_setschedprio.c b/src/thread/pthread_setschedprio.c
index dc745b42a4616adfb23a7c6e8916d740b8e500ba..c353f6b53942dc9d21066aba5948d83bb3d89b58 100644 (file)
--- a/src/thread/pthread_setschedprio.c
+++ b/src/thread/pthread_setschedprio.c
@@ -4,7 +4,7 @@ int pthread_setschedprio(pthread_t t, int prio)
 {
        int r;
        LOCK(t->killlock);
-       r = t->dead ? ESRCH : -__syscall(SYS_sched_setparam, t->tid, &prio);
+       r = !t->tid ? ESRCH : -__syscall(SYS_sched_setparam, t->tid, &prio);
        UNLOCK(t->killlock);
        return r;
 }