[PATCH] Cleanup signal handling in source4/smbd/process_standard.c
Jeremy Allison
jra at samba.org
Sat Apr 8 05:03:09 UTC 2017
Hi Ralph and Uri,
I'm working towards removing the allocation
of the main server event context from the
talloc_autofree_context() which causes problems
with ultimately making the code thread-safe
and causes destructor ordering problems when
called from atexit() (as I already found).
I'm still finishing up that patchset (it
already passes make test but isn't quite as clean
as I'd like), but here is a prerequisite patch
that cleans up the freeing of the tevent_context
and signal handling on exit in the source4
process_standard.c code.
Currently the fork()'ed processes in here
inherit their SIGHUP and SIGTERM handlers
from the parent process in source4/smbd/server.c,
but these are old-style signal handlers,
installed with CatchSignal(), not new
tevent-style ones that are safe to make
non signal-safe calls. My follow up patch
fixes this of course, but it's easier to
start fixing up the child process code to
make these signal handlers tevent signal
handlers first.
Passes full make test. This won't affect
Andrew's LDAP multi-process patch and can
go in either before or after that.
Description follows :
#1: Move talloc_free(ev) call to just before exit().
#2: Add missing talloc_free(ev) before exit().
#3: Add missing return checking for tevent_add_fd calls.
#4: Add SIGHUP tevent signal handler - will override
old handler inherited from parent (same functionality).
#5: Add SIGTERM tevent signal handler - will override
old handler inherited from parent (same functionality).
Please review and push if happy !
Jeremy.
-------------- next part --------------
>From e28d8bb9ea40fbca9cebd48171d91e446cbc2f19 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 7 Apr 2017 15:08:13 -0700
Subject: [PATCH 1/5] s4: process_standard: Move talloc_free of event context
so it is last thing freed before exit().
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source4/smbd/process_standard.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/source4/smbd/process_standard.c b/source4/smbd/process_standard.c
index ca93f93..f786d4d 100644
--- a/source4/smbd/process_standard.c
+++ b/source4/smbd/process_standard.c
@@ -388,12 +388,13 @@ _NORETURN_ static void standard_terminate(struct tevent_context *ev, struct load
{
DEBUG(2,("standard_terminate: reason[%s]\n",reason));
- talloc_free(ev);
-
/* this reload_charcnv() has the effect of freeing the iconv context memory,
which makes leak checking easier */
reload_charcnv(lp_ctx);
+ /* Always free event context last before exit. */
+ talloc_free(ev);
+
/* terminate this process */
exit(0);
}
--
2.7.4
>From 371e720039dd28841090306ef13f2a9c211d9046 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 7 Apr 2017 15:10:09 -0700
Subject: [PATCH 2/5] s4: process_standard: Always free tevent_context before
exit().
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source4/smbd/process_standard.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/source4/smbd/process_standard.c b/source4/smbd/process_standard.c
index f786d4d..766b996 100644
--- a/source4/smbd/process_standard.c
+++ b/source4/smbd/process_standard.c
@@ -65,6 +65,7 @@ static void standard_pipe_handler(struct tevent_context *event_ctx, struct teven
uint16_t flags, void *private_data)
{
DEBUG(10,("Child %d exiting\n", (int)getpid()));
+ talloc_free(event_ctx);
exit(0);
}
--
2.7.4
>From edb0dabfdaa7c04ca41993d6d928544f0cd3e6b2 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 7 Apr 2017 15:12:51 -0700
Subject: [PATCH 3/5] s4: process_standard: Add return checking for
tevent_add_fd() to standard_accept_connection() and standard_new_task().
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source4/smbd/process_standard.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/source4/smbd/process_standard.c b/source4/smbd/process_standard.c
index 766b996..39edbfb 100644
--- a/source4/smbd/process_standard.c
+++ b/source4/smbd/process_standard.c
@@ -212,6 +212,7 @@ static void standard_accept_connection(struct tevent_context *ev,
pid_t pid;
struct socket_address *c, *s;
struct standard_child_state *state;
+ struct tevent_fd *fde = NULL;
state = setup_standard_child_pipe(ev, NULL);
if (state == NULL) {
@@ -278,8 +279,12 @@ static void standard_accept_connection(struct tevent_context *ev,
smb_panic("Failed to re-initialise imessaging after fork");
}
- tevent_add_fd(ev, ev, child_pipe[0], TEVENT_FD_READ,
+ fde = tevent_add_fd(ev, ev, child_pipe[0], TEVENT_FD_READ,
standard_pipe_handler, NULL);
+ if (fde == NULL) {
+ smb_panic("Failed to add fd handler after fork");
+ }
+
if (child_pipe[1] != -1) {
close(child_pipe[1]);
child_pipe[1] = -1;
@@ -319,6 +324,7 @@ static void standard_new_task(struct tevent_context *ev,
pid_t pid;
NTSTATUS status;
struct standard_child_state *state;
+ struct tevent_fd *fde = NULL;
state = setup_standard_child_pipe(ev, service_name);
if (state == NULL) {
@@ -361,8 +367,11 @@ static void standard_new_task(struct tevent_context *ev,
smb_panic("Failed to re-initialise imessaging after fork");
}
- tevent_add_fd(ev, ev, child_pipe[0], TEVENT_FD_READ,
+ fde = tevent_add_fd(ev, ev, child_pipe[0], TEVENT_FD_READ,
standard_pipe_handler, NULL);
+ if (fde == NULL) {
+ smb_panic("Failed to add fd handler after fork");
+ }
if (child_pipe[1] != -1) {
close(child_pipe[1]);
child_pipe[1] = -1;
--
2.7.4
>From 97c1f70aa651263c107a44cb58889772208586de Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 7 Apr 2017 15:31:57 -0700
Subject: [PATCH 4/5] s4: process_standard: Add tevent SIGHUP signal handler to
standard_accept_connection() and standard_new_task().
This makes us independent of parent SIGHUP signal handling.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source4/smbd/process_standard.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/source4/smbd/process_standard.c b/source4/smbd/process_standard.c
index 39edbfb..776e497 100644
--- a/source4/smbd/process_standard.c
+++ b/source4/smbd/process_standard.c
@@ -29,6 +29,7 @@
#include "param/param.h"
#include "ldb_wrap.h"
#include "lib/messaging/messaging.h"
+#include "lib/util/debug.h"
struct standard_child_state {
const char *name;
@@ -58,6 +59,14 @@ static void standard_model_init(void)
}
}
+static void sighup_signal_handler(struct tevent_context *ev,
+ struct tevent_signal *se,
+ int signum, int count, void *siginfo,
+ void *private_data)
+{
+ debug_schedule_reopen_logs();
+}
+
/*
handle EOF on the parent-to-all-children pipe in the child
*/
@@ -213,6 +222,7 @@ static void standard_accept_connection(struct tevent_context *ev,
struct socket_address *c, *s;
struct standard_child_state *state;
struct tevent_fd *fde = NULL;
+ struct tevent_signal *se = NULL;
state = setup_standard_child_pipe(ev, NULL);
if (state == NULL) {
@@ -290,6 +300,16 @@ static void standard_accept_connection(struct tevent_context *ev,
child_pipe[1] = -1;
}
+ se = tevent_add_signal(ev,
+ ev,
+ SIGHUP,
+ 0,
+ sighup_signal_handler,
+ NULL);
+ if (se == NULL) {
+ smb_panic("Failed to add SIGHUP handler after fork");
+ }
+
/* setup the process title */
c = socket_get_peer_addr(sock2, ev);
s = socket_get_my_addr(sock2, ev);
@@ -325,6 +345,7 @@ static void standard_new_task(struct tevent_context *ev,
NTSTATUS status;
struct standard_child_state *state;
struct tevent_fd *fde = NULL;
+ struct tevent_signal *se = NULL;
state = setup_standard_child_pipe(ev, service_name);
if (state == NULL) {
@@ -377,6 +398,16 @@ static void standard_new_task(struct tevent_context *ev,
child_pipe[1] = -1;
}
+ se = tevent_add_signal(ev,
+ ev,
+ SIGHUP,
+ 0,
+ sighup_signal_handler,
+ NULL);
+ if (se == NULL) {
+ smb_panic("Failed to add SIGHUP handler after fork");
+ }
+
setproctitle("task %s server_id[%d]", service_name, (int)pid);
/* setup this new task. Cluster ID is PID based for this process model */
--
2.7.4
>From c5789c72d4b716e5859733875826ec389654937b Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 7 Apr 2017 15:45:41 -0700
Subject: [PATCH 5/5] s4: process_standard: Add a simplified SIGTERM handler
based on code from source4/smbd/server.c. Use from a tevent handler added to
standard_accept_connection() and standard_new_task()
Allows us to be independnet of parent SIGTERM signal handling.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source4/smbd/process_standard.c | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/source4/smbd/process_standard.c b/source4/smbd/process_standard.c
index 776e497..92f07ad 100644
--- a/source4/smbd/process_standard.c
+++ b/source4/smbd/process_standard.c
@@ -67,6 +67,26 @@ static void sighup_signal_handler(struct tevent_context *ev,
debug_schedule_reopen_logs();
}
+static void sigterm_signal_handler(struct tevent_context *ev,
+ struct tevent_signal *se,
+ int signum, int count, void *siginfo,
+ void *private_data)
+{
+#if HAVE_GETPGRP
+ if (getpgrp() == getpid()) {
+ /*
+ * We're the process group leader, send
+ * SIGTERM to our process group.
+ */
+ DEBUG(0,("SIGTERM: killing children\n"));
+ kill(-getpgrp(), SIGTERM);
+ }
+#endif
+ DEBUG(0,("Exiting pid %u on SIGTERM\n", (unsigned int)getpid()));
+ talloc_free(ev);
+ exit(127);
+}
+
/*
handle EOF on the parent-to-all-children pipe in the child
*/
@@ -310,6 +330,16 @@ static void standard_accept_connection(struct tevent_context *ev,
smb_panic("Failed to add SIGHUP handler after fork");
}
+ se = tevent_add_signal(ev,
+ ev,
+ SIGTERM,
+ 0,
+ sigterm_signal_handler,
+ NULL);
+ if (se == NULL) {
+ smb_panic("Failed to add SIGTERM handler after fork");
+ }
+
/* setup the process title */
c = socket_get_peer_addr(sock2, ev);
s = socket_get_my_addr(sock2, ev);
@@ -408,6 +438,16 @@ static void standard_new_task(struct tevent_context *ev,
smb_panic("Failed to add SIGHUP handler after fork");
}
+ se = tevent_add_signal(ev,
+ ev,
+ SIGTERM,
+ 0,
+ sigterm_signal_handler,
+ NULL);
+ if (se == NULL) {
+ smb_panic("Failed to add SIGTERM handler after fork");
+ }
+
setproctitle("task %s server_id[%d]", service_name, (int)pid);
/* setup this new task. Cluster ID is PID based for this process model */
--
2.7.4
More information about the samba-technical
mailing list