Re: [PATCH] Use getgrouplist where available.

[Jim Meyering]

James Youngman <address@hidden> wrote:
> 2008-02-18  James Youngman  <address@hidden>
>
>       * gl/m4/mgetgroups.m4: Check for getgrouplist.
>       * gl/lib/mgetgroups.c (mgetgroups): Use getgrouplist, if
>       available.
>       * TODO: Remove the item about switching to getgrouplist.
...

Hi James,

Thanks a lot for working on this.
It was long overdue.

> diff --git a/gl/lib/mgetgroups.c b/gl/lib/mgetgroups.c
> index 6a4a422..3da1f58 100644
> --- a/gl/lib/mgetgroups.c
> +++ b/gl/lib/mgetgroups.c
> @@ -23,11 +23,29 @@
...
> +      *groups = g;
> +      if (max_n_groups > InitialGroupBufSize)
> +     {
> +       return getgrouplist (username, gid, g, &max_n_groups);
> +       /* XXX: Ignoring the race with group size increase */

If/when the race hits, getgrouplist returns -1, even though *groups is
already modified, thus breaking the contract defined by the function
comments (potential leak).  So this is worth fixing.  See the additional
patch below.

> +     }
> +      else
> +     {
> +       /* smallbuf was big enough, so we already have our data */
> +       memcpy (g, smallbuf, max_n_groups * sizeof *g);
> +       return 0;

That should be "return max_n_groups;".
Returning 0 works only when smallbuf is trivially small.

> +     }
> +      /* getgrouplist failed, fall through and use getugroups instead. */
> +    }
> +  /* else no username, so fall through and use getgroups. */
> +#endif
> +
>    max_n_groups = (username
>                 ? getugroups (0, NULL, username, gid)
>                 : getgroups (0, NULL));
> @@ -52,14 +110,8 @@ mgetgroups (const char *username, gid_t gid, GETGROUPS_T 
> **groups)
>    if (max_n_groups < 0)
>        max_n_groups = 5;
>
> -  if (xalloc_oversized (max_n_groups, sizeof *g))
> -    {
> -      errno = ENOMEM;
> -      return -1;
> -    }
> -
> -  g = malloc (max_n_groups * sizeof *g);
> -  if (g == NULL)
> +  g = allocate_groupbuf (max_n_groups);
> +  if (NULL == g)

Please don't use that "(NULL == variable)" style in coreutils.
With the gcc warnings I always use, there is no need.

Here are two change sets, yours with some small modifications,
and mine to fix the remaining race/contract problem:

2008-02-22  Jim Meyering  <address@hidden>

        id: avoid race when a group is added between getgrouplist calls
        * gl/lib/mgetgroups.c (mgetgroups) [N_GROUPS_INIT]: Rename enum.
        Use a larger value.
        Update *groups only upon success.
        Iterate upon failed getgrouplist.

>From 49f7ebaac45f4d20a70c83c8302444b64259c6d3 Mon Sep 17 00:00:00 2001
From: James Youngman <address@hidden>
Date: Thu, 21 Feb 2008 15:01:15 +0100
Subject: [PATCH] id: use getgrouplist when possible

* gl/m4/mgetgroups.m4: Check for getgrouplist.
* gl/lib/mgetgroups.c (mgetgroups): Use getgrouplist, if available.
* TODO: Remove the item about switching to getgrouplist.
* NEWS: mention this

Signed-off-by: Jim Meyering <address@hidden>
---
 NEWS                |    3 ++
 TODO                |   11 --------
 gl/lib/mgetgroups.c |   67 ++++++++++++++++++++++++++++++++++++++++++++-------
 gl/m4/mgetgroups.m4 |    5 ++-
 4 files changed, 64 insertions(+), 22 deletions(-)

diff --git a/NEWS b/NEWS
index 5a5a0a0..b549513 100644
--- a/NEWS
+++ b/NEWS
@@ -6,6 +6,9 @@ GNU coreutils NEWS                                    -*- 
outline -*-

   configure --enable-no-install-program=groups now works.

+  id now uses getgrouplist, when possible.  This results in
+  much better performance when there are many users and/or groups.
+
   ls no longer segfaults on files in /proc when linked with an older version
   of libselinux.  E.g., ls -l /proc/sys would dereference a NULL pointer.

diff --git a/TODO b/TODO
index 8d53caa..4e0b298 100644
--- a/TODO
+++ b/TODO
@@ -142,17 +142,6 @@ Add a distcheck-time test to ensure that every distributed
 file is either read-only(indicating generated) or is
 version-controlled and up to date.

-Implement Ulrich Drepper's suggestion to use getgrouplist rather than
-  getugroups.  This affects both `id' and `setuidgid', but makes a big
-  difference on systems with many users and/or groups, and makes id usable
-  once again on systems where access restrictions make getugroups fail.
-  But first we'll need a run-test (either in an autoconf macro or at
-  run time) to avoid the segfault bug in libc-2.3.2's getgrouplist.
-  In that case, we'd revert to using a new (to-be-written) getgrouplist
-  module that does most of what `id' already does.  Or just avoid the
-  buggy use of getgrouplist by never passing it a buffer of length zero.
-  See http://bugzilla.redhat.com/200327
-
 remove `%s' notation (now that they're all gone, add a Makefile.maint sc_
     rule to ensure no new ones are added):
   grep -E "\`%.{,4}s'" src/*.c
diff --git a/gl/lib/mgetgroups.c b/gl/lib/mgetgroups.c
index 6a4a422..b63436a 100644
--- a/gl/lib/mgetgroups.c
+++ b/gl/lib/mgetgroups.c
@@ -23,11 +23,27 @@

 #include <unistd.h>
 #include <stdint.h>
+#include <string.h>
 #include <errno.h>
-
+#if HAVE_GETGROUPLIST
+#include <grp.h>
+#endif
 #include "getugroups.h"
 #include "xalloc.h"

+
+static void *
+allocate_groupbuf (int size)
+{
+  if (xalloc_oversized (size, sizeof (GETGROUPS_T)))
+    {
+      errno = ENOMEM;
+      return NULL;
+    }
+
+  return malloc (size * sizeof (GETGROUPS_T));
+}
+
 /* Like getugroups, but store the result in malloc'd storage.
    Set *GROUPS to the malloc'd list of all group IDs of which USERNAME
    is a member.  If GID is not -1, store it first.  GID should be the
@@ -37,12 +53,51 @@
    the number of groups.  */

 int
-mgetgroups (const char *username, gid_t gid, GETGROUPS_T **groups)
+mgetgroups (char const *username, gid_t gid, GETGROUPS_T **groups)
 {
   int max_n_groups;
   int ng;
   GETGROUPS_T *g;

+#if HAVE_GETGROUPLIST
+  /* We prefer to use getgrouplist if available, because it has better
+     performance characteristics.
+
+     In glibc 2.3.2, getgrouplist is buggy.  If you pass a zero as the
+     size of the output buffer, getgrouplist will still write to the
+     buffer.  Contrary to what some versions of the getgrouplist
+     manpage say, this doesn't happen with nonzero buffer sizes.
+     Therefore our usage here just avoids a zero sized buffer.  */
+  if (username)
+    {
+      enum { INITIAL_GROUP_BUFSIZE = 1u };
+      /* INITIAL_GROUP_BUFSIZE is initially small to ensure good test coverage 
*/
+      GETGROUPS_T smallbuf[INITIAL_GROUP_BUFSIZE];
+
+      max_n_groups = INITIAL_GROUP_BUFSIZE;
+      ng = getgrouplist (username, gid, smallbuf, &max_n_groups);
+
+      g = allocate_groupbuf (max_n_groups);
+      if (g == NULL)
+       return -1;
+
+      *groups = g;
+      if (INITIAL_GROUP_BUFSIZE < max_n_groups)
+       {
+         return getgrouplist (username, gid, g, &max_n_groups);
+         /* XXX: Ignoring the race with group size increase */
+       }
+      else
+       {
+         /* smallbuf was big enough, so we already have our data */
+         memcpy (g, smallbuf, max_n_groups * sizeof *g);
+         return 0;
+       }
+      /* getgrouplist failed, fall through and use getugroups instead. */
+    }
+  /* else no username, so fall through and use getgroups. */
+#endif
+
   max_n_groups = (username
                  ? getugroups (0, NULL, username, gid)
                  : getgroups (0, NULL));
@@ -52,13 +107,7 @@ mgetgroups (const char *username, gid_t gid, GETGROUPS_T 
**groups)
   if (max_n_groups < 0)
       max_n_groups = 5;

-  if (xalloc_oversized (max_n_groups, sizeof *g))
-    {
-      errno = ENOMEM;
-      return -1;
-    }
-
-  g = malloc (max_n_groups * sizeof *g);
+  g = allocate_groupbuf (max_n_groups);
   if (g == NULL)
     return -1;

diff --git a/gl/m4/mgetgroups.m4 b/gl/m4/mgetgroups.m4
index 8183541..da55731 100644
--- a/gl/m4/mgetgroups.m4
+++ b/gl/m4/mgetgroups.m4
@@ -1,10 +1,11 @@
-#serial 1
-dnl Copyright (C) 2007 Free Software Foundation, Inc.
+#serial 2
+dnl Copyright (C) 2007-2008 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
 dnl with or without modifications, as long as this notice is preserved.

 AC_DEFUN([gl_MGETGROUPS],
 [
+  AC_CHECK_FUNCS(getgrouplist)
   AC_LIBOBJ([mgetgroups])
 ])
--
1.5.4.2.185.gf5f8


>From e268fdf057de2d4cc7ccf52dd194ad5bf49480a6 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Fri, 22 Feb 2008 10:01:36 +0100
Subject: [PATCH] id: avoid race when a group is added between getgrouplist calls

* gl/lib/mgetgroups.c (mgetgroups) [N_GROUPS_INIT]: Rename enum.
Use a larger value.
Update *groups only upon success.
Iterate upon failed getgrouplist.

Signed-off-by: Jim Meyering <address@hidden>
---
 gl/lib/mgetgroups.c |   45 +++++++++++++++++++++++++++++++--------------
 1 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/gl/lib/mgetgroups.c b/gl/lib/mgetgroups.c
index b63436a..317cc7c 100644
--- a/gl/lib/mgetgroups.c
+++ b/gl/lib/mgetgroups.c
@@ -26,7 +26,7 @@
 #include <string.h>
 #include <errno.h>
 #if HAVE_GETGROUPLIST
-#include <grp.h>
+# include <grp.h>
 #endif
 #include "getugroups.h"
 #include "xalloc.h"
@@ -70,30 +70,47 @@ mgetgroups (char const *username, gid_t gid, GETGROUPS_T 
**groups)
      Therefore our usage here just avoids a zero sized buffer.  */
   if (username)
     {
-      enum { INITIAL_GROUP_BUFSIZE = 1u };
-      /* INITIAL_GROUP_BUFSIZE is initially small to ensure good test coverage 
*/
-      GETGROUPS_T smallbuf[INITIAL_GROUP_BUFSIZE];
+      enum { N_GROUPS_INIT = 10 };
+      GETGROUPS_T smallbuf[N_GROUPS_INIT];

-      max_n_groups = INITIAL_GROUP_BUFSIZE;
+      max_n_groups = N_GROUPS_INIT;
       ng = getgrouplist (username, gid, smallbuf, &max_n_groups);

       g = allocate_groupbuf (max_n_groups);
       if (g == NULL)
        return -1;

-      *groups = g;
-      if (INITIAL_GROUP_BUFSIZE < max_n_groups)
-       {
-         return getgrouplist (username, gid, g, &max_n_groups);
-         /* XXX: Ignoring the race with group size increase */
-       }
-      else
+      if (max_n_groups <= N_GROUPS_INIT)
        {
          /* smallbuf was big enough, so we already have our data */
          memcpy (g, smallbuf, max_n_groups * sizeof *g);
-         return 0;
+         *groups = g;
+         return max_n_groups;
+       }
+
+      while (1)
+       {
+         GETGROUPS_T *h;
+         ng = getgrouplist (username, gid, g, &max_n_groups);
+         if (0 <= ng)
+           {
+             *groups = g;
+             return ng;
+           }
+
+         /* When getgrouplist fails, it guarantees that
+            max_n_groups reflects the new number of groups.  */
+
+         h = realloc (g, max_n_groups * sizeof *h);
+         if (h == NULL)
+           {
+             int saved_errno = errno;
+             free (g);
+             errno = saved_errno;
+             return -1;
+           }
+         g = h;
        }
-      /* getgrouplist failed, fall through and use getugroups instead. */
     }
   /* else no username, so fall through and use getgroups. */
 #endif
--
1.5.4.2.185.gf5f8