From owner-cvs-all  Sat Nov 25  8:10:13 2000
Delivered-To: cvs-all@freebsd.org
Received: from fw.wintelcom.net (ns1.wintelcom.net [209.1.153.20])
	by hub.freebsd.org (Postfix) with ESMTP
	id CF88637B4CF; Sat, 25 Nov 2000 08:10:06 -0800 (PST)
Received: (from bright@localhost)
	by fw.wintelcom.net (8.10.0/8.10.0) id eAPGA6o25471;
	Sat, 25 Nov 2000 08:10:06 -0800 (PST)
Date: Sat, 25 Nov 2000 08:10:06 -0800
From: Alfred Perlstein <bright@wintelcom.net>
To: "Brian F. Feldman" <green@FreeBSD.org>
Cc: cvs-committers@FreeBSD.org, cvs-all@FreeBSD.org
Subject: Re: cvs commit: src/usr.sbin/inetd builtins.c
Message-ID: <20001125081005.K8051@fw.wintelcom.net>
References: <bright@wintelcom.net> <200011251415.eAPEFL566372@green.dyndns.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.2.5i
In-Reply-To: <200011251415.eAPEFL566372@green.dyndns.org>; from green@FreeBSD.org on Sat, Nov 25, 2000 at 09:15:21AM -0500
Sender: owner-cvs-all@FreeBSD.ORG
Precedence: bulk
X-Loop: FreeBSD.ORG

* Brian F. Feldman <green@FreeBSD.org> [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().
2) set the fakeid_fd to -1 so that the cleanup after the else clause
   doesn't close a bad fd
3) use isspace() to walk the string instead of strcspn()
   This is just how I'd do it, nothing really important here.
4) use MAXLOGNAME rather than a hardcoded '16', (is this correct?)
5) add some braces to reduce the chance of future headaches.

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;
@@ -606,21 +608,21 @@
 				goto printit;
 			}
 			fclose(fakeid);
+			/* hide from the close() after the else clause */
+			fakeid_fd = -1;	
 			/*
-			 * Usually, the file will have the desired identity
-			 * in the form "identity\n", so we use strcspn() to
-			 * end the string (which fgets() doesn't do.)
+			 * Remove leading/trailing whitespace
 			 */
-			buf[strcspn(buf, "\r\n")] = '\0';
 			cp = buf;
-			/* Allow for beginning white space... */
 			while (isspace(*cp))
 				cp++;
-			/* ...and ending white space. */
-			cp[strcspn(cp, " \t")] = '\0';
-			/* User names of >16 characters are invalid */
-			if (strlen(cp) > 16)
-				cp[16] = '\0';
+			walk = cp;
+			while (!isspace(*walk) && *walk != '\0')
+				walk++;
+			*walk = '\0';
+			/* User names of >= MAXLOGNAME characters are invalid */
+			if (strlen(cp) > MAXLOGNAME - 1)
+				cp[MAXLOGNAME - 1] = '\0';
 			/*
 			 * If the name is a zero-length string or matches
 			 * the name of another user, it's invalid, so
@@ -633,14 +635,16 @@
 					iderror(lport, fport, s, errno);
 				cp = pw->pw_name;
 			}
-		} else
+		} else {
 			cp = pw->pw_name;
+		}
 		if (fakeid_fd != -1)
 			close(fakeid_fd);
-	} else if (!usedfallback)
+	} else if (!usedfallback) {
 		cp = pw->pw_name;
-	else
+	} else {
 		cp = fallback;
+	}
 printit:
 	/* Finally, we make and send the reply. */
 	if (asprintf(&p, "%d , %d : USERID : %s : %s\r\n", lport, fport, osname,


-- 
-Alfred Perlstein - [bright@wintelcom.net|alfred@freebsd.org]
"I have the heart of a child; I keep it in a jar on my desk."


To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe cvs-all" in the body of the message