[PATCH 05/11] nptl: Deallocate the thread stack on setup failure (BZ #19511)

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

To setup either the thread scheduling parameters or affinity,
pthread_create enforce synchronization on created thread wait until
its parent either unlock the 'struct pthread' or send a cancellation
signal if a failure occurs.

However, cancelling the thread does not deallocate the newly created
stack since cancellation expects that a pthread_join to deallocate any
allocated thread resouces (threads stack or TLS).

I used a different strategy than the one proposed on BZ#19511
comment #4.  Making the parent responsible for the cleanup requires
additional synchronization similar to what pthread_join does.  Instead,
this patch reassigns an unused 'struct pthread' member,
parent_cancelhandling_unsed, to indicate whether the setup has failed
and set the thread itself to deallocate the allocated resouces (similar
to what detached mode does).

This strategy also simplifies by not using thread cancellation and
thus not running libgcc_so load in the signal handler (which is
avoided in thread cancellation since 'pthread_cancel' is the one
responsible to dlopen libgcc_s).

Checked on x86_64-linux-gnu and aarch64-linux-gnu.
---
 nptl/allocatestack.c  |  1 +
 nptl/descr.h          |  5 ++--
 nptl/pthread_create.c | 56 +++++++++++++++++++------------------------
 3 files changed, 29 insertions(+), 33 deletions(-)

diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index dc81a2ca73..2114bd2e27 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -161,6 +161,7 @@ get_cached_stack (size_t *sizep, void **memp)
   /* Cancellation handling is back to the default.  */
   result->cancelhandling = 0;
   result->cleanup = NULL;
+  result->setup_failed = 0;
 
   /* No pending event.  */
   result->nextevent = NULL;
diff --git a/nptl/descr.h b/nptl/descr.h
index 3de9535449..9d8297b45f 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -340,8 +340,9 @@ struct pthread
   /* True if thread must stop at startup time.  */
   bool stopped_start;
 
-  /* Formerly used for dealing with cancellation.  */
-  int parent_cancelhandling_unsed;
+  /* Indicate that a thread creation setup has failed (for instance the
+     scheduler or affinity).  */
+  int setup_failed;
 
   /* Lock to synchronize access to the descriptor.  */
   int lock;
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index b53e3f30a0..d6b907827a 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -176,8 +176,6 @@ late_init (void)
        state.  Ownership cannot be released to the process via the
        return of pthread_create since a non-zero result entails PD is
        undefined and therefore cannot be joined to free the resources.
-       We privately call pthread_join on the thread to finish handling
-       the resource shutdown (Or at least we should, see bug 19511).
 
    (e) If the thread creation failed and THREAD_RAN is false, then the
        creating thread retains ownership of PD and must cleanup state.
@@ -313,28 +311,19 @@ static int create_thread (struct pthread *pd, const struct pthread_attr *attr,
   /* Now we have the possibility to set scheduling parameters etc.  */
   if (attr != NULL)
     {
-      int res;
-
       /* Set the affinity mask if necessary.  */
       if (need_setaffinity)
 	{
 	  assert (*stopped_start);
 
-	  res = INTERNAL_SYSCALL_CALL (sched_setaffinity, pd->tid,
-				       attr->extension->cpusetsize,
-				       attr->extension->cpuset);
-
+	  int res = INTERNAL_SYSCALL_CALL (sched_setaffinity, pd->tid,
+				           attr->extension->cpusetsize,
+				           attr->extension->cpuset);
 	  if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (res)))
-	  err_out:
-	    {
-	      /* The operation failed.  We have to kill the thread.
-		 We let the normal cancellation mechanism do the work.  */
-
-	      pid_t pid = __getpid ();
-	      INTERNAL_SYSCALL_CALL (tgkill, pid, pd->tid, SIGCANCEL);
-
-	      return INTERNAL_SYSCALL_ERRNO (res);
-	    }
+            {
+              pd->setup_failed = 1;
+              return INTERNAL_SYSCALL_ERRNO (res);
+            }
 	}
 
       /* Set the scheduling parameters.  */
@@ -342,11 +331,13 @@ static int create_thread (struct pthread *pd, const struct pthread_attr *attr,
 	{
 	  assert (*stopped_start);
 
-	  res = INTERNAL_SYSCALL_CALL (sched_setscheduler, pd->tid,
-				       pd->schedpolicy, &pd->schedparam);
-
+	  int res = INTERNAL_SYSCALL_CALL (sched_setscheduler, pd->tid,
+				           pd->schedpolicy, &pd->schedparam);
 	  if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (res)))
-	    goto err_out;
+            {
+              pd->setup_failed = 1;
+              return INTERNAL_SYSCALL_ERRNO (res);
+            }
 	}
     }
 
@@ -426,8 +417,6 @@ start_thread (void *arg)
 	 have ownership (see CONCURRENCY NOTES above).  */
       if (__glibc_unlikely (pd->stopped_start))
 	{
-	  int oldtype = LIBC_CANCEL_ASYNC ();
-
 	  /* Get the lock the parent locked to force synchronization.  */
 	  lll_lock (pd->lock, LLL_PRIVATE);
 
@@ -435,10 +424,11 @@ start_thread (void *arg)
 
 	  /* And give it up right away.  */
 	  lll_unlock (pd->lock, LLL_PRIVATE);
-
-	  LIBC_CANCEL_RESET (oldtype);
 	}
 
+      if (pd->setup_failed == 1)
+        __do_cancel ();
+
       LIBC_PROBE (pthread_start, 3, (pthread_t) pd, pd->start_routine, pd->arg);
 
       /* Run the code the user provided.  */
@@ -563,8 +553,8 @@ start_thread (void *arg)
       pd->setxid_futex = 0;
     }
 
-  /* If the thread is detached free the TCB.  */
-  if (IS_DETACHED (pd))
+  /* If the thread is detached or an setup error occurred, free the TCB.  */
+  if (IS_DETACHED (pd) || pd->setup_failed == 1)
     /* Free the TCB.  */
     __nptl_free_tcb (pd);
 
@@ -819,9 +809,13 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
 	   CONCURRENCY NOTES above).  We can assert that STOPPED_START
 	   must have been true because thread creation didn't fail, but
 	   thread attribute setting did.  */
-	/* See bug 19511 which explains why doing nothing here is a
-	   resource leak for a joinable thread.  */
-	assert (stopped_start);
+        {
+	  assert (stopped_start);
+          if (stopped_start)
+  	    /* State (a), we own PD. The thread blocked on this lock will
+               finish due setup_failed being set.  */
+	    lll_unlock (pd->lock, LLL_PRIVATE);
+        }
       else
 	{
 	  /* State (e) and we have ownership of PD (see CONCURRENCY
-- 
2.30.2