On Fri, Feb 27, 2015 at 10:13:28AM +0100, Jakub Hrozek wrote: > On Fri, Feb 27, 2015 at 09:59:57AM +0100, Sumit Bose wrote: > > > 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 > > That's a good point, I think using the 'ret' variable would be fine, > since we return krberr.
The modified patch is attached. I used the same DEBUG that is being used in the rename but changed the failure to MINOR because it doesn't seem like a CRIT to me. Thoughts? Daniel
>From 306a274089965cd7936de289b64ac58555bccd01 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 | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c index e9aebf5a6319b5d848aadfb27061099fc153a7f6..774cff9c2d942b589e69933d3f201f6245fa904d 100644 --- a/src/providers/ldap/ldap_child.c +++ b/src/providers/ldap/ldap_child.c @@ -489,16 +489,26 @@ 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) { + DEBUG(SSSDBG_TRACE_INTERNAL, "Unlinking [%s]\n", ccname_file_dummy); + ret = unlink(ccname_file_dummy); + if (ret == -1) { + ret = errno; + DEBUG(SSSDBG_MINOR_FAILURE, + "Unlink failed [%d][%s].\n", ret, strerror(ret)); + } + } + 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
