From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:37716) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RTKn7-0000jm-BY for qemu-devel@nongnu.org; Wed, 23 Nov 2011 16:55:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RTKn6-0004Eq-5d for qemu-devel@nongnu.org; Wed, 23 Nov 2011 16:55:41 -0500 Received: from mail-vx0-f173.google.com ([209.85.220.173]:38301) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RTKn6-0004Em-3G for qemu-devel@nongnu.org; Wed, 23 Nov 2011 16:55:40 -0500 Received: by vcbfl11 with SMTP id fl11so1973751vcb.4 for ; Wed, 23 Nov 2011 13:55:39 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <1322080707-13527-1-git-send-email-agraf@suse.de> References: <1322080707-13527-1-git-send-email-agraf@suse.de> Date: Wed, 23 Nov 2011 21:55:38 +0000 Message-ID: From: Peter Maydell Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] linux-user: fix wait* syscall status returns List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: riku.voipio@iki.fi, qemu-devel Developers On 23 November 2011 20:38, Alexander Graf wrote: > When calling wait4 or waitpid with a status pointer and WNOHANG, the > syscall can potentially not modify the status pointer input. Now if we > have guest code like: > > =C2=A0int status =3D 0; > =C2=A0waitpid(pid, &status, WNOHANG); > =C2=A0if (status) > =C2=A0 =C2=A0 > > then we have to make sure that in case status did not change we actually > return the guest's initialized status variable instead of our own uniniti= alized. > We fail to do so today, as we proxy everything through an uninitialized s= tatus > variable which for me ended up always containing the last error code. > > This patch fixes some test cases when building yast2-core in OBS for ARM. > > Signed-off-by: Alexander Graf > --- > =C2=A0linux-user/syscall.c | =C2=A0 =C2=A08 +++++++- > =C2=A01 files changed, 7 insertions(+), 1 deletions(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 3e6f3bd..f86fe4a 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -4833,7 +4833,10 @@ abi_long do_syscall(void *cpu_env, int num, abi_lo= ng arg1, > =C2=A0#ifdef TARGET_NR_waitpid > =C2=A0 =C2=A0 case TARGET_NR_waitpid: > =C2=A0 =C2=A0 =C2=A0 =C2=A0 { > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0int status; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0int status =3D 0; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (arg2) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0get_user_s32(sta= tus, arg2); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ret =3D get_errno(waitpid(arg1,= &status, arg3)); > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!is_error(ret) && arg2 > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 && put_user_s32(h= ost_to_target_waitstatus(status), arg2)) If the problem is that waitpid() can return success without writing to status, then this code is still not right because we will get the initial target waitstatus into status, and then pass it through host_to_target_waitstatus(), possibly modifying it, before writing it back to guest memory. I think waitpid() will always and only write to status if the return value is > 0 (ie it's a PID, not 0 or -1). So I think the right fix for this problem is to have the if() protecting the put_user_s32() read "if (ret && !is_error(ret) && ...". (ret =3D=3D 0 is of course the WNOHANG-and-no-child-yet case you are hittin= g.) > @@ -6389,6 +6392,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_lon= g arg1, > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 rusage_ptr =3D &r= usage; > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 else > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 rusage_ptr =3D NU= LL; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (status_ptr) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0get_user_s32(sta= tus, status_ptr); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ret =3D get_errno(wait4(arg1, &= status, arg3, rusage_ptr)); > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!is_error(ret)) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (status_ptr) { ...and similarly here. -- PMM