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(-)
>
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
