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

[Tito]

On Monday 13 June 2011 16:00:14 Tito wrote:
> 
> 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

Was wrong on this:
 		On error -1 is returned, which ends up in *n 

	4) nn < 0          m = 0   /* something bad happened , dont realloc */
> 
> 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(-)
> > 
> 
> _______________________________________________
> busybox mailing list
> busybox at busybox.net
> http://lists.busybox.net/mailman/listinfo/busybox
>