[SSSD] [PATCH] NSS: Check allocation result

Pavel Březina pbrezina at redhat.com
Fri Nov 1 10:05:51 UTC 2013


On 10/31/2013 07:51 PM, Jakub Hrozek wrote:
> On Thu, Oct 31, 2013 at 10:37:13AM +0100, Pavel Březina wrote:
>> On 10/22/2013 12:29 PM, Jakub Hrozek wrote:
>>> On Mon, Oct 21, 2013 at 05:33:33PM +0200, Jakub Hrozek wrote:
>>>> On Mon, Oct 21, 2013 at 10:38:19AM -0400, Simo Sorce wrote:
>>>>> On Mon, 2013-10-21 at 11:03 +0200, Jakub Hrozek wrote:
>>>>>> Another small bug I found when looking for #1020945
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> plain text
>>>>>> document
>>>>>> attachment
>>>>>> (0001-NSS-Check-allocation-result.patch)
>>>>>>
>>>>>>  From b1d04686f085e25f10dde82f1e19c89278883001 Mon Sep 17 00:00:00 2001
>>>>>> From: Jakub Hrozek <jhrozek at redhat.com>
>>>>>> Date: Sun, 20 Oct 2013 19:24:04 +0200
>>>>>> Subject: [PATCH] NSS: Check allocation result
>>>>>>
>>>>>> ---
>>>>>>   src/responder/nss/nsssrv_cmd.c | 2 ++
>>>>>>   1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/src/responder/nss/nsssrv_cmd.c
>>>>>> b/src/responder/nss/nsssrv_cmd.c
>>>>>> index
>>>>>> d37b4707cb734375011650632bca6d429042038c..a1938b2fc8d0a88460027d5f0d400f840fade02b 100644
>>>>>> --- a/src/responder/nss/nsssrv_cmd.c
>>>>>> +++ b/src/responder/nss/nsssrv_cmd.c
>>>>>> @@ -320,6 +320,7 @@ static int fill_pwent(struct sss_packet *packet,
>>>>>>       for (i = 0; i < *count; i++) {
>>>>>>           talloc_zfree(tmp_ctx);
>>>>>>           tmp_ctx = talloc_new(NULL);
>>>>>> +        if (tmp_ctx == NULL) return ENOMEM;
>>>>>>
>>>>>>           msg = msgs[i];
>>>>>>
>>>>>> @@ -2325,6 +2326,7 @@ static int fill_grent(struct sss_packet *packet,
>>>>>>       for (i = 0; i < *count; i++) {
>>>>>>           talloc_zfree(tmp_ctx);
>>>>>>           tmp_ctx = talloc_new(NULL);
>>>>>> +        if (tmp_ctx == NULL) return ENOMEM;
>>>>>>           msg = msgs[i];
>>>>>>
>>>>>>           /* new group */
>>>>>
>>>>> Sorry to shot down another small fix, but I think what you want to do
>>>>> here is not talloc_zfree(tmp_ctx) + talloc_new(NULL), but rather use
>>>>> simply talloc_free_children(tmp_ctx);
>>>>>
>>>>> Simo.
>>>>
>>>> OK, but that wasn't the point of the patch :-) The point was to check
>>>> result of allocation, which was missing.
>>>>
>>>> I will prepare a new patch.
>>>
>>> New patch is attached -- talloc_free_children is used now instead of
>>> talloc_free() && talloc_new().
>>>
>>> The fill_* functions return gracefully instead of erroring out now
>>> (although I don't think this makes too much of a difference, if sssd
>>> runs out of memory, it should just care about not crashing and ending the
>>> request somehow).
>>
>> Hi,
>> sorry but nack. We should return ENOMEM in fill_* to stay consistent
>> with rest of the code.
>>
>> These functions already return errno so I really don't get why you
>> chose to do it this way :-)
>
> That's exactly the opposite of what Sumit requested earlier. Please make
> up your mind :)

Further in the code it makes sense not to report an error, since at 
least some members may have been evaluated correctly. But it is not the 
case with this allocation error so it should be reported.




More information about the sssd-devel mailing list