Re: [Qemu-devel] [PATCH] linux-user: fix wait* syscall status returns

[Peter Maydell <peter.maydell@linaro.org>]

On 23 November 2011 20:38, Alexander Graf <agraf@suse.de> 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:
>
>  int status = 0;
>  waitpid(pid, &status, WNOHANG);
>  if (status)
>     <breakage>
>
> 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 uninitialized.
> We fail to do so today, as we proxy everything through an uninitialized status
> 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 <agraf@suse.de>
> ---
>  linux-user/syscall.c |    8 +++++++-
>  1 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_long arg1,
>  #ifdef TARGET_NR_waitpid
>     case TARGET_NR_waitpid:
>         {
> -            int status;
> +            int status = 0;
> +            if (arg2) {
> +                get_user_s32(status, arg2);
> +            }
>             ret = get_errno(waitpid(arg1, &status, arg3));
>             if (!is_error(ret) && arg2
>                 && put_user_s32(host_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 == 0 is of course the WNOHANG-and-no-child-yet case you are hitting.)

> @@ -6389,6 +6392,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                 rusage_ptr = &rusage;
>             else
>                 rusage_ptr = NULL;
> +            if (status_ptr) {
> +                get_user_s32(status, status_ptr);
> +            }
>             ret = get_errno(wait4(arg1, &status, arg3, rusage_ptr));
>             if (!is_error(ret)) {
>                 if (status_ptr) {

...and similarly here.

-- PMM