From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x72d.google.com (mail-qk1-x72d.google.com [IPv6:2607:f8b0:4864:20::72d]) by sourceware.org (Postfix) with ESMTPS id A8CE43857416 for ; Thu, 20 May 2021 19:15:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A8CE43857416 Received: by mail-qk1-x72d.google.com with SMTP id c20so17375742qkm.3 for ; Thu, 20 May 2021 12:15:35 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=TeA6bd0QwBydIuVx80Q+rHuhnLPCk7Qmi6JUEj8THRI=; b=TniUZVqkPHNKbut2HqXynDdsGrVIwTVjUSYwncZxOLMYGy3uwAOWnS9PTlmhgIQRLA yCW0HjgJhkp0waIx4JVVsie3sT8rQ9LBp/vp6nBbOkNCZMCfMTHNcedsTcJx9gmouRyt jEdkI+fyuRostHf2qSQVWjAQFBWJhYVypHpAMG8JsI4fWnjlUtCmM/o4gmK2fIQoH/rt sskXjvSVUdXPX5wVX4AXBAjdHlPkTyhngFNUVGYd4ROvib72WnGlWfbSYtohEnT6HBMZ BjVZy/q33SvUIS++pkNq2ng/dVZuqJe/WpEHKfAfkfeP/s82fKArqIOToOdWAcrLle5b LwLQ== X-Gm-Message-State: AOAM530TcbdAke7lG2yqGsjrhi50o3rvRmOcwT0hDeu6gfE3gyHq/IXA JVq/aUSd9prvSLrzLkxSZ9W3hkVXqD/now== X-Google-Smtp-Source: ABdhPJxGiIKIEiMFbOv5qAHm/3l3WN/nZFki1mrUB8s5R5qxphnkU1wS2LicNTnLG3L9gSItxkC+Ug== X-Received: by 2002:a05:620a:2941:: with SMTP id n1mr6967431qkp.330.1621538134577; Thu, 20 May 2021 12:15:34 -0700 (PDT) Received: from [192.168.1.4] ([177.194.37.86]) by smtp.gmail.com with ESMTPSA id r16sm2560633qtx.36.2021.05.20.12.15.33 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 20 May 2021 12:15:34 -0700 (PDT) Subject: Re: [PATCH 01/10] nptl: Perform signal initialization upon pthread_create To: Florian Weimer , libc-alpha@sourceware.org References: From: Adhemerval Zanella Message-ID: Date: Thu, 20 May 2021 16:15:32 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 May 2021 19:15:37 -0000 On 18/05/2021 11:24, Florian Weimer via Libc-alpha wrote: > Install signal handlers and unblock signals before pthread_create > creates the first thread. > --- > nptl/Versions | 5 ++- > nptl/nptl-init.c | 75 ------------------------------------ > nptl/pthreadP.h | 6 +++ > nptl/pthread_cancel.c | 88 ++++++++++++++++++++++++++++++++++++++----- > nptl/pthread_create.c | 45 +++++++++++++++++++++- > 5 files changed, 131 insertions(+), 88 deletions(-) > > diff --git a/nptl/Versions b/nptl/Versions > index e7883cbb49..d96b830d05 100644 > --- a/nptl/Versions > +++ b/nptl/Versions > @@ -367,8 +367,6 @@ libc { > tss_set; > } > GLIBC_PRIVATE { > - __nptl_create_event; > - __nptl_death_event; > __default_pthread_attr; > __default_pthread_attr_lock; > __futex_abstimed_wait64; > @@ -386,11 +384,14 @@ libc { > __lll_trylock_elision; > __lll_unlock_elision; > __mutex_aconf; > + __nptl_create_event; > __nptl_deallocate_stack; > __nptl_deallocate_tsd; > + __nptl_death_event; > __nptl_free_tcb; > __nptl_nthreads; > __nptl_setxid_sighandler; > + __nptl_sigcancel_handler; > __nptl_stack_list_add; > __nptl_stack_list_del; > __pthread_attr_copy; > diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c > index f4b86fbfaf..bc4831ac89 100644 > --- a/nptl/nptl-init.c > +++ b/nptl/nptl-init.c > @@ -44,84 +44,9 @@ size_t __static_tls_align_m1; > /* Version of the library, used in libthread_db to detect mismatches. */ > static const char nptl_version[] __attribute_used__ = VERSION; > > -/* For asynchronous cancellation we use a signal. This is the handler. */ > -static void > -sigcancel_handler (int sig, siginfo_t *si, void *ctx) > -{ > - /* Safety check. It would be possible to call this function for > - other signals and send a signal from another process. This is not > - correct and might even be a security problem. Try to catch as > - many incorrect invocations as possible. */ > - if (sig != SIGCANCEL > - || si->si_pid != __getpid() > - || si->si_code != SI_TKILL) > - return; > - > - struct pthread *self = THREAD_SELF; > - > - int oldval = THREAD_GETMEM (self, cancelhandling); > - while (1) > - { > - /* We are canceled now. When canceled by another thread this flag > - is already set but if the signal is directly send (internally or > - from another process) is has to be done here. */ > - int newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK; > - > - if (oldval == newval || (oldval & EXITING_BITMASK) != 0) > - /* Already canceled or exiting. */ > - break; > - > - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval, > - oldval); > - if (curval == oldval) > - { > - /* Set the return value. */ > - THREAD_SETMEM (self, result, PTHREAD_CANCELED); > - > - /* Make sure asynchronous cancellation is still enabled. */ > - if ((newval & CANCELTYPE_BITMASK) != 0) > - /* Run the registered destructors and terminate the thread. */ > - __do_cancel (); > - > - break; > - } > - > - oldval = curval; > - } > -} > - > - > -/* When using __thread for this, we do it in libc so as not > - to give libpthread its own TLS segment just for this. */ > -extern void **__libc_dl_error_tsd (void) __attribute__ ((const)); > - > - > void > __pthread_initialize_minimal_internal (void) > { > - struct sigaction sa; > - __sigemptyset (&sa.sa_mask); > - > - /* Install the cancellation signal handler. If for some reason we > - cannot install the handler we do not abort. Maybe we should, but > - it is only asynchronous cancellation which is affected. */ > - sa.sa_sigaction = sigcancel_handler; > - sa.sa_flags = SA_SIGINFO; > - (void) __libc_sigaction (SIGCANCEL, &sa, NULL); > - > - /* Install the handle to change the threads' uid/gid. */ > - sa.sa_sigaction = __nptl_setxid_sighandler; > - sa.sa_flags = SA_SIGINFO | SA_RESTART; > - (void) __libc_sigaction (SIGSETXID, &sa, NULL); > - > - /* The parent process might have left the signals blocked. Just in > - case, unblock it. We reuse the signal mask in the sigaction > - structure. It is already cleared. */ > - __sigaddset (&sa.sa_mask, SIGCANCEL); > - __sigaddset (&sa.sa_mask, SIGSETXID); > - INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_UNBLOCK, &sa.sa_mask, > - NULL, __NSIG_BYTES); > - > /* Get the size of the static and alignment requirements for the TLS > block. */ > size_t static_tls_align; > diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h > index f93806e540..497c2ad3d9 100644 > --- a/nptl/pthreadP.h > +++ b/nptl/pthreadP.h > @@ -571,6 +571,12 @@ libc_hidden_proto (__pthread_attr_setsigmask_internal) > extern __typeof (pthread_attr_getsigmask_np) __pthread_attr_getsigmask_np; > libc_hidden_proto (__pthread_attr_getsigmask_np) > > +/* The cancellation signal handler defined alongside with > + pthread_cancel. This is included in statically linked binaries > + only if pthread_cancel is linked in. */ > +void __nptl_sigcancel_handler (int sig, siginfo_t *si, void *ctx); > +libc_hidden_proto (__nptl_sigcancel_handler) > + > /* Special versions which use non-exported functions. */ > extern void __pthread_cleanup_push (struct _pthread_cleanup_buffer *buffer, > void (*routine) (void *), void *arg); > diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c > index e4ad602900..802c691874 100644 > --- a/nptl/pthread_cancel.c > +++ b/nptl/pthread_cancel.c > @@ -26,6 +26,63 @@ > #include > #include > #include > +#include > + > +/* For asynchronous cancellation we use a signal. This is the core > + logic of the signal handler. */ > +static void > +sigcancel_handler (void) > +{ > + struct pthread *self = THREAD_SELF; > + > + int oldval = THREAD_GETMEM (self, cancelhandling); > + while (1) > + { > + /* We are canceled now. When canceled by another thread this flag > + is already set but if the signal is directly send (internally or > + from another process) is has to be done here. */ > + int newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK; > + > + if (oldval == newval || (oldval & EXITING_BITMASK) != 0) > + /* Already canceled or exiting. */ > + break; > + > + int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval, > + oldval); > + if (curval == oldval) > + { > + /* Set the return value. */ > + THREAD_SETMEM (self, result, PTHREAD_CANCELED); > + > + /* Make sure asynchronous cancellation is still enabled. */ > + if ((newval & CANCELTYPE_BITMASK) != 0) > + /* Run the registered destructors and terminate the thread. */ > + __do_cancel (); > + > + break; > + } > + > + oldval = curval; > + } > +} > + > +/* This is the actually installed SIGCANCEL handler. It adds some > + safety checks before performing the cancellation. */ > +void > +__nptl_sigcancel_handler (int sig, siginfo_t *si, void *ctx) > +{ > + /* Safety check. It would be possible to call this function for > + other signals and send a signal from another process. This is not > + correct and might even be a security problem. Try to catch as > + many incorrect invocations as possible. */ > + if (sig != SIGCANCEL > + || si->si_pid != __getpid() > + || si->si_code != SI_TKILL) > + return; > + > + sigcancel_handler (); > +} > +libc_hidden_def (__nptl_sigcancel_handler) > > int > __pthread_cancel (pthread_t th) > @@ -72,14 +129,23 @@ __pthread_cancel (pthread_t th) > oldval)) > goto again; > > - /* The cancellation handler will take care of marking the > - thread as canceled. */ > - pid_t pid = __getpid (); > - > - int val = INTERNAL_SYSCALL_CALL (tgkill, pid, pd->tid, > - SIGCANCEL); > - if (INTERNAL_SYSCALL_ERROR_P (val)) > - result = INTERNAL_SYSCALL_ERRNO (val); > + if (pd == THREAD_SELF) > + /* This is not merely an optimization: An application may > + call pthread_cancel (pthread_self ()) without calling > + pthread_create, so the signal handler may not have been > + set up for a self-cancel. */ > + sigcancel_handler (); I think it would be simple to just call __pthread_exit (PTHREAD_CANCELED) here, it won't require to split the cancellation handler, it already unwind if cancel state is enabled and asynchronous, and it does not require add another PTHREAD_STATIC_FN_REQUIRE hack. It would require an extra __libc_unwind_link_get call, but I think we can optimize it later (I am working on a patch to simplify it). > + else > + { > + /* The cancellation handler will take care of marking the > + thread as canceled. */ > + pid_t pid = __getpid (); > + > + int val = INTERNAL_SYSCALL_CALL (tgkill, pid, pd->tid, > + SIGCANCEL); > + if (INTERNAL_SYSCALL_ERROR_P (val)) > + result = INTERNAL_SYSCALL_ERRNO (val); > + } > > break; > } > @@ -106,4 +172,8 @@ versioned_symbol (libc, __pthread_cancel, pthread_cancel, GLIBC_2_34); > compat_symbol (libpthread, __pthread_cancel, pthread_cancel, GLIBC_2_0); > #endif > > -PTHREAD_STATIC_FN_REQUIRE (__pthread_create) > +/* Ensure that the unwinder is always linked in (the __pthread_unwind > + reference from __do_cancel is weak). Use ___pthread_unwind_next > + (three underscores) to produce a strong reference to the same > + file. */ > +PTHREAD_STATIC_FN_REQUIRE (___pthread_unwind_next) > diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c > index 770656453d..772b5efcc6 100644 > --- a/nptl/pthread_create.c > +++ b/nptl/pthread_create.c > @@ -56,6 +56,43 @@ static struct rtld_global *__nptl_rtld_global __attribute_used__ > = &_rtld_global; > #endif > > +/* This performs the initialization necessary when going from > + single-threaded to multi-threaded mode for the first time. */ > +static void > +late_init (void) > +{ > + struct sigaction sa; > + __sigemptyset (&sa.sa_mask); > + > + /* Install the cancellation signal handler (in static builds only if > + pthread_cancel has been linked in). If for some reason we cannot > + install the handler we do not abort. Maybe we should, but it is > + only asynchronous cancellation which is affected. */ > +#ifndef SHARED > + extern __typeof (__nptl_sigcancel_handler) __nptl_sigcancel_handler > + __attribute__ ((weak)); > + if (__nptl_sigcancel_handler != NULL) > +#endif This weak symbol can be avoided if we move the cancellation setup on pthread_cancel instead. I still think this is best approach, it disentangle the cancellation handling. > + { > + sa.sa_sigaction = __nptl_sigcancel_handler; > + sa.sa_flags = SA_SIGINFO; > + (void) __libc_sigaction (SIGCANCEL, &sa, NULL); > + } > + > + /* Install the handle to change the threads' uid/gid. */ > + sa.sa_sigaction = __nptl_setxid_sighandler; > + sa.sa_flags = SA_SIGINFO | SA_RESTART; > + (void) __libc_sigaction (SIGSETXID, &sa, NULL); > + > + /* The parent process might have left the signals blocked. Just in > + case, unblock it. We reuse the signal mask in the sigaction > + structure. It is already cleared. */ > + __sigaddset (&sa.sa_mask, SIGCANCEL); > + __sigaddset (&sa.sa_mask, SIGSETXID); > + INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_UNBLOCK, &sa.sa_mask, > + NULL, __NSIG_BYTES); > +} > + > /* Code to allocate and deallocate a stack. */ > #include "allocatestack.c" > > @@ -459,9 +496,13 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr, > { > STACK_VARIABLES; > > - /* Avoid a data race in the multi-threaded case. */ > + /* Avoid a data race in the multi-threaded case, and call the > + deferred initialization only once. */ > if (__libc_single_threaded) > - __libc_single_threaded = 0; > + { > + late_init (); > + __libc_single_threaded = 0; > + } > > const struct pthread_attr *iattr = (struct pthread_attr *) attr; > union pthread_attr_transparent default_attr; >