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

Jakub Hrozek jhrozek at redhat.com
Thu Mar 7 17:48:49 UTC 2013


On Thu, Mar 07, 2013 at 11:53:12AM -0500, Simo Sorce wrote:
> On Thu, 2013-03-07 at 17:44 +0100, Michal Židek wrote:
> > On 03/07/2013 04:19 PM, Simo Sorce wrote:
> > > On Thu, 2013-03-07 at 15:46 +0100, Michal Židek wrote:
> > >> On 03/07/2013 03:22 PM, Simo Sorce wrote:
> > >>> On Thu, 2013-03-07 at 15:02 +0100, Michal Židek wrote:
> > >>>> +        /* Closing the file descriptor and ummaping the file
> > >>>> +         * from memory is done in the mc_ctx_destructor. */
> > >>>>            if (mc_ctx && mc_ctx->fd != -1) {
> > >>>> -            close(mc_ctx->fd);
> > >>>> -            ret = unlink(mc_ctx->file);
> > >>>> -            if (ret == -1) {
> > >>>> -                DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to rm mmap file %
> > >>>> s: %d(%s)\n",
> > >>>> -                                            mc_ctx->file, ret,
> > >>>> strerror(ret)));
> > >>>> +            dret = unlink(mc_ctx->file);
> > >>>> +            if (dret == -1) {
> > >>>> +                DEBUG(SSSDBG_CRIT_FAILURE,
> > >>>> +                      ("Failed to rm mmap file %s: %d(%s)\n",
> > >>>> mc_ctx->file,
> > >>>> +                       dret, strerror(dret)));
> > >>>>                }
> > >>>
> > >>> sorry I just also realized this is probably incorrect, you should test
> > >>> mc_ctx->file not mc_ctx->fd
> > >>
> > >> Not really. The flow is to allocate mc_ctx->file and after that call
> > >> open... so if fd is not -1 then the mc_ctx->file must be already
> > >> allocated (not null). Testing for mc_ctx->file would be incorrect,
> > >> because we might successfully create mc_ctx->file, but then fail during
> > >> open()... so checking for fd is correct here.
> > >
> > > This will be correct until someone changes the code. It's about
> > > defensive programming. The unlink() call only depends on the 'file'
> > > name, it doesn't care about the file descriptor.
> > > So we might want to perform this action only if fd is not = -1, however
> > > we still want to validate that file is not null.
> > >
> > > However I just checked that unlink(NULL) is just a no-op and seem not to
> > > crash (at least on modern Linux+glibc), so we can ignore it for now.
> > 
> > On the other hand, I have nothing against adding it as additional
> > condition (added in the new patch). You are right that it is more
> > defensive.
> > 
> > >
> > >>>
> > >>> I also see we have a minor issue in sss_mc_create_file(), we should
> > >>> probably remove close()/unlink() as well from the error condition in
> > >>> case sss_br_lock_file() fails, or at the very least set fd = -1 and file
> > >>> = NULL when close() and unlink() are called respectively.
> > >>
> > >> Yes. But simply setting the fd = -1 should be enough (for the same
> > >> reason as described above).
> > >
> > > Yes fd = -1 is enough but not for the reason above, simply because
> > > freeing file is not necessary as it will be freed later when we call
> > > talloc_free(*mc_ctx) anyway.
> > 
> > I leave the fd = -1 here. I think it is more readable if the file
> > is closed and unlinked in the function where it failed to be created
> > (and where open() was called), even if the upper layer
> > (sss_mmap_cache_init) would do it.
> > 
> > >
> > > Simo.
> > >
> > 
> > So this patch moves back the unlink() to the same position as it was 
> > before so it is unlinked unconditionally, but adds the condition not
> > to scream to the logs if the file did not exist.
> 
> ACK.
> 
> Simo.

No more leaks in my testing and fast cache still works fine.

Pushed to master and sssd-1-9



More information about the sssd-devel mailing list