[ALTERNATIVE PATCH] coreutils/id.c: broken logic ?
Alexey Fomenko
ext-alexey.fomenko at nokia.com
Tue Jun 14 09:11:00 UTC 2011
On Tue, 2011-06-14 at 02:10 +0200, ext Tito wrote:
>
> 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."
Agree, my brain has inverted the expected result value of the comparison
"- (nn > *n)", so I've missed this part.
>
> Please take a look at the attached alternative patch that seems to work for me:
Works for me as well. Thanks.
>
> id: fix for the case when the calling process is a member
> of more than 64 supplementary groups.
>
> Signed-off-by: Tito Ragusa <farmatito at tiscali.it>
>
> --- 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
>
More information about the busybox
mailing list