Date: Tue, 11 Sep 2001 04:46:51 -0400 (EDT) From: "Andrew R. Reiter" <arr@watson.org> To: Ruslan Ermilov <ru@FreeBSD.ORG> Cc: freebsd-audit@FreeBSD.ORG Subject: Re: select diffs Message-ID: <Pine.NEB.3.96L.1010911044302.47624A-300000@fledge.watson.org> In-Reply-To: <20010911102900.C4979@sunbay.com>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] Ahh yes, I knew this would happen :-) Mike said the same thing -- I cringed since I had just done the entire update without a rcs and sent the patch anyway. Nevertheless, I have the two patches done (and attached). The first (rlogin.c.1.diff) does the style(9) changes... Mike was key in pointing out numerous problems. The second (rlogin.c.2.diff) does the select changes -- dynamic allocation of fd_set bit arrays. Let me know if I missed something in turning it into 2 diffs. thanks, Andrew On Tue, 11 Sep 2001, Ruslan Ermilov wrote: :On Tue, Sep 11, 2001 at 02:05:32AM -0400, Andrew R. Reiter wrote: :> :> 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. :> :Please don't mix style and functionality fixes!!! :These are at least hard to review. : : :Cheers, :-- :Ruslan Ermilov Oracle Developer/DBA, :ru@sunbay.com Sunbay Software AG, :ru@FreeBSD.org FreeBSD committer, :+380.652.512.251 Simferopol, Ukraine : :http://www.FreeBSD.org The Power To Serve :http://www.oracle.com Enabling The Information Age : *-------------................................................. | 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 03:30:42 2001 @@ -78,6 +78,7 @@ #include <stdlib.h> #include <string.h> #include <unistd.h> + #include "pathnames.h" @@ -103,6 +104,15 @@ struct passwd *pwd; +int child; +int netf; +char line[MAXPATHLEN]; +int confirmed; +struct winsize win = { 0, 0, 0, 0 }; + +char magic[2] = { 0377, 0377 }; +char oobdata[] = {TIOCPKT_WINDOW}; + union sockunion { struct sockinet { u_char si_len; @@ -167,7 +177,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,24 +189,17 @@ 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) + 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; } -int child; -int netf; -char line[MAXPATHLEN]; -int confirmed; - -struct winsize win = { 0, 0, 0, 0 }; - void doit(f, fromp) @@ -208,6 +211,9 @@ char hostname[2 * MAXHOSTNAMELEN + 1]; char nameinfo[2 * INET6_ADDRSTRLEN + 1]; char c; + u_char optbuf[BUFSIZ/3], s, rropt[5], remote_ip[16]; + int optsize = sizeof(optbuf), ipproto, i; + struct protoent *ip; alarm(60); read(f, &c, 1); @@ -218,34 +224,35 @@ alarm(0); realhostname_sa(hostname, sizeof(hostname) - 1, - (struct sockaddr *)fromp, fromp->su_len); + (struct sockaddr *)fromp, fromp->su_len); + hostname[sizeof(hostname) - 1] = '\0'; /* 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) - { - u_char optbuf[BUFSIZ/3]; - int optsize = sizeof(optbuf), ipproto, i; - struct protoent *ip; + if (fromp->su_family == AF_INET) { + memset(optbuf, 0, BUFSIZ/3); + optsize = sizeof(optbuf); + ipproto = i = 0; + ip = NULL; if ((ip = getprotobyname("ip")) != NULL) ipproto = ip->p_proto; @@ -254,34 +261,46 @@ if (getsockopt(0, ipproto, IP_OPTIONS, (char *)optbuf, &optsize) == 0 && optsize != 0) { for (i = 0; i < optsize; ) { - u_char c = optbuf[i]; - if (c == IPOPT_LSRR || c == IPOPT_SSRR) { + s = optbuf[i]; + if (s == IPOPT_LSRR + || s == IPOPT_SSRR) { + if (s == 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) + if (s == IPOPT_EOL) break; - i += (c == IPOPT_NOP) ? 1 : optbuf[i+1]; + i += (s == IPOPT_NOP) ? 1 : optbuf[i+1]; } } - } + } #endif - if (do_rlogin(fromp) == 0) + 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,17 +313,17 @@ } 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); @@ -327,8 +346,6 @@ cleanup(0); } -char magic[2] = { 0377, 0377 }; -char oobdata[] = {TIOCPKT_WINDOW}; /* * Handle a "control" request (signaled by magic being present) @@ -366,13 +383,16 @@ int pcc = 0, fcc = 0; int cc, nfd, n; char cntl; + fd_set ibits, obits, ebits, *omask; + register char *cp; + int left, m; /* * Must ignore SIGTTOU, otherwise we'll stop * 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; @@ -382,13 +402,12 @@ syslog(LOG_ERR, "select mask too small, increase FD_SETSIZE"); fatal(f, "internal error (select mask too small)", 0); } - for (;;) { - fd_set ibits, obits, ebits, *omask; + for (;;) { FD_ZERO(&ebits); FD_ZERO(&ibits); FD_ZERO(&obits); - omask = (fd_set *)NULL; + omask = NULL; if (fcc) { FD_SET(p, &obits); omask = &obits; @@ -428,30 +447,30 @@ #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)); if (fcc < 0 && errno == EWOULDBLOCK) fcc = 0; else { - register char *cp; - int left, n; + cp = NULL; + left = m = 0; if (fcc <= 0) break; fbp = fibuf; - top: for (cp = fibuf; cp < fibuf+fcc-1; cp++) if (cp[0] == magic[0] && cp[1] == magic[1]) { - left = fcc - (cp-fibuf); + left = fcc - (cp - fibuf); n = control(p, cp, left); if (n) { left -= n; if (left > 0) - bcopy(cp+n, cp, left); + bcopy(cp+n, + cp, left); fcc -= n; goto top; /* n^2 */ } @@ -469,7 +488,7 @@ } if (FD_ISSET(p, &ibits)) { - pcc = read(p, pibuf, sizeof (pibuf)); + pcc = read(p, pibuf, sizeof(pibuf)); pbp = pibuf; if (pcc < 0 && errno == EWOULDBLOCK) pcc = 0; @@ -493,7 +512,7 @@ #ifdef CRYPT if (doencrypt) cc = des_enc_write(f, pbp, pcc, - schedule, &kdata->session); + schedule, &kdata->session); else #endif cc = write(f, pbp, pcc); @@ -557,7 +576,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 +595,7 @@ /* XXX why don't we syslog() failure? */ return (iruserok_sa(dest, dest->su_len, pwd->pw_uid == 0, rusername, - lusername)); + lusername)); } void [-- Attachment #3 --] --- rlogind.c.orig Tue Sep 11 03:41:31 2001 +++ rlogind.c Tue Sep 11 03:41:09 2001 @@ -88,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 @@ -383,7 +385,7 @@ int pcc = 0, fcc = 0; int cc, nfd, n; char cntl; - fd_set ibits, obits, ebits, *omask; + fd_set *ibits, *obits, *ebits, *omask; register char *cp; int left, m; @@ -404,46 +406,67 @@ } for (;;) { - FD_ZERO(&ebits); - FD_ZERO(&ibits); - FD_ZERO(&obits); + 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); + } + + 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), @@ -457,8 +480,12 @@ cp = NULL; left = m = 0; - if (fcc <= 0) + if (fcc <= 0) { + free(ibits); + free(obits); + free(ebits); break; + } fbp = fibuf; top: for (cp = fibuf; cp < fibuf+fcc-1; cp++) @@ -475,11 +502,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; @@ -487,7 +514,7 @@ } } - if (FD_ISSET(p, &ibits)) { + if (FD_ISSET(p, ibits)) { pcc = read(p, pibuf, sizeof(pibuf)); pbp = pibuf; if (pcc < 0 && errno == EWOULDBLOCK) @@ -499,7 +526,7 @@ #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]; @@ -508,7 +535,7 @@ 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, @@ -522,8 +549,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(ibits); + free(obits); + free(ebits); continue; } if (cc > 0) {
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.NEB.3.96L.1010911044302.47624A-300000>