Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 5 Jan 2020 21:35:04 +0000 (UTC)
From:      Kyle Evans <kevans@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org
Subject:   svn commit: r356388 - in stable: 11/usr.sbin/inetd 12/usr.sbin/inetd
Message-ID:  <202001052135.005LZ4s0080169@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: kevans
Date: Sun Jan  5 21:35:02 2020
New Revision: 356388
URL: https://svnweb.freebsd.org/changeset/base/356388

Log:
  MFC further inetd(8) cleanup: r356204, r356215, r356217-r356218,
  r356246-r356248, r356254, r356318
  
  r356204:
  inetd: don't leak `policy` on return
  
  sep->se_policy gets a strdup'd version of policy, so we don't need it to
  stick around afterwards.
  
  While here, remove a couple of NULL checks prior to free(policy).
  
  r356215:
  inetd: knock out some clang analyze warnings
  
  chargen_dg: clang-analyze is convinced that endring could be non-NULL at
  entry, and thus wants to assume that rs == NULL. Just independently
  initialize rs if it's NULL to appease the analyzer.
  
  getconfigent: policy leaks on return
  
  free_connlist: reorganize the loop to make it clear that we're not going to
  access `conn` after it's been freed.
  
  cpmip/hashval: left-shifts performed will result in UB as we take
  signed 0xABC3D20F and left shift it by 5.
  
  r356217:
  inetd: prefer strtonum(3) to strspn(3)+atoi(3), NFC
  
  strtonum(3) does effectively the same validation as we had, but it's more
  concise.
  
  r356218:
  inetd: prefer strlcpy to strlen(3) check + strcpy(3), NFC
  
  This is again functionally equivalent but more concise.
  
  r356246:
  inetd: add some macros for checking child limits, NFC
  
  The main point here is capturing the maxchild > 0 check. A future change to
  inetd will start tracking all of the child pids so that it can give proper
  and consistent notification of process exit/signalling.
  
  r356247:
  inetd: track all child pids, regardless of maxchild spec
  
  Currently, child pids are only tracked if maxchildren is specified. As a
  consequence, without a maxchild limit we do not get a notice in syslog on
  children aborting abnormally. This turns out to be a great debugging aide at
  times.
  
  Children are now tracked in a LIST; the management interface is decidedly
  less painful when there's no upper bound on the number of entries we may
  have at the cost of one small allocation per connection.
  
  r356248:
  inetd: convert remaining bzero(3) to memset(3), NFC
  
  This change is purely in the name of noise reduction from static analyzers
  that want to complain that bzero(3) is obsolete in favor of memset(3).
  
  With this, clang-analyze at least is now noise free. WARNS= 6 also appears
  to have been OK for some time now, so drop the current setting and opt for
  the default.
  
  r356254:
  inetd: final round of trivial cleanup, NFC
  
  Highlights:
  - Use MAX() for maxsock raising; small readability improvement IMO
  - malloc(3) + memset(3) -> calloc(3) where appropriate
  - stop casting the return value of malloc(3)
  - mallloc(3) -> reallocarray(3) where appropriate
  
  A future change may enter capability mode when forking for some of the
  built-in handlers.
  
  r356318:
  inetd: fix WITHOUT_TCP_WRAPPERS build after r356248
  
  After increasing WARNS, building WITHOUT_TCP_WRAPPERS failed because of
  some unused variables.

Modified:
  stable/12/usr.sbin/inetd/Makefile
  stable/12/usr.sbin/inetd/builtins.c
  stable/12/usr.sbin/inetd/inetd.c
  stable/12/usr.sbin/inetd/inetd.h
Directory Properties:
  stable/12/   (props changed)

Changes in other areas also in this revision:
Modified:
  stable/11/usr.sbin/inetd/Makefile
  stable/11/usr.sbin/inetd/builtins.c
  stable/11/usr.sbin/inetd/inetd.c
  stable/11/usr.sbin/inetd/inetd.h
Directory Properties:
  stable/11/   (props changed)

Modified: stable/12/usr.sbin/inetd/Makefile
==============================================================================
--- stable/12/usr.sbin/inetd/Makefile	Sun Jan  5 21:32:41 2020	(r356387)
+++ stable/12/usr.sbin/inetd/Makefile	Sun Jan  5 21:35:02 2020	(r356388)
@@ -9,7 +9,6 @@ MAN=	inetd.8
 MLINKS=	inetd.8 inetd.conf.5
 SRCS=	inetd.c builtins.c
 
-WARNS?=	3
 CFLAGS+= -DLOGIN_CAP
 #CFLAGS+= -DSANITY_CHECK
 

Modified: stable/12/usr.sbin/inetd/builtins.c
==============================================================================
--- stable/12/usr.sbin/inetd/builtins.c	Sun Jan  5 21:32:41 2020	(r356387)
+++ stable/12/usr.sbin/inetd/builtins.c	Sun Jan  5 21:35:02 2020	(r356388)
@@ -132,10 +132,10 @@ chargen_dg(int s, struct servtab *sep)
 	socklen_t size;
 	char text[LINESIZ+2];
 
-	if (endring == 0) {
+	if (endring == NULL)
 		initring();
+	if (rs == NULL)
 		rs = ring;
-	}
 
 	size = sizeof(ss);
 	if (recvfrom(s, text, sizeof(text), 0,
@@ -649,8 +649,14 @@ ident_stream(int s, struct servtab *sep)
 			goto fakeid_fail;
 		if (!Fflag) {
 			if (iflag) {
-				if (p[strspn(p, "0123456789")] == '\0' &&
-				    getpwuid(atoi(p)) != NULL)
+				const char *errstr;
+				uid_t uid;
+
+				uid = strtonum(p, 0, UID_MAX, &errstr);
+				if (errstr != NULL)
+					goto fakeid_fail;
+
+				if (getpwuid(uid) != NULL)
 					goto fakeid_fail;
 			} else {
 				if (getpwnam(p) != NULL)

Modified: stable/12/usr.sbin/inetd/inetd.c
==============================================================================
--- stable/12/usr.sbin/inetd/inetd.c	Sun Jan  5 21:32:41 2020	(r356387)
+++ stable/12/usr.sbin/inetd/inetd.c	Sun Jan  5 21:35:02 2020	(r356388)
@@ -250,9 +250,11 @@ static char	*sskip(char **);
 static char	*newstr(const char *);
 static void	print_service(const char *, const struct servtab *);
 
+#ifdef LIBWRAP
 /* tcpd.h */
 int	allow_severity;
 int	deny_severity;
+#endif
 
 static int	wrap_ex = 0;
 static int	wrap_bi = 0;
@@ -410,7 +412,7 @@ main(int argc, char **argv)
 	 */
 	servname = (hostname == NULL) ? "0" /* dummy */ : NULL;
 
-	bzero(&hints, sizeof(struct addrinfo));
+	memset(&hints, 0, sizeof(struct addrinfo));
 	hints.ai_flags = AI_PASSIVE;
 	hints.ai_family = AF_UNSPEC;
 	hints.ai_socktype = SOCK_STREAM;	/* dummy */
@@ -564,10 +566,7 @@ main(int argc, char **argv)
 #ifdef SANITY_CHECK
 	nsock++;
 #endif
-	if (signalpipe[0] > maxsock)
-	    maxsock = signalpipe[0];
-	if (signalpipe[1] > maxsock)
-	    maxsock = signalpipe[1];
+	maxsock = MAX(MAX(maxsock, signalpipe[0]), signalpipe[1]);
 
 	for (;;) {
 	    int n, ctrl;
@@ -922,25 +921,33 @@ flag_signal(int signo)
 static void
 addchild(struct servtab *sep, pid_t pid)
 {
-	if (sep->se_maxchild <= 0)
-		return;
+	struct stabchild *sc;
+
 #ifdef SANITY_CHECK
-	if (sep->se_numchild >= sep->se_maxchild) {
+	if (SERVTAB_EXCEEDS_LIMIT(sep)) {
 		syslog(LOG_ERR, "%s: %d >= %d",
 		    __func__, sep->se_numchild, sep->se_maxchild);
 		exit(EX_SOFTWARE);
 	}
 #endif
-	sep->se_pids[sep->se_numchild++] = pid;
-	if (sep->se_numchild == sep->se_maxchild)
+	sc = calloc(1, sizeof(*sc));
+	if (sc == NULL) {
+		syslog(LOG_ERR, "calloc: %m");
+		exit(EX_OSERR);
+	}
+	sc->sc_pid = pid;
+	LIST_INSERT_HEAD(&sep->se_children, sc, sc_link);
+	++sep->se_numchild;
+	if (SERVTAB_AT_LIMIT(sep))
 		disable(sep);
 }
 
 static void
 reapchild(void)
 {
-	int k, status;
+	int status;
 	pid_t pid;
+	struct stabchild *sc;
 	struct servtab *sep;
 
 	for (;;) {
@@ -953,14 +960,17 @@ reapchild(void)
 			    WIFEXITED(status) ? WEXITSTATUS(status)
 				: WTERMSIG(status));
 		for (sep = servtab; sep; sep = sep->se_next) {
-			for (k = 0; k < sep->se_numchild; k++)
-				if (sep->se_pids[k] == pid)
+			LIST_FOREACH(sc, &sep->se_children, sc_link) {
+				if (sc->sc_pid == pid)
 					break;
-			if (k == sep->se_numchild)
+			}
+			if (sc == NULL)
 				continue;
-			if (sep->se_numchild == sep->se_maxchild)
+			if (SERVTAB_AT_LIMIT(sep))
 				enable(sep);
-			sep->se_pids[k] = sep->se_pids[--sep->se_numchild];
+			LIST_REMOVE(sc, sc_link);
+			free(sc);
+			--sep->se_numchild;
 			if (WIFSIGNALED(status) || WEXITSTATUS(status))
 				syslog(LOG_WARNING,
 				    "%s[%d]: exited, %s %u",
@@ -1032,25 +1042,20 @@ config(void)
 					sep->se_nomapped = new->se_nomapped;
 				sep->se_reset = 1;
 			}
-			/* copy over outstanding child pids */
-			if (sep->se_maxchild > 0 && new->se_maxchild > 0) {
-				new->se_numchild = sep->se_numchild;
-				if (new->se_numchild > new->se_maxchild)
-					new->se_numchild = new->se_maxchild;
-				memcpy(new->se_pids, sep->se_pids,
-				    new->se_numchild * sizeof(*new->se_pids));
-			}
-			SWAP(pid_t *, sep->se_pids, new->se_pids);
-			sep->se_maxchild = new->se_maxchild;
-			sep->se_numchild = new->se_numchild;
+
+			/*
+			 * The children tracked remain; we want numchild to
+			 * still reflect how many jobs are running so we don't
+			 * throw off our accounting.
+			 */
 			sep->se_maxcpm = new->se_maxcpm;
+			sep->se_maxchild = new->se_maxchild;
 			resize_conn(sep, new->se_maxperip);
 			sep->se_maxperip = new->se_maxperip;
 			sep->se_bi = new->se_bi;
 			/* might need to turn on or off service now */
 			if (sep->se_fd >= 0) {
-			      if (sep->se_maxchild > 0
-				  && sep->se_numchild == sep->se_maxchild) {
+			      if (SERVTAB_EXCEEDS_LIMIT(sep)) {
 				      if (FD_ISSET(sep->se_fd, &allsock))
 					  disable(sep);
 			      } else {
@@ -1494,8 +1499,8 @@ enter(struct servtab *cp)
 	struct servtab *sep;
 	long omask;
 
-	sep = (struct servtab *)malloc(sizeof (*sep));
-	if (sep == (struct servtab *)0) {
+	sep = malloc(sizeof(*sep));
+	if (sep == NULL) {
 		syslog(LOG_ERR, "malloc: %m");
 		exit(EX_OSERR);
 	}
@@ -1533,8 +1538,7 @@ enable(struct servtab *sep)
 	nsock++;
 #endif
 	FD_SET(sep->se_fd, &allsock);
-	if (sep->se_fd > maxsock)
-		maxsock = sep->se_fd;
+	maxsock = MAX(maxsock, sep->se_fd);
 }
 
 static void
@@ -1612,6 +1616,7 @@ getconfigent(void)
 	int v6bind;
 #endif
 	int i;
+	size_t unsz;
 
 #ifdef IPSEC
 	policy = NULL;
@@ -1629,12 +1634,10 @@ more:
 			for (p = cp + 2; p && *p && isspace(*p); p++)
 				;
 			if (*p == '\0') {
-				if (policy)
-					free(policy);
+				free(policy);
 				policy = NULL;
 			} else if (ipsec_get_policylen(p) >= 0) {
-				if (policy)
-					free(policy);
+				free(policy);
 				policy = newstr(p);
 			} else {
 				syslog(LOG_ERR,
@@ -1648,8 +1651,11 @@ more:
 			continue;
 		break;
 	}
-	if (cp == NULL)
-		return ((struct servtab *)0);
+	if (cp == NULL) {
+		free(policy);
+		return (NULL);
+	}
+
 	/*
 	 * clear the static buffer, since some fields (se_ctrladdr,
 	 * for example) don't get initialized here.
@@ -1831,16 +1837,18 @@ more:
 		break;
 #endif
 	case AF_UNIX:
-		if (strlen(sep->se_service) >= sizeof(sep->se_ctrladdr_un.sun_path)) {
-			syslog(LOG_ERR, 
+#define	SUN_PATH_MAXSIZE	sizeof(sep->se_ctrladdr_un.sun_path)
+		memset(&sep->se_ctrladdr, 0, sizeof(sep->se_ctrladdr));
+		sep->se_ctrladdr_un.sun_family = sep->se_family;
+		if ((unsz = strlcpy(sep->se_ctrladdr_un.sun_path,
+		    sep->se_service, SUN_PATH_MAXSIZE) >= SUN_PATH_MAXSIZE)) {
+			syslog(LOG_ERR,
 			    "domain socket pathname too long for service %s",
 			    sep->se_service);
 			goto more;
 		}
-		memset(&sep->se_ctrladdr, 0, sizeof(sep->se_ctrladdr));
-		sep->se_ctrladdr_un.sun_family = sep->se_family;
-		sep->se_ctrladdr_un.sun_len = strlen(sep->se_service);
-		strcpy(sep->se_ctrladdr_un.sun_path, sep->se_service);
+		sep->se_ctrladdr_un.sun_len = unsz;
+#undef SUN_PATH_MAXSIZE
 		sep->se_ctrladdr_size = SUN_LEN(&sep->se_ctrladdr_un);
 	}
 	arg = sskip(&cp);
@@ -1946,13 +1954,7 @@ more:
 		else
 			sep->se_maxchild = 1;
 	}
-	if (sep->se_maxchild > 0) {
-		sep->se_pids = malloc(sep->se_maxchild * sizeof(*sep->se_pids));
-		if (sep->se_pids == NULL) {
-			syslog(LOG_ERR, "malloc: %m");
-			exit(EX_OSERR);
-		}
-	}
+	LIST_INIT(&sep->se_children);
 	argc = 0;
 	for (arg = skip(&cp); cp; arg = skip(&cp))
 		if (argc < MAXARGV) {
@@ -1969,6 +1971,7 @@ more:
 		LIST_INIT(&sep->se_conn[i]);
 #ifdef IPSEC
 	sep->se_policy = policy ? newstr(policy) : NULL;
+	free(policy);
 #endif
 	return (sep);
 }
@@ -1976,31 +1979,28 @@ more:
 static void
 freeconfig(struct servtab *cp)
 {
+	struct stabchild *sc;
 	int i;
 
-	if (cp->se_service)
-		free(cp->se_service);
-	if (cp->se_proto)
-		free(cp->se_proto);
-	if (cp->se_user)
-		free(cp->se_user);
-	if (cp->se_group)
-		free(cp->se_group);
+	free(cp->se_service);
+	free(cp->se_proto);
+	free(cp->se_user);
+	free(cp->se_group);
 #ifdef LOGIN_CAP
-	if (cp->se_class)
-		free(cp->se_class);
+	free(cp->se_class);
 #endif
-	if (cp->se_server)
-		free(cp->se_server);
-	if (cp->se_pids)
-		free(cp->se_pids);
+	free(cp->se_server);
+	while (!LIST_EMPTY(&cp->se_children)) {
+		sc = LIST_FIRST(&cp->se_children);
+		LIST_REMOVE(sc, sc_link);
+		free(sc);
+	}
 	for (i = 0; i < MAXARGV; i++)
 		if (cp->se_argv[i])
 			free(cp->se_argv[i]);
 	free_connlist(cp);
 #ifdef IPSEC
-	if (cp->se_policy)
-		free(cp->se_policy);
+	free(cp->se_policy);
 #endif
 }
 
@@ -2207,7 +2207,7 @@ cpmip(const struct servtab *sep, int ctrl)
 	   (sep->se_family == AF_INET || sep->se_family == AF_INET6) &&
 	    getpeername(ctrl, (struct sockaddr *)&rss, &rssLen) == 0 ) {
 		time_t t = time(NULL);
-		int hv = 0xABC3D20F;
+		unsigned int hv = 0xABC3D20F;
 		int i;
 		int cnt = 0;
 		CHash *chBest = NULL;
@@ -2280,10 +2280,9 @@ cpmip(const struct servtab *sep, int ctrl)
 		    strcmp(sep->se_service, chBest->ch_Service) != 0) {
 			chBest->ch_Family = sin4->sin_family;
 			chBest->ch_Addr4 = sin4->sin_addr;
-			if (chBest->ch_Service)
-				free(chBest->ch_Service);
+			free(chBest->ch_Service);
 			chBest->ch_Service = strdup(sep->se_service);
-			bzero(chBest->ch_Times, sizeof(chBest->ch_Times));
+			memset(chBest->ch_Times, 0, sizeof(chBest->ch_Times));
 		} 
 #ifdef INET6
 		if ((rss.ss_family == AF_INET6 &&
@@ -2294,10 +2293,9 @@ cpmip(const struct servtab *sep, int ctrl)
 		    strcmp(sep->se_service, chBest->ch_Service) != 0) {
 			chBest->ch_Family = sin6->sin6_family;
 			chBest->ch_Addr6 = sin6->sin6_addr;
-			if (chBest->ch_Service)
-				free(chBest->ch_Service);
+			free(chBest->ch_Service);
 			chBest->ch_Service = strdup(sep->se_service);
-			bzero(chBest->ch_Times, sizeof(chBest->ch_Times));
+			memset(chBest->ch_Times, 0, sizeof(chBest->ch_Times));
 		}
 #endif
 		chBest->ch_LTime = t;
@@ -2388,9 +2386,10 @@ search_conn(struct servtab *sep, int ctrl)
 			syslog(LOG_ERR, "malloc: %m");
 			exit(EX_OSERR);
 		}
-		conn->co_proc = malloc(sep->se_maxperip * sizeof(*conn->co_proc));
+		conn->co_proc = reallocarray(NULL, sep->se_maxperip,
+		    sizeof(*conn->co_proc));
 		if (conn->co_proc == NULL) {
-			syslog(LOG_ERR, "malloc: %m");
+			syslog(LOG_ERR, "reallocarray: %m");
 			exit(EX_OSERR);
 		}
 		memcpy(&conn->co_addr, (struct sockaddr *)&ss, sslen);
@@ -2479,10 +2478,10 @@ resize_conn(struct servtab *sep, int maxpip)
 		LIST_FOREACH(conn, &sep->se_conn[i], co_link) {
 			for (j = maxpip; j < conn->co_numchild; ++j)
 				free_proc(conn->co_proc[j]);
-			conn->co_proc = realloc(conn->co_proc,
-			    maxpip * sizeof(*conn->co_proc));
+			conn->co_proc = reallocarray(conn->co_proc, maxpip,
+			    sizeof(*conn->co_proc));
 			if (conn->co_proc == NULL) {
-				syslog(LOG_ERR, "realloc: %m");
+				syslog(LOG_ERR, "reallocarray: %m");
 				exit(EX_OSERR);
 			}
 			if (conn->co_numchild > maxpip)
@@ -2494,11 +2493,15 @@ resize_conn(struct servtab *sep, int maxpip)
 static void
 free_connlist(struct servtab *sep)
 {
-	struct conninfo *conn;
+	struct conninfo *conn, *conn_temp;
 	int i, j;
 
 	for (i = 0; i < PERIPSIZE; ++i) {
-		while ((conn = LIST_FIRST(&sep->se_conn[i])) != NULL) {
+		LIST_FOREACH_SAFE(conn, &sep->se_conn[i], co_link, conn_temp) {
+			if (conn == NULL) {
+				LIST_REMOVE(conn, co_link);
+				continue;
+			}
 			for (j = 0; j < conn->co_numchild; ++j)
 				free_proc(conn->co_proc[j]);
 			conn->co_numchild = 0;
@@ -2554,7 +2557,8 @@ free_proc(struct procinfo *proc)
 static int
 hashval(char *p, int len)
 {
-	int i, hv = 0xABC3D20F;
+	unsigned int hv = 0xABC3D20F;
+	int i;
 
 	for (i = 0; i < len; ++i, ++p)
 		hv = (hv << 5) ^ (hv >> 23) ^ *p;

Modified: stable/12/usr.sbin/inetd/inetd.h
==============================================================================
--- stable/12/usr.sbin/inetd/inetd.h	Sun Jan  5 21:32:41 2020	(r356387)
+++ stable/12/usr.sbin/inetd/inetd.h	Sun Jan  5 21:35:02 2020	(r356388)
@@ -66,6 +66,11 @@ struct conninfo {
 
 #define PERIPSIZE	256
 
+struct	stabchild {
+	LIST_ENTRY(stabchild)	sc_link;
+	pid_t			sc_pid;
+};
+
 struct	servtab {
 	char	*se_service;		/* name of service */
 	int	se_socktype;		/* type of socket to use */
@@ -74,7 +79,6 @@ struct	servtab {
 	int	se_maxchild;		/* max number of children */
 	int	se_maxcpm;		/* max connects per IP per minute */
 	int	se_numchild;		/* current number of children */
-	pid_t	*se_pids;		/* array of child pids */
 	char	*se_user;		/* user name to run as */
 	char    *se_group;              /* group name to run as */
 #ifdef  LOGIN_CAP
@@ -119,10 +123,16 @@ struct	servtab {
 	} se_flags;
 	int	se_maxperip;		/* max number of children per src */
 	LIST_HEAD(, conninfo) se_conn[PERIPSIZE];
+	LIST_HEAD(, stabchild) se_children;
 };
 
 #define	se_nomapped		se_flags.se_nomapped
 #define	se_reset		se_flags.se_reset
+
+#define	SERVTAB_AT_LIMIT(sep)		\
+	((sep)->se_maxchild > 0 && (sep)->se_numchild == (sep)->se_maxchild)
+#define	SERVTAB_EXCEEDS_LIMIT(sep)	\
+	((sep)->se_maxchild > 0 && (sep)->se_numchild >= (sep)->se_maxchild)
 
 int		check_loop(const struct sockaddr *, const struct servtab *sep);
 void		inetd_setproctitle(const char *, int);



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202001052135.005LZ4s0080169>