On 08/28/2014 10:55 AM, Lukas Slebodnik wrote:
On (28/08/14 10:41), Pavel Reichl wrote:
> On 08/27/2014 07:23 PM, Lukas Slebodnik wrote:
>> On (27/08/14 17:54), Pavel Reichl wrote:
>>> On 08/27/2014 05:42 PM, Lukas Slebodnik wrote:
>>>> On (27/08/14 17:21), Pavel Reichl wrote:
>>>>> On 08/27/2014 05:12 PM, Lukas Slebodnik wrote:
>>>>>> On (27/08/14 17:10), Pavel Reichl wrote:
>>>>>>> On 08/27/2014 10:09 AM, Lukas Slebodnik wrote:
>>>>>>>> ehlo,
>>>>>>>>
>>>>>>>> warning reported by coverity
>>>>>>>>
>>>>>>>> patch is attached
>>>>>>>>
>>>>>>>> LS
>>>>>>> As I understand the code there is no way that 0 could be
passed as second
>>>>>>> argument to the realloc call, right? Because that would lead
to freeing same
>>>>>>> memory twice.
>>>>>>>
>>>>>>> Would it be more defensive approach to test for zero argument
explicitly?
>>>>>>> What do you think Lukas?
>>>>>>>
>>>>>> No,
>>>>>>
>>>>>> NULL can be returned if there is not memory for allocation
(ENOMEM).
>>>>>> For example you will try to allocate 2^31 bytes
>>>>>>
>>>>>> LS
>>>>> NULL can be also returned if you pass 0 as the size parameter, which
would in
>>>>> our case lead to calling free on the same pointer twice, which is
baaaad!
>>>>>
>>>>> If you are explicitly interested in ENOMEM case, you may consider
checking
>>>>> value of errno.
>>>>>
>>>>> Still I think it is better to test value of size parameter.
>>>>>
>>>> I do not agree.
>>>>
>>>> Documentation of getgrouplist is clear.
>>>>
>>>> RETURN VALUE
>>>> If the number of groups of which user is a member *is less than or
equal*
>>>> to *ngroups, then the value *ngroups is returned.
>>>>
>>>> If the user is a member of more than *ngroups groups, then
getgrouā
>>>> plist() returns -1. In this case, the value returned in *ngroups
can
>>>> be used to resize the buffer passed to a further call
getgrouplist().
>>>>
>>>> And the initial value of ngroups is 32. It means it cannot be lower.
>>>>
>>>> LS
>>> OK, I agree it is supposed to work, I'm just worried that next time
anybody
>>> reading the code will have to look to man pages to make sure it won't
crash.
>>>
>> Sorry I still do not agree with you.
>>
>> The purpose of this patch is to prevent crash.
>> Anyway, it is best practice to check return value of allocation functions
>> (malloc, calloc, realloc). And calling realloc with 0 is very very rare and
>> unexpected.
>>
>> If something magical happen on nostandard UNIX system(which we do not supprort
>> anyway) and ngroups will be zero.
>> realloc will return NULL and we will crash.
>>
>> 771 do {
>> 772 ret = getgrouplist(username, pw->pw_gid, groups, &ngroups);
>> 773 if (ret < ngroups) {
>> 774 groups = realloc(groups, ngroups * sizeof(gid_t));
>> 775 }
>> 776 } while (ret != ngroups);
>> 777
>> 778 groups_tuple = PyTuple_New((Py_ssize_t) ngroups);
>> 779 if (groups_tuple == NULL) {
>> 780 goto fail;
>> 781 }
>> 782
>> 783 /* Populate a tuple with names of groups
>> 784 * In unlikely case of group not being able to resolve, skip it
>> 785 * We also need to resize resulting tuple to avoid empty elements there
*/
>> 786 idx = 0;
>> 787 for (i = 0; i < ngroups; i++) {
>> 788 gr = getgrgid(groups[i]);
>> ^^^^^^^^^
>> dereference of NULL pointer
>>
>> LS
> Lukas, you patch is indeed improving the situation and I agree it is needed.
> I was just proposing how to IMO do it even better. I understand that you find
> my concerns as excessive and I can live with that, that's why there is the
> word tentative before ACK.
>
You did not get my point.
That testing ngroups for zero is deadcode. It can never happen.
There isn't a reason to add new dead code.
LS
OK Lukas, unless you are going to add a comment to the code about the
matter we discussed. I consider the patch as final.
ACK
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel