From owner-freebsd-audit  Thu May 16  0:37:48 2002
Delivered-To: freebsd-audit@freebsd.org
Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16])
	by hub.freebsd.org (Postfix) with ESMTP
	id 047A437B408; Thu, 16 May 2002 00:37:41 -0700 (PDT)
Received: from bde.zeta.org.au (bde.zeta.org.au [203.2.228.102])
	by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id RAA20279;
	Thu, 16 May 2002 17:37:38 +1000
Date: Thu, 16 May 2002 17:40:01 +1000 (EST)
From: Bruce Evans <bde@zeta.org.au>
X-X-Sender: bde@gamplex.bde.org
To: Giorgos Keramidas <keramida@FreeBSD.ORG>
Cc: Mike Makonnen <makonnen@pacbell.net>, <freebsd-audit@FreeBSD.ORG>
Subject: Re: RFC: Port of NetBSD cat(1)'s -f option.
In-Reply-To: <20020515211758.GB68380@hades.hell.gr>
Message-ID: <20020516164332.B1704-100000@gamplex.bde.org>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: owner-freebsd-audit@FreeBSD.ORG
Precedence: bulk
List-ID: <freebsd-audit.FreeBSD.ORG>
List-Archive: <http://docs.freebsd.org/mail/> (Web Archive)
List-Help: <mailto:majordomo@FreeBSD.ORG?subject=help> (List Instructions)
List-Subscribe: <mailto:majordomo@FreeBSD.ORG?subject=subscribe%20freebsd-audit>
List-Unsubscribe: <mailto:majordomo@FreeBSD.ORG?subject=unsubscribe%20freebsd-audit>
X-Loop: FreeBSD.ORG

On Thu, 16 May 2002, Giorgos Keramidas wrote:

> In message: <1021491063.45509.645.camel@kokeb.ambesa.net>
>             Mike Makonnen <makonnen@pacbell.net> writes:
> > > @@ -138,7 +141,15 @@
> > >  			filename = "stdin";
> > >  			fd = STDIN_FILENO;
> > >  		} else {
> > > +			struct stat st;
> > > +
> > >  			filename = path;
> > > +			if (stat(path, &st) < 0 ||
> > > +			    S_ISREG(st.st_mode) == 0) {
> > > +				i++;		/* Skip to next file. */
> > > +				continue;
> > > +			}
> > > +
> >
> > just a minor nit,
> >
> > You might want to show a warning, since stat(2) can fail for any number
> > of reasons not related to the type of file.
>
> Sure, I've changed it to the following.  Since this is a minor change,
> I'll see that is gets done later tonight, if nobody objects 'til then.
>
> %%%
> +			if (stat(path, &st) < 0) {
> +				warn("%s", path);
> +				i++;		/* Skip to next file. */
> +				continue;
> +			}
> +			if (S_ISREG(st.st_mode) == 0) {
> +				i++;		/* Skip to next file. */
> +				continue;
> +			}
> %%%

I object.  If stat() fails, then we have no idea about the file type,
so we shouldn't classify it as regular and must set rval so that the
error is reflected in cat's exit status.  Letting the old code handle
the error seems best.

Using stat() instead of fstat() gives some races.  NetBSD reduces the
races by checking the mode both before and after open().  I didn't
notice if your full patch does this (and deleted it :).  Checking the
mode again after open (after we have found fd > 0) would also fix the
missing (?) check that stdin is a regular file.  NetBSD doesn't check
the mode of stdin but its man page says that -f causes non-plain files
to be skipped.  Checking the mode of stdin is also slightly wrong if
stdin is refered to as /dev/stdin instead of "-".  /dev/stdin is a
(symlink to a) cdev, so it is never considered to be "plain".

A more fundamental bug:
The new variable fflag is never used.

Style bugs:
- nested declaration of st.
- more verbose and bogus handling of the variable 'i'.  'i' is just the
  loop counter for a `for' loop that is obfuscated as a `while' loop.

Bruce


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