[Qemu-devel] [PATCH] linux-user: fix mq_getsetattr implementation

[Qemu-devel] [PATCH] linux-user: fix mq_getsetattr implementation

[Max Filippov]

mq_getsetattr implementation does not set errno correctly in case of
error. Also in the presence of both 2nd and 3rd arguments it calls both
mq_getattr and mq_setattr, whereas only the latter call would suffice.

Don't call mq_getattr in the presence of the 3rd argument. Use get_errno
to set errno value.

This fixes test rt/tst-mqueue2 from the glibc testsuite.

Cc: Lionel Landwerlin <lionel.landwerlin@openwide.fr>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Riku Voipio <riku.voipio@iki.fi>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Laurent Vivier <laurent@vivier.eu>
Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 linux-user/syscall.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 18ea79140f16..8b364551c311 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -12092,15 +12092,16 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         {
             struct mq_attr posix_mq_attr_in, posix_mq_attr_out;
             ret = 0;
-            if (arg3 != 0) {
-                ret = mq_getattr(arg1, &posix_mq_attr_out);
-                copy_to_user_mq_attr(arg3, &posix_mq_attr_out);
-            }
             if (arg2 != 0) {
                 copy_from_user_mq_attr(&posix_mq_attr_in, arg2);
-                ret |= mq_setattr(arg1, &posix_mq_attr_in, &posix_mq_attr_out);
+                ret = mq_setattr(arg1, &posix_mq_attr_in, &posix_mq_attr_out);
+            } else if (arg3 != 0) {
+                ret = mq_getattr(arg1, &posix_mq_attr_out);
             }
-
+            if (arg3 != 0) {
+                copy_to_user_mq_attr(arg3, &posix_mq_attr_out);
+            }
+            ret = get_errno(ret);
         }
         break;
 #endif
-- 
2.11.0

Re: [Qemu-devel] [PATCH] linux-user: fix mq_getsetattr implementation

[Laurent Vivier]

Le 31/03/2018 à 18:23, Max Filippov a écrit :
> mq_getsetattr implementation does not set errno correctly in case of
> error. Also in the presence of both 2nd and 3rd arguments it calls both
> mq_getattr and mq_setattr, whereas only the latter call would suffice.
> 
> Don't call mq_getattr in the presence of the 3rd argument. Use get_errno
> to set errno value.
> 
> This fixes test rt/tst-mqueue2 from the glibc testsuite.
> 
> Cc: Lionel Landwerlin <lionel.landwerlin@openwide.fr>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Riku Voipio <riku.voipio@iki.fi>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Laurent Vivier <laurent@vivier.eu>
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> ---
>  linux-user/syscall.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 18ea79140f16..8b364551c311 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -12092,15 +12092,16 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>          {
>              struct mq_attr posix_mq_attr_in, posix_mq_attr_out;
>              ret = 0;

you can remove this line

> -            if (arg3 != 0) {
> -                ret = mq_getattr(arg1, &posix_mq_attr_out);
> -                copy_to_user_mq_attr(arg3, &posix_mq_attr_out);
> -            }
>              if (arg2 != 0) {
>                  copy_from_user_mq_attr(&posix_mq_attr_in, arg2);
> -                ret |= mq_setattr(arg1, &posix_mq_attr_in, &posix_mq_attr_out);
> +                ret = mq_setattr(arg1, &posix_mq_attr_in, &posix_mq_attr_out);

you can call get_errno(mq_setattr(..)) here...

> +            } else if (arg3 != 0) {
> +                ret = mq_getattr(arg1, &posix_mq_attr_out);

and get_errno(q_getattr(..)) here.

>              }
> -
> +            if (arg3 != 0) {

you should also check ret < 0 before copying to arg3

> +                copy_to_user_mq_attr(arg3, &posix_mq_attr_out);
> +            }
> +            ret = get_errno(ret);

you can remove this line if you call get_errno() directly before.


>          }
>          break;
>  #endif
> 

Thanks,
Laurent

Re: [Qemu-devel] [PATCH] linux-user: fix mq_getsetattr implementation

[Laurent Vivier]

Le 31/03/2018 à 18:57, Laurent Vivier a écrit :
> Le 31/03/2018 à 18:23, Max Filippov a écrit :
>> mq_getsetattr implementation does not set errno correctly in case of
>> error. Also in the presence of both 2nd and 3rd arguments it calls both
>> mq_getattr and mq_setattr, whereas only the latter call would suffice.
>>
>> Don't call mq_getattr in the presence of the 3rd argument. Use get_errno
>> to set errno value.
>>
>> This fixes test rt/tst-mqueue2 from the glibc testsuite.
>>
>> Cc: Lionel Landwerlin <lionel.landwerlin@openwide.fr>
>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> Cc: Riku Voipio <riku.voipio@iki.fi>
>> Cc: Aurelien Jarno <aurelien@aurel32.net>
>> Cc: Laurent Vivier <laurent@vivier.eu>
>> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
>> ---
>>  linux-user/syscall.c | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 18ea79140f16..8b364551c311 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -12092,15 +12092,16 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>>          {
>>              struct mq_attr posix_mq_attr_in, posix_mq_attr_out;
>>              ret = 0;
> 
> you can remove this line

in fact, you must keep this line because ret is not update if arg2 ==
NULL and arg3 == NULL.

Thanks,
Laurent