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

[Jim Meyering]

Pádraig Brady wrote:
..

Hi Pádraig,
Thanks for working on that.

This NEWS entry makes it look like these are coreutils bugs.
True, it's a regression, but not due to a bug in coreutils,
unless you count the decision to use POSIX getgroups as the bug ;-)
The old, inefficient version used to work well enough on those systems,
but the new, improved-to-use-POSIX-getgroups version failed due
to non-conforming getgroups on Darwin and NetBSD.
So to be a little more clear, how about something like this?

> +  `id -G $USER` now works correctly on Darwin and NetBSD. Previously it

s/on Darwin/even on Darwin/

> +  would either truncate the group list to 10 or go into an infinite loop.

s/loop/loop, due to their non-conforming getgroups implementations.

> +  [truncation bug introduced in coreutils-6.11]
> +  [infinite loop bug introduced in coreutils-7.1]

  [bogus getgroups caused truncation in coreutils-6.11]
  [bogus getgroups caused infloop in coreutils-7.1]

Actually, that entry belongs in the "** Portability" section,
not in "** Bug fixes".

> 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))

Please write that differently: no excess parens, prefer < over >.

          if (ng < 0 && 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;

If it returns -1, then the above isn't reached.
Would this work as well?

              /* Some vendor getgrouplist functions return 0,
                 so return max_n_groups rather than 0.  */
              return ng ? ng : max_n_groups;