[Patch] coreutils/id.c: broken logic ?

[Alexey Fomenko]

On Wed, 2011-06-22 at 05:24 +0200, ext Denys Vlasenko wrote:
> On Monday 13 June 2011 14:11, Alexey Fomenko wrote:
> > Hello!
> > 
> > In coreutils/id.c get_groups() gets groups list for further printing out
> > on screen. According to id's main function - return value from
> > get_groups is expected even < 0 in order to extend (xrealloc) the
> > list size for the groups in case if there's more than 64:
> > >     123 int id_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
> > >     124 int id_main(int argc UNUSED_PARAM, char **argv)
> > >     125 {
> > ...  
> > >     180                 n = 64;
> > >     181                 if (get_groups(username, rgid, groups, &n) < 0) {
> > >     182                         /* Need bigger buffer after all */
> > >     183                         groups = xrealloc(groups, n * sizeof(gid_t));
> > >     184                         get_groups(username, rgid, groups, &n);
> > >     185                 }
> > >     186                 if (n > 0) {
> > But get_groups() allowed to return only >=0 value:
> > >     96 static int get_groups(const char *username, gid_t rgid, gid_t *groups, int *n)
> > ...
> > >     118         if (*n < 0)
> > >     119                 return 0; /* error, don't return < 0! */
> > >     120         return m;
> > >     121 }
> 
> Why do you think so? It says "if (*n < 0)", not "if (m < 0)".
Yes, but *n gets value from nn, and nn is the result of getgroups, and
man getroups says:
> If the calling process is a member of more than size supplementary groups, then an error results.
So if we have more groups than we can put in supplied array, we'll get an EINVAL
from getgroups; => nn = EINVAL; *n = nn (which is EINVAL); and then check after
if (*n < 0) we're returning zero like it's everything allright here, we didn't have any problems.
When we're returning from get_groups to main function we're supposed to check if we need to realloc
the buffer, but returned value was zero, what means everything is Ok no need to realloc, at the 
same time further code checks if n is below zero (and it is in a fact) and we just getting the error

> from main function: 
> ...
> } else if (n < 0) { /* error in get_groups() */
>                         if (ENABLE_DESKTOP)
>                                 bb_error_msg_and_die("can't get groups");
>                         return EXIT_FAILURE;
>                 }

> 
>