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

Michal Židek mzidek at redhat.com
Thu Mar 7 16:44:58 UTC 2013


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.

Michal

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-File-descriptor-leak-in-nss-responder.patch
Type: text/x-patch
Size: 4767 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130307/03cda31d/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Debug-message-in-sss_mc_create_file.patch
Type: text/x-patch
Size: 1002 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130307/03cda31d/attachment-0001.bin>


More information about the sssd-devel mailing list