[PATCH] Zombie processes on Samba AD DC

[Ralph Böhme]

Hi!

Attached is a patch to fix an issue in samba_runcmd_send where zombie processes
would be left around in case the request times out or is freed.

https://bugzilla.samba.org/show_bug.cgi?id=13062

Already signed-off by metze & myself, will push later if noone objects.

Cheerio!
-slow
-------------- next part --------------
From 71ef0487d507ec4d14928bf9087168b1944a4c22 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 29 Sep 2017 12:45:24 +0200
Subject: [PATCH 1/4] lib/util/run_cmd: prevent zombies in samba_runcmd_send on
 timeout

Ensure the state desctructor calls tfork_destroy to reap the waiter and
worker processes. Otherwise we leave the waiter process as a zombie
behind us as we never call waitpid on it in case of a timeout
or talloc_free() from the caller.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=13062

Pair-programmed-with: Stefan Metzmacher <metze at samba.org>

Signed-off-by: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Ralph Boehme <slow at samba.org>
---
 lib/util/util_runcmd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/util/util_runcmd.c b/lib/util/util_runcmd.c
index 6077fdd..ef3402a 100644
--- a/lib/util/util_runcmd.c
+++ b/lib/util/util_runcmd.c
@@ -34,11 +34,10 @@
 
 static int samba_runcmd_state_destructor(struct samba_runcmd_state *state)
 {
-	if (state->pid > 0) {
-		kill(state->pid, SIGKILL);
-		waitpid(state->pid, NULL, 0);
-		state->pid = -1;
+	if (state->tfork != NULL) {
+		tfork_destroy(&state->tfork);
 	}
+	state->pid = -1;
 
 	if (state->fd_stdin != -1) {
 		close(state->fd_stdin);
@@ -275,6 +274,7 @@ static void samba_runcmd_io_handler(struct tevent_context *ev,
 			tevent_req_error(req, errno);
 			return;
 		}
+		state->pid = -1;
 		TALLOC_FREE(fde);
 
 		if (WIFEXITED(status)) {
-- 
1.9.1


From 0b4e95567cfd7863f7f832f6849486e9b45ca872 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 29 Sep 2017 13:06:08 +0200
Subject: [PATCH 2/4] lib/util/run_cmd: ensure fd_stdin gets set to -1 in the
 destructor

Bug: https://bugzilla.samba.org/show_bug.cgi?id=13062

Pair-programmed-with: Stefan Metzmacher <metze at samba.org>

Signed-off-by: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Ralph Boehme <slow at samba.org>
---
 lib/util/util_runcmd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/util/util_runcmd.c b/lib/util/util_runcmd.c
index ef3402a..9c6cb30 100644
--- a/lib/util/util_runcmd.c
+++ b/lib/util/util_runcmd.c
@@ -41,6 +41,7 @@ static int samba_runcmd_state_destructor(struct samba_runcmd_state *state)
 
 	if (state->fd_stdin != -1) {
 		close(state->fd_stdin);
+		state->fd_stdin = -1;
 	}
 	return 0;
 }
-- 
1.9.1


From 0d7024c02d51f0a7fb07568022e8707ce72b47e4 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 29 Sep 2017 13:07:26 +0200
Subject: [PATCH 3/4] lib/util/run_cmd: remove a printf

Bug: https://bugzilla.samba.org/show_bug.cgi?id=13062

Pair-programmed-with: Stefan Metzmacher <metze at samba.org>

Signed-off-by: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Ralph Boehme <slow at samba.org>
---
 lib/util/util_runcmd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/util/util_runcmd.c b/lib/util/util_runcmd.c
index 9c6cb30..b2f0d9f 100644
--- a/lib/util/util_runcmd.c
+++ b/lib/util/util_runcmd.c
@@ -110,7 +110,6 @@ struct tevent_req *samba_runcmd_send(TALLOC_CTX *mem_ctx,
 
 	state->tfork = tfork_create();
 	if (state->tfork == NULL) {
-		printf("state->tfork == NULL\n");
 		close(p1[0]);
 		close(p1[1]);
 		close(p2[0]);
-- 
1.9.1


From 4a6ee4f3f1e10a3c3125147e2652989806321887 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 29 Sep 2017 13:07:53 +0200
Subject: [PATCH 4/4] lib/util/run_cmd: use a cleanup function instead of a
 destructor

Bug: https://bugzilla.samba.org/show_bug.cgi?id=13062

Pair-programmed-with: Stefan Metzmacher <metze at samba.org>

Signed-off-by: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Ralph Boehme <slow at samba.org>
---
 lib/util/util_runcmd.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/util/util_runcmd.c b/lib/util/util_runcmd.c
index b2f0d9f..42d84a8 100644
--- a/lib/util/util_runcmd.c
+++ b/lib/util/util_runcmd.c
@@ -32,8 +32,12 @@
 #include "../lib/util/tfork.h"
 #include "../lib/util/sys_rw.h"
 
-static int samba_runcmd_state_destructor(struct samba_runcmd_state *state)
+static void samba_runcmd_cleanup_fn(struct tevent_req *req,
+				    enum tevent_req_state req_state)
 {
+	struct samba_runcmd_state *state = tevent_req_data(
+		req, struct samba_runcmd_state);
+
 	if (state->tfork != NULL) {
 		tfork_destroy(&state->tfork);
 	}
@@ -43,7 +47,6 @@ static int samba_runcmd_state_destructor(struct samba_runcmd_state *state)
 		close(state->fd_stdin);
 		state->fd_stdin = -1;
 	}
-	return 0;
 }
 
 static void samba_runcmd_io_handler(struct tevent_context *ev,
@@ -140,7 +143,7 @@ struct tevent_req *samba_runcmd_send(TALLOC_CTX *mem_ctx,
 		smb_set_close_on_exec(state->fd_stderr);
 		smb_set_close_on_exec(state->fd_status);
 
-		talloc_set_destructor(state, samba_runcmd_state_destructor);
+		tevent_req_set_cleanup_fn(req, samba_runcmd_cleanup_fn);
 
 		state->fde_stdout = tevent_add_fd(ev, state,
 						  state->fd_stdout,
-- 
1.9.1