From owner-freebsd-bugs@FreeBSD.ORG Sun Apr 7 02:41:12 2013 Return-Path: Delivered-To: freebsd-bugs@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id A2E77B00; Sun, 7 Apr 2013 02:41:12 +0000 (UTC) (envelope-from davidxu@freebsd.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:1900:2254:206c::16:87]) by mx1.freebsd.org (Postfix) with ESMTP id 9268DE6E; Sun, 7 Apr 2013 02:41:12 +0000 (UTC) Received: from xyf.my.dom (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.6/8.14.6) with ESMTP id r372f9fK052354; Sun, 7 Apr 2013 02:41:10 GMT (envelope-from davidxu@freebsd.org) Message-ID: <5160DCD9.5070503@freebsd.org> Date: Sun, 07 Apr 2013 10:41:29 +0800 From: David Xu User-Agent: Mozilla/5.0 (X11; FreeBSD i386; rv:14.0) Gecko/20120822 Thunderbird/14.0 MIME-Version: 1.0 To: Bruce Evans Subject: Re: misc/177624: Swapcontext can get compiled incorrectly References: <201304040232.r342WFTC020054@red.freebsd.org> <20130404232206.S1025@besplex.bde.org> <20130405011027.Y1350@besplex.bde.org> <20130405044424.Y2557@besplex.bde.org> In-Reply-To: <20130405044424.Y2557@besplex.bde.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Cc: freebsd-gnats-submit@FreeBSD.org, freebsd-bugs@FreeBSD.org, Brian Demsky X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 07 Apr 2013 02:41:12 -0000 On 2013/04/05 03:38, Bruce Evans wrote: > On Thu, 4 Apr 2013, Brian Demsky wrote: > >> On Apr 4, 2013, at 8:16 AM, Bruce Evans wrote: >> >>> On Fri, 5 Apr 2013, Bruce Evans wrote: >>> >>>> On Thu, 4 Apr 2013, Brian Demsky wrote: >>>> >>>>>> Description: >>>>> Here is the code for swap context: >>>>> int >>>>> swapcontext(ucontext_t *oucp, const ucontext_t *ucp) >>>>> { >>>>> int ret; >>>>> if ((oucp == NULL) || (ucp == NULL)) { >>>>> errno = EINVAL; >>>>> return (-1); >>>>> } >>>>> oucp->uc_flags &= ~UCF_SWAPPED; >>>>> ret = getcontext(oucp); >>>>> if ((ret == 0) && !(oucp->uc_flags & UCF_SWAPPED)) { >>>>> oucp->uc_flags |= UCF_SWAPPED; >>>>> ret = setcontext(ucp); >>>>> } >>>>> return (ret); >>>>> } >>>> >>>>> On the OS X port of libc in Mac OSX 10.7.5, this gets compiled as: >>>> >>>>> ... >>>>> 0x00007fff901e870b : pop %rbx >>>>> 0x00007fff901e870c : pop %r14 >>>>> 0x00007fff901e870e : jmpq 0x7fff90262855 >>>>> >>>>> The problem is that rbx is callee saved by compiled version of >>>>> swapcontext and then reused before getcontext is called. >>>>> Getcontext then stores the wrong value for rbx and setcontext later >>>>> restores the wrong value for rbx. If the caller had any value in >>>>> rbx, it has been trashed at this point. >>>> >>>> Later you wrote: >>>> >>>>> The analysis is a little wrong about the problem. Ultimately, the >>>>> tail call to set context trashes the copies of bx and r14 on the >>>>> stack�. >>>> >>>> The bug seems to be in setcontext(). It must preserve the callee-saved >>>> registers, not restore them. This would happen automatically if more >>>> were written in C. But setcontext() can't be written entirely in C, >>>> since it must save all callee-saved registers including ones not used >>>> and therefore not normally saved by any C function that it might be in, >>>> and possibly also including callee-saved registers for nonstandard or >>>> non-C ABIs. In FreeBSD, it is apparently always a syscall. >>> >>> This is more than a little wrong. When setcontext() succeeds, it >>> doesn't return here. Then it acts like longjmp() and must restore all >>> the callee-saved to whatever they were when getcontext() was called. >>> Otherwise, it must not clobber any callee-saved registers (then it >>> differs from longjmp(). longjmp() just can't fail). >>> >>> Now I don't see any bug here. If the saved state is returned to, then >>> it is as if getcontext() returned, and the intermediately-saved %rbx >>> is correct (we will restore the orginal %rbx if we return). If >>> setcontext() fails, then it should preserve all callee-saved registers. >>> In the tail-call case, we have already restored the orginal %rbx and >>> the failing setcontext() should preserve that. >>> >>> Bruce >> >> Take at setcontext: >> >> (gdb) disassemble setcontext >> Dump of assembler code for function setcontext: >> 0x00007fff90262855 : push %rbx >> 0x00007fff90262856 : lea 0x38(%rdi),%rbx >> 0x00007fff9026285a : cmp 0x30(%rdi),%rbx >> 0x00007fff9026285e : je 0x7fff90262864 >> >> 0x00007fff90262860 : mov %rbx,0x30(%rdi) >> 0x00007fff90262864 : mov 0x4(%rdi),%edi >> 0x00007fff90262867 : callq 0x7fff90262998 >> >> 0x00007fff9026286c : mov %rbx,%rdi >> 0x00007fff9026286f : pop %rbx >> 0x00007fff90262870 : jmpq 0x7fff90262875 >> <_setcontext> >> End of assembler dump. >> >> The stack from swapcontext had rbx and r14 popped after getcontext >> stored everything. Now we push rbx and then later call sigsetmask. >> Those two actions guarantee that the memory locations where rbx and >> r14 were on the stack have been overwritten. When we later return to >> the save context, it will start up swapcontext and pop the wrong >> values off of the stack for rbx and r14. > > Ah, it is not really rbx and r14, but rsp and the whole stack frame of > swapcontext() that are mishandled. Even returning from swapcontext() > leaves the saved rsp pointing to garbage. The stack frame could have > had almost anything on it before it became invalid, but here it has mainly > the saved rbx and r14 (not rbp; however, when compiled by clang on FreeBSD, > it also has the saved rbp, and when compiled with -O0 it also has the > local variable). > > Now I think swapcontext() can't be written in C, for the same reason that > setjmp() can't be written in C -- the stack frame cannot be controlled in > C, and if it has anything at all on it (even the return address requires > special handling), then the stack pointer saved in the context becomes > invalid when the function returns, or even earlier for tail calls and > other optimizations. This reminds me that I can not override swapcontext in libthr, I had put a wrapper for swapcontext in libthr, I am considering to remove it now ...