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 <jhrozek(a)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 <jhrozek(a)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