Re: Segfault in _Unwind_* code called from pthread_exit

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 ?

> 
> 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.