James Youngman <[EMAIL PROTECTED]> wrote:
> 2008-02-18 James Youngman <[EMAIL PROTECTED]>
>
> * 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 <[EMAIL PROTECTED]>
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 <[EMAIL PROTECTED]>
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 <[EMAIL PROTECTED]>
---
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 <[EMAIL PROTECTED]>
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 <[EMAIL PROTECTED]>
---
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
_______________________________________________
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils
