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

Simo Sorce simo at redhat.com
Wed Mar 6 23:02:14 UTC 2013


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.

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




More information about the sssd-devel mailing list