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

diff --git a/NEWS b/NEWS
index 5a5a0a0..b549513 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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])
 ])