From owner-freebsd-standards Tue Feb 12 14: 8:48 2002 Delivered-To: freebsd-standards@freebsd.org Received: from espresso.q9media.com (espresso.q9media.com [216.254.138.122]) by hub.freebsd.org (Postfix) with ESMTP id DBC2537B405 for ; Tue, 12 Feb 2002 14:07:30 -0800 (PST) Received: (from mike@localhost) by espresso.q9media.com (8.11.6/8.11.6) id g1CM33O02997; Tue, 12 Feb 2002 17:03:03 -0500 (EST) (envelope-from mike) Date: Tue, 12 Feb 2002 17:03:03 -0500 From: Mike Barcroft To: Chuck Rouillard Cc: FreeBSD-Standards Subject: Re: pathchk - review Message-ID: <20020212170303.B55750@espresso.q9media.com> References: <20020129210829.GC50337@madman.nectar.cc> <20020205232519.N7805-101000@opus.sandiegoca.ncr.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 Organization: The FreeBSD Project Sender: owner-freebsd-standards@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG Chuck Rouillard writes: > A revised `pathchk' is being submitted for review. Sorry for the delay in my review. > Makefile: `# $FreeBSD$' is needed here. > PROG= pathchk > > .include > 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 > #include > > #include > #include > #include > #include > #include > #include > #include > > 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 -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