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 <d...@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 <d...@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@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
