[Qemu-devel] [PATCH] linux-user, fix getgroups/getgroups32 when both args are zero.

[Qemu-devel] [PATCH] linux-user, fix getgroups/getgroups32 when both args are zero.

[Takashi Yoshii]

getgroups() and getgroups32() returns NGROUPS_MAX when both its two args are
zero. But because we pass a ptr to allocated space as 2nd arg, this function
are interfered. The patch attached fixed it.
/yoshii
---
    linux-user/syscall.c: fix getgroups{,32} when both args are zero.

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -5185,7 +5185,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         ret = get_errno(setregid(low2highgid(arg1), low2highgid(arg2)));
         break;
     case TARGET_NR_getgroups:
-        {
+        if (arg1 == 0 && arg2 == 0)
+            ret = get_errno(getgroups(0, 0));
+        else {
             int gidsetsize = arg1;
             uint16_t *target_grouplist;
             gid_t *grouplist;
@@ -5335,7 +5337,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 #endif
 #ifdef TARGET_NR_getgroups32
     case TARGET_NR_getgroups32:
-        {
+        if (arg1 == 0 && arg2 == 0)
+            ret = get_errno(getgroups(0, 0));
+        else {
             int gidsetsize = arg1;
             uint32_t *target_grouplist;
             gid_t *grouplist;

Re: [Qemu-devel] [PATCH] linux-user, fix getgroups/getgroups32 when both args are zero.

[Kirill A. Shutemov]

[-- Attachment #1: Type: text/plain, Size: 2922 bytes --]

On [Sat, 23.02.2008 12:00], Takashi Yoshii wrote:
> getgroups() and getgroups32() returns NGROUPS_MAX when both its two args are
> zero. But because we pass a ptr to allocated space as 2nd arg, this function
> are interfered. The patch attached fixed it.
> /yoshii
> ---
>     linux-user/syscall.c: fix getgroups{,32} when both args are zero.
> 

My version of patch to fix same problem:

    getgroups: return total number of supplementary group IDs for the
    process if size == 0

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index ad97871..96a11a9 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -5029,6 +5029,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 
             grouplist = alloca(gidsetsize * sizeof(gid_t));
             ret = get_errno(getgroups(gidsetsize, grouplist));
+            if (gidsetsize == 0)
+                break;
             if (!is_error(ret)) {
                 target_grouplist = lock_user(VERIFY_WRITE, arg2, gidsetsize * 2, 0);
                 if (!target_grouplist)
@@ -5179,6 +5181,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 
             grouplist = alloca(gidsetsize * sizeof(gid_t));
             ret = get_errno(getgroups(gidsetsize, grouplist));
+            if (gidsetsize == 0)
+                break;
             if (!is_error(ret)) {
                 target_grouplist = lock_user(VERIFY_WRITE, arg2, gidsetsize * 4, 0);
                 if (!target_grouplist) {

Another patch for getgroups:

    Trivial optimization of getgroups syscall implementation:
    swap only returned groups, not all group in list.

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 6f2872f..ad97871 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -5033,7 +5033,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                 target_grouplist = lock_user(VERIFY_WRITE, arg2, gidsetsize * 2, 0);
                 if (!target_grouplist)
                     goto efault;
-                for(i = 0;i < gidsetsize; i++)
+                for(i = 0;i < ret; i++)
                     target_grouplist[i] = tswap16(grouplist[i]);
                 unlock_user(target_grouplist, arg2, gidsetsize * 2);
             }
@@ -5185,7 +5185,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                     ret = -TARGET_EFAULT;
                     goto fail;
                 }
-                for(i = 0;i < gidsetsize; i++)
+                for(i = 0;i < ret; i++)
                     target_grouplist[i] = tswap32(grouplist[i]);
                 unlock_user(target_grouplist, arg2, gidsetsize * 4);
             }

It makes getgroups much faster in some cases.

-- 
Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + Velesys Ltd, http://www.velesys.com/
 + ALT Linux Team, http://www.altlinux.com/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [Qemu-devel] [PATCH] linux-user, fix getgroups/getgroups32 when both args are zero.

[Takashi Yoshii]

Hi,

> My version of patch to fix same problem:
Oh, yes. You are right.
I have read manpage again, and I found it says,
  if size is zero, list is not modified, but ...
So, mine is wrong. We should check only "size".
/yoshii

Re: [Qemu-devel] [PATCH] linux-user, fix getgroups/getgroups32 when both args are zero.

[Kirill A. Shutemov]

[-- Attachment #1: Type: text/plain, Size: 3135 bytes --]

On [Sat, 23.02.2008 09:25], Kirill A. Shutemov wrote:
> On [Sat, 23.02.2008 12:00], Takashi Yoshii wrote:
> > getgroups() and getgroups32() returns NGROUPS_MAX when both its two args are
> > zero. But because we pass a ptr to allocated space as 2nd arg, this function
> > are interfered. The patch attached fixed it.
> > /yoshii
> > ---
> >     linux-user/syscall.c: fix getgroups{,32} when both args are zero.
> > 
> 
> My version of patch to fix same problem:
> 
>     getgroups: return total number of supplementary group IDs for the
>     process if size == 0
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index ad97871..96a11a9 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -5029,6 +5029,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>  
>              grouplist = alloca(gidsetsize * sizeof(gid_t));
>              ret = get_errno(getgroups(gidsetsize, grouplist));
> +            if (gidsetsize == 0)
> +                break;
>              if (!is_error(ret)) {
>                  target_grouplist = lock_user(VERIFY_WRITE, arg2, gidsetsize * 2, 0);
>                  if (!target_grouplist)
> @@ -5179,6 +5181,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>  
>              grouplist = alloca(gidsetsize * sizeof(gid_t));
>              ret = get_errno(getgroups(gidsetsize, grouplist));
> +            if (gidsetsize == 0)
> +                break;
>              if (!is_error(ret)) {
>                  target_grouplist = lock_user(VERIFY_WRITE, arg2, gidsetsize * 4, 0);
>                  if (!target_grouplist) {
> 
> Another patch for getgroups:
> 
>     Trivial optimization of getgroups syscall implementation:
>     swap only returned groups, not all group in list.
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 6f2872f..ad97871 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -5033,7 +5033,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                  target_grouplist = lock_user(VERIFY_WRITE, arg2, gidsetsize * 2, 0);
>                  if (!target_grouplist)
>                      goto efault;
> -                for(i = 0;i < gidsetsize; i++)
> +                for(i = 0;i < ret; i++)
>                      target_grouplist[i] = tswap16(grouplist[i]);
>                  unlock_user(target_grouplist, arg2, gidsetsize * 2);
>              }
> @@ -5185,7 +5185,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                      ret = -TARGET_EFAULT;
>                      goto fail;
>                  }
> -                for(i = 0;i < gidsetsize; i++)
> +                for(i = 0;i < ret; i++)
>                      target_grouplist[i] = tswap32(grouplist[i]);
>                  unlock_user(target_grouplist, arg2, gidsetsize * 4);
>              }
> 
> It makes getgroups much faster in some cases.

Can anybody commit it?

-- 
Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + Velesys Ltd, http://www.velesys.com/
 + ALT Linux Team, http://www.altlinux.com/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]