Re: Segfault in _Unwind_* code called from pthread_exit

On Sat, 26 Aug 2017 02:44:42 +0300 Konstantin Belousov <kostikbel_at_gmail.com> wrote:
> On Fri, Aug 25, 2017 at 05:38:51PM +0200, Tijl Coosemans wrote:
>> So both GCC and LLVM unwinding look up the return address in the CFI
>> table and fail when the return address is garbage, but LLVM treats this
>> as an end-of-stack condition while GCC further tries to see if the
>> return address points to a signal trampoline by testing the instruction
>> bytes at that address.  On amd64 the garbage address is unreadable so it
>> segfaults.  On i386 it is readable, the test fails and GCC returns
>> end-of-stack.  
> How does llvm unwinder detects that the return address is a garbage ?

It just stops unwinding when it can't find frame information (stored in
.eh_frame sections).  GCC unwinder doesn't give up yet and checks if the
return address points to the signal trampoline (which means the current
frame is that of a signal handler).  It has built-in knowledge of how to
unwind to the signal trampoline frame.

>> To fix the crash and get predictable behaviour in the other cases I
>> propose always setting the return address to 0.  The attached patch does
>> this for i386 and amd64.  I don't know if other architectures need a
>> similar patch.  
>>
>> Index: sys/amd64/amd64/vm_machdep.c
>> ===================================================================
>> --- sys/amd64/amd64/vm_machdep.c	(revision 322802)
>> +++ sys/amd64/amd64/vm_machdep.c	(working copy)
>> _at__at_ -507,6 +507,9 _at__at_ cpu_set_upcall(struct thread *td, void (*entry)(void *
>>  		   (((uintptr_t)stack->ss_sp + stack->ss_size - 4) & ~0x0f) - 4;
>>  		td->td_frame->tf_rip = (uintptr_t)entry;
>>  
>> +		/* Sentinel return address to stop stack unwinding. */
>> +		suword32((void *)td->td_frame->tf_rsp, 0);
>> +
>>  		/* Pass the argument to the entry point. */
>>  		suword32((void *)(td->td_frame->tf_rsp + sizeof(int32_t)),
>>  		    (uint32_t)(uintptr_t)arg);
>> _at__at_ -529,6 +532,9 _at__at_ cpu_set_upcall(struct thread *td, void (*entry)(void *
>>  	td->td_frame->tf_fs = _ufssel;
>>  	td->td_frame->tf_gs = _ugssel;
>>  	td->td_frame->tf_flags = TF_HASSEGS;
>> +
>> +	/* Sentinel return address to stop stack unwinding. */
>> +	suword((void *)td->td_frame->tf_rsp, 0);
>>  
>>  	/* Pass the argument to the entry point. */
>>  	td->td_frame->tf_rdi = (register_t)arg;
>> Index: sys/i386/i386/vm_machdep.c
>> ===================================================================
>> --- sys/i386/i386/vm_machdep.c	(revision 322802)
>> +++ sys/i386/i386/vm_machdep.c	(working copy)
>> _at__at_ -524,6 +524,9 _at__at_ cpu_set_upcall(struct thread *td, void (*entry)(void *
>>  	    (((int)stack->ss_sp + stack->ss_size - 4) & ~0x0f) - 4;
>>  	td->td_frame->tf_eip = (int)entry;
>>  
>> +	/* Sentinel return address to stop stack unwinding. */
>> +	suword((void *)td->td_frame->tf_esp, 0);
>> +
>>  	/* Pass the argument to the entry point. */
>>  	suword((void *)(td->td_frame->tf_esp + sizeof(void *)),
>>  	    (int)arg);  
> 
> I do not object against this, but I believe that a better solution exists
> for the system side (putting my change for gcc unwinder to detect the
> signal frame aside).  The thread_start() sentinel in libthr should get
> proper dwarf annotation of not having the return address.  May be
> normal function attributes of no return are enough to force compilers
> to generate required unwind data.  Might be some more magic with inline
> asm and .cfi_return_column set to undefined.

A noreturn attribute isn't enough.  You can still unwind such functions.
They are allowed to throw exceptions for example.  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)