Date: Sat, 25 Nov 2000 11:56:55 -0500 From: "Brian F. Feldman" <green@FreeBSD.org> To: Alfred Perlstein <bright@wintelcom.net> Cc: cvs-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/usr.sbin/inetd builtins.c Message-ID: <200011251656.eAPGuu571317@green.dyndns.org> In-Reply-To: Message from Alfred Perlstein <bright@wintelcom.net> of "Sat, 25 Nov 2000 08:10:06 PST." <20001125081005.K8051@fw.wintelcom.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Alfred Perlstein <bright@wintelcom.net> wrote:
> * 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().
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200011251656.eAPGuu571317>