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

[Tito]

On Monday 13 June 2011 14:11:50 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:

Hi,
get_groups seems able to return value < 0:

A) in case getgrouplist is used:

from man getgrouplist:

  The n(groups) argument is a value-result argument: on  return  it  always
       contains  the  number  of  groups found for user, including group; this
       value may be greater than the number of groups stored in groups.
     
so we just catch an unexpected exception in case of *n < 0, 

 If the number of groups of which user is a member is less than or equal
       to *n, then the value *n is returned.

       If  the  user  is  a member of more than *n groups, then getgrouplist() returns -1

so here we have:
      1)  number of groups <= n             m = 0          
      2)  number of groups >   n              m = -1       *n = number of groups needed for  realloc
      3)  *n < 0                                         m = 0        /* something bad happened , dont realloc */

B) in case getgroups is used:

  On success, getgroups() returns the number of supplementary group  IDs.
       On error, -1 is returned, and errno is set appropriately.

       1) nn > *n          m = - 1
       2) nn = *n          m = 0
       3) nn < *n          m = 0
        
         *n < 0 is not possible in this case

So it seems to me that get_groups could return a value < 0.
  
static int get_groups(const char *username, gid_t rgid, gid_t *groups, int *n)
{
	int m;

	if (username) {
		/* If the user is a member of more than
		 * *n groups, then -1 is returned. Otherwise >= 0.
		 * (and no defined way of detecting errors?!) */
		m = getgrouplist(username, rgid, groups, n);
		/* I guess *n < 0 might indicate error. Anyway,
		 * malloc'ing -1 bytes won't be good, so: */
		//if (*n < 0)
		//	return 0;
		//return m;
		//commented out here, happens below anyway
	} else {
		/* On error -1 is returned, which ends up in *n */
		int nn = getgroups(*n, groups);
		/* 0: nn <= *n, groups[] was big enough; -1 otherwise */
		m = - (nn > *n);
		*n = nn;
	}
	if (*n < 0)
		return 0; /* error, don't return < 0! */
	return m;
}


Ciao,
Tito


> >     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 }
> And as a result - no way to get more than 64 groups, it goes strait to the bb_err_msg:
> >     196                 } else if (n < 0) { /* error in get_groups() */
> >     197                         if (ENABLE_DESKTOP)
> >     198                                 bb_error_msg_and_die("can't get groups");
> >     199                         return EXIT_FAILURE;
> >     200                 }
> Is there any reason for such kind of restriction in get_groups? I'm suggesting
> to substitute this restriction with the call to "getgroups(0, groups)", to get an 
> actual amount of groups before returning -1. In this case we're calling getgroups()
> 3 times. But only if we have more than 64 groups, it's a rare case. Another way -
> is to call getgroups() twice but always, first time to get amount of groups and
> then the actual groups list. I guess to call three times but in rare cases is
> better than twice but always.
> 
> Patch is in attachment:
> 
> >From 3a99379834788c6169a7dd992473524c9652e6a4 Mon Sep 17 00:00:00 2001
> 
> Alexey Fomenko (1):
>   id: fix return value when trying to get more than 64 groups
> 
>  coreutils/id.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>