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); + 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
