[Midnightbsd-cvs] src [8877] trunk/sbin/devd: fix a descriptor leak

[laffer1 at midnightbsd.org]

Revision: 8877
          http://svnweb.midnightbsd.org/src/?rev=8877
Author:   laffer1
Date:     2016-09-26 18:18:57 -0400 (Mon, 26 Sep 2016)
Log Message:
-----------
fix a descriptor leak

Modified Paths:
--------------
    trunk/sbin/devd/devd.8
    trunk/sbin/devd/devd.cc

Modified: trunk/sbin/devd/devd.8
===================================================================
--- trunk/sbin/devd/devd.8	2016-09-26 13:29:31 UTC (rev 8876)
+++ trunk/sbin/devd/devd.8	2016-09-26 22:18:57 UTC (rev 8877)
@@ -35,6 +35,7 @@
 .Nm
 .Op Fl Ddn
 .Op Fl f Ar file
+.Op Fl l Ar num
 .Sh DESCRIPTION
 The
 .Nm
@@ -55,6 +56,12 @@
 If option
 .Fl f
 is specified more than once, the last file specified is used.
+.It Fl l Ar num
+Limit concurrent
+.Pa /var/run/devd.pipe
+connections to
+.Ar num .
+The default connection limit is 10.
 .It Fl n
 Do not process all pending events before becoming a daemon.
 Instead, call daemon right away.

Modified: trunk/sbin/devd/devd.cc
===================================================================
--- trunk/sbin/devd/devd.cc	2016-09-26 13:29:31 UTC (rev 8876)
+++ trunk/sbin/devd/devd.cc	2016-09-26 22:18:57 UTC (rev 8877)
@@ -80,6 +80,7 @@
 #include <fcntl.h>
 #include <libutil.h>
 #include <paths.h>
+#include <poll.h>
 #include <regex.h>
 #include <signal.h>
 #include <stdlib.h>
@@ -805,23 +806,58 @@
 	return (fd);
 }
 
+unsigned int max_clients = 10;	/* Default, can be overriden on cmdline. */
+unsigned int num_clients;
 list<int> clients;
 
 void
 notify_clients(const char *data, int len)
 {
-	list<int> bad;
-	list<int>::const_iterator i;
+	list<int>::iterator i;
 
-	for (i = clients.begin(); i != clients.end(); ++i) {
-		if (write(*i, data, len) <= 0) {
-			bad.push_back(*i);
+	/*
+	 * Deliver the data to all clients.  Throw clients overboard at the
+	 * first sign of trouble.  This reaps clients who've died or closed
+	 * their sockets, and also clients who are alive but failing to keep up
+	 * (or who are maliciously not reading, to consume buffer space in
+	 * kernel memory or tie up the limited number of available connections).
+	 */
+	for (i = clients.begin(); i != clients.end(); ) {
+		if (write(*i, data, len) != len) {
+			--num_clients;
 			close(*i);
-		}
+			i = clients.erase(i);
+		} else
+			++i;
 	}
+}
 
-	for (i = bad.begin(); i != bad.end(); ++i)
-		clients.erase(find(clients.begin(), clients.end(), *i));
+void
+check_clients(void)
+{
+	int s;
+	struct pollfd pfd;
+	list<int>::iterator i;
+
+	/*
+	 * Check all existing clients to see if any of them have disappeared.
+	 * Normally we reap clients when we get an error trying to send them an
+	 * event.  This check eliminates the problem of an ever-growing list of
+	 * zombie clients because we're never writing to them on a system
+	 * without frequent device-change activity.
+	 */
+	pfd.events = 0;
+	for (i = clients.begin(); i != clients.end(); ) {
+		pfd.fd = *i;
+		s = poll(&pfd, 1, 0);
+		if ((s < 0 && s != EINTR ) ||
+		    (s > 0 && (pfd.revents & POLLHUP))) {
+			--num_clients;
+			close(*i);
+			i = clients.erase(i);
+		} else
+			++i;
+	}
 }
 
 void
@@ -829,9 +865,18 @@
 {
 	int s;
 
+	/*
+	 * First go reap any zombie clients, then accept the connection, and
+	 * shut down the read side to stop clients from consuming kernel memory
+	 * by sending large buffers full of data we'll never read.
+	 */
+	check_clients();
 	s = accept(fd, NULL, NULL);
-	if (s != -1)
+	if (s != -1) {
+		shutdown(s, SHUT_RD);
 		clients.push_back(s);
+		++num_clients;
+	}
 }
 
 static void
@@ -842,6 +887,7 @@
 	char buffer[DEVCTL_MAXBUF];
 	int once = 0;
 	int server_fd, max_fd;
+	int accepting;
 	timeval tv;
 	fd_set fds;
 
@@ -851,6 +897,7 @@
 	if (fcntl(fd, F_SETFD, FD_CLOEXEC) != 0)
 		err(1, "Can't set close-on-exec flag on devctl");
 	server_fd = create_socket(PIPE);
+	accepting = 1;
 	max_fd = max(fd, server_fd) + 1;
 	while (1) {
 		if (romeo_must_die)
@@ -873,15 +920,38 @@
 				once++;
 			}
 		}
+		/*
+		 * When we've already got the max number of clients, stop
+		 * accepting new connections (don't put server_fd in the set),
+		 * shrink the accept() queue to reject connections quickly, and
+		 * poll the existing clients more often, so that we notice more
+		 * quickly when any of them disappear to free up client slots.
+		 */
 		FD_ZERO(&fds);
 		FD_SET(fd, &fds);
-		FD_SET(server_fd, &fds);
-		rv = select(max_fd, &fds, NULL, NULL, NULL);
+		if (num_clients < max_clients) {
+			if (!accepting) {
+				listen(server_fd, max_clients);
+				accepting = 1;
+			}
+			FD_SET(server_fd, &fds);
+			tv.tv_sec = 60;
+			tv.tv_usec = 0;
+		} else {
+			if (accepting) {
+				listen(server_fd, 0);
+				accepting = 0;
+			}
+			tv.tv_sec = 2;
+			tv.tv_usec = 0;
+		}
+		rv = select(max_fd, &fds, NULL, NULL, &tv);
 		if (rv == -1) {
 			if (errno == EINTR)
 				continue;
 			err(1, "select");
-		}
+		} else if (rv == 0)
+			check_clients();
 		if (FD_ISSET(fd, &fds)) {
 			rv = read(fd, buffer, sizeof(buffer) - 1);
 			if (rv > 0) {
@@ -1000,7 +1070,8 @@
 static void
 usage()
 {
-	fprintf(stderr, "usage: %s [-Ddn] [-f file]\n", getprogname());
+	fprintf(stderr, "usage: %s [-Ddn] [-l connlimit] [-f file]\n",
+	    getprogname());
 	exit(1);
 }
 
@@ -1029,7 +1100,7 @@
 	int ch;
 
 	check_devd_enabled();
-	while ((ch = getopt(argc, argv, "Ddf:n")) != -1) {
+	while ((ch = getopt(argc, argv, "Ddf:l:n")) != -1) {
 		switch (ch) {
 		case 'D':
 			Dflag++;
@@ -1040,6 +1111,9 @@
 		case 'f':
 			configfile = optarg;
 			break;
+		case 'l':
+			max_clients = MAX(1, strtoul(optarg, NULL, 0));
+			break;
 		case 'n':
 			nflag++;
 			break;