bug-coreutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[PATCH] Use getgrouplist where available.


From: James Youngman
Subject: [PATCH] Use getgrouplist where available.
Date: Mon, 18 Feb 2008 01:26:08 +0000

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.

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

diff --git a/TODO b/TODO
index 8c6b6fc..ae40efe 100644
--- a/TODO
+++ b/TODO
@@ -146,17 +146,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..3da1f58 100644
--- a/gl/lib/mgetgroups.c
+++ b/gl/lib/mgetgroups.c
@@ -23,11 +23,29 @@
 
 #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;
+    }
+  else
+    {
+      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
@@ -43,6 +61,46 @@ mgetgroups (const char *username, gid_t gid, GETGROUPS_T 
**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 { InitialGroupBufSize=1u };
+      /* InitialGroupBufSize is initially small to ensure good test coverage */
+      GETGROUPS_T smallbuf[InitialGroupBufSize];
+
+      max_n_groups = InitialGroupBufSize;
+      ng = getgrouplist (username, gid, smallbuf, &max_n_groups);
+
+      g = allocate_groupbuf (max_n_groups);
+      if (NULL == g)
+       return -1;
+      
+      *groups = g;
+      if (max_n_groups > InitialGroupBufSize)
+       {
+         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,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)
     return -1;
 
   ng = (username
diff --git a/gl/m4/mgetgroups.m4 b/gl/m4/mgetgroups.m4
index 8183541..0cc9ae8 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.3.8





reply via email to

[Prev in Thread] Current Thread [Next in Thread]