Re: RFC: Port of NetBSD cat(1)'s -f option.

On Thu, 16 May 2002, Giorgos Keramidas wrote:

> In message: <[email protected]>
>             Mike Makonnen <[email protected]> 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 [email protected]
with "unsubscribe freebsd-audit" in the body of the message