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

Michal Židek mzidek at redhat.com
Thu Mar 7 12:44:17 UTC 2013


On 03/07/2013 03:56 AM, Simo Sorce wrote:
> On Thu, 2013-03-07 at 03:15 +0100, Michal Židek wrote:
>> 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,
>
> I thinks this is incorrect, You will end up killing the cache file every
> time you shut down sssd.
> We currently don't do this because we want to retain the cache on
> machine reboot for example so at startup processes can hit the cache if
> still valid.
>
>> because the destruction is called as part of memory cache
>> reinitialisation,
>
> but not exclusively. The destructor is called every time the memory
> context is freed.
> Which I think means it was wrong for me to ask to add unlink() in there
> too. Unlink should probably not be done in this destructor for the
> reasons in the above paragraph.
>
>>   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).
>
> ok
>
>> 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).
>
> Ok sorry for the churn, but now the destructor is doing a little too
> much :)
>
> Simo.
>

Ok :)
So I removed the unlink() from destructor (and the sss_mc_set_recycled
with it). I added new dret variable to catch return value from unlink()
in the done: part of sss_mc_create_file(), so that we do not overwrite
the original error code.

Also the mc_ctx->fd should be set to -1 if the file construction in
sss_mc_create_file (so that we do not try to close garbage file
descriptor in the destructor). So I added this too.

New patches are attached.

Michal
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-File-descriptor-leak-in-nss-responder.patch
Type: text/x-patch
Size: 5821 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130307/3b5fc366/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/3b5fc366/attachment-0001.bin>


More information about the sssd-devel mailing list