From owner-cvs-all Sat Nov 25 8:57: 9 2000 Delivered-To: cvs-all@freebsd.org Received: from green.dyndns.org (localhost [127.0.0.1]) by hub.freebsd.org (Postfix) with ESMTP id 8B16E37B479; Sat, 25 Nov 2000 08:57:01 -0800 (PST) Received: from localhost (lym745@localhost [127.0.0.1]) by green.dyndns.org (8.11.0/8.11.0) with ESMTP id eAPGuu571317; Sat, 25 Nov 2000 11:56:57 -0500 (EST) (envelope-from green@FreeBSD.org) Message-Id: <200011251656.eAPGuu571317@green.dyndns.org> X-Mailer: exmh version 2.2 06/23/2000 with nmh-1.0.4 To: Alfred Perlstein Cc: cvs-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/usr.sbin/inetd builtins.c In-Reply-To: Message from Alfred Perlstein of "Sat, 25 Nov 2000 08:10:06 PST." <20001125081005.K8051@fw.wintelcom.net> From: "Brian F. Feldman" Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Date: Sat, 25 Nov 2000 11:56:55 -0500 Sender: owner-cvs-all@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG Alfred Perlstein wrote: > * Brian F. Feldman [001125 06:15] wrote: > > > > Yes, thank you for taking time out on this. Do you want to make the style > > changes? I think it would make sense to be consistent. If so, this is all > > I can think of to make the code more generally useful to the "persistent" > > program case: > > > > try working with this: > 1) re-order the stat/fdopen order so that we don't leak the FILE * > and reduce the complexity of the fclose/close logic. we also > do the fstat() before the fdopen() that might block therefore > we can do the S_ISREG() check before the fdopen(). There was no problem with what you suggested before, if (fakeid != NULL) fclose(fakeid; else if (fakeid_fd != -1) close(fakeid_fd); as fakeid is initialied to NULL. This DTRT. However it does make sense to fstat the file first before doing the fdopen(). The fdopen() doesn't block (it doesn't actually "do" much of anything), the open() is what would have blocked were it not O_NONBLOCK. > 2) set the fakeid_fd to -1 so that the cleanup after the else clause > doesn't close a bad fd With the (fakeid != NULL) logic, this isn't a problem. The only case close() will be called is if open() succeeds but fdopen() fails. > 3) use isspace() to walk the string instead of strcspn() > This is just how I'd do it, nothing really important here. I was surprised that there was already a library function for what I wanted :) strcspn(3) is good! > 4) use MAXLOGNAME rather than a hardcoded '16', (is this correct?) Not really. The number I chose was an arbitrary limit to the length of user name to be accepted. I thought 16 would be reasonable; the limit of MAXLOGNAME is enforced elsewhere; here, we just use the pw->pw_name. There has to be an artificial limit on the fakeids, which may as well be whatever it may be. > 5) add some braces to reduce the chance of future headaches. You say potato... > > I think 1 and 2 are really needed, the rest I'll leave up to your > judgement. 4 seems like it's needed, but I'm not sure. > > Index: builtins.c > =================================================================== > RCS file: /home/ncvs/src/usr.sbin/inetd/builtins.c,v > retrieving revision 1.26 > diff -u -u -r1.26 builtins.c > --- builtins.c 2000/11/25 04:13:05 1.26 > +++ builtins.c 2000/11/25 06:19:32 > @@ -597,8 +597,10 @@ > */ > fakeid_fd = open(p, O_RDONLY | O_NONBLOCK); > free(p); > - if ((fakeid = fdopen(fakeid_fd, "r")) != NULL && > - fstat(fileno(fakeid), &sb) != -1 && S_ISREG(sb.st_mode)) { > + if ((fstat(fileno(fakeid), &sb) != -1) && S_ISREG(sb.st_mode) && > + (fakeid = fdopen(fakeid_fd, "r")) != NULL) { > + unsigned char *walk; > + > buf[sizeof(buf) - 1] = '\0'; > if (fgets(buf, sizeof(buf), fakeid) == NULL) { > cp = pw->pw_name; This is a good idea, but what you pasted will segfault, since fakeid is NULL. if (fstat(fakeid_fd, &sb) != -1 && S_ISREG(sb.st_mode) && (fakeid = fdopen(fakeid_fd, "r")) != NULL) { -- Brian Fundakowski Feldman \ FreeBSD: The Power to Serve! / green@FreeBSD.org `------------------------------' To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe cvs-all" in the body of the message