Cyrus IMAPd 2.3.10 Released

[Hajimu UMEMOTO]

Hi,

>>>>> On Fri, 26 Oct 2007 11:13:01 -0400
>>>>> Ken Murchison <murch at andrew.cmu.edu> said:

murch> Tomas Janousek wrote:
> Hello,
> 
> On Fri, Oct 26, 2007 at 04:50:02PM +0200, Simon Matter wrote:
>>>>> --- auth_unix.c.~1.46.~	2007-09-27 16:02:45.000000000 -0400
>>>>> +++ auth_unix.c	2007-10-25 23:02:15.000000000 -0400
>>>>> @@ -225,7 +225,7 @@
>>>>>      struct group *grp;
>>>>>  #ifdef HAVE_GETGROUPLIST
>>>>>      gid_t gid, *groupids = NULL;
>>>>> -    int ret, ngroups = 0;
>>>>> +    int ret, ngroups = 10;
>>>>>  #else
>>>>>      char **mem;
>>>>>  #endif
>>>>> @@ -248,10 +248,7 @@
>>>>>  #ifdef HAVE_GETGROUPLIST
>>>>>      gid = pwd ? pwd->pw_gid : (gid_t) -1;
>>>>>
>>>>> -    /* get number of groups user is member of into ngroups */
>>>>> -    getgrouplist(identifier, gid, NULL, &ngroups);
>>>>> -
>>>>> -    /* get the actual group ids */
>>>>> +    /* get the group ids */
>>>>>      do {
>>>>>  	groupids = (gid_t *)xrealloc((gid_t *)groupids,
>>>>>  				     ngroups * sizeof(gid_t));
> 
> This should not change behaviour nor cause additional problems, I think.
> 
> The gnu manpage of getgrouplist does not mention the NULL case either, though
> it works. If it segfaults, I'd consider that a libc failure, since when
> ngroups == 0, the pointer shouldn't be used at all (it should point to the
> first element of the array but array of 0 elements has no first element).
> 
> If it really is a wrong code of mine, then sorry for not testing on anything
> other than Linux.

murch> Its not a problem.  Since we might have to realloc() the grouplist 
murch> anyways, it really doesn't make much sense to just get the count first.

The code is suspicious to me.  Isn't the test of `ret != -1' is
opposite?
Further, it seems that the test of `ngroups == newstate->ngroups'
assumes that newstate->ngroups holds the actual number of groups
found, by calling getgrouplist() in the first place.
Perhaps, it should be:

    do {
	groupids = (gid_t *)xrealloc((gid_t *)groupids,
				     ngroups * sizeof(gid_t));

	newstate->ngroups = ngroups; /* copy of ngroups for comparision */
	ret = getgrouplist(identifier, gid, groupids, &ngroups);
	/*
	 * This is tricky. We do this as long as getgrouplist tells us to
	 * realloc _and_ the number of groups changes. It tells us to realloc
	 * also in the case of failure...
	 */
    } while (ret == -1 && ngroups == newstate->ngroups);
             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I'm not sure that we can expect `ngroups == newstate->ngroups',
though.

Sincerely,

--
Hajimu UMEMOTO @ Internet Mutual Aid Society Yokohama, Japan
ume at mahoroba.org  ume@{,jp.}FreeBSD.org
http://www.imasy.org/~ume/