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