Attachment 3245 Details for Bug 2953 – reuse startup_pipes; fix race with rexeced children too

[patch] reuse startup_pipes; fix race with rexeced children too

bz2953.diff (text/plain), 6.47 KB, created by Damien Miller on 2019-02-22 16:53:31 AEDT

(hide)

Description:

Filename:

MIME Type:

Creator: Damien Miller

Created: 2019-02-22 16:53:31 AEDT

Size: 6.47 KB

patch

obsolete

>diff --git a/sshd.c b/sshd.c
>index 21c8889..5374784 100644
>--- a/sshd.c
>+++ b/sshd.c
>@@ -194,9 +194,26 @@ u_int session_id2_len = 0;
> /* record remote hostname or ip */
> u_int utmp_len = HOST_NAME_MAX+1;
> 
>-/* options.max_startup sized array of fd ints */
>+/*
>+ * startup_pipes/flags are used for tracking children of the listening sshd
>+ * process early in their lifespans. This tracking is needed for three things:
>+ *
>+ * 1) Implementing the MaxStartups limit of concurrent unauthenticated
>+ *    connections.
>+ * 2) Avoiding a race condition for SIGHUP processing, where child processes
>+ *    may have listen_socks open that could collide with main listener process
>+ *    after it restarts.
>+ * 3) Ensuring that rexec'd sshd processes have received their initial state
>+ *    from the parent listen process before handling SIGHUP.
>+ *
>+ * Child processes signal that they have completed closure of the listen_socks
>+ * and (if applicable) received their rexec state by sending a char over their
>+ * sock. Child processes signal that authentication has completed by closing
>+ * the sock (or by exiting).
>+ */
> static int *startup_pipes = NULL;
>-static int startup_pipe;		/* in child */
>+static int *startup_flags = NULL;	/* Indicates child closed listener */
>+static int startup_pipe = -1;		/* in child */
> 
> /* variables used for privilege separation */
> int use_privsep = -1;
>@@ -850,14 +867,9 @@ server_accept_inetd(int *sock_in, int *sock_out)
> {
> 	int fd;
> 
>-	startup_pipe = -1;
> 	if (rexeced_flag) {
> 		close(REEXEC_CONFIG_PASS_FD);
> 		*sock_in = *sock_out = dup(STDIN_FILENO);
>-		if (!debug_flag) {
>-			startup_pipe = dup(REEXEC_STARTUP_PIPE_FD);
>-			close(REEXEC_STARTUP_PIPE_FD);
>-		}
> 	} else {
> 		*sock_in = dup(STDIN_FILENO);
> 		*sock_out = dup(STDOUT_FILENO);
>@@ -978,8 +990,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 = 0, lameduck = 0;
> 	int startup_p[2] = { -1 , -1 };
>+	char c = 0;
> 	struct sockaddr_storage from;
> 	socklen_t fromlen;
> 	pid_t pid;
>@@ -992,6 +1005,7 @@ server_accept_loop(int *sock_in, int *sock_out, int *newsock, int *config_s)
> 			maxfd = listen_socks[i];
> 	/* pipes connected to unauthenticated childs */
> 	startup_pipes = xcalloc(options.max_startups, sizeof(int));
>+	startup_flags = xcalloc(options.max_startups, sizeof(int));
> 	for (i = 0; i < options.max_startups; i++)
> 		startup_pipes[i] = -1;
> 
>@@ -1000,8 +1014,15 @@ server_accept_loop(int *sock_in, int *sock_out, int *newsock, int *config_s)
> 	 * the daemon is killed with a signal.
> 	 */
> 	for (;;) {
>-		if (received_sighup)
>-			sighup_restart();
>+		if (received_sighup) {
>+			if (!lameduck) {
>+				debug("Received SIGHUP; waiting for children");
>+				close_listen_socks();
>+				lameduck = 1;
>+			}
>+			if (listening <= 0)
>+				sighup_restart();
>+		}
> 		free(fdset);
> 		fdset = xcalloc(howmany(maxfd + 1, NFDBITS),
> 		    sizeof(fd_mask));
>@@ -1027,19 +1048,32 @@ 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 (startup_pipes[i] != -1 &&
>-			    FD_ISSET(startup_pipes[i], fdset)) {
>-				/*
>-				 * the read end of the pipe is ready
>-				 * if the child has closed the pipe
>-				 * after successful authentication
>-				 * or if the child has died
>-				 */
>+		for (i = 0; i < options.max_startups; i++) {
>+			if (startup_pipes[i] == -1 ||
>+			    !FD_ISSET(startup_pipes[i], fdset))
>+				continue;
>+			switch (read(startup_pipes[i], &c, sizeof(c))) {
>+			case -1:
>+				if (errno == EINTR || errno == EAGAIN)
>+					continue;
>+				error("%s: startup pipe %d: read %s",
>+				    __func__, i, strerror(errno));
>+				/* FALLTHROUGH */
>+			case 0:
>+				/* child exited or completed auth */
> 				close(startup_pipes[i]);
> 				startup_pipes[i] = -1;
> 				startups--;
>+				if (startup_flags[i])
>+					listening--;
>+				break;
>+			case 1:
>+				/* child has finished preliminaries */
>+				listening--;
>+				startup_flags[i] = 0;
>+				break;
> 			}
>+		}
> 		for (i = 0; i < num_listen_socks; i++) {
> 			if (!FD_ISSET(listen_socks[i], fdset))
> 				continue;
>@@ -1093,6 +1127,7 @@ server_accept_loop(int *sock_in, int *sock_out, int *newsock, int *config_s)
> 					if (maxfd < startup_p[0])
> 						maxfd = startup_p[0];
> 					startups++;
>+					startup_flags[j] = 1;
> 					break;
> 				}
> 
>@@ -1118,7 +1153,7 @@ server_accept_loop(int *sock_in, int *sock_out, int *newsock, int *config_s)
> 					send_rexec_state(config_s[0], cfg);
> 					close(config_s[0]);
> 				}
>-				break;
>+				return;
> 			}
> 
> 			/*
>@@ -1126,13 +1161,14 @@ server_accept_loop(int *sock_in, int *sock_out, int *newsock, int *config_s)
> 			 * the child process the connection. The
> 			 * parent continues listening.
> 			 */
>+			listening++;
> 			if ((pid = fork()) == 0) {
> 				/*
> 				 * Child.  Close the listening and
> 				 * max_startup sockets.  Start using
> 				 * the accepted socket. Reinitialize
> 				 * logging (since our pid has changed).
>-				 * We break out of the loop to handle
>+				 * We return from this function to handle
> 				 * the connection.
> 				 */
> 				startup_pipe = startup_p[1];
>@@ -1146,7 +1182,18 @@ server_accept_loop(int *sock_in, int *sock_out, int *newsock, int *config_s)
> 				    log_stderr);
> 				if (rexec_flag)
> 					close(config_s[0]);
>-				break;
>+				else {
>+					/*
>+					 * Signal parent that the preliminaries
>+					 * for this child are complete. For the
>+					 * re-exec case, this happens after the
>+					 * child has received the rexec state
>+					 * from the server.
>+					 */
>+					(void)atomicio(vwrite, startup_pipe,
>+					    "\0", 1);
>+				}
>+				return;
> 			}
> 
> 			/* Parent.  Stay in the loop. */
>@@ -1164,10 +1211,6 @@ server_accept_loop(int *sock_in, int *sock_out, int *newsock, int *config_s)
> 			}
> 			close(*newsock);
> 		}
>-
>-		/* child process check (or debug mode) */
>-		if (num_listen_socks < 0)
>-			break;
> 	}
> }
> 
>@@ -1456,8 +1499,18 @@ main(int ac, char **av)
> 	/* Fetch our configuration */
> 	if ((cfg = sshbuf_new()) == NULL)
> 		fatal("%s: sshbuf_new failed", __func__);
>-	if (rexeced_flag)
>+	if (rexeced_flag) {
> 		recv_rexec_state(REEXEC_CONFIG_PASS_FD, cfg);
>+		if (!debug_flag) {
>+			startup_pipe = dup(REEXEC_STARTUP_PIPE_FD);
>+			close(REEXEC_STARTUP_PIPE_FD);
>+			/*
>+			 * Signal parent that this child is at a point where
>+			 * they can go away if they have a SIGHUP pending.
>+			 */
>+			(void)atomicio(vwrite, startup_pipe, "\0", 1);
>+		}
>+	}
> 	else if (strcasecmp(config_file_name, "none") != 0)
> 		load_server_config(config_file_name, cfg);
>

Actions: View | Diff

Attachments on bug 2953: 3222 | 3245