[SSSD] [PATCH] pysss: test return value of realloc.
Lukas Slebodnik
lslebodn at redhat.com
Thu Aug 28 08:55:38 UTC 2014
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
More information about the sssd-devel
mailing list