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.
Are we clear now?
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel