On Fri, May 27, 2016 at 10:58:23AM +0200, Petr Cech wrote: > On 05/19/2016 10:17 PM, Jakub Hrozek wrote: > > Hi, > > > > the attached two patches fix issues with handling of pipes towards our > > child processes. The first patch is more important as the leak occurs > > always. I don't think we need to write_fd to the renewal process at all > > at this point, but wanted to do a more minimal fix first. This one fixes > > #3017. > > > > The second patch is more about defensive programming and fixes #3006. > > Hello Jakub, > > I tested your patch set on SSSD client connected to MS AD server with option > "ad_machine_account_password_renewal_opts = 5:5" in domain section. And I > used command > "ls -l /proc/$(pidof sssd_be)/fd | wc -l && lsof | grep $(pidof sssd_be) | > wc -l" for counting handled files, pipes. It worked how you described. > > CI tests passed: > http://sssd-ci.duckdns.org/logs/job/44/02/summary.html > > The code looks good to me. > > > 0001-AD-Do-not-leak-file-descriptors-during-machine-passw.patch > > > > > > From 2f88d95d8c72f1333ce1fc12a1ba18249447c11e Mon Sep 17 00:00:00 2001 > > From: Jakub Hrozek <jhro...@redhat.com> > > Date: Thu, 19 May 2016 17:24:51 +0200 > > Subject: [PATCH 1/2] AD: Do not leak file descriptors during machine > > password > > renewal > > > > Resolves: > > https://fedorahosted.org/sssd/ticket/3017 > > > > The AD renewal task was opening a pipe to write to the child process but > > never closed it, leaking the fd. This patch uses a desctructor we > > already use for pipes towards other child processes. > > --- > > src/providers/ad/ad_machine_pw_renewal.c | 28 +++++++++++++++------------- > > 1 file changed, 15 insertions(+), 13 deletions(-) > > => ACK > > > > 0002-Do-not-leak-fds-in-case-of-failures-setting-up-a-chi.patch > > > > > > From 3cf7d0b0df090ee90ccab9921c7235b9c5ddc02f Mon Sep 17 00:00:00 2001 > > From: Jakub Hrozek <jhro...@redhat.com> > > Date: Thu, 19 May 2016 18:12:17 +0200 > > Subject: [PATCH 2/2] Do not leak fds in case of failures setting up a child > > process > > > > Resolves: > > https://fedorahosted.org/sssd/ticket/3006 > > > > The handling of open pipes in failure cases was suboptimal. Moreover, > > the faulty logic was copied all over the place. This patch introduces > > helper macros to: > > - initialize the pipe endpoints to -1 > > - close an open pipe fd and set it to -1 afterwards > > - close both ends unless already closed > > These macros are used in the child handling code. > > > > The patch also uses child_io_destructor in the p11_child code for safer > > fd handling. > > --- > > src/providers/ad/ad_gpo.c | 36 ++++++++++++++------------ > > src/providers/ad/ad_machine_pw_renewal.c | 10 +++++--- > > src/providers/dp_dyndns.c | 16 ++++++++---- > > src/providers/ipa/ipa_selinux.c | 27 +++++++++++--------- > > src/providers/krb5/krb5_child_handler.c | 42 > > +++++++++++++++--------------- > > src/providers/ldap/sdap_child_helpers.c | 36 ++++++++++++++------------ > > src/responder/pam/pamsrv_p11.c | 44 > > ++++++++++++++++++-------------- > > src/util/util.h | 14 ++++++++++ > > 8 files changed, 132 insertions(+), 93 deletions(-) > > => ACK
* master: * 518f5b83fd546e3188da01e4743ddb27a574e08f * 45e11be651dbd3855a35de4abd2922e5b9d4b963 * sssd-1-13: * 312d211e03b9f3769a0362f1767cc59792e32746 * 2fb750062a665dbf318b5ffb2e53f1060e43b8dc _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org