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

Simo Sorce simo at redhat.com
Thu Mar 7 16:53:12 UTC 2013


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.

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




More information about the sssd-devel mailing list