Date: Tue, 12 Feb 2002 17:03:03 -0500 From: Mike Barcroft <mike@FreeBSD.ORG> To: Chuck Rouillard <chuckr@opus.sandiegoca.ncr.com> Cc: FreeBSD-Standards <freebsd-standards@FreeBSD.ORG> Subject: Re: pathchk - review Message-ID: <20020212170303.B55750@espresso.q9media.com> In-Reply-To: <20020205232519.N7805-101000@opus.sandiegoca.ncr.com>; from chuckr@opus.sandiegoca.ncr.com on Tue, Feb 05, 2002 at 11:54:26PM -0800 References: <20020129210829.GC50337@madman.nectar.cc> <20020205232519.N7805-101000@opus.sandiegoca.ncr.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Chuck Rouillard <chuckr@opus.sandiegoca.ncr.com> writes:
> A revised `pathchk' is being submitted for review.
Sorry for the delay in my review.
> Makefile:
`# $FreeBSD$' is needed here.
> PROG= pathchk
>
> .include <bsd.prog.mk>
> pathchk.1:
> .\" Copyright (c) 2001, 2002 Chuck Rouillard
> .\" All rights reserved.
> .\"
> .\" Redistribution and use in source and binary forms, with or without
> .\" modification, are permitted provided that the following conditions
> .\" are met:
> .\" 1. Redistributions of source code must retain the above copyright
> .\" notice, this list of conditions and the following disclaimer.
> .\" 2. Redistributions in binary form must reproduce the above copyright
> .\" notice, this list of conditions and the following disclaimer in the
> .\" documentation and/or other materials provided with the distribution.
> .\" 3. The name of the author may not be used to endorse or promote
> .\" products derived from this software without specific prior written
> .\" permission.
> .\"
> .\" THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS
> .\" OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> .\" WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> .\" ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
> .\" DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> .\" DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> .\" OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> .\" HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> .\" LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> .\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> .\" SUCH DAMAGE.
> .\"
> .\"
This line should be `.\" $FreeBSD$'.
> .\"
> .Dd January 2, 2002
This could be refreshed. :)
> .Dt PATHCHK 1
> .Os
> .Sh NAME
> .Nm pathchk
> .Nd checks pathnames
> .Sh SYNOPSIS
> .Nm pathchk
> .Op Fl p
> .Ar pathname
> .Ar ...
> .Sh DESCRIPTION
> The
> .Nm pathchk
The `pathchk' argument here is redundent. I don't comment on this
problem below.
> utility verifies whether a specified
> .Ar pathname
> may be used to access or create a file without error. By default,
New sentences need to begin on new lines. I don't comment on this
problem below.
> the overall path, and each
> .Ar pathname
> component, is checked against the underlying filesystem. Any of the
> following errors will result in a message along with the offending
> .Ar pathname
> sent to stderr.
> .Pp
> .Bl -bullet
> .It
> The
> .Ar pathname
> is longer than the maximum defined by PATH_MAX for the underlying
> filesystem.
> .It
> The
> .Ar pathname
> contains a path component longer than the maximum defined by NAME_MAX
> for the underlying filesystem.
> .It
> A component of the
> .Ar pathname
> which is not searchable or inaccessable by the
> underlying filesystem.
> .It
> Contains any character(s) which are invalid for the containing directory.
> .El
> .Pp
> Any
> .Ar pathname
> containing a non-existent path component is not considered an error if
> none of the remaining components nor the overall path length exceed the
> maximums defined by the underlying filesystem.
> .Pp
> The options are as follows:
> .Bl -tag -width indent
> .It Fl p
> Verify each
> .Ar pathname
> using portability checks instead of the underlying filesystem. Report
> any of the following as errors to console.
> .Pp
> .Bl -bullet
> .It
> The
> .Ar pathname
> is longer than the maximum _POSIX_PATH_MAX.
> .It
> The
> .Ar pathname
> contains a path component longer than the maximum _POXIX_NAME_MAX.
> .It
> The
> .Ar pathname
> contains at least one character which is not in the set of A-Z, a-z, 0-9,
I think these should be listed in individual .Dq macros.
> `.'(period), `-'(hyphen), or `_'(underscore). The `-'(hyphen) may not be
.Sq and .Pq should be used to here to wrap.
> used as the first character of any path component.
> .El
> .El
> .Pp
`.Sh DIAGNOSTICS' needed here.
> .Nm pathchk
> exits 0 on success, 1 if an error occurred.
The .Ex macro should be used here.
> .Sh SEE ALSO
> .Xr stat 2 ,
> .Xr pathconf 2
Misordered references.
> .Sh STANDARDS
> The
> .Nm pathchk
> utility is expected to conform to
> .St -susv2
> and
> .St -p1003.2 .
I think this should be SUSv3 and not refer to POSIX.2.
You may want to have Ruslan review this too. I don't have a lot of
mdoc-fu.
> pathchk.c
> /*
> * Copyright (c) 2001, 2002 Chuck Rouillard
> * All rights reserved.
> *
> * Redistribution and use in source and binary forms, with or without
> * modification, are permitted provided that the following conditions
> * are met:
> * 1. Redistributions of source code must retain the above copyright
> * notice, this list of conditions and the following disclaimer.
> * 2. Redistributions in binary form must reproduce the above copyright
> * notice, this list of conditions and the following disclaimer in the
> * documentation and/or other materials provided with the distribution.
> * 3. The name of the author may not be used to endorse or promote
> * products derived from this software without specific prior written
> * permission.
> *
> * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS
> * OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
> * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> * SUCH DAMAGE.
Needs a ` *' line and a ` * $FreeBSD$' line.
> */
> #include <sys/stat.h>
> #include <sys/types.h>
>
> #include <errno.h>
> #include <limits.h>
> #include <locale.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
>
> const char *illegalchar = "pathchk: Illegal character";
> const char *memoryerror = "pathchk: No memory to allocate";
> const char *nametoolong = "pathchk: Path component too long";
> const char *pathchkhelp = "usage: pathchk [-p] path ...\n";
> const char *pathtoolong = "pathchk: Pathname too long";
> const char *posix_chars = "0123456789._-"
> "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
> "abcdefghijklmnopqrstuvwxyz";
These should all be #define's, unless I missed somewhere that they are
changed.
> /*
> * Local function prototypes.
> */
> int posixpath(char *);
> int pe(const char *, int);
> int systempath(char *);
> void usage(void);
These should all have `static' keywords. That will eliminate the need
for the comment too.
>
> /*
This line needs to be `/*-' to follow KNF, since you do special
tabbing in this comment. See indent(1) for details. I don't comment
on this below.
> * Verify path portability by checking if the:
> * - complete path and filename <= _POSIX_PATH_MAX,
> * - each path component length <= _POSIX_NAME_MAX,
> * - and that each path component contains no
> * disallowed characters.
> * Return 0 for an accepted path, 1 otherwise.
> */
> int
> posixpath(char *pathname)
> {
> int namelen, pathlen, rval;
> char *name, *path;
> char *wpath, *wptr;
These two lines can be combined. I think pointers are generally
considered to be larger than integers, so it might be a good idea to
move the combined line to the top.
>
> pathlen = strlen(pathname);
> if (pathlen > _POSIX_PATH_MAX) {
> (void)fprintf(stderr, "%s\n%s\n", pathtoolong, pathname);
> return 1;
Return values need to be wrapped in parens. I don't comment on this
below.
> }
>
> /*
> * Create space in which to build a validated
Comment lines should be wrapped at 80 characters. I don't comment on
this below.
> * path. Upon any error, this path is shown
> * in its current state.
> */
> if ((path = malloc(pathlen + 1)) == NULL) {
> (void)fprintf(stderr, "%s\n", memoryerror);
> exit(1);
I thought this was mentioned in a earlier review, but this should use
err(1). Since the errno can provide a more traditional message or a
locale-specific message (if we had support for that :). I don't
comment on this below.
Please use <sysexists.h>-style exit levels everywhere.
> }
> *path = NULL;
>
> /*
> * Since `strsep' mangles the strings sent to it,
> * create a working copy of pathname to preserve
> * the original argument value. Note that an
> * additional working pointer is set for use
> * with `free'.
> */
> if ((wpath = strdup(pathname)) == NULL) {
> (void)fprintf(stderr, "%s\n", memoryerror);
> exit(1);
> }
> wptr = wpath;
>
> /*
> * Begin path validation by checking each path
> * component for any exceeded length and any
> * disallowed character(s).
> */
> rval = 0;
> while ((name = strsep(&wpath, "/")) != NULL) {
> if ((namelen = strlen(name)) != 0) {
> (void)strcat(path, name);
Presumably `name' is guaranteed to fit into `path'? (I didn't check.)
> if (namelen > _POSIX_NAME_MAX) {
> (void)fprintf(stderr, "%s\n%s\n",
> nametoolong, path);
Wrapped lines begin four spaces further than the line above. See
style(9) for details. I don't comment on this below.
> rval = 1;
> break;
> }
> if (*name == '-' ||
> strspn(name, posix_chars) != namelen) {
> (void)fprintf(stderr, "%s\n%s\n",
> illegalchar, path);
> rval = 1;
> break;
> }
> }
> if (wpath != NULL)
> (void)strcat(path, "/");
> }
> free(wptr);
> free(path);
> return rval;
> }
>
> /*
> * Assume all error types are path errors except in the
> * case where a path/file does not exist.
> */
> int
> pe(const char *path, int type)
> {
Need an extra line here. Specifically, functions that don't have
local variables still put a blank space here. I don't comment on this
below.
> switch (type) {
> case ENOENT:
> return 0;
> default:
> perror("pathchk");
> (void)fprintf(stderr, "%s\n", path);
> break;
> }
> return 1;
> }
>
> /*
> * Check the current pathname against the underlying
> * filesystem for access, overall path length, and
> * path component length.
> *
> * If the path component is a directory and is:
> * - the last component : return 0
> * - not the last component but searchable : return 0
> * - last component, but not searchable : return 1
> *
> * If the path component is a file and is:
> * - the last component: return 0
> * - not the last component : return 1
> *
> * If a path component (and conseqently the entire path
> * from that point) is found to not exist, then only the
> * overall path and each path component are checked for
> * excessive length.
> */
> int
> systempath(char *pathname)
> {
> struct stat statbuf;
> int namelen, namemax;
> int pathlen, pathmax;
> int pexist, rval;
These three lines can be combined.
> char *name, *path;
> char *wpath, *wptr;
These two lines can be combined. See comment above about pointers.
>
> /*
> * Do an initial check to determine any critical
> * errors and prime max values.
> */
> pexist = 1;
> path = *pathname == '/' ? "/" : ".";
> namemax = pathconf(path , _PC_NAME_MAX);
> pathmax = pathconf(path , _PC_PATH_MAX);
I think pathconf(2)'s return value should be checked for error. I
don't comment on this below.
> if (namemax == -1 || pathmax == -1) {
> if ((pexist = pe(pathname, errno)) == 1)
> return 1;
> }
>
> /*
> * Create space in which to build a validated
> * path. Upon any error, this path is shown
> * in its current state.
> */
> pathlen = strlen(pathname);
> if ((path = malloc(pathlen + 1)) == NULL) {
> (void)fprintf(stderr, "%s\n", memoryerror);
> exit(1);
> }
> *path = NULL;
>
> /*
> * Since `strsep' mangles the strings sent to it,
This should probably say `strsep(3)'.
> * create a working copy of pathname to preserve
> * the original argument value. Note that an
> * additional working pointer is set for use
> * with `free'.
> */
> if ((wpath = strdup(pathname)) == NULL) {
> (void)fprintf(stderr, "%s\n", memoryerror);
> exit(1);
> }
> wptr = wpath;
>
> /*
> * Begin path validation by checking each path
> * component for errors and reached maximums. Any
> * error (except for non-existing paths) will end
> * further testing and exit function.
> */
> rval = 0;
> while ((name = strsep(&wpath, "/")) != NULL) {
> if ((namelen = strlen(name)) != 0) {
> (void)strcat(path, name);
> if (pexist) {
> if (stat(path, &statbuf) == -1) {
> /*
> * Since stat failed, check errno and
> * and show error to console -except-
> * if the path doesn't exist (ENOENT).
> */
> if ((pexist = pe(path, errno)) == 1) {
> rval = 1;
> break;
> }
> } else {
> /*
> * As long as the path is still valid,
> * verify the allowable path length
> * and path component maximums for
> * the current filesystem.
> */
> namemax = pathconf(path, _PC_NAME_MAX);
> pathmax = pathconf(path, _PC_PATH_MAX);
> if (namemax == -1 || pathmax == -1) {
> if ((pexist =
> pe(path, errno)) == 1) {
> rval = 1;
> break;
> }
> }
> }
> } else {
> /*
> * If the path doesn't exist, the only
> * realistic checks are those based on
> * overal path length and path component
> * length. Maximums are based on last
> * successful call to pathconf.
> */
> if (namelen > namemax) {
> (void)fprintf(stderr, "%s\n%s\n",
> nametoolong, path);
> rval = 1;
> break;
> }
> if (pathlen > pathmax) {
> (void)fprintf(stderr, "%s\n%s\n",
> pathtoolong, path);
> rval = 1;
> break;
> }
> }
> }
> if (wpath != NULL)
> (void)strcat(path, "/");
> }
> free(wptr);
> free(path);
> return rval;
> }
>
> void
> usage(void)
> {
> (void)fprintf(stderr, "%s\n", pathchkhelp);
> exit(1);
> }
>
> int
> main(int argc, char **argv)
> {
> int (*fptr)();
> int c, pflag, rval;
>
> pflag = rval = 0;
> while ((c = getopt(argc, argv, "p")) != -1) {
> switch (c) {
> case 'p':
> pflag = 1;
> break;
> case '?':
> default:
> usage();
> }
> }
> argc -= optind;
> argv += optind;
> if (argc == 0)
> usage();
> fptr = (pflag == 0) ? systempath : posixpath;
> while (argc--)
> rval |= (fptr)(*argv++);
> exit(rval);
> }
Best regards,
Mike Barcroft
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-standards" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020212170303.B55750>