Re: [PATCH] pthread_once hangs when init routine throws an exception [BZ #18435]

[Florian Weimer <fweimer@redhat.com>]

* 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
  <https://sourceware.org/pipermail/libc-alpha/2021-March/123205.html>

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