On Monday 13 June 2011 16:46:21 Tito wrote:
>
> 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(-)
> > >
Hi,
by looking closer at it I now see the bug (sorry for the previous noise).
It seems to me that your proposed patch doesn't fix it and causes
garbled text to be printed to the terminal. This is easily demonstrated
by setting n to a low number e.g. n=2 and running id as user with more than 2
groups:
./busybox id
uid=1000(tito) gid=1000(tito)
groups=0(root),0(root),0(root),0(root),0(root),0(root),0(root),0(root),0(root),0(root),0(root),0(root),0(root),135092064,4294967295
It is due to the fact that (from man getgroups):
" If size is zero, __list is not modified__, but the total number of
supplementary group IDs for the process is returned."
Please take a look at the attached alternative patch that seems to work for me:
id: fix for the case when the calling process is a member
of more than 64 supplementary groups.
Signed-off-by: Tito Ragusa <[email protected]>
--- coreutils/id.c.original 2011-06-14 01:18:56.000000000 +0200
+++ coreutils/id.c 2011-06-14 01:58:06.000000000 +0200
@@ -110,11 +110,10 @@ static int get_groups(const char *userna
//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 ((m = getgroups(*n, groups)) < 0 /* && errno == EINVAL */)
+ *n = getgroups(0, groups);
+ else
+ *n = m;
}
if (*n < 0)
return 0; /* error, don't return < 0! */
Ciao,
Tito
id: fix for the case when the calling process is a member
of more than 64 supplementary groups.
Signed-off-by: Tito Ragusa <[email protected]>
--- coreutils/id.c.original 2011-06-14 01:18:56.000000000 +0200
+++ coreutils/id.c 2011-06-14 02:03:26.000000000 +0200
@@ -110,11 +110,10 @@ static int get_groups(const char *userna
//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 ((m = getgroups(*n, groups)) < 0 /* && errno == EINVAL */)
+ *n = getgroups(0, groups);
+ else
+ *n = m;
}
if (*n < 0)
return 0; /* error, don't return < 0! */
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
