From owner-freebsd-current@freebsd.org Sat Aug 26 18:29:33 2017 Return-Path: Delivered-To: freebsd-current@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id F3B50DD7D58 for ; Sat, 26 Aug 2017 18:29:32 +0000 (UTC) (envelope-from tijl@freebsd.org) Received: from mailrelay108.isp.belgacom.be (mailrelay108.isp.belgacom.be [195.238.20.135]) (using TLSv1.2 with cipher RC4-SHA (128/128 bits)) (Client CN "relay.skynet.be", Issuer "GlobalSign Organization Validation CA - SHA256 - G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id E488174576; Sat, 26 Aug 2017 18:29:31 +0000 (UTC) (envelope-from tijl@freebsd.org) X-Belgacom-Dynamic: yes IronPort-PHdr: =?us-ascii?q?9a23=3An/dP4RVZqWzHk4a5NHjNjvsKxbrV8LGtZVwlr6E/?= =?us-ascii?q?grcLSJyIuqrYYxeCt8tkgFKBZ4jH8fUM07OQ6PGwHzRYqb+681k6OKRWUBEEjc?= =?us-ascii?q?hE1ycBO+WiTXPBEfjxciYhF95DXlI2t1uyMExSBdqsLwaK+i764jEdAAjwOhRo?= =?us-ascii?q?LerpBIHSk9631+ev8JHPfglEnjSwbLdxIRmssQndqtQdjJd/JKo21hbHuGZDdf?= =?us-ascii?q?5MxWNvK1KTnhL86dm18ZV+7SleuO8v+tBZX6nicKs2UbJXDDI9M2Ao/8LrrgXM?= =?us-ascii?q?TRGO5nQHTGoblAdDDhXf4xH7WpfxtTb6tvZ41SKHM8D6Uaw4VDK/5KptVRTmij?= =?us-ascii?q?oINyQh/W/ZisJ+kr9VrhGjqBxxzIHbfI6bOeFifq7fYd8WWXZNUtpPWyFHH4iy?= =?us-ascii?q?b5EPD+0EPetAsYf9plkOrR+jDgSyA+PvzSRIiWHz3aIg1eQhChzN0Qs8H9IPsn?= =?us-ascii?q?TUqM74OqcIUe+r0qbF0CjNYf1M1Tf68ojIfQksrPeRVrxzacrc0UoiGx7fglmO?= =?us-ascii?q?poHoPymZ2vkOvmWf9eZsSOyihm8hpgpsuDag3N0shZPMho8Nz1DE8jh2z5gtKN?= =?us-ascii?q?2jTU57fcakEJxNtyGGL4d2Qt0tQ2VvuCsiyb0Jo5q7fCkPyJs53R7fbOaLc5SJ?= =?us-ascii?q?4hLhUOadOyt3hHVieLKkmRmy9FKvyuvnVsWu11ZKtCVFnsHNtnALyRPT9tCKR/?= =?us-ascii?q?hg8ku7xzqC2ADe5vtZLU03kafXMYMtz7Axm5YLtETMBC72mEH4jK+McUUk//Cl?= =?us-ascii?q?6/jmYrXkop+RLIF0ihvgPaswgcO/Gvk3PhIJX2iB9uSwzKfj8lHhQLVWkv02lb?= =?us-ascii?q?HUsJPdJcQAuq65AgxV3Z095Ba7FDqm39EYkmMGLFJBYh6Ik4/pO1SdaMz/WNS4?= =?us-ascii?q?hU+wmTF3xvaOFLDlBYjWKWaLxLTmZqp86ERRzCI8yNle49RfDbRXc9zpXUqkiN?= =?us-ascii?q?3aClcSNAuvzuPuDs41gp8fW2anLLWUPYnpnRmP/O15cLrEX5McpDuoc6tt3PXp?= =?us-ascii?q?l3JswVI=3D?= X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: =?us-ascii?q?A2AkAwDOvKFZ/4i99VFcHAEBBAEBCgEBF?= =?us-ascii?q?wEBBAEBCgEBgwItVIEljhR0jyIBAYFwLwFHh0KNbYIShUcCg2JAGAEBAQEBAQE?= =?us-ascii?q?BAQEBaiiCMyKCQwEBAQECAScTHCMQCw4EBgklDxIYEA4GExuJfgMNDLIhOoczD?= =?us-ascii?q?YQbAQEBAQEBBAEBAQEkgyqFM4JzNYJXgh4shUgFoCg8j1CEaX+RdkiMAYl0Hzi?= =?us-ascii?q?BDVMxCIVeAxyBaT42iF8pghgBAQE?= X-IPAS-Result: =?us-ascii?q?A2AkAwDOvKFZ/4i99VFcHAEBBAEBCgEBFwEBBAEBCgEBgwI?= =?us-ascii?q?tVIEljhR0jyIBAYFwLwFHh0KNbYIShUcCg2JAGAEBAQEBAQEBAQEBaiiCMyKCQ?= =?us-ascii?q?wEBAQECAScTHCMQCw4EBgklDxIYEA4GExuJfgMNDLIhOoczDYQbAQEBAQEBBAE?= =?us-ascii?q?BAQEkgyqFM4JzNYJXgh4shUgFoCg8j1CEaX+RdkiMAYl0HziBDVMxCIVeAxyBa?= =?us-ascii?q?T42iF8pghgBAQE?= Received: from 136.189-245-81.adsl-dyn.isp.belgacom.be (HELO kalimero.tijl.coosemans.org) ([81.245.189.136]) by relay.skynet.be with ESMTP; 26 Aug 2017 20:28:15 +0200 Received: from kalimero.tijl.coosemans.org (kalimero.tijl.coosemans.org [127.0.0.1]) by kalimero.tijl.coosemans.org (8.15.2/8.15.2) with ESMTP id v7QISEjs004672; Sat, 26 Aug 2017 20:28:14 +0200 (CEST) (envelope-from tijl@FreeBSD.org) Date: Sat, 26 Aug 2017 20:28:13 +0200 From: Tijl Coosemans To: Konstantin Belousov 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> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 26 Aug 2017 18:29:33 -0000 On Sat, 26 Aug 2017 02:44:42 +0300 Konstantin Belousov 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)