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> In-Reply-To: <200011251415.eAPEFL566372@green.dyndns.org>; from green@FreeBSD.org on Sat, Nov 25, 2000 at 09:15:21AM -0500 References: <bright@wintelcom.net> <200011251415.eAPEFL566372@green.dyndns.org>
next in thread | previous in thread | raw e-mail | index | archive | help
* 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20001125081005.K8051>