[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [Patch] coreutils/id.c: broken logic ?
From:       Denys Vlasenko <vda.linux () googlemail ! com>
Date:       2011-06-22 14:43:32
Message-ID: BANLkTikJ-EhX2Tgb71n93m0SXwYHkEC3Qg () mail ! gmail ! com
[Download RAW message or body]

On Wed, Jun 22, 2011 at 9:57 AM, Alexey Fomenko
<ext-alexey.fomenko@nokia.com> wrote:
> On Wed, 2011-06-22 at 05:24 +0200, ext Denys Vlasenko wrote:
>> On Monday 13 June 2011 14:11, 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:
>> > >     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 }
>>
>> Why do you think so? It says "if (*n < 0)", not "if (m < 0)".
> Yes, but *n gets value from nn, and nn is the result of getgroups, and
> man getroups says:
>> If the calling process is a member of more than size supplementary groups, then an error results.
> So if we have more groups than we can put in supplied array, we'll get an EINVAL
> from getgroups; => nn = EINVAL; *n = nn (which is EINVAL); and then check after
> if (*n < 0) we're returning zero like it's everything allright here, we didn't have any problems.
> When we're returning from get_groups to main function we're supposed to check if we need to realloc
> the buffer, but returned value was zero, what means everything is Ok no need to realloc, at the
> same time further code checks if n is below zero (and it is in a fact) and we just getting the error
>
>> from main function:
>> ...
>> } else if (n < 0) { /* error in get_groups() */
>>                         if (ENABLE_DESKTOP)
>>                                 bb_error_msg_and_die("can't get groups");
>>                         return EXIT_FAILURE;
>>                 }


Thanks, understood. Please try current git.
-- 
vda
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[prev in list] [next in list] [prev in thread] [next in thread]