bug-gnulib
[Top][All Lists]
Advanced

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

mgetgroups improvements


From: Eric Blake
Subject: mgetgroups improvements
Date: Fri, 4 Dec 2009 22:17:31 +0000 (UTC)
User-agent: Loom/3.14 (http://gmane.org/)

The first was a bug I introduced when moving mgetgroups from coreutils to 
gnulib (comparing a gid_t with -1 will fail on platforms where gid_t is 
uint16_t; the coreutils version only used the gid argument in the getugroups 
case, so the bug came from my new code for the getgroups case).  I didn't 
notice it earlier because cygwin has 32-bit gid_t.  The second of these was 
already proposed on the coreutils list, but has been further tweaked to fix 
minor issues I've spotted during a re-review of the patch.  And the third is 
something I've previously talked about, which makes mgetgroups easier to use by 
avoiding obvious duplicates with no degredation in scaling effects.

Duplicates can still exist depending on the OS implementation of getgroups: for 
example, on one cygwin machine, I see:

$ printf %s\\n `id -G` | wc
     46      46     264
$ printf %s\\n `id -G` | sort -u | wc
     24      24     136

even after this patch.


Eric Blake (3):
      mgetgroups: avoid argument promotion issues with -1
      mgetgroups: add xgetgroups, and avoid ENOSYS failures
      mgetgroups: reduce duplicate listings



>From 15bc4cfc5a96b5619f63f7d239ee6f518cca852a Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Fri, 4 Dec 2009 14:37:32 -0700
Subject: [PATCH 1/3] mgetgroups: avoid argument promotion issues with -1

On platforms where gid_t is equivalent to uint16_t, argument
promotion states that -1 != (gid_t) -1.

* lib/mgetgroups.c (mgetgroups): A cast is required when checking
for invalid gid_t.
* tests/test-chown.h (getegid, test_chown): Likewise.
* tests/test-lchown.h (getegid, test_lchown): Likewise.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog           |    8 ++++++++
 lib/mgetgroups.c    |    6 +++---
 tests/test-chown.h  |    9 +++++----
 tests/test-lchown.h |    9 +++++----
 4 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index da260c1..15bdded 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2009-12-04  Eric Blake  <address@hidden>
+
+       mgetgroups: avoid argument promotion issues with -1
+       * lib/mgetgroups.c (mgetgroups): A cast is required when checking
+       for invalid gid_t.
+       * tests/test-chown.h (getegid, test_chown): Likewise.
+       * tests/test-lchown.h (getegid, test_lchown): Likewise.
+
 2009-12-03  Paolo Bonzini  <address@hidden>

        exclude: Fix header file problems.
diff --git a/lib/mgetgroups.c b/lib/mgetgroups.c
index f68e28f..9a733d5 100644
--- a/lib/mgetgroups.c
+++ b/lib/mgetgroups.c
@@ -116,7 +116,7 @@ mgetgroups (char const *username, gid_t gid, gid_t

   max_n_groups = (username
                   ? getugroups (0, NULL, username, gid)
-                  : getgroups (0, NULL) + (gid != -1));
+                  : getgroups (0, NULL) + (gid != (gid_t) -1));

   /* If we failed to count groups with NULL for a buffer,
      try again with a non-NULL one, just in case.  */
@@ -129,7 +129,7 @@ mgetgroups (char const *username, gid_t gid, gid_t

   ng = (username
         ? getugroups (max_n_groups, g, username, gid)
-        : getgroups (max_n_groups, g + (gid != -1)));
+        : getgroups (max_n_groups, g + (gid != (gid_t) -1)));

   if (ng < 0)
     {
@@ -139,7 +139,7 @@ mgetgroups (char const *username, gid_t gid, gid_t
       return -1;
     }

-  if (!username && gid != -1)
+  if (!username && gid != (gid_t) -1)
     {
       *g = gid;
       ng++;
diff --git a/tests/test-chown.h b/tests/test-chown.h
index ab98682..098b77d 100644
--- a/tests/test-chown.h
+++ b/tests/test-chown.h
@@ -62,7 +62,7 @@ nap (void)
 }

 #if !HAVE_GETEGID
-# define getegid() (-1)
+# define getegid() ((gid_t) -1)
 #endif

 /* This file is designed to test chown(n,o,g) and
@@ -113,8 +113,8 @@ test_chown (int (*func) (char const *, uid_t, gid_t),

   ASSERT (close (creat (BASE "dir/file", 0600)) == 0);
   ASSERT (stat (BASE "dir/file", &st1) == 0);
-  ASSERT (st1.st_uid != -1);
-  ASSERT (st1.st_gid != -1);
+  ASSERT (st1.st_uid != (uid_t) -1);
+  ASSERT (st1.st_gid != (uid_t) -1);
   ASSERT (st1.st_gid == getegid ());

   /* Sanity check of error cases.  */
@@ -134,6 +134,7 @@ test_chown (int (*func) (char const *, uid_t, gid_t),
   /* Check that -1 does not alter ownership.  */
   ASSERT (func (BASE "dir/file", -1, st1.st_gid) == 0);
   ASSERT (func (BASE "dir/file", st1.st_uid, -1) == 0);
+  ASSERT (func (BASE "dir/file", (uid_t) -1, (gid_t) -1) == 0);
   ASSERT (stat (BASE "dir/file", &st2) == 0);
   ASSERT (st1.st_uid == st2.st_uid);
   ASSERT (st1.st_gid == st2.st_gid);
@@ -183,7 +184,7 @@ test_chown (int (*func) (char const *, uid_t, gid_t),
           gids[0] = gids[1];
         }
       ASSERT (gids[0] != st1.st_gid);
-      ASSERT (gids[0] != -1);
+      ASSERT (gids[0] != (gid_t) -1);
       ASSERT (lstat (BASE "dir/link", &st2) == 0);
       ASSERT (st1.st_uid == st2.st_uid);
       ASSERT (st1.st_gid == st2.st_gid);
diff --git a/tests/test-lchown.h b/tests/test-lchown.h
index f3ddb7a..43690ac 100644
--- a/tests/test-lchown.h
+++ b/tests/test-lchown.h
@@ -63,7 +63,7 @@ nap (void)
 #endif /* !TEST_CHOWN_NAP */

 #if !HAVE_GETEGID
-# define getegid() (-1)
+# define getegid() ((gid_t) -1)
 #endif

 #ifndef HAVE_LCHMOD
@@ -122,8 +122,8 @@ test_lchown (int (*func) (char const *, uid_t, gid_t),

   ASSERT (close (creat (BASE "dir/file", 0600)) == 0);
   ASSERT (stat (BASE "dir/file", &st1) == 0);
-  ASSERT (st1.st_uid != -1);
-  ASSERT (st1.st_gid != -1);
+  ASSERT (st1.st_uid != (uid_t) -1);
+  ASSERT (st1.st_gid != (gid_t) -1);
   ASSERT (st1.st_gid == getegid ());

   /* Sanity check of error cases.  */
@@ -143,6 +143,7 @@ test_lchown (int (*func) (char const *, uid_t, gid_t),
   /* Check that -1 does not alter ownership.  */
   ASSERT (func (BASE "dir/file", -1, st1.st_gid) == 0);
   ASSERT (func (BASE "dir/file", st1.st_uid, -1) == 0);
+  ASSERT (func (BASE "dir/file", (uid_t) -1, (gid_t) -1) == 0);
   ASSERT (stat (BASE "dir/file", &st2) == 0);
   ASSERT (st1.st_uid == st2.st_uid);
   ASSERT (st1.st_gid == st2.st_gid);
@@ -202,7 +203,7 @@ test_lchown (int (*func) (char const *, uid_t, gid_t),
           gids[0] = gids[1];
         }
       ASSERT (gids[0] != st1.st_gid);
-      ASSERT (gids[0] != -1);
+      ASSERT (gids[0] != (gid_t) -1);
       ASSERT (lstat (BASE "dir/link", &st2) == 0);
       ASSERT (st1.st_uid == st2.st_uid);
       ASSERT (st1.st_gid == st2.st_gid);
-- 
1.6.4.2


>From 3bdaa607253bb67b637763f4411057736cef1b2b Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Fri, 4 Dec 2009 08:26:23 -0700
Subject: [PATCH 2/3] mgetgroups: add xgetgroups, and avoid ENOSYS failures

ENOSYS implies that there are no supplemental groups, so we can
treat it the same as a return of 0 from getgroups rather than
exposing failure to the user.  This in turn fixes a crash in
coreutils' id, which freed an uninitialized pointer.

* lib/mgetgroups.h (xgetgroups): New prototype.
* lib/mgetgroups.c (xgetgroups): New wrapper.
(mgetgroups): Handle ENOSYS.
* modules/mgetgroups (Depends-on): Add realloc.
Reported by Scott Harrison <address@hidden>.

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

diff --git a/ChangeLog b/ChangeLog
index 15bdded..48e1026 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@
 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.
+       Reported by Scott Harrison <address@hidden>.
+
        mgetgroups: avoid argument promotion issues with -1
        * lib/mgetgroups.c (mgetgroups): A cast is required when checking
        for invalid gid_t.
diff --git a/lib/mgetgroups.c b/lib/mgetgroups.c
index 9a733d5..7a61db4 100644
--- a/lib/mgetgroups.c
+++ b/lib/mgetgroups.c
@@ -116,13 +116,24 @@ mgetgroups (char const *username, gid_t gid, gid_t

   max_n_groups = (username
                   ? getugroups (0, NULL, username, gid)
-                  : getgroups (0, NULL) + (gid != (gid_t) -1));
+                  : getgroups (0, NULL));

-  /* 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 != (gid_t) -1;
+        }
+      return -1;
+    }

+  if (!username && gid != (gid_t) -1)
+    max_n_groups++;
   g = realloc_groupbuf (NULL, max_n_groups);
   if (g == NULL)
     return -1;
@@ -133,6 +144,7 @@ mgetgroups (char const *username, gid_t gid, gid_t

   if (ng < 0)
     {
+      /* Failure is unexpected, but handle it anyway.  */
       int saved_errno = errno;
       free (g);
       errno = saved_errno;
@@ -147,3 +159,14 @@ mgetgroups (char const *username, gid_t gid, gid_t
   *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..cd249db 100644
--- a/modules/mgetgroups
+++ b/modules/mgetgroups
@@ -9,6 +9,7 @@ m4/mgetgroups.m4
 Depends-on:
 getgroups
 getugroups
+realloc
 xalloc

 configure.ac:
-- 
1.6.4.2


>From c9b9d9ad1356fe28504aa9e6163009749aa4f863 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Fri, 4 Dec 2009 14:07:58 -0700
Subject: [PATCH 3/3] mgetgroups: reduce duplicate listings

POSIX doesn't guarantee whether the effective gid is included in
the list of supplementary groups returned by getgroups.  On the
other hand, some platforms include the effective gid twice in
the list.  Meanwhile, mgetgroups can independently add a duplicate.
Rather than spend a full-blown O(n log n) cleanup, we just remove
the most common forms of duplicate groups with an O(n) pass.

* lib/mgetgroups.c (mgetgroups): Reduce duplicates from the
resulting array.
* tests/test-chown.h (test_chown): Simplify client.
* tests/test-lchown.h (test_lchown): Likewise.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog           |    6 ++++++
 lib/mgetgroups.c    |   21 ++++++++++++++++++++-
 tests/test-chown.h  |    5 +----
 tests/test-lchown.h |    4 +---
 4 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 48e1026..19ba980 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
 2009-12-04  Eric Blake  <address@hidden>

+       mgetgroups: reduce duplicate listings
+       * lib/mgetgroups.c (mgetgroups): Reduce duplicates from the
+       resulting array.
+       * tests/test-chown.h (test_chown): Simplify client.
+       * tests/test-lchown.h (test_lchown): Likewise.
+
        mgetgroups: add xgetgroups, and avoid ENOSYS failures
        * lib/mgetgroups.h (xgetgroups): New prototype.
        * lib/mgetgroups.c (xgetgroups): New wrapper.
diff --git a/lib/mgetgroups.c b/lib/mgetgroups.c
index 7a61db4..db5c045 100644
--- a/lib/mgetgroups.c
+++ b/lib/mgetgroups.c
@@ -53,7 +53,8 @@ realloc_groupbuf (gid_t *g, size_t num)
    NULL, store the supplementary groups of the current process, and GID
    should be -1 or the effective group ID (getegid).  Upon failure,
    don't modify *GROUPS, set errno, and return -1.  Otherwise, return
-   the number of groups.  */
+   the number of groups.  The resulting list may contain duplicates,
+   but adjacent members will be distinct.  */

 int
 mgetgroups (char const *username, gid_t gid, gid_t **groups)
@@ -157,6 +158,24 @@ mgetgroups (char const *username, gid_t gid, gid_t
       ng++;
     }
   *groups = g;
+
+  /* Remove pair-wise duplicates, as well as any duplicate of the
+     first element.  Rather than do a full O(n log n) duplicate
+     removal, this only visits the most common duplicates.  */
+  {
+    gid_t first = *g;
+    gid_t *next;
+    gid_t *last = g + ng;
+
+    for (next = g + 1; next < last; next++)
+      {
+        if (*next == first || *next == *g)
+          ng--;
+        else
+          *++g = *next;
+      }
+  }
+
   return ng;
 }

diff --git a/tests/test-chown.h b/tests/test-chown.h
index 098b77d..e40f432 100644
--- a/tests/test-chown.h
+++ b/tests/test-chown.h
@@ -170,11 +170,8 @@ test_chown (int (*func) (char const *, uid_t, gid_t),
      changing group ownership of a file we own.  If we belong to at
      least two groups, then verifying the correct change is simple.
      But if we belong to only one group, then we fall back on the
-     other observable effect of chown: the ctime must be updated.
-     Be careful of duplicates returned by getgroups.  */
+     other observable effect of chown: the ctime must be updated.  */
   gids_count = mgetgroups (NULL, -1, &gids);
-  if (2 <= gids_count && gids[0] == gids[1] && 2 < gids_count--)
-    gids[1] = gids[2];
   if (1 < gids_count || (gids_count == 1 && gids[0] != st1.st_gid))
     {
       if (gids[0] == st1.st_gid)
diff --git a/tests/test-lchown.h b/tests/test-lchown.h
index 43690ac..0b89ff1 100644
--- a/tests/test-lchown.h
+++ b/tests/test-lchown.h
@@ -189,11 +189,9 @@ test_lchown (int (*func) (char const *, uid_t, gid_t),
      changing group ownership of a file we own.  If we belong to at
      least two groups, then verifying the correct change is simple.
      But if we belong to only one group, then we fall back on the
-     other observable effect of lchown: the ctime must be updated.
+     other observable effect of lchown: the ctime must be updated.  */
      Be careful of duplicates returned by getgroups.  */
   gids_count = mgetgroups (NULL, -1, &gids);
-  if (2 <= gids_count && gids[0] == gids[1] && 2 < gids_count--)
-    gids[1] = gids[2];
   if (1 < gids_count || (gids_count == 1 && gids[0] != st1.st_gid))
     {
       if (gids[0] == st1.st_gid)
-- 
1.6.4.2







reply via email to

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