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

Simo Sorce simo at redhat.com
Thu Mar 7 15:19:37 UTC 2013


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.

> >
> > 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.

Simo.

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




More information about the sssd-devel mailing list