Re: Problem with id when mgetgroups returns no groups.

[Eric Blake]

Eric Blake <ebb9 <at> byu.net> writes:

> Thanks for your report.  Indeed, looking at mgetgroups.c, if getgroups
> fails, mgetgroups returns -1 without assigning through *groups (likewise
> if realloc_groupbug fails, but that sets errno to ENOMEM).  I also wonder
> if mgetgroups should be taught to recognize ENOSYS itself, and guarantee a
> zero-length array rather than returning with -1 in that case.

I spotted another problem - id was not handling ENOMEM failures consistently 
(in other words, it didn't go through xalloc_die).

How about this alternative patch, which makes gnulib aware of ENOSYS, and 
changes coreutils to use the more robust interface?


From: Eric Blake <address@hidden>
Date: Fri, 4 Dec 2009 08:26:23 -0700
Subject: [PATCH] mgetgroups: add xgetgroups, and avoid ENOSYS failures

* lib/mgetgroups.h (xgetgroups): New prototype.
* lib/mgetgroups.c (xgetgroups): New wrapper.
(mgetgroups): Handle ENOSYS.
* modules/mgetgroups (Depends-on): Add realloc-posix.
Reported by Scott <scott.gnu.2009 AT scottrix.co.uk>.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog          |    8 ++++++++
 lib/mgetgroups.c   |   27 ++++++++++++++++++++++++---
 lib/mgetgroups.h   |    1 +
 modules/mgetgroups |    1 +
 4 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index f913f41..319f3c1 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2009-12-04  Eric Blake  <address@hidden>
+
+       mgetgroups: add xgetgroups, and avoid ENOSYS failures
+       * lib/mgetgroups.h (xgetgroups): New prototype.
+       * lib/mgetgroups.c (xgetgroups): New wrapper.
+       (mgetgroups): Handle ENOSYS.
+       * modules/mgetgroups (Depends-on): Add realloc-posix.
+
 2009-11-17  Eric Blake  <address@hidden>

        manywarnings: add more warnings
diff --git a/lib/mgetgroups.c b/lib/mgetgroups.c
index f68e28f..3c5ec8d 100644
--- a/lib/mgetgroups.c
+++ b/lib/mgetgroups.c
@@ -118,10 +118,19 @@ mgetgroups (char const *username, gid_t gid, gid_t 
**groups)
                   ? getugroups (0, NULL, username, gid)
                   : getgroups (0, NULL) + (gid != -1));

-  /* If we failed to count groups with NULL for a buffer,
-     try again with a non-NULL one, just in case.  */
+  /* If we failed to count groups because there is no supplemental
+     group support, then return an array containing just GID.
+     Otherwise, we fail for the same reason.  */
   if (max_n_groups < 0)
-      max_n_groups = 5;
+    {
+      if (errno == ENOSYS && (g = realloc_groupbuf (NULL, 1)))
+        {
+          *groups = g;
+          *g = gid;
+          return gid != -1;
+        }
+      return -1;
+    }

   g = realloc_groupbuf (NULL, max_n_groups);
   if (g == NULL)
@@ -133,6 +142,7 @@ mgetgroups (char const *username, gid_t gid, gid_t **groups)

   if (ng < 0)
     {
+      /* Failure is unexpected, but handle it anyway.  */
       int saved_errno = errno;
       free (g);
       errno = saved_errno;
@@ -147,3 +157,14 @@ mgetgroups (char const *username, gid_t gid, gid_t 
**groups)
   *groups = g;
   return ng;
 }
+
+/* Like mgetgroups, but call xalloc_die on allocation failure.  */
+
+int
+xgetgroups (char const *username, gid_t gid, gid_t **groups)
+{
+  int result = mgetgroups (username, gid, groups);
+  if (result == -1 && errno == ENOMEM)
+    xalloc_die ();
+  return result;
+}
diff --git a/lib/mgetgroups.h b/lib/mgetgroups.h
index 909d84c..4868d28 100644
--- a/lib/mgetgroups.h
+++ b/lib/mgetgroups.h
@@ -17,3 +17,4 @@
 #include <sys/types.h>

 int mgetgroups (const char *username, gid_t gid, gid_t **groups);
+int xgetgroups (const char *username, gid_t gid, gid_t **groups);
diff --git a/modules/mgetgroups b/modules/mgetgroups
index 58ef740..45ae412 100644
--- a/modules/mgetgroups
+++ b/modules/mgetgroups
@@ -9,6 +9,7 @@ m4/mgetgroups.m4
 Depends-on:
 getgroups
 getugroups
+realloc-posix
 xalloc

 configure.ac:
-- 
1.6.4.2




From: Eric Blake <address@hidden>
Date: Fri, 4 Dec 2009 08:06:55 -0700
Subject: [PATCH] id: handle systems without getgroups support

If getgroups failed with ENOSYS, mgetgroups would unnecessarily
fail, and that provoked id into freeing an uninitialized pointer.
Meanwhile, we were not using xalloc_die properly.  Both issues
are better solved in gnulib, by introducing xgetgroups; this
patch uses the new interface.

* gnulib: Update, for mgetgroups improvments.
* src/id.c (print_full_info): Adjust caller to die on allocation
failure, and no longer worry about ENOSYS.
* src/group-list.c (print_group_list): Likewise.
* src/setuidgid.c (main): Likewise.
---
 gnulib           |    2 +-
 src/group-list.c |    4 ++--
 src/id.c         |    4 ++--
 src/setuidgid.c  |    2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/gnulib b/gnulib
index cdfe647..c5588be 160000
--- a/gnulib
+++ b/gnulib
@@ -1 +1 @@
-Subproject commit cdfe647f8d29540cdfe90cef0fa568c5d2fd4481
+Subproject commit 64a72bbf6ed20208d633b9e9d849000aee279a01
diff --git a/src/group-list.c b/src/group-list.c
index 1fadd0c..a4b1f6d 100644
--- a/src/group-list.c
+++ b/src/group-list.c
@@ -58,9 +58,9 @@ print_group_list (const char *username,
     gid_t *groups;
     int i;

-    int n_groups = mgetgroups (username, (pwd ? pwd->pw_gid : (gid_t) -1),
+    int n_groups = xgetgroups (username, (pwd ? pwd->pw_gid : (gid_t) -1),
                                &groups);
-    if (n_groups < 0 && errno != ENOSYS)
+    if (n_groups < 0)
       {
         if (username)
           {
diff --git a/src/id.c b/src/id.c
index 9a00f5c..96d8e96 100644
--- a/src/id.c
+++ b/src/id.c
@@ -296,9 +296,9 @@ print_full_info (const char *username)
     gid_t *groups;
     int i;

-    int n_groups = mgetgroups (username, (pwd ? pwd->pw_gid : (gid_t) -1),
+    int n_groups = xgetgroups (username, (pwd ? pwd->pw_gid : (gid_t) -1),
                                &groups);
-    if (n_groups < 0 && errno != ENOSYS)
+    if (n_groups < 0)
       {
         if (username)
           {
diff --git a/src/setuidgid.c b/src/setuidgid.c
index 0adac21..2710bc9 100644
--- a/src/setuidgid.c
+++ b/src/setuidgid.c
@@ -179,7 +179,7 @@ main (int argc, char **argv)
 #if HAVE_SETGROUPS
     if (n_gids == 0)
       {
-        int n = mgetgroups (pwd->pw_name, pwd->pw_gid, &gids);
+        int n = xgetgroups (pwd->pw_name, pwd->pw_gid, &gids);
         if (n <= 0)
           error (EXIT_FAILURE, errno, _("failed to get groups for user %s"),
                  quote (pwd->pw_name));
-- 
1.6.4.2