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
