Cyrus IMAPd 2.3.10 Released

[Ken Murchison]

Hajimu UMEMOTO wrote:
> 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 think you're right on the 'ret == -1' test.  We want to stay in the 
loop if getgrouplist() fails, AND it returns a different number of 
groups.  Apparently, on Linux getgrouplist() can fail for some reason 
other than ngroups being insufficient.  So we want to break out of the 
loop in this case.

Tomas, is my logic correct?


-- 
Kenneth Murchison
Systems Programmer
Project Cyrus Developer/Maintainer
Carnegie Mellon University