[SSSD] [PATCH] File descriptor leak in nss responder.

Michal Židek mzidek at redhat.com
Thu Mar 7 02:15:54 UTC 2013


On 03/07/2013 12:02 AM, Simo Sorce wrote:
> On Wed, 2013-03-06 at 23:19 +0100, Michal Židek wrote:
>> On 03/06/2013 08:13 PM, Simo Sorce wrote:
>>> On Wed, 2013-03-06 at 19:33 +0100, Michal Židek wrote:
>>>> On 03/06/2013 07:27 PM, Michal Židek wrote:
>>>>> On 03/06/2013 07:18 PM, Michal Židek wrote:
>>>>>> On 03/06/2013 06:33 PM, Simo Sorce wrote:
>>>>>>> On Wed, 2013-03-06 at 17:09 +0100, Michal Židek wrote:
>>>>>>>> https://fedorahosted.org/sssd/ticket/1826
>>>>>>>>
>>>>>>>> See commit message.
>>>>>>>
>>>>>>> It would be better if you can use a destructor attached to the mc_ctx
>>>>>>> so any other path where we need to free it is automatically covered.
>>>>>>>
>>>>>>> Simo.
>>>>>>>
>>>>>>
>>>>>> New patch attached.
>>>>>>
>>>>>> Thanks
>>>>>> Michal
>>>>>>
>>>>>>
>>>>> There should be goto done, not ret... will send another patch soon.
>>>>>
>>>>
>>>> Here it is.
>>>
>>> The destructors can return only 0 or -1.
>>>
>>> And they shoul return -1 exclusively when they want to make the
>>> talloc_free() operation fail.
>>>
>>> In this case we should probably never return anything but 0, because
>>> even on failure we want to still free the mc_ctx.
>>>
>>
>> OK.
>>
>>> Also I think failing to close the file or unmapping should be major
>>> failures (much higher debug level) as they cause memory/resource leaks
>>> that will quickly make the process unusuable.
>>
>> I changed it to critical failure.
>
> The destructor looks fine now, however now that you have created it I
> would reverse munmap and close of file descriptor operations order and
> add unlink(mc_ctx->file); after you close the fd.
> I would also add checks that fd is not already -1 and that mmap_base is
> not NULL aqnd file is not NULL.
>
> In sss_mmap_cache_init() I would add the destructor much earlier, right
> after mc_ctx is allocated and fd is asigned -1. Then remove
> munmap/close/unlink operations from the exception handling (done label)
> and leave there just the talloc_free().
>
> If you do this both code paths will use the same destructor to free
> these resources and we do not risk a change in one path to make the 2
> diverge.
>
> Simo.
>

Ok. I also added the the sss_mc_set_recycled call to the destructor, 
because the destruction is called as part of memory cache 
reinitialisation, but I left the logic to handle this in the init 
(sss_mc_create_file) part too, for the case if mc files were left on 
disk after SSSD crashed and destructor failed or was not called when 
SSSD was running last time. The unlink() call in the init function is 
now called only if the call to open the old mc file succeeded (so the 
file really exists).

The second patch adds debug message for the case if opening old
instance of mc file fails. It has not much to do with the destructor but 
it applies on top of the first patch so I added it to this thread.

New version of the patch is attached (along with the second patch).

Thanks
Michal
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-File-descriptor-leak-in-nss-responder.patch
Type: text/x-patch
Size: 5434 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130307/5184391e/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Debug-message-in-sss_mc_create_file.patch
Type: text/x-patch
Size: 1131 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130307/5184391e/attachment-0001.bin>


More information about the sssd-devel mailing list