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

[Alexander Graf]

On 23.11.2011, at 22:55, Peter Maydell <address@hidden> wrote:

> On 23 November 2011 20:38, Alexander Graf <address@hidden> 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 <address@hidden>
>> ---
>>  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.

Yes. Maybe we should add a check if input_state != output_state and only then 
do the conversion?

> 
> 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.)

The man page wasn't really clear here. It sounded as if you can also get 0 as 
return value and still have status change. That's why I jumped through this 
hoop in the first place :)


Alex

> 
>> @@ -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
>