[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