Attachment 3222 Details for Bug 2953 – Prevent restarting while children are listening

[patch] Prevent restarting while children are listening

0001-Prevent-restarting-while-children-are-listening.patch (text/plain), 6.51 KB, created by Michal Koutný on 2019-01-10 00:46:32 AEDT

(hide)

Description:

Filename:

MIME Type:

Creator: Michal Koutný

Created: 2019-01-10 00:46:32 AEDT

Size: 6.51 KB

patch

obsolete

>From af3fe4431723fa9db74e1abeae9ecd71966fb3b0 Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Michal=20Koutn=C3=BD?= <mkoutny@suse.com>
>Date: Wed, 9 Jan 2019 14:16:02 +0100
>Subject: [PATCH] Prevent restarting while children are listening
>
>There is a short window when forked children are still referencing the
>listen socket after a new client connects. When the parent handles
>SIGHUP and reloads itself, it may fail to bind the new sockets if it
>hits this window.
>
>Linux manual page socket(7) on SO_REUSEADDR:
>> For AF_INET sockets this means that a socket may bind, except when
>> there is an active listening socket bound to the address.
>
>Hence we delay the restart until we are sure there are no children listening.
>We use the mechanism of notification via closing a pipe from child to
>parent (duplicates how MaxStartups is implemented).
>---
> sshd.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 61 insertions(+), 6 deletions(-)
>
>diff --git a/sshd.c b/sshd.c
>index 3461383a..51baba4a 100644
>--- a/sshd.c
>+++ b/sshd.c
>@@ -217,6 +217,9 @@ u_int utmp_len = HOST_NAME_MAX+1;
> int *startup_pipes = NULL;
> int startup_pipe;		/* in child */
> 
>+/* options.max_startup sized array of fd ints */
>+int *listener_pipes = NULL;
>+
> /* variables used for privilege separation */
> int use_privsep = -1;
> struct monitor *pmonitor = NULL;
>@@ -257,7 +260,7 @@ close_listen_socks(void)
> }
> 
> static void
>-close_startup_pipes(void)
>+close_notification_pipes(void)
> {
> 	int i;
> 
>@@ -265,6 +268,11 @@ close_startup_pipes(void)
> 		for (i = 0; i < options.max_startups; i++)
> 			if (startup_pipes[i] != -1)
> 				close(startup_pipes[i]);
>+
>+	if (listener_pipes)
>+		for (i = 0; i < options.max_startups; i++)
>+			if (listener_pipes[i] != -1)
>+				close(listener_pipes[i]);
> }
> 
> /*
>@@ -295,7 +303,7 @@ sighup_restart(void)
> 		unlink(options.pid_file);
> 	platform_pre_restart();
> 	close_listen_socks();
>-	close_startup_pipes();
>+	close_notification_pipes();
> 	alarm(0);  /* alarm timer persists across exec */
> 	signal(SIGHUP, SIG_IGN); /* will be restored after exec */
> 	execv(saved_argv[0], saved_argv);
>@@ -1022,8 +1030,9 @@ server_accept_loop(int *sock_in, int *sock_out, int *newsock, int *config_s)
> {
> 	fd_set *fdset;
> 	int i, j, ret, maxfd;
>-	int startups = 0;
>+	int startups = 0, listening_children = 0;
> 	int startup_p[2] = { -1 , -1 };
>+	int listener_p[2] = { -1 , -1 };
> 	struct sockaddr_storage from;
> 	socklen_t fromlen;
> 	pid_t pid;
>@@ -1040,12 +1049,18 @@ server_accept_loop(int *sock_in, int *sock_out, int *newsock, int *config_s)
> 	for (i = 0; i < options.max_startups; i++)
> 		startup_pipes[i] = -1;
> 
>+	/* pipes connected to listening childs */
>+	listener_pipes = xcalloc(options.max_startups, sizeof(int));
>+	for (i = 0; i < options.max_startups; i++)
>+		listener_pipes[i] = -1;
>+
> 	/*
> 	 * Stay listening for connections until the system crashes or
> 	 * the daemon is killed with a signal.
> 	 */
> 	for (;;) {
>-		if (received_sighup)
>+		if (received_sighup &&
>+		    listening_children == 0) // TODO too heavy traffic may prevent us restarting forever
> 			sighup_restart();
> 		free(fdset);
> 		fdset = xcalloc(howmany(maxfd + 1, NFDBITS),
>@@ -1054,8 +1069,11 @@ server_accept_loop(int *sock_in, int *sock_out, int *newsock, int *config_s)
> 		for (i = 0; i < num_listen_socks; i++)
> 			FD_SET(listen_socks[i], fdset);
> 		for (i = 0; i < options.max_startups; i++)
>-			if (startup_pipes[i] != -1)
>+			if (startup_pipes[i] != -1) {
> 				FD_SET(startup_pipes[i], fdset);
>+				// assert(listener_pipes[i] != -1)
>+				FD_SET(listener_pipes[i], fdset);
>+			}
> 
> 		/* Wait in select until there is a connection. */
> 		ret = select(maxfd+1, fdset, NULL, NULL, NULL);
>@@ -1072,6 +1090,21 @@ server_accept_loop(int *sock_in, int *sock_out, int *newsock, int *config_s)
> 		if (ret < 0)
> 			continue;
> 
>+		for (i = 0; i < options.max_startups; i++)
>+			if (listener_pipes[i] != -1 &&
>+			    FD_ISSET(listener_pipes[i], fdset)) {
>+				/*
>+				 * the read end of the pipe is ready
>+				 * if the child has closed the pipe
>+				 * after closing listen socket
>+				 * or if the child has died (that is handled
>+				 * via startup_pipes)
>+				 */
>+				close(listener_pipes[i]);
>+				listener_pipes[i] = -1;
>+				listening_children--;
>+			}
>+
> 		for (i = 0; i < options.max_startups; i++)
> 			if (startup_pipes[i] != -1 &&
> 			    FD_ISSET(startup_pipes[i], fdset)) {
>@@ -1085,6 +1118,10 @@ server_accept_loop(int *sock_in, int *sock_out, int *newsock, int *config_s)
> 				startup_pipes[i] = -1;
> 				startups--;
> 			}
>+
>+		/* Delay accepting new clients after we restart */
>+		if (received_sighup)
>+			continue;
> 		for (i = 0; i < num_listen_socks; i++) {
> 			if (!FD_ISSET(listen_socks[i], fdset))
> 				continue;
>@@ -1121,6 +1158,12 @@ server_accept_loop(int *sock_in, int *sock_out, int *newsock, int *config_s)
> 				close(*newsock);
> 				continue;
> 			}
>+			if (pipe(listener_p) == -1) {
>+				close(startup_p[0]);
>+				close(startup_p[1]);
>+				close(*newsock);
>+				continue;
>+			}
> 
> 			if (rexec_flag && socketpair(AF_UNIX,
> 			    SOCK_STREAM, 0, config_s) == -1) {
>@@ -1129,6 +1172,8 @@ server_accept_loop(int *sock_in, int *sock_out, int *newsock, int *config_s)
> 				close(*newsock);
> 				close(startup_p[0]);
> 				close(startup_p[1]);
>+				close(listener_p[0]);
>+				close(listener_p[1]);
> 				continue;
> 			}
> 
>@@ -1138,6 +1183,11 @@ server_accept_loop(int *sock_in, int *sock_out, int *newsock, int *config_s)
> 					if (maxfd < startup_p[0])
> 						maxfd = startup_p[0];
> 					startups++;
>+
>+					listener_pipes[j] = listener_p[0];
>+					if (maxfd < listener_p[0])
>+						maxfd = listener_p[0];
>+					listening_children++;
> 					break;
> 				}
> 
>@@ -1157,6 +1207,8 @@ server_accept_loop(int *sock_in, int *sock_out, int *newsock, int *config_s)
> 				*sock_out = *newsock;
> 				close(startup_p[0]);
> 				close(startup_p[1]);
>+				close(listener_p[0]);
>+				close(listener_p[1]);
> 				startup_pipe = -1;
> 				pid = getpid();
> 				if (rexec_flag) {
>@@ -1183,8 +1235,10 @@ server_accept_loop(int *sock_in, int *sock_out, int *newsock, int *config_s)
> 				 */
> 				platform_post_fork_child();
> 				startup_pipe = startup_p[1];
>-				close_startup_pipes();
>+				close_notification_pipes();
> 				close_listen_socks();
>+				/* Notify parent about free'd listen sock */
>+				close(listener_p[1]);
> 				*sock_in = *newsock;
> 				*sock_out = *newsock;
> 				log_init(__progname,
>@@ -1204,6 +1258,7 @@ server_accept_loop(int *sock_in, int *sock_out, int *newsock, int *config_s)
> 				debug("Forked child %ld.", (long)pid);
> 
> 			close(startup_p[1]);
>+			close(listener_p[1]);
> 
> 			if (rexec_flag) {
> 				send_rexec_state(config_s[0], cfg);
>-- 
>2.16.4
>

Actions: View | Diff

Attachments on bug 2953: 3222 | 3245