Date: Fri, 2 Mar 2001 12:25:02 -0600 From: "Jacques A. Vidrine" <n@nectar.com> To: Warner Losh <imp@harmony.village.org> Cc: audit@freebsd.org Subject: Re: PATH_MAX vs MAXPATHLEN Message-ID: <20010302122502.B63024@hamlet.nectar.com> In-Reply-To: <200103020206.f2226Md53114@harmony.village.org>; from imp@harmony.village.org on Thu, Mar 01, 2001 at 07:06:21PM -0700 References: <20010302115105.A63024@hamlet.nectar.com> <200103020206.f2226Md53114@harmony.village.org> <20010302115105.A63024@hamlet.nectar.com> <200103021814.f22IE3d58463@harmony.village.org> <200103020206.f2226Md53114@harmony.village.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Mar 02, 2001 at 11:14:02AM -0700, Warner Losh wrote:
> In message <20010302115105.A63024@hamlet.nectar.com> "Jacques A. Vidrine" writes:
> : I think (strlen(path) > PATH_MAX) is now an off-by-one error,
> : considering the thread of yesterday. It is definately so in
> : some of the code you included (e.g. `char p_path[PATH_MAX]').
>
> It was an off by one error yesterday too :-).
No, `yesterday' PATH_MAX didn't include room for the NUL terminator.
> : These are probably just the result of doing s/MAXPATHLEN + 1/PATH_MAX/
> : in definitions, but s/MAXPATHLEN/PATH_MAX/ in comparisons.
>
> But I didn't do anything like that... BTW, which code fragment are we
> talking about?
I'll include the ones that looked suspect to me below. Comments
follow the fragment in question.
On Thu, Mar 01, 2001 at 07:06:21PM -0700, Warner Losh wrote:
> --- cp/cp.c 1999/11/28 09:34:21 1.24
> +++ cp/cp.c 2001/03/02 02:01:16
> @@ -177,7 +178,7 @@
>
> /* Save the target base in "to". */
> target = argv[--argc];
> - if (strlen(target) > MAXPATHLEN)
> + if (strlen(target) > PATH_MAX)
> errx(1, "%s: name too long", target);
> (void)strcpy(to.p_path, target);
> to.p_end = to.p_path + strlen(to.p_path);
Here, you've changed p_path from MAXPATHLEN+1 to PATH_MAX (in another
chunk below), so the comparison should now be (strlen(target) >= PATH_MAX).
> @@ -318,7 +319,7 @@
> if (*p != '/' && target_mid[-1] != '/')
> *target_mid++ = '/';
> *target_mid = 0;
> - if (target_mid - to.p_path + nlen > MAXPATHLEN) {
> + if (target_mid - to.p_path + nlen > PATH_MAX) {
> warnx("%s%s: name too long (not copied)",
> to.p_path, p);
> badcp = rval = 1;
I haven't looked carefully, but this probably has the same issue.
> --- cp/extern.h 1999/08/27 23:13:39 1.9
> +++ cp/extern.h 2001/03/02 01:55:20
> @@ -37,7 +37,7 @@
> typedef struct {
> char *p_end; /* pointer to NULL at end of path */
> char *target_end; /* pointer to end of target base */
> - char p_path[MAXPATHLEN + 1]; /* pointer to the start of a path */
> + char p_path[PATH_MAX]; /* pointer to the start of a path */
> } PATH_T;
> extern PATH_T to;
This is what I was talking about earlier.
> --- ed/main.c 2000/11/27 06:26:48 1.17
> +++ ed/main.c 2001/03/02 01:57:44
> @@ -96,7 +96,7 @@
> int sigflags = 0; /* if set, signals received while mutex set */
> int sigactive = 0; /* if set, signal handlers are enabled */
>
> -char old_filename[MAXPATHLEN + 1] = ""; /* default filename */
> +char old_filename[PATH_MAX] = ""; /* default filename */
> long current_addr; /* current address in editor buffer */
> long addr_last; /* last address in editor buffer */
> int lineno; /* script line number */
> @@ -950,7 +950,7 @@
> return NULL;
> if (n) printf("%s\n", shcmd + 1);
> return shcmd;
> - } else if (n - 1 > MAXPATHLEN) {
> + } else if (n - 1 > PATH_MAX) {
> sprintf(errmsg, "filename too long");
> return NULL;
> }
Same issue: should now be (n > PATH_MAX).
> @@ -961,7 +961,7 @@
> return NULL;
> }
> #endif
> - REALLOC(file, filesz, MAXPATHLEN + 1, NULL);
> + REALLOC(file, filesz, PATH_MAX, NULL);
> for (n = 0; *ibufp != '\n';)
> file[n++] = *ibufp++;
> file[n] = '\0';
Et cetera... basically one byte less is getting allocated here and
there, but the length checks have not been updated to reflect this.
[snip the rest -- the issues are the same]
Cheers,
--
Jacques Vidrine / n@nectar.com / jvidrine@verio.net / nectar@FreeBSD.org
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-audit" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010302122502.B63024>