Date: Tue, 11 Sep 2001 02:05:32 -0400 (EDT) From: "Andrew R. Reiter" <arr@watson.org> To: freebsd-audit@FreeBSD.ORG Subject: Re: select diffs Message-ID: <Pine.NEB.3.96L.1010911020108.46088A-200000@fledge.watson.org> In-Reply-To: <Pine.NEB.3.96L.1010910235947.45198A-200000@fledge.watson.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] Thanks to Mike Barcroft for some comments and beating my style(9) skillessness (word?) into helping to clean rlogind.c. Attached is an updated diff for the select "problems" as well as updates to rlogind.c to clean it up in terms of style(9). If I missed something, tips are appreciated -- Im a style(9) retard, to be politically incorrect. Sorry that it's not 2 diffs, one for select and one for style(9) fixes, but such is life. Hope this helps, cheers, andrew On Tue, 11 Sep 2001, Andrew R. Reiter wrote: :Hey, : :I am pondering starting on moving alot of the select() usages in :src/libexec and src/usr.sbin to using dynamically allocated fd_set bit :arrays in order to be extra cautious about fd usage. Therefore, I did a :patch for rlogind so that people could review changes that would most :likely occur to the sets of code that utilize select(2). I would :appreciate comments... : :if you're wondering why select instead of perhaps moving to kqueue.... : :After speaking with Jonathan Lemon about whether it was worth moving from :select() on all of these to a kqueue-ified system, I believe we/he :concluded that probably none of these programs (libexec and usr.sbin) :would really require kqueue-ification (except probably inetd), so I :decided to move towards doing the fd_set's dyn. alloc'd rather than moving :to a kq system. : :anyway, attached is a patch, but it can also be seen at: : : http://www.watson.org/~arr/fbsd-audit/libexec/rlogind/rlogin.c.diff : :This diff was made against current from 9/16/01 (evening pacific time) : :Andrew : :*-------------................................................. :| Andrew R. Reiter :| arr@fledge.watson.org :| "It requires a very unusual mind :| to undertake the analysis of the obvious" -- A.N. Whitehead : *-------------................................................. | Andrew R. Reiter | arr@fledge.watson.org | "It requires a very unusual mind | to undertake the analysis of the obvious" -- A.N. Whitehead [-- Attachment #2 --] --- ./rlogind.c.orig Mon Sep 10 22:32:28 2001 +++ ./rlogind.c Tue Sep 11 00:59:40 2001 @@ -78,6 +78,7 @@ #include <stdlib.h> #include <string.h> #include <unistd.h> + #include "pathnames.h" @@ -87,6 +88,8 @@ #define ARGSTR "Dalnx" +#define fd_howmany(x, y) (((x) + ((y) - 1)) / (y)) + /* wrapper for KAME-special getnameinfo() */ #ifndef NI_WITHSCOPEID #define NI_WITHSCOPEID 0 @@ -167,7 +170,7 @@ argc -= optind; argv += optind; - fromlen = sizeof (from); + fromlen = sizeof(from); if (getpeername(0, (struct sockaddr *)&from, &fromlen) < 0) { syslog(LOG_ERR,"Can't get peer name of remote host: %m"); fatal(STDERR_FILENO, "Can't get peer name of remote host", 1); @@ -179,12 +182,12 @@ if (no_delay && setsockopt(0, IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on)) < 0) syslog(LOG_WARNING, "setsockopt (TCP_NODELAY): %m"); - if (from.su_family == AF_INET) - { - on = IPTOS_LOWDELAY; - if (setsockopt(0, IPPROTO_IP, IP_TOS, (char *)&on, sizeof(int)) < 0) - syslog(LOG_WARNING, "setsockopt (IP_TOS): %m"); - } + if (from.su_family == AF_INET) { + on = IPTOS_LOWDELAY; + if (setsockopt(0, IPPROTO_IP, IP_TOS, (char *)&on, + sizeof(int)) < 0) + syslog(LOG_WARNING, "setsockopt (IP_TOS): %m"); + } doit(0, &from); return 0; @@ -218,31 +221,30 @@ alarm(0); realhostname_sa(hostname, sizeof(hostname) - 1, - (struct sockaddr *)fromp, fromp->su_len); + (struct sockaddr *)fromp, fromp->su_len); + /* error check ? */ fromp->su_port = ntohs((u_short)fromp->su_port); hostname[sizeof(hostname) - 1] = '\0'; - { - if ((fromp->su_family != AF_INET + if ((fromp->su_family != AF_INET #ifdef INET6 - && fromp->su_family != AF_INET6 + && fromp->su_family != AF_INET6 #endif - ) || - fromp->su_port >= IPPORT_RESERVED || - fromp->su_port < IPPORT_RESERVED/2) { - getnameinfo((struct sockaddr *)fromp, - fromp->su_len, - nameinfo, sizeof(nameinfo), NULL, 0, - NI_NUMERICHOST|NI_WITHSCOPEID); - /* error check ? */ - syslog(LOG_NOTICE, "Connection from %s on illegal port", - nameinfo); - fatal(f, "Permission denied", 0); - } + ) || + fromp->su_port >= IPPORT_RESERVED || + fromp->su_port < IPPORT_RESERVED/2) { + getnameinfo((struct sockaddr *)fromp, + fromp->su_len, + nameinfo, sizeof(nameinfo), NULL, 0, + NI_NUMERICHOST|NI_WITHSCOPEID); + /* error check ? */ + syslog(LOG_NOTICE, "Connection from %s on illegal port", + nameinfo); + fatal(f, "Permission denied", 0); + } #ifdef IP_OPTIONS - if (fromp->su_family == AF_INET) - { + if (fromp->su_family == AF_INET) { u_char optbuf[BUFSIZ/3]; int optsize = sizeof(optbuf), ipproto, i; struct protoent *ip; @@ -252,14 +254,29 @@ else ipproto = IPPROTO_IP; if (getsockopt(0, ipproto, IP_OPTIONS, (char *)optbuf, - &optsize) == 0 && optsize != 0) { + &optsize) == 0 && optsize != 0) { for (i = 0; i < optsize; ) { u_char c = optbuf[i]; - if (c == IPOPT_LSRR || c == IPOPT_SSRR) { + char rropt[5], remote_ip[16]; + + if (c == IPOPT_LSRR + || c == IPOPT_SSRR) { + if (c == IPOPT_LSRR) + (void)strlcpy(rropt, "LSRR", + sizeof(rropt)); + else + (void)strlcpy(rropt, "SSRR", + sizeof(rropt)); + + memset(remote_ip, 0, sizeof(remote_ip)); + (void)strlcpy(remote_ip, + inet_ntoa(fromp->su_sin.sin_addr), + sizeof(remote_ip)); + syslog(LOG_NOTICE, - "Connection refused from %s with IP option %s", - inet_ntoa(fromp->su_sin.sin_addr), - c == IPOPT_LSRR ? "LSRR" : "SSRR"); + "Connection refused from %s" + " with IP option %s", + remote_ip, rropt); exit(1); } if (c == IPOPT_EOL) @@ -267,21 +284,21 @@ i += (c == IPOPT_NOP) ? 1 : optbuf[i+1]; } } - } -#endif - if (do_rlogin(fromp) == 0) - authenticated++; } +#endif + if (do_rlogin(fromp) == 0) + authenticated++; + if (confirmed == 0) { write(f, "", 1); confirmed = 1; /* we sent the null! */ } #ifdef CRYPT if (doencrypt) - (void) des_enc_write(f, - SECURE_MESSAGE, - strlen(SECURE_MESSAGE), - schedule, &kdata->session); + (void)des_enc_write(f, + SECURE_MESSAGE, + strlen(SECURE_MESSAGE), + schedule, &kdata->session); #endif netf = f; @@ -294,19 +311,20 @@ } if (pid == 0) { if (f > 2) /* f should always be 0, but... */ - (void) close(f); + (void)close(f); setup_term(0); - if (*lusername=='-') { + if (*lusername == '-') { syslog(LOG_ERR, "tried to pass user \"%s\" to login", - lusername); + lusername); fatal(STDERR_FILENO, "invalid user", 0); } - if (authenticated) { + if (authenticated) execl(_PATH_LOGIN, "login", "-p", "-h", hostname, "-f", lusername, (char *)NULL); - } else + else execl(_PATH_LOGIN, "login", "-p", "-h", hostname, lusername, (char *)NULL); + fatal(STDERR_FILENO, _PATH_LOGIN, 1); /*NOTREACHED*/ } @@ -343,7 +361,7 @@ { struct winsize w; - if (n < 4+sizeof (w) || cp[2] != 's' || cp[3] != 's') + if (n < 4 + sizeof(w) || cp[2] != 's' || cp[3] != 's') return (0); oobdata[0] &= ~TIOCPKT_WINDOW; /* we know he heard */ bcopy(cp+4, (char *)&w, sizeof(w)); @@ -372,7 +390,7 @@ * when we try and set slave pty's window shape * (our controlling tty is the master pty). */ - (void) signal(SIGTTOU, SIG_IGN); + (void)signal(SIGTTOU, SIG_IGN); send(f, oobdata, 1, MSG_OOB); /* indicate new rlogin */ if (f > p) nfd = f + 1; @@ -383,52 +401,73 @@ fatal(f, "internal error (select mask too small)", 0); } for (;;) { - fd_set ibits, obits, ebits, *omask; + fd_set *ibits, *obits, *ebits, *omask; + + ibits = calloc(fd_howmany(nfd, NFDBITS), sizeof(fd_mask)); + if (ibits == NULL) + fatal(f, "calloc", 1); + + obits = calloc(fd_howmany(nfd, NFDBITS), sizeof(fd_mask)); + if (obits == NULL) { + free(ibits); + fatal(f, "calloc", 1); + } - FD_ZERO(&ebits); - FD_ZERO(&ibits); - FD_ZERO(&obits); - omask = (fd_set *)NULL; + ebits = calloc(fd_howmany(nfd, NFDBITS), sizeof(fd_mask)); + if (ebits == NULL) { + free(obits); + free(ibits); + fatal(f, "calloc", 1); + } + + omask = NULL; + if (fcc) { - FD_SET(p, &obits); - omask = &obits; + FD_SET(p, obits); + omask = obits; } else - FD_SET(f, &ibits); + FD_SET(f, ibits); if (pcc >= 0) { if (pcc) { - FD_SET(f, &obits); - omask = &obits; + FD_SET(f, obits); + omask = obits; } else - FD_SET(p, &ibits); + FD_SET(p, ibits); } - FD_SET(p, &ebits); - if ((n = select(nfd, &ibits, omask, &ebits, 0)) < 0) { + FD_SET(p, ebits); + if ((n = select(nfd, ibits, omask, ebits, 0)) < 0) { + free(ibits); + free(obits); + free(ebits); if (errno == EINTR) continue; fatal(f, "select", 1); } if (n == 0) { /* shouldn't happen... */ + free(ibits); + free(obits); + free(ebits); sleep(5); continue; } #define pkcontrol(c) ((c)&(TIOCPKT_FLUSHWRITE|TIOCPKT_NOSTOP|TIOCPKT_DOSTOP)) - if (FD_ISSET(p, &ebits)) { + if (FD_ISSET(p, ebits)) { cc = read(p, &cntl, 1); if (cc == 1 && pkcontrol(cntl)) { cntl |= oobdata[0]; send(f, &cntl, 1, MSG_OOB); if (cntl & TIOCPKT_FLUSHWRITE) { pcc = 0; - FD_CLR(p, &ibits); + FD_CLR(p, ibits); } } } - if (FD_ISSET(f, &ibits)) { + if (FD_ISSET(f, ibits)) { #ifdef CRYPT if (doencrypt) fcc = des_enc_read(f, fibuf, sizeof(fibuf), - schedule, &kdata->session); + schedule, &kdata->session); else #endif fcc = read(f, fibuf, sizeof(fibuf)); @@ -438,8 +477,12 @@ register char *cp; int left, n; - if (fcc <= 0) + if (fcc <= 0) { + free(obits); + free(ibits); + free(ebits); break; + } fbp = fibuf; top: @@ -456,11 +499,11 @@ goto top; /* n^2 */ } } - FD_SET(p, &obits); /* try write */ + FD_SET(p, obits); /* try write */ } } - if (FD_ISSET(p, &obits) && fcc > 0) { + if (FD_ISSET(p, obits) && fcc > 0) { cc = write(p, fbp, fcc); if (cc > 0) { fcc -= cc; @@ -468,19 +511,23 @@ } } - if (FD_ISSET(p, &ibits)) { + if (FD_ISSET(p, ibits)) { pcc = read(p, pibuf, sizeof (pibuf)); pbp = pibuf; if (pcc < 0 && errno == EWOULDBLOCK) pcc = 0; - else if (pcc <= 0) + else if (pcc <= 0) { + free(ibits); + free(obits); + free(ebits); break; + } else if (pibuf[0] == 0) { pbp++, pcc--; #ifdef CRYPT if (!doencrypt) #endif - FD_SET(f, &obits); /* try write */ + FD_SET(f, obits); /* try write */ } else { if (pkcontrol(pibuf[0])) { pibuf[0] |= oobdata[0]; @@ -489,11 +536,11 @@ pcc = 0; } } - if ((FD_ISSET(f, &obits)) && pcc > 0) { + if ((FD_ISSET(f, obits)) && pcc > 0) { #ifdef CRYPT if (doencrypt) cc = des_enc_write(f, pbp, pcc, - schedule, &kdata->session); + schedule, &kdata->session); else #endif cc = write(f, pbp, pcc); @@ -503,8 +550,11 @@ * from p, but some old kernels balk at large * writes even when select returns true. */ - if (!FD_ISSET(p, &ibits)) + if (!FD_ISSET(p, ibits)) sleep(5); + free(obits); + free(ibits); + free(ebits); continue; } if (cc > 0) { @@ -557,7 +607,7 @@ len = snprintf(bp, sizeof(buf), "rlogind: %s.\r\n", msg); if (len < 0) len = 0; - (void) write(f, buf, bp + len - buf); + (void)write(f, buf, bp + len - buf); exit(1); } @@ -576,7 +626,7 @@ /* XXX why don't we syslog() failure? */ return (iruserok_sa(dest, dest->su_len, pwd->pw_uid == 0, rusername, - lusername)); + lusername)); } void
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.NEB.3.96L.1010911020108.46088A-200000>