2:59 a.m.
On Thu, Feb 26, 2015 at 03:46:46PM -0700, Daniel Hjorth wrote:
On Thu, Feb 26, 2015 at 09:46:10PM +0100, Jakub Hrozek wrote:
> On Thu, Feb 26, 2015 at 12:22:53PM -0700, Daniel Hjorth wrote:
> > On Thu, Feb 26, 2015 at 01:13:05PM +0100, Jakub Hrozek wrote:
> > > On Wed, Feb 25, 2015 at 01:47:36PM -0700, Daniel Hjorth wrote:
> > > > https://fedorahosted.org/sssd/ticket/2592
> > > >
> > > > If there is an error after ccname_file_dummy is created but before it
is
> > > > renamed then the file isn't removed. This can cause a lot of
files to
> > > > be created and take up inodes in a filesystem.
> > >
> > > > From f50db04f5d0cf73edb2e8aa27da9450562dae08e Mon Sep 17 00:00:00
2001
> > > > From: Daniel Hjorth <dh(a)dhjorth.com>
> > > > Date: Wed, 25 Feb 2015 13:07:35 -0700
> > > > Subject: [PATCH] LDAP: unlink ccname_file_dummy if there is an error
> > > >
> > > > https://fedorahosted.org/sssd/ticket/2592
> > > >
> > > > If there is an error after ccname_file_dummy is created but before it
is
> > > > renamed then the file isn't removed. This can cause a lot of
files to be
> > > > created and take up inodes in a filesystem.
> > > > ---
> > > > src/providers/ldap/ldap_child.c | 1 +
> > > > 1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/src/providers/ldap/ldap_child.c
b/src/providers/ldap/ldap_child.c
> > > > index
e9aebf5a6319b5d848aadfb27061099fc153a7f6..676f0c7c5033f1941feb24d070cbff253bdbadbf 100644
> > > > --- a/src/providers/ldap/ldap_child.c
> > > > +++ b/src/providers/ldap/ldap_child.c
> > > > @@ -499,6 +499,7 @@ done:
> > > > if (krberr != 0) KRB5_SYSLOG(krberr);
> > > > if (keytab) krb5_kt_close(context, keytab);
> > > > if (context) krb5_free_context(context);
> > > > + if (ccname_file_dummy) unlink(ccname_file_dummy);
> > > > return krberr;
> > > > }
> > > >
> > > > --
> > > > 1.9.3
> > > >
> > >
> > > Thank you very much for catching the issue and the patch! I wonder if it
> > > would be better to call talloc_zfree(ccname_file_dummy) in case the
> > > rename(2) succeeds in order to only call the unlink if the dummy file is
> > > still around?
> >
> > Hi Jakub,
> >
> > You're welcome, I'm happy to help. I'm not sure I understand your
> > question though. Because talloc_free(tmp_ctx) is being called
> > wouldn't that already free the memory for ccname_file_dummy?
>
> Ah, yes :-) we need to move talloc_free(tmp_ctx) as well.
>
> > Maybe
> > something like this would prevent the unlink if the rename is
> > successful?
> >
> > if (ccname_file_dummy && krberr != 0) {
> > unlink(ccname_file_dummy);
> > }
> >
> > Daniel
>
> Could we simply make the end of the function look like this?
>
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ret = rename(ccname_file_dummy, ccname_file);
> if (ret == -1) {
> ret = errno;
> DEBUG(SSSDBG_CRIT_FAILURE,
> "rename failed [%d][%s].\n", ret, strerror(ret));
> goto done;
> }
> ccname_file_dummy = NULL; /* optional, just to avoid seeing ENOENT failures when
stracing the ldap_child and ccname_file_dummy was already renamed */
>
> done:
> if (krberr != 0) KRB5_SYSLOG(krberr);
> if (keytab) krb5_kt_close(context, keytab);
> if (context) krb5_free_context(context);
> if (ccname_file_dummy) unlink(ccname_file_dummy); /* Added */
> talloc_free(tmp_ctx); /* Moved to avoid freeing
ccname_file_dummy which is allocated on top of tmp_ctx */
> return krberr;
> }
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Would that work for you?
Yeah that looks great. I've attached the modified patch, does it look
correct?
Daniel
From 992df27bbf238a342d044584a6ebdcb381b60c52 Mon Sep 17 00:00:00
2001
From: Daniel Hjorth <dh(a)dhjorth.com>
Date: Wed, 25 Feb 2015 13:07:35 -0700
Subject: [PATCH] LDAP: unlink ccname_file_dummy if there is an error
https://fedorahosted.org/sssd/ticket/2592
If there is an error after ccname_file_dummy is created but before it is
renamed then the file isn't removed. This can cause a lot of files to be
created and take up inodes in a filesystem.
---
src/providers/ldap/ldap_child.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c
index e9aebf5a6319b5d848aadfb27061099fc153a7f6..cee1e3faa6d4857e8456f5ef421e2c925bd08b17
100644
--- a/src/providers/ldap/ldap_child.c
+++ b/src/providers/ldap/ldap_child.c
@@ -489,16 +489,18 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
"rename failed [%d][%s].\n", ret, strerror(ret));
goto done;
}
+ ccname_file_dummy = NULL;
krberr = 0;
*ccname_out = talloc_steal(memctx, ccname);
*expire_time_out = my_creds.times.endtime - kdc_time_offset;
done:
- talloc_free(tmp_ctx);
if (krberr != 0) KRB5_SYSLOG(krberr);
if (keytab) krb5_kt_close(context, keytab);
if (context) krb5_free_context(context);
+ if (ccname_file_dummy) unlink(ccname_file_dummy);
It would be nice if you can check the return code of unlink(). I know it
is a bit useless because we can't do anything if unlink() fails. But we
try to keep the SSSD build free of warning even with high warning levels
and when using static analyzers like Coverity or the one from clang. I
would expect that at least some of them will complain about the
unchecked return value. Additionally a debug message telling why unlink()
failed might help debug issue when the dummy file is not removed
properly.
bye,
Sumit
+ talloc_free(tmp_ctx);
return krberr;
}
--
1.9.3
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel