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

Simo Sorce simo at redhat.com
Thu Mar 7 02:56:07 UTC 2013


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.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list