Re: inifite loop in recent mgetgroups.c:mgetgroups on Darwin/MacOS

[Pádraig Brady]

Steven Parkes wrote:
> Very similar to my "correct" patch. There's one other issue I found when I
> was coming up with a patch which is that Darwin:getgrouplist also doesn't
> return the number of groups. It sets the in/out parameter, but then just
> returns 0.

Right so your previous patch would have always returned 0 for
users in more than 10 groups.

I noticed that the docs don't actually say the number of groups
is returned, and was going to ask you to test a new version returning
max_n_groups rather than ng still works, as I thought it more robust.
So you've already tested that and shown in fact that it's required.

> The only thing I wasn't too sure about was how risky it is to just keep
> doubling. If some bizzaro-world getgrouplist was just implemented to return
> -1, well, the results wouldn’t be great. But maybe that's not a significant
> concern.

If there was an existing getgrouplist that always returned -1
then we should handle it. But that would be a very unlikely bug
and if introduced in future would be noticed quickly and fixed
at source. So I'm not sure about handling limits here.
For reference some relevant limits on my 32 bit linux system.
  $ cat /proc/sys/kernel/overflowgid
  65534
  $ eval $(git/coreutils/src/getlimits); echo $GID_T_MAX
  4294967295

Updated patch attached.

cheers,
Pádraig.

>From 47df4c1efcce97413786ef84452e5cfedcb56fc6 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?P=C3=A1draig=20Brady?= <address@hidden>
Date: Wed, 8 Apr 2009 10:43:15 +0100
Subject: [PATCH] id: fix infinite loop on some systems

Steven Parkes reported that `id -G $USER` went into an infinite loop
on Darwin systems for users in more than 10 groups:
http://bugs.gentoo.org/show_bug.cgi?id=264007
* gl/lib/mgetgroups.c (mgetgroups): Work around buggy getgrouplist
implementations that don't update the required size correctly,
by doubling the result buffer and retrying. Also return the
parameter updated by getgrouplist rather than its return value,
as the documentation doesn't actually state the number of groups
stored is returned by getgrouplist.
* tests/misc/id-groups: Add test to exercise this logic
* tests/Makefile.am: Reference new test
* NEWS: Mention the fix
* THANKS: Update
---
 NEWS                 |    5 +++++
 THANKS               |    1 +
 gl/lib/mgetgroups.c  |   10 +++++++++-
 tests/Makefile.am    |    1 +
 tests/misc/id-groups |   28 ++++++++++++++++++++++++++++
 5 files changed, 44 insertions(+), 1 deletions(-)
 create mode 100755 tests/misc/id-groups

diff --git a/NEWS b/NEWS
index de1db44..976e3a7 100644
--- a/NEWS
+++ b/NEWS
@@ -7,6 +7,11 @@ GNU coreutils NEWS                                    -*- 
outline -*-
   ls now aligns output correctly in the presence of abbreviated month
   names from the locale database that have differing widths.
 
+  `id -G $USER` now works correctly on Darwin and NetBSD. Previously it
+  would either truncate the group list to 10 or go into an infinite loop.
+  [truncation bug introduced in coreutils-6.11]
+  [infinite loop bug introduced in coreutils-7.1]
+
 ** Changes in behavior
 
   shred, sort, shuf: now use an internal pseudorandom generator by default.
diff --git a/THANKS b/THANKS
index 6a918a4..fe523fe 100644
--- a/THANKS
+++ b/THANKS
@@ -525,6 +525,7 @@ Steve McIntyre                      address@hidden
 Steve Ward                          address@hidden
 Steven G. Johnson                   address@hidden
 Steven Mocking                      address@hidden
+Steven Parkes                       address@hidden
 Steven Schveighoffer                address@hidden
 Steven P Watson                     address@hidden
 Stuart Kemp                         address@hidden
diff --git a/gl/lib/mgetgroups.c b/gl/lib/mgetgroups.c
index e697013..ac1d398 100644
--- a/gl/lib/mgetgroups.c
+++ b/gl/lib/mgetgroups.c
@@ -81,10 +81,16 @@ mgetgroups (char const *username, gid_t gid, GETGROUPS_T 
**groups)
       while (1)
        {
          GETGROUPS_T *h;
+         int last_n_groups = max_n_groups;
 
          /* getgrouplist updates max_n_groups to num required.  */
          ng = getgrouplist (username, gid, g, &max_n_groups);
 
+         /* Some systems (like Darwin) have a bug where they
+            never increase max_n_groups.  */
+         if ((0 > ng) && (last_n_groups == max_n_groups))
+           max_n_groups *= 2;
+
          if ((h = realloc_groupbuf (g, max_n_groups)) == NULL)
            {
              int saved_errno = errno;
@@ -97,7 +103,9 @@ mgetgroups (char const *username, gid_t gid, GETGROUPS_T 
**groups)
          if (0 <= ng)
            {
              *groups = g;
-             return ng;
+             /* Some systems just return 0 or -1 from getgrouplist,
+                so return max_n_groups rather than ng.  */
+             return max_n_groups;
            }
        }
     }
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 07f34ec..8ce6a21 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -171,6 +171,7 @@ TESTS =                                             \
   misc/head-c                                  \
   misc/head-pos                                        \
   misc/id-context                              \
+  misc/id-groups                               \
   misc/md5sum                                  \
   misc/md5sum-newline                          \
   misc/mknod                                   \
diff --git a/tests/misc/id-groups b/tests/misc/id-groups
new file mode 100755
index 0000000..dc0f54c
--- /dev/null
+++ b/tests/misc/id-groups
@@ -0,0 +1,28 @@
+#!/bin/sh
+# Ensure that "id" outputs groups for a user
+# Copyright (C) 2009 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if test "$VERBOSE" = yes; then
+  set -x
+  id --version
+fi
+
+. $srcdir/test-lib.sh
+
+fail=0
+id -G $(id -nu) || fail=1
+
+Exit $fail
-- 
1.5.3.6