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.
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@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?
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@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? Maybe something like this would prevent the unlink if the rename is successful?
if (ccname_file_dummy && krberr != 0) { unlink(ccname_file_dummy); }
Daniel
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@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?
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@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
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@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@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
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.
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
On Fri, Feb 27, 2015 at 12:04:59PM -0700, Daniel Hjorth wrote:
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
ACK
I tested by tricking ldap_child into failure in gdb: 417 krberr = krb5_get_init_creds_keytab(context, &my_creds, kprinc, (gdb) n 419 krb5_kt_close(context, keytab); (gdb) 420 keytab = NULL; (gdb) 421 if (krberr) { (gdb) set krberr=123 (gdb) p krberr $1 = 123 (gdb) n 422 DEBUG(SSSDBG_FATAL_FAILURE, (gdb) 425 sss_log(SSS_LOG_ERR, (gdb) 430 goto done; (gdb) 499 if (krberr != 0) KRB5_SYSLOG(krberr); (gdb) 500 if (keytab) krb5_kt_close(context, keytab); (gdb) 501 if (context) krb5_free_context(context); (gdb) 502 if (ccname_file_dummy) { (gdb) 503 DEBUG(SSSDBG_TRACE_INTERNAL, "Unlinking [%s]\n", ccname_file_dummy); (gdb) 504 ret = unlink(ccname_file_dummy); (gdb) 505 if (ret == -1) { (gdb) 511 talloc_free(tmp_ctx); (gdb) c Continuing.
Thanks for the patch again!
On Wed, Mar 04, 2015 at 02:25:54PM +0100, Jakub Hrozek wrote:
On Fri, Feb 27, 2015 at 12:04:59PM -0700, Daniel Hjorth wrote:
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
ACK
* master: 2b20ff2e33ad3993a9cad910c4b4b828513613df * sssd-1-12: 0b5036e4c652e6983a3352c045c8701d6573587b
sssd-devel@lists.fedorahosted.org