PATCH: PATH_MAX and mailstat, part I

PATCH: PATH_MAX and mailstat, part I

[Clint Adams]

This makes 'file' be dynamically allocated in mailstat.

Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.16
diff -u -r1.16 utils.c
--- Src/utils.c	2000/09/18 03:36:22	1.16
+++ Src/utils.c	2000/09/18 03:49:16
@@ -3789,8 +3789,8 @@
        struct stat             st_ret, st_tmp;
        static struct stat      st_new_last, st_ret_last;
        char                    dir[PATH_MAX * 2];
-       char                    file[PATH_MAX * 2];
-       int                     i, l;
+       char                    *file;
+       int                     i;
        time_t                  atime = 0, mtime = 0;
 
        /* First see if it's a directory. */
@@ -3832,28 +3832,26 @@
                return 0;
        }
        st_new_last = st_tmp;
-
+       
        /* Loop over new/ and cur/ */
        for (i = 0; i < 2; i++) {
-               sprintf(dir, "%s/%s", path, i ? "cur" : "new");
-               sprintf(file, "%s/", dir);
-               l = strlen(file);
+	   sprintf(dir, "%s/%s", path, i ? "cur" : "new");
                if ((dd = opendir(dir)) == NULL)
-                       return 0;
+		   return 0;
                while ((fn = readdir(dd)) != NULL) {
-                       if (fn->d_name[0] == '.' ||
-                           strlen(fn->d_name) + l >= sizeof(file))
-                               continue;
-                       strcpy(file + l, fn->d_name);
-                       if (stat(file, &st_tmp) != 0)
-                               continue;
-                       st_ret.st_size += st_tmp.st_size;
-                       st_ret.st_blocks++;
-                       if (st_tmp.st_atime != st_tmp.st_mtime &&
-                           st_tmp.st_atime > atime)
-                               atime = st_tmp.st_atime;
-                       if (st_tmp.st_mtime > mtime)
-                               mtime = st_tmp.st_mtime;
+		   if (fn->d_name[0] == '.')
+		       continue;
+
+		   file = zhtricat(dir, "/", fn->d.name);
+		   if (stat(file, &st_tmp) != 0)
+		       continue;
+		   st_ret.st_size += st_tmp.st_size;
+		   st_ret.st_blocks++;
+		   if (st_tmp.st_atime != st_tmp.st_mtime &&
+		       st_tmp.st_atime > atime)
+		       atime = st_tmp.st_atime;
+		   if (st_tmp.st_mtime > mtime)
+		       mtime = st_tmp.st_mtime;
                }
                closedir(dd);
        }

PATCH: PATH_MAX and mailstat, part II

[Clint Adams]

And this makes 'dir' be dynamically allocated in mailstat.

What's missing now are calls to pushheap() and popheap().
I don't see how to do this cleanly in the face of the ubiquitous 
returns.

Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.17
diff -u -r1.17 utils.c
--- Src/utils.c	2000/09/18 03:52:59	1.17
+++ Src/utils.c	2000/09/18 04:02:22
@@ -3788,18 +3788,13 @@
        struct                  dirent *fn;
        struct stat             st_ret, st_tmp;
        static struct stat      st_new_last, st_ret_last;
-       char                    dir[PATH_MAX * 2];
-       char                    *file;
+       char                    *dir, *file;
        int                     i;
        time_t                  atime = 0, mtime = 0;
 
        /* First see if it's a directory. */
        if ((i = stat(path, st)) != 0 || !S_ISDIR(st->st_mode))
                return i;
-       if (strlen(path) > sizeof(dir) - 5) {
-               errno = ENAMETOOLONG;
-               return -1;
-       }
 
        st_ret = *st;
        st_ret.st_nlink = 1;
@@ -3809,17 +3804,17 @@
        st_ret.st_mode  |= S_IFREG;
 
        /* See if cur/ is present */
-       sprintf(dir, "%s/cur", path);
+       dir = dyncat(path, "/cur");
        if (stat(dir, &st_tmp) || !S_ISDIR(st_tmp.st_mode)) return 0;
        st_ret.st_atime = st_tmp.st_atime;
 
        /* See if tmp/ is present */
-       sprintf(dir, "%s/tmp", path);
+       dir = dyncat(path, "/tmp");
        if (stat(dir, &st_tmp) || !S_ISDIR(st_tmp.st_mode)) return 0;
        st_ret.st_mtime = st_tmp.st_mtime;
 
        /* And new/ */
-       sprintf(dir, "%s/new", path);
+       dir = dyncat(path, "/new");
        if (stat(dir, &st_tmp) || !S_ISDIR(st_tmp.st_mode)) return 0;
        st_ret.st_mtime = st_tmp.st_mtime;
 
@@ -3828,32 +3823,32 @@
            st_tmp.st_ino == st_new_last.st_ino &&
            st_tmp.st_atime == st_new_last.st_atime &&
            st_tmp.st_mtime == st_new_last.st_mtime) {
-               *st = st_ret_last;
-               return 0;
+	   *st = st_ret_last;
+	   return 0;
        }
        st_new_last = st_tmp;
-       
+
        /* Loop over new/ and cur/ */
        for (i = 0; i < 2; i++) {
-	   sprintf(dir, "%s/%s", path, i ? "cur" : "new");
-               if ((dd = opendir(dir)) == NULL)
-		   return 0;
-               while ((fn = readdir(dd)) != NULL) {
-		   if (fn->d_name[0] == '.')
-		       continue;
-
-		   file = zhtricat(dir, "/", fn->d.name);
-		   if (stat(file, &st_tmp) != 0)
-		       continue;
-		   st_ret.st_size += st_tmp.st_size;
-		   st_ret.st_blocks++;
-		   if (st_tmp.st_atime != st_tmp.st_mtime &&
-		       st_tmp.st_atime > atime)
-		       atime = st_tmp.st_atime;
-		   if (st_tmp.st_mtime > mtime)
-		       mtime = st_tmp.st_mtime;
-               }
-               closedir(dd);
+	   dir = tricat(path, "/", i ? "cur" : "new");
+	   if ((dd = opendir(dir)) == NULL)
+	       return 0;
+	   while ((fn = readdir(dd)) != NULL) {
+	       if (fn->d_name[0] == '.')
+		   continue;
+
+	       file = zhtricat(dir, "/", fn->d.name);
+	       if (stat(file, &st_tmp) != 0)
+		   continue;
+	       st_ret.st_size += st_tmp.st_size;
+	       st_ret.st_blocks++;
+	       if (st_tmp.st_atime != st_tmp.st_mtime &&
+		   st_tmp.st_atime > atime)
+		   atime = st_tmp.st_atime;
+	       if (st_tmp.st_mtime > mtime)
+		   mtime = st_tmp.st_mtime;
+	   }
+	   closedir(dd);
        }
 
        if (atime) st_ret.st_atime = atime;

PATCH: Redo a couple of PATH_MAX changes

[Bart Schaefer]

On Sep 18, 12:09am, Clint Adams wrote:
} Subject: PATCH: PATH_MAX and mailstat, part II
}
} And this makes 'dir' be dynamically allocated in mailstat.

After 12828, utils.c doesn't compile when MAILDIR_SUPPORT is defined.
It must have been issuing warnings about prototypes even before that, so
I can't believe it's a very widely-used [*] feature, but as commited it
has a structure-access error (->d.name should be ->d_name).

[*] I mistyped that as "wisely-used" twice before getting it right.  The
sentence would have been true either way ...

The following patch does several things.

* Move dyncat() and tricat() from glob.c to utils.c; they've not been
  specific to the glob code for some years now.  (Though it may be time
  to break utils.c up into a couple of files, or move some of the string
  allocation functions to mem.c where ztrdup() lives.)

* Use dyncat()'s trick of remembering the lengths and using strcpy()
  [rather than strcat()] in tricat() and zhtricat().  Also use size_t
  rather than int.

* Use VARARR() in sourcehome().  No need for a malloc/free, we can find
  the size in advance and we don't need to reallocate.

* Use appstr() rather than zhtricat() in mailstat(), because it does a
  realloc() -- we're always sticking different similar-length tails on
  the same head, so there's no reason to allocate and copy the head
  every time around the nested loops.

I'm certain we can replace all uses of PATH_MAX with one of VARARR(),
tricat(), zhtricat(), or ztrdup()+appstr().  The point is to analyze the
way the buffer is used and pick the best of those alternatives, not just
to pick one and use it everywhere.

Index: Src/glob.c
===================================================================
@@ -1661,38 +1661,6 @@
     return ret;
 }
 
-/* concatenate s1 and s2 in dynamically allocated buffer */
-
-/**/
-mod_export char *
-dyncat(char *s1, char *s2)
-{
-    /* This version always uses space from the current heap. */
-    char *ptr;
-    int l1 = strlen(s1);
-
-    ptr = (char *)zhalloc(l1 + strlen(s2) + 1);
-    strcpy(ptr, s1);
-    strcpy(ptr + l1, s2);
-    return ptr;
-}
-
-/* concatenate s1, s2, and s3 in dynamically allocated buffer */
-
-/**/
-mod_export char *
-tricat(char const *s1, char const *s2, char const *s3)
-{
-    /* This version always uses permanently-allocated space. */
-    char *ptr;
-
-    ptr = (char *)zalloc(strlen(s1) + strlen(s2) + strlen(s3) + 1);
-    strcpy(ptr, s1);
-    strcat(ptr, s2);
-    strcat(ptr, s3);
-    return ptr;
-}
-
 /* brace expansion */
 
 /**/
Index: Src/init.c
===================================================================
@@ -1020,22 +1020,18 @@
 void
 sourcehome(char *s)
 {
-    char *buf;
     char *h;
 
     if (emulation == EMULATE_SH || emulation == EMULATE_KSH ||
 	!(h = getsparam("ZDOTDIR")))
 	h = home;
-/* Let source() complain if it's too long */
-#if 0
-    if (strlen(h) + strlen(s) + 1 >= PATH_MAX) {
-	zerr("path too long: %s", s, 0);
-	return;
+
+    {
+	/* Let source() complain if path is too long */
+	VARARR(char, buf, strlen(h) + strlen(s) + 2);
+	sprintf(buf, "%s/%s", h, s);
+	source(buf);
     }
-#endif
-    buf = tricat(h, "/", s);
-    source(buf);
-    zsfree(buf);
 }
 
 /**/
Index: Src/utils.c
===================================================================
@@ -3469,21 +3469,56 @@
     return strcat(realloc(base, strlen(base) + strlen(append) + 1), append);
 }
 
+/* concatenate s1, s2, and s3 in dynamically allocated buffer */
+
+/**/
+mod_export char *
+tricat(char const *s1, char const *s2, char const *s3)
+{
+    /* This version always uses permanently-allocated space. */
+    char *ptr;
+    size_t l1 = strlen(s1);
+    size_t l2 = strlen(s2);
+
+    ptr = (char *)zalloc(l1 + l2 + strlen(s3) + 1);
+    strcpy(ptr, s1);
+    strcpy(ptr + l1, s2);
+    strcpy(ptr + l1 + l2, s3);
+    return ptr;
+}
+
 /**/
 mod_export char *
 zhtricat(char const *s1, char const *s2, char const *s3)
 {
     char *ptr;
+    size_t l1 = strlen(s1);
+    size_t l2 = strlen(s2);
     
-    ptr = (char *)zhalloc(strlen(s1) + strlen(s2) + strlen(s3) + 1);
+    ptr = (char *)zhalloc(l1 + l2 + strlen(s3) + 1);
     strcpy(ptr, s1);
-    strcat(ptr, s2);
-    strcat(ptr, s3);
+    strcpy(ptr + l1, s2);
+    strcpy(ptr + l1 + l2, s3);
     return ptr;
 }
 
+/* concatenate s1 and s2 in dynamically allocated buffer */
 
 /**/
+mod_export char *
+dyncat(char *s1, char *s2)
+{
+    /* This version always uses space from the current heap. */
+    char *ptr;
+    size_t l1 = strlen(s1);
+
+    ptr = (char *)zhalloc(l1 + strlen(s2) + 1);
+    strcpy(ptr, s1);
+    strcpy(ptr + l1, s2);
+    return ptr;
+}
+
+/**/
 static int
 upchdir(int n)
 {
@@ -3782,6 +3817,8 @@
  *
  *     This is good enough for most mail-checking applications.
  */
+
+/**/
 int
 mailstat(char *path, struct stat *st)
 {
@@ -3789,9 +3826,10 @@
        struct                  dirent *fn;
        struct stat             st_ret, st_tmp;
        static struct stat      st_new_last, st_ret_last;
-       char                    *dir, *file;
+       char                    *dir, *file = 0;
        int                     i;
        time_t                  atime = 0, mtime = 0;
+       size_t                  plen = strlen(path), dlen;
 
        /* First see if it's a directory. */
        if ((i = stat(path, st)) != 0 || !S_ISDIR(st->st_mode))
@@ -3805,17 +3843,19 @@
        st_ret.st_mode  |= S_IFREG;
 
        /* See if cur/ is present */
-       dir = dyncat(path, "/cur");
+       dir = appstr(ztrdup(path), "/cur");
        if (stat(dir, &st_tmp) || !S_ISDIR(st_tmp.st_mode)) return 0;
        st_ret.st_atime = st_tmp.st_atime;
 
        /* See if tmp/ is present */
-       dir = dyncat(path, "/tmp");
+       dir[plen] = 0;
+       dir = appstr(dir, "/tmp");
        if (stat(dir, &st_tmp) || !S_ISDIR(st_tmp.st_mode)) return 0;
        st_ret.st_mtime = st_tmp.st_mtime;
 
        /* And new/ */
-       dir = dyncat(path, "/new");
+       dir[plen] = 0;
+       dir = appstr(dir, "/new");
        if (stat(dir, &st_tmp) || !S_ISDIR(st_tmp.st_mode)) return 0;
        st_ret.st_mtime = st_tmp.st_mtime;
 
@@ -3831,14 +3871,23 @@
 
        /* Loop over new/ and cur/ */
        for (i = 0; i < 2; i++) {
-	   dir = tricat(path, "/", i ? "cur" : "new");
-	   if ((dd = opendir(dir)) == NULL)
+	   dir[plen] = 0;
+	   dir = appstr(dir, i ? "/cur" : "/new");
+	   if ((dd = opendir(dir)) == NULL) {
+	       zsfree(file);
+	       zsfree(dir);
 	       return 0;
+	   }
+	   dlen = strlen(dir) + 1; /* include the "/" */
 	   while ((fn = readdir(dd)) != NULL) {
 	       if (fn->d_name[0] == '.')
 		   continue;
-
-	       file = zhtricat(dir, "/", fn->d.name);
+	       if (file) {
+		   file[dlen] = 0;
+		   file = appstr(file, fn->d_name);
+	       } else {
+		   file = tricat(dir, "/", fn->d_name);
+	       }
 	       if (stat(file, &st_tmp) != 0)
 		   continue;
 	       st_ret.st_size += st_tmp.st_size;
@@ -3851,6 +3900,8 @@
 	   }
 	   closedir(dd);
        }
+       zsfree(file);
+       zsfree(dir);
 
        if (atime) st_ret.st_atime = atime;
        if (mtime) st_ret.st_mtime = mtime;

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

PATCH: bin_ln - PATH_MAX

[Clint Adams]

> After 12828, utils.c doesn't compile when MAILDIR_SUPPORT is defined.
> It must have been issuing warnings about prototypes even before that, so
> I can't believe it's a very widely-used [*] feature, but as commited it
> has a structure-access error (->d.name should be ->d_name).

That was my typo in 12827.  Thanks for the cleanup.

> * Move dyncat() and tricat() from glob.c to utils.c; they've not been
>   specific to the glob code for some years now.  (Though it may be time
>   to break utils.c up into a couple of files, or move some of the string
>   allocation functions to mem.c where ztrdup() lives.)

Why not a string.c for all of those?

> I'm certain we can replace all uses of PATH_MAX with one of VARARR(),
> tricat(), zhtricat(), or ztrdup()+appstr().  The point is to analyze the
> way the buffer is used and pick the best of those alternatives, not just
> to pick one and use it everywhere.

PATH_MAX removed from bin_ln

Index: Src/Modules/files.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Modules/files.c,v
retrieving revision 1.7
diff -u -r1.7 files.c
--- Src/Modules/files.c	2000/08/14 07:30:29	1.7
+++ Src/Modules/files.c	2000/09/18 17:10:23
@@ -186,10 +186,10 @@
 bin_ln(char *nam, char **args, char *ops, int func)
 {
     MoveFunc move;
-    int flags, space, err = 0;
-    char **a, *ptr, *rp;
+    int flags, err = 0;
+    char **a, *ptr, *rp, *buf;
     struct stat st;
-    char buf[PATH_MAX * 2 + 1];
+    size_t blen;
 
 
     if(func == BIN_MV) {
@@ -203,7 +203,7 @@
 	    move = (MoveFunc) symlink;
 	else
 #endif
-	     {
+	{
 	    move = (MoveFunc) link;
 	    if(!ops['d'])
 		flags |= MV_NODIRS;
@@ -229,31 +229,24 @@
 	    args[1] = args[0];
     }
     return domove(nam, move, args[0], args[1], flags);
-    havedir:
-    strcpy(buf, *a);
+ havedir:
+    buf = ztrdup(*a);
     *a = NULL;
-    space = PATH_MAX - 1 - ztrlen(buf);
-    rp = strchr(buf, 0);
-    *rp++ = '/';
+    buf = appstr(buf, "/");
+    blen = strlen(buf);
     for(; *args; args++) {
-	if(ztrlen(*args) > PATH_MAX) {
-	    zwarnnam(nam, "%s: %e", *args, ENAMETOOLONG);
-	    err = 1;
-	    continue;
-	}
+
 	ptr = strrchr(*args, '/');
 	if(ptr)
 	    ptr++;
 	else
 	    ptr = *args;
-	if(ztrlen(ptr) > space) {
-	    zwarnnam(nam, "%s: %e", ptr, ENAMETOOLONG);
-	    err = 1;
-	    continue;
-	}
-	strcpy(rp, ptr);
+
+	buf[blen] = 0;
+	buf = appstr(buf, ptr);
 	err |= domove(nam, move, *args, buf, flags);
     }
+    zsfree(buf);
     return err;
 }
 

Re: PATCH: bin_ln - PATH_MAX

[Bart Schaefer]

On Sep 18,  1:21pm, Clint Adams wrote:
} 
} >   (Though it may be time
} >   to break utils.c up into a couple of files, or move some of the string
} >   allocation functions to mem.c where ztrdup() lives.)
} 
} Why not a string.c for all of those?

Probably wouldn't hurt, but let's wait for Sven and PWS to weigh in.
 
} PATH_MAX removed from bin_ln

Looks good.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

PATCH: files module - PATH_MAX

[Clint Adams]

> Looks good.

This should take care of the last direct dependency on PATH_MAX by the
files module.

Index: Src/Modules/files.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Modules/files.c,v
retrieving revision 1.8
diff -u -r1.8 files.c
--- Src/Modules/files.c	2000/09/18 17:29:07	1.8
+++ Src/Modules/files.c	2000/09/19 15:08:50
@@ -255,14 +255,15 @@
 domove(char *nam, MoveFunc move, char *p, char *q, int flags)
 {
     struct stat st;
-    char *qbuf;
-    char pbuf[PATH_MAX + 1];
-    strcpy(pbuf, unmeta(p));
+    char *pbuf, *qbuf;
+
+    pbuf = ztrdup(unmeta(p));
     qbuf = unmeta(q);
     if(flags & MV_NODIRS) {
 	errno = EISDIR;
 	if(lstat(pbuf, &st) || S_ISDIR(st.st_mode)) {
 	    zwarnnam(nam, "%s: %e", p, errno);
+	    zsfree(pbuf);
 	    return 1;
 	}
     }
@@ -270,6 +271,7 @@
 	int doit = flags & MV_FORCE;
 	if(S_ISDIR(st.st_mode)) {
 	    zwarnnam(nam, "%s: cannot overwrite directory", q, 0);
+	    zsfree(pbuf);
 	    return 1;
 	} else if(flags & MV_INTER) {
 	    nicezputs(nam, stderr);
@@ -277,8 +279,10 @@
 	    nicezputs(q, stderr);
 	    fputs("'? ", stderr);
 	    fflush(stderr);
-	    if(!ask())
+	    if(!ask()) {
+		zsfree(pbuf);
 		return 0;
+	    }
 	    doit = 1;
 	} else if((flags & MV_ASKNW) &&
 		!S_ISLNK(st.st_mode) &&
@@ -289,8 +293,10 @@
 	    fprintf(stderr, "', overriding mode %04o? ",
 		mode_to_octal(st.st_mode));
 	    fflush(stderr);
-	    if(!ask())
+	    if(!ask()) {
+		zsfree(pbuf);
 		return 0;
+	    }
 	    doit = 1;
 	}
 	if(doit && !(flags & MV_ATOMIC))
@@ -298,8 +304,10 @@
     }
     if(move(pbuf, qbuf)) {
 	zwarnnam(nam, "%s: %e", p, errno);
+	zsfree(pbuf);
 	return 1;
     }
+    zsfree(pbuf);
     return 0;
 }
 

PATCH: whence - PATH_MAX

[Clint Adams]

This removes direct dependency on PATH_MAX from Src/builtin.c.

Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.35
diff -u -r1.35 builtin.c
--- Src/builtin.c	2000/09/18 03:31:32	1.35
+++ Src/builtin.c	2000/09/19 16:15:11
@@ -2390,17 +2390,14 @@
 	/* Option -a is to search the entire path, *
 	 * rather than just looking for one match. */
 	if (all) {
-	    char **pp, buf[PATH_MAX], *z;
+	    char **pp, *buf, *z;
 
+	    pushheap();
 	    for (pp = path; *pp; pp++) {
-		z = buf;
 		if (**pp) {
-		    strucpy(&z, *pp);
-		    *z++ = '/';
-		}
-		if ((z - buf) + strlen(*argv) >= PATH_MAX)
-		    continue;
-		strcpy(z, *argv);
+		    z = dyncat(*pp, "/");
+		} else z = NULL;
+		buf = dyncat(z, *argv);
 		if (iscom(buf)) {
 		    if (wd) {
 			printf("%s: command\n", *argv);
@@ -2420,6 +2417,7 @@
 		puts(wd ? ": none" : " not found");
 		returnval = 1;
 	    }
+	    popheap();
 	} else if ((cnam = findcmd(*argv, 1))) {
 	    /* Found external command. */
 	    if (wd) {