[SSSD] [PATCH] pysss: test return value of realloc.

Pavel Reichl preichl at redhat.com
Mon Sep 1 08:53:14 UTC 2014


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 at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel




More information about the sssd-devel mailing list