Re: Segfault in _Unwind_* code called from pthread_exit

On Sun, 29 Oct 2017 21:13:58 +0200 Konstantin Belousov <kostikbel_at_gmail.com> wrote:
> On Sun, Oct 29, 2017 at 06:23:51PM +0100, Tijl Coosemans wrote:
>> On Sat, 26 Aug 2017 21:40:34 +0300 Konstantin Belousov <kostikbel_at_gmail.com> wrote:  
>>> On Sat, Aug 26, 2017 at 08:28:13PM +0200, Tijl Coosemans wrote:  
>>>> I did consider using
>>>> a CFI directive (see patch below) and it works, but it's architecture
>>>> specific and it's inserted after the function prologue so there's still
>>>> a window of a few instructions where a stack unwinder will try to use
>>>> the return address.
>>>> 
>>>> Index: lib/libthr/thread/thr_create.c
>>>> ===================================================================
>>>> --- lib/libthr/thread/thr_create.c      (revision 322802)
>>>> +++ lib/libthr/thread/thr_create.c      (working copy)
>>>> _at__at_ -251,6 +251,7 _at__at_ create_stack(struct pthread_attr *pattr)
>>>>  static void
>>>>  thread_start(struct pthread *curthread)
>>>>  {
>>>> +       __asm(".cfi_undefined %rip");
>>>>         sigset_t set;
>>>>  
>>>>         if (curthread->attr.suspend == THR_CREATE_SUSPENDED)    
>>> 
>>> I like this approach much more than the previous patch.  What can be
>>> done is to provide asm trampoline which calls thread_start().  There you
>>> can add the .cfi_undefined right at the entry.
>>>
>>> It is somewhat more work than just setting the return address on the
>>> kernel-constructed pseudo stack frame, but I believe this is ultimately
>>> correct way.  You still can do it only on some arches, if you do not
>>> have incentive to code asm for all of them.  
>> 
>> Ok, but then there are two ways to implement the trampoline:
>> 
>> 1)
>> 	movq $0,(%rsp)
>> 	jmp thread_start
>> 2)
>> 	subq $8,%rsp
>> 	call thread_start
>> 	/* NOTREACHED */
>> 
>> With 1) you're setting the return address to zero anyway, so you might
>> as well do that in the kernel like my first patch.  With 2) you're
>> setting up a new call frame, basically redoing what the kernel already
>> did and on i386 this also means copying the function argument.  
> I do not quite understand the second variant, because the stack is not
> guaranteed to be zeroed, and it is often not if reused after the previously
> exited thread.

The problem is that the return address of thread_start can be garbage.
Solution 1 sets it to zero.  Nothing else changes, once thread_start runs
it's as if the trampoline never existed.  Solution 2 creates a new frame
and calls thread_start giving it a valid return address back to the
trampoline.  The trampoline still has a return address that can be garbage,
but it has a CFI directive saying its return address is undefined so
that's ok.

> The first variant is what I like, but perhaps we need to emulate the
> frame as well, i.e. push two zero longs.

That would be a hybrid of 1 and 2.

> Currently kernel does not access the usermode stack for the new thread
> unless dictated by ABI (i.e. it does not touch it for 64bit process
> on amd64, but have to for 32bit).  I like this property.  Also, the
> previous paragraph is indicative: we do not really know in kernel
> what ABI the userspace follows.  It might want frame, may be it does
> not need it.  It could use other register than %rbp as the frame base,
> etc.

Yes, but then the kernel should just set the stack pointer to the first
byte after the stack and pass the argument in a register.  Then userspace
can align the stack and store function arguments the way it likes and
call a C function or whatever else it wants to do.  Now the kernel is
already setting up the stack so the entry point can be a C function.

But anyway, we're both talking about hypothetical situations now so I'm
just going to make the decision and go with my earlier patch.  Then it's
clear that the kernel sets up the frame and there isn't some sort of split
responsibility between kernel and userland.  If there's ever a situation
where the kernel should not set up a call frame a new syscall can be
added.  In the end we're talking about the best way to set a word to zero
here.  We've spent too many emails on that already.

>> Do you have any preference (or better alternatives), because I think I
>> still prefer my first patch.  It's the caller's job to set up the call
>> frame, in this case the kernel.  And if the kernel handles it then it
>> also works with (hypothetical) implementations other than libthr.
>> 
>>> Also crt1 probably should get the same treatment, despite we already set
>>> %rbp to zero AFAIR.
>> 
>> I haven't checked but I imagine the return address of the process entry
>> point is always zero because the stack is all zeros.
> Stack is not zero. The environment and argument strings and auxv are copied
> at top, and at the bottom the ps_strings structure is located, so it
> is not.

Yes, what I meant is that it's newly allocated so even if the return
address is uninitialised it will be zero.  If it wasn't people would
have reported problems with stack unwinding.