Re: [RFC PATCH glibc v2] pthread_setspecific: Provide signal-safety across keys

[Mathieu Desnoyers <mathieu dot desnoyers at efficios dot com>]

----- On Oct 18, 2017, at 1:33 AM, Florian Weimer fw@deneb.enyo.de wrote:

> * Mathieu Desnoyers:
> 
>> @@ -327,7 +328,7 @@ __nptl_deallocate_tsd (void)
>>  	    {
>>  	      /* The first block is allocated as part of the thread
>>  		 descriptor.  */
>> -	      free (level2);
>> +	      (void) munmap (level2, PTHREAD_KEY_2NDLEVEL_SIZE * sizeof (*level2));
>>  	      THREAD_SETMEM_NC (self, specific, cnt, NULL);
> 
> I think __nptl_deallocate_tsd needs to disable signals as well, even
> when the level2 pointer is NULL, and keep them disabled until the
> thread exits.  This is not a localized change; I don't know if it is
> safe.

I'd hate to slow down thread exit for everyone though. Disabling
and re-enabling signals is not exactly a cheap operation.

How about we add a new "thread_exiting" flag to struct pthread, and
do something like the following ?

The main change there would affect pthread_{get,set}specific called
within pthread_key destr_function handlers: pthread_getspecific would
return NULL, and pthread_setspecific would return EPERM (which would
be a non-POSIX return value).

This would also ensure that if pthread_{get,set}specific are called
within signal handlers nested over pthread teardown, those return
NULL and EPERM.

Thoughts ?

Thanks,

Mathieu

diff --git a/nptl/descr.h b/nptl/descr.h
index c5ad0c8dba..6a4ef4559a 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -335,6 +335,9 @@ struct pthread
   /* True if thread must stop at startup time.  */
   bool stopped_start;
 
+  /* True if thread is exiting.  */
+  bool thread_exiting;
+
   /* The parent's cancel handling at the time of the pthread_create
      call.  This might be needed to undo the effects of a cancellation.  */
   int parent_cancelhandling;
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 78abc07f91..fba5e2dbde 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -464,6 +464,8 @@ START_THREAD_DEFN
       THREAD_SETMEM (pd, result, pd->start_routine (pd->arg));
     }
 
+  THREAD_SETMEM (pd, thread_exiting, true);
+
   /* Call destructors for the thread_local TLS variables.  */
 #ifndef SHARED
   if (&__call_tls_dtors != NULL)
diff --git a/nptl/pthread_getspecific.c b/nptl/pthread_getspecific.c
index 114d6da29b..351ddd486f 100644
--- a/nptl/pthread_getspecific.c
+++ b/nptl/pthread_getspecific.c
@@ -25,6 +25,10 @@ __pthread_getspecific (pthread_key_t key)
 {
   struct pthread_key_data *data;
 
+  /* Operation is not permitted while thread exits.  */
+  if (THREAD_GETMEM_NC (THREAD_SELF, thread_exiting))
+    return NULL;
+
   /* Special case access to the first 2nd-level block.  This is the
      usual case.  */
   if (__glibc_likely (key < PTHREAD_KEY_2NDLEVEL_SIZE))
diff --git a/nptl/pthread_setspecific.c b/nptl/pthread_setspecific.c
index 2beb7f97fc..1c0a14dce8 100644
--- a/nptl/pthread_setspecific.c
+++ b/nptl/pthread_setspecific.c
@@ -33,6 +33,10 @@ __pthread_setspecific (pthread_key_t key, const void *value)
 
   self = THREAD_SELF;
 
+  /* Operation is not permitted while thread exits.  */
+  if (THREAD_GETMEM_NC (self, thread_exiting))
+    return EPERM;
+
   /* Special case access to the first 2nd-level block.  This is the
      usual case.  */
   if (__glibc_likely (key < PTHREAD_KEY_2NDLEVEL_SIZE))




-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com