[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