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
Regards
--
Petr^4 Čech