From owner-freebsd-bugs@FreeBSD.ORG Thu Apr 4 16:43:09 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 DA355BF2; Thu, 4 Apr 2013 16:43:09 +0000 (UTC) (envelope-from bdemsky@uci.edu) Received: from esmtp1.es.uci.edu (esmtp1.es.uci.edu [128.195.153.131]) by mx1.freebsd.org (Postfix) with ESMTP id C35CEFBA; Thu, 4 Apr 2013 16:43:09 +0000 (UTC) Received: from [192.168.0.103] (adsl-75-50-191-170.dsl.lsan03.sbcglobal.net [75.50.191.170]) (authenticated bits=0) by esmtp1.es.uci.edu (8.13.8/8.13.8) with ESMTP id r34Gh6GY675195 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NOT); Thu, 4 Apr 2013 09:43:08 -0700 X-UCInetID: bdemsky Subject: Re: misc/177624: Swapcontext can get compiled incorrectly Mime-Version: 1.0 (Apple Message framework v1283) Content-Type: text/plain; charset=windows-1252 From: Brian Demsky In-Reply-To: <20130405011027.Y1350@besplex.bde.org> Date: Thu, 4 Apr 2013 09:43:06 -0700 Content-Transfer-Encoding: quoted-printable Message-Id: References: <201304040232.r342WFTC020054@red.freebsd.org> <20130404232206.S1025@besplex.bde.org> <20130405011027.Y1350@besplex.bde.org> To: Bruce Evans X-Mailer: Apple Mail (2.1283) Cc: freebsd-bugs@freebsd.org, freebsd-gnats-submit@freebsd.org 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: Thu, 04 Apr 2013 16:43:09 -0000 On Apr 4, 2013, at 8:16 AM, Bruce Evans wrote: > On Fri, 5 Apr 2013, Bruce Evans wrote: >=20 >> On Thu, 4 Apr 2013, Brian Demsky wrote: >>=20 >>>> Description: >>> Here is the code for swap context: >>> int >>> swapcontext(ucontext_t *oucp, const ucontext_t *ucp) >>> { >>> int ret; >>> if ((oucp =3D=3D NULL) || (ucp =3D=3D NULL)) { >>> errno =3D EINVAL; >>> return (-1); >>> } >>> oucp->uc_flags &=3D ~UCF_SWAPPED; >>> ret =3D getcontext(oucp); >>> if ((ret =3D=3D 0) && !(oucp->uc_flags & UCF_SWAPPED)) { >>> oucp->uc_flags |=3D UCF_SWAPPED; >>> ret =3D setcontext(ucp); >>> } >>> return (ret); >>> } >>=20 >>> On the OS X port of libc in Mac OSX 10.7.5, this gets compiled as: >>=20 >>> ... >>> 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. >>=20 >> Later you wrote: >>=20 >>> 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=85.= >>=20 >> 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. >=20 > 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). >=20 > 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. >=20 > Bruce Take at setcontext:=20 (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. Brian