conntrackd: fix deadlock due to wrong nested signal blocking

The existing code may nest several signal blocking and unblocking
calls in different paths of the code. This may result in deadlocks
while receiving signals.

This patch simplifies the signal blocking approach. Now signals
are blocked in three paths:

1) Internal timers handling, while running timer callback
   for expired timers.

2) File descriptor handling, while running file descriptor
   callbacks.

3) While handling signals, to avoid that SIGINT and SIGTERM
   in a row results in a deadlock.

Thanks a lot to Ulrich Weber <ulrich.weber@sophos.com> for
discussing a fix for this problem.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

2 files changed, 9 insertions, 16 deletions

diff --git a/src/process.c b/src/process.c
index 9b9734c..7f0a395 100644
--- a/src/process.c
+++ b/src/process.c
@@ -27,25 +27,19 @@ int fork_process_new(int type, int flags, void (*cb)(void *data), void *data)
 	struct child_process *c, *this;
 	int pid;
 
-	/* block SIGCHLD to avoid the access of the list concurrently */
-	sigprocmask(SIG_BLOCK, &STATE(block), NULL);
-
 	/* We only want one process of this type at the same time. This is
 	 * useful if you want to prevent two child processes from accessing
 	 * a shared descriptor at the same time. */
 	if (flags & CTD_PROC_F_EXCL) {
 		list_for_each_entry(this, &process_list, head) {
 			if (this->type == type) {
-				sigprocmask(SIG_UNBLOCK, &STATE(block), NULL);
 				return -1;
 			}
 		}
 	}
 	c = calloc(sizeof(struct child_process), 1);
-	if (c == NULL) {
-		sigprocmask(SIG_UNBLOCK, &STATE(block), NULL);
+	if (c == NULL)
 		return -1;
-	}
 
 	c->type = type;
 	c->cb = cb;
@@ -55,8 +49,6 @@ int fork_process_new(int type, int flags, void (*cb)(void *data), void *data)
 	if (c->pid > 0)
 		list_add(&c->head, &process_list);
 
-	sigprocmask(SIG_UNBLOCK, &STATE(block), NULL);
-
 	return pid;
 }
 
@@ -89,7 +81,6 @@ void fork_process_dump(int fd)
 	char buf[4096];
 	int size = 0;
 
-	sigprocmask(SIG_BLOCK, &STATE(block), NULL);
 	list_for_each_entry(this, &process_list, head) {
 		size += snprintf(buf+size, sizeof(buf),
 				 "PID=%u type=%s\n",
@@ -97,7 +88,6 @@ void fork_process_dump(int fd)
 				 this->type < CTD_PROC_MAX ?
 				 process_type_to_name[this->type] : "unknown");
 	}
-	sigprocmask(SIG_UNBLOCK, &STATE(block), NULL);
 
 	send(fd, buf, size, 0);
 }
diff --git a/src/run.c b/src/run.c
index 3337694..44a179f 100644
--- a/src/run.c
+++ b/src/run.c
@@ -40,10 +40,15 @@
 #include <time.h>
 #include <fcntl.h>
 
-void killer(int foo)
+void killer(int signal)
 {
-	/* no signals while handling signals */
-	sigprocmask(SIG_BLOCK, &STATE(block), NULL);
+	/* Signals are re-entrant, disable signal handling to avoid problems
+	 * in case we receive SIGINT and SIGTERM in a row. This function is
+	 * also called via -k from the unix socket context, we already disabled
+	 * signals in that path, so don't do it.
+	 */
+	if (signal)
+		sigprocmask(SIG_BLOCK, &STATE(block), NULL);
 
 	local_server_destroy(&STATE(local));
 
@@ -58,8 +63,6 @@ void killer(int foo)
 	dlog(LOG_NOTICE, "---- shutdown received ----");
 	close_log();
 
-	sigprocmask(SIG_UNBLOCK, &STATE(block), NULL);
-
 	exit(0);
 }