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