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

Pavel Reichl preichl at redhat.com
Thu Aug 28 08:41:42 UTC 2014


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




More information about the sssd-devel mailing list