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

Michal Židek mzidek at redhat.com
Thu Mar 7 14:46:08 UTC 2013


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.

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

I'll upload new patch soon.

>
> Simo.
>




More information about the sssd-devel mailing list