From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by sourceware.org (Postfix) with ESMTP id F20453857C52 for ; Thu, 4 Mar 2021 13:26:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org F20453857C52 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-242-rd_9A1C6PdiM1HjePV54qw-1; Thu, 04 Mar 2021 08:26:27 -0500 X-MC-Unique: rd_9A1C6PdiM1HjePV54qw-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 5707E108BD06 for ; Thu, 4 Mar 2021 13:26:26 +0000 (UTC) Received: from oldenburg.str.redhat.com (ovpn-112-56.ams2.redhat.com [10.36.112.56]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4BB0260CCB; Thu, 4 Mar 2021 13:26:25 +0000 (UTC) From: Florian Weimer To: Jakub Jelinek Cc: Jakub Jelinek via Libc-alpha Subject: Re: [PATCH] pthread_once hangs when init routine throws an exception [BZ #18435] References: <20210303125227.GO3748@tucnak> <87blbznnmp.fsf@oldenburg.str.redhat.com> <20210304130003.GA745611@tucnak> Date: Thu, 04 Mar 2021 14:26:32 +0100 In-Reply-To: <20210304130003.GA745611@tucnak> (Jakub Jelinek's message of "Thu, 4 Mar 2021 14:00:03 +0100") Message-ID: <87czwfkq2f.fsf@oldenburg.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-6.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, 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, 04 Mar 2021 13:26:31 -0000 * Jakub Jelinek: > On Thu, Mar 04, 2021 at 12:50:54PM +0100, Florian Weimer wrote: >> I initially thought that we had to use setjmp-based registration >> approach, but that does not seem to be the case. >> >> Any idea why we have the setjmp-based approach (for -fno-exceptions) for >> external use, and not use the legacy approach internal to glibc? One >> difference is that it restores the value of callee-saved registers, >> which could matter if we switch between two kinds of unwinding (callback >> registration and DWARF). > > I'm afraid I don't know, I thought the public pthread_cleanup_push/pop > for -fno-exceptions is the same as the pthreadP.h one except that it uses > _pthread_cleanup_* instead of __pthread_cleanup_*. > All I remember that the normal unwinding does a longjmp at the end after > processing all the self->cleanup registered cleanups, but I thought it is > just to the pthread_create start. I'm going to assume it's the callee saved registers issue. >> Do you think this approach should used for all callback-based functions, >> not just pthread_once? > > If we want to support throw from the callbacks that would do cleanups and > continue throwing outside of the libc routines and at the same time > support no unwind callbacks with cancellation points. > I guess it would need separate analysis of: > argp/Makefile:CFLAGS-argp-help.c += $(uses-callbacks) -fexceptions > argp/Makefile:CFLAGS-argp-parse.c += $(uses-callbacks) > dirent/Makefile:CFLAGS-scandir.c += $(uses-callbacks) > dirent/Makefile:CFLAGS-scandir64.c += $(uses-callbacks) > dirent/Makefile:CFLAGS-scandir-tail.c += $(uses-callbacks) > dirent/Makefile:CFLAGS-scandir64-tail.c += $(uses-callbacks) > elf/Makefile:CFLAGS-dl-iteratephdr.c += $(uses-callbacks) > io/Makefile:CFLAGS-fts.c += -Wno-uninitialized $(uses-callbacks) -fexceptions > io/Makefile:CFLAGS-fts64.c += -Wno-uninitialized $(uses-callbacks) -fexceptions > io/Makefile:CFLAGS-ftw.c += $(uses-callbacks) -fexceptions > io/Makefile:CFLAGS-ftw64.c += $(uses-callbacks) -fexceptions > malloc/Makefile:CFLAGS-obstack.c += $(uses-callbacks) > misc/Makefile:CFLAGS-tsearch.c += $(uses-callbacks) > misc/Makefile:CFLAGS-lsearch.c += $(uses-callbacks) > nptl/Makefile:CFLAGS-pthread_once.c += $(uses-callbacks) -fexceptions \ > posix/Makefile:CFLAGS-glob.c += $(uses-callbacks) -fexceptions > posix/Makefile:CFLAGS-glob64.c += $(uses-callbacks) -fexceptions > stdlib/Makefile:CFLAGS-bsearch.c += $(uses-callbacks) > stdlib/Makefile:CFLAGS-msort.c += $(uses-callbacks) > stdlib/Makefile:CFLAGS-qsort.c += $(uses-callbacks) > find out what we support currently and what we need to support. Or rename the macros and fix the stuff that breaks, yes. For the internal uses, we could use a centralized cleanup function which uses a switch statement over the cleanup type. This would likely be easier to use, too, and the indirect calls go away. >> > --- a/nptl/cleanup_compat.c >> > +++ b/nptl/cleanup_compat.c >> > @@ -48,3 +48,11 @@ _pthread_cleanup_pop (struct _pthread_cleanup_buffer *buffer, int execute) >> > buffer->__routine (buffer->__arg); >> > } >> > strong_alias (_pthread_cleanup_pop, __pthread_cleanup_pop) >> > + >> > +void >> > +__pthread_cleanup_cleanup (struct _pthread_cleanup_buffer *buffer) >> > +{ >> > + struct pthread *self = THREAD_SELF; >> > + if (THREAD_GETMEM (self, cleanup) == buffer) >> > + THREAD_SETMEM (self, cleanup, buffer->__prev); >> > +} >> >> It's surprising that the cleanup field update is conditional. How does >> that happen? Is it really safe? > > As I've tried to explain, there are several cases. > One is throw in the callback, that invokes __pthread_cleanup_combined_routine > only and not the other routine and we need to cleanup. In that case we need > to do the cleanup and self->cleanup ought to be equal to the buffer. > > Another is cancellation with unwind info in callback, from debugging it > seems that the self->cleanup registered callbacks are invoked first and > those unregister the cleanup. In earlier versions of the patch > __pthread_cleanup_cleanup has been called unconditionally from > __pthread_cleanup_combined_routine and in that case __pthread_cleanup_cleanup > would need to be conditional, but in the current version that is not needed. > > Another is cancellation without unwind info in callback, that does > just self->cleanup registered callbacks and no problematic cases. > > Another is normal return from the callback, in that case whether the > cleanups are run depends on the execute argument (which is 0 for > pthread_once). The self->cleanup cleanup is unregistered with hardcoded 0 > so callback isn't invoked, and then the cleanup attribute based one uses > the execute passed to the macro. If it would be non-zero, we need > conditional cleanup because __pthread_cleanup_pop has already unregistered > it, if we only support 0, then __pthread_cleanup_cleanup can be > unconditional. Okay, we'll have to revisit this if we ever change the interfaces and do the consolidation. >> Is there a way to turn the call to __frame->__cancel_routine into a >> direct call? For more general use (outside pthread_once) that might be >> desirable from a security hardening perspective. I expect it will >> happen if __pthread_cleanup_combined_routine_voidptr and >> __pthread_cleanup_combined_routine do not use the sam variable. > > Looking at GCC tree dumps, __pthread_cleanup_combined_routine is inlined > (but because it is considered expensive, it is inlined only partially (i.e. > the __frame->__do_it test). > __attribute__((__always_inline__)) can force it to be inlined fully. > The other thing is the __cancel_routine call, and that is sadly indirect > even if I change it so that while __clframe.__do_it = 1; is done before > __pthread_cleanup_push, __clframe.__cancel_{routine,arg} is initialized > after it returns. For the compiler, the address of __clframe escaped > and so it doesn't know if init_routine will not modify it. > > For __pthread_cleanup_push registered callback, I'm afraid there is no way > out of that. > For the unwind info registered callback, maybe having one structure that > wouldn't be really address taken (containing __cancel_routine, __cancel_arg > and pointer to another structure containing the pthread cleanup buffer > and __do_it var) could do the job. For __pthread_cleanup_push, we can avoid the indirect call on the non-exception path if we move it out of the function, like I did here: [PATCH 3/8] nptl: Move legacy unwinding implementation into libc Again, for pthread_once this does not matter. >> > +static inline void >> > +__pthread_cleanup_combined_routine_voidptr (void *__arg) >> > +{ >> > + struct __pthread_cleanup_combined_frame *__frame >> > + = (struct __pthread_cleanup_combined_frame *) __arg; >> > + if (__frame->__do_it) >> > + { >> > + __frame->__cancel_routine (__frame->__cancel_arg); >> > + __frame->__do_it = 0; >> > + } >> > +} >> >> I think you can make this an out-of-line routine because it can never be >> inlined. But given that this is called only once, it probably does not >> matter. > > This one indeed could be out of line and the other could have out of line > copy and extern inline version for inlining (or static inline > always_inline). I didn't bother when it is for a single spot, but if > it would be for multiple ones, sure. Understood, thanks. I think the patch is okay, except for the nits I pointed out. Thanks, Florian