[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