Date: Sat, 26 Aug 2017 20:28:13 +0200 From: Tijl Coosemans <tijl@FreeBSD.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: freebsd-current@FreeBSD.org, gerald@FreeBSD.org Subject: Re: Segfault in _Unwind_* code called from pthread_exit Message-ID: <20170826202813.1240a1ef@kalimero.tijl.coosemans.org> In-Reply-To: <20170825234442.GO1700@kib.kiev.ua> References: <20170823163707.096f93ab@kalimero.tijl.coosemans.org> <20170824154235.GD1700@kib.kiev.ua> <20170824180830.199885b0@kalimero.tijl.coosemans.org> <20170825173851.09116ddc@kalimero.tijl.coosemans.org> <20170825234442.GO1700@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 26 Aug 2017 02:44:42 +0300 Konstantin Belousov <kostikbel@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)
>> @@ -507,6 +507,9 @@ 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);
>> @@ -529,6 +532,9 @@ 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)
>> @@ -524,6 +524,9 @@ 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)
@@ -251,6 +251,7 @@ 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)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170826202813.1240a1ef>