On 23.11.2011, at 22:55, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 23 November 2011 20:38, Alexander Graf <ag...@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 <ag...@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.
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
>
