From patchwork Wed Oct 18 16:13:15 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mathieu Desnoyers X-Patchwork-Id: 23673 Received: (qmail 80790 invoked by alias); 18 Oct 2017 16:11:13 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 80775 invoked by uid 89); 18 Oct 2017 16:11:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=3359, hate, H*M:zimbra X-HELO: mail.efficios.com Date: Wed, 18 Oct 2017 16:13:15 +0000 (UTC) From: Mathieu Desnoyers To: Florian Weimer Cc: Carlos O'Donell , Ben Maurer , libc-alpha Message-ID: <1042541496.43815.1508343195033.JavaMail.zimbra@efficios.com> In-Reply-To: <87lgk9c95r.fsf@mid.deneb.enyo.de> References: <20171017232440.5134-1-mathieu.desnoyers@efficios.com> <87lgk9c95r.fsf@mid.deneb.enyo.de> Subject: Re: [RFC PATCH glibc v2] pthread_setspecific: Provide signal-safety across keys MIME-Version: 1.0 ----- 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))