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>