PATCH: zasprintf

PATCH: zasprintf

[Clint Adams]

In continuation of the crusade against PATH_MAX, this implements
zasprintf.  It will break on systems where there is a stdarg or
varargs implementation but neither asprintf nor vsnprintf.  I'd
bet that on such a system there would be a vsprintf(), so the
fix would be to write a compatibility function or merely abandon
vsnprintf in favor of vsprintf.

More important is that it will probably break where there exists
varargs but no stdarg, and the compiler is not gcc2.  Either gcc
needs to be fixed, kludges found for other compilers, or makepro.awk
needs some tweaking to handle this special case.

zasprintf() is currently only used in one instance to minimize
potential breakage.

Index: configure.in
===================================================================
RCS file: /cvsroot/zsh/zsh/configure.in,v
retrieving revision 1.30
diff -u -r1.30 configure.in
--- configure.in	2000/09/06 15:50:16	1.30
+++ configure.in	2000/09/16 18:30:01
@@ -451,7 +451,7 @@
 		 limits.h fcntl.h libc.h sys/utsname.h sys/resource.h \
 		 locale.h errno.h stdlib.h unistd.h sys/capability.h \
 		 utmp.h utmpx.h sys/types.h pwd.h grp.h poll.h sys/mman.h \
-		 linux/tasks.h netinet/in_systm.h)
+		 linux/tasks.h netinet/in_systm.h stdarg.h varargs.h)
 if test $dynamic = yes; then
   AC_CHECK_HEADERS(dlfcn.h)
   AC_CHECK_HEADERS(dl.h)
@@ -860,6 +860,7 @@
 	       initgroups nis_list \
 	       setuid seteuid setreuid setresuid setsid \
 	       memcpy memmove strstr strerror \
+	       snprintf vsnprintf asprintf \
 	       cap_get_proc \
 	       getrlimit \
 	       setlocale \
Index: Src/compat.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/compat.c,v
retrieving revision 1.7
diff -u -r1.7 compat.c
--- Src/compat.c	2000/08/14 07:30:28	1.7
+++ Src/compat.c	2000/09/16 18:30:04
@@ -407,6 +407,80 @@
 #endif
 }
 
+#ifndef HAVE_ASPRINTF
+/**/
+# ifndef USE_VARARGS
+/**/
+mod_export int
+zasprintf(char **strp, const char *format, int arg1, int arg2)
+{
+    char *buf;
+    
+    buf = (char *)zalloc(PATH_MAX);
+
+    snprintf(buf, PATH_MAX, format, arg1, arg2);
+    *strp = buf;
+    return strlen(buf);
+}
+
+/**/
+# else
+
+/**/
+#  ifdef PREFER_STDARG
+
+/**/
+mod_export int
+zasprintf(char **strp, const char *format, ...)
+{
+
+/**/
+#  else
+
+/**/
+#   if __GNUC__ > 1
+/* Doing this the "right way" will effect a broken compat.epro file.      *
+ * This kludge should work with gcc2 until a better solution is available */
+
+/**/
+mod_export int
+zasprintf(char **strp, const char *format, __builtin_va_alist_t __builtin_va_alist, __va_ellipsis)
+{
+	
+/**/
+#    else
+#    error varargs prototyping kludge failed
+/**/
+#    endif
+
+/**/
+#  endif
+    va_list arg;
+
+    char *buf;
+    
+    buf = (char *)zalloc(PATH_MAX);
+
+# ifdef PREFER_STDARG
+    va_start(arg, format);
+# else
+    va_start(arg);
+# endif
+
+#ifdef HAVE_VSNPRINTF
+    vsnprintf(buf, PATH_MAX, format, arg);
+#else
+#error You have varargs but no vsnprintf
+#endif
+    va_end (arg);
+    
+    *strp = buf;
+    return strlen(buf);
+}
+/**/
+# endif
+#endif
+
 /*
  * How to print out a 64 bit integer.  This isn't needed (1) if longs
  * are 64 bit, since ordinary %ld will work (2) if we couldn't find a
Index: Src/init.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/init.c,v
retrieving revision 1.11
diff -u -r1.11 init.c
--- Src/init.c	2000/08/08 14:57:03	1.11
+++ Src/init.c	2000/09/16 18:30:07
@@ -1020,18 +1020,22 @@
 void
 sourcehome(char *s)
 {
-    char buf[PATH_MAX];
+    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;
     }
-    sprintf(buf, "%s/%s", h, s);
+#endif
+    zasprintf(&buf, "%s/%s", h, s);
     source(buf);
+    zsfree(buf);
 }
 
 /**/
Index: Src/system.h
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/system.h,v
retrieving revision 1.8
diff -u -r1.8 system.h
--- Src/system.h	2000/09/08 12:51:11	1.8
+++ Src/system.h	2000/09/16 18:30:08
@@ -227,6 +227,26 @@
 # define zopenmax() ((long) OPEN_MAX)
 #endif
 
+#if defined (__STDC__) && defined (HAVE_STDARG_H)
+# define PREFER_STDARG
+# define USE_VARARGS
+# include <stdarg.h>
+#else
+# ifdef HAVE_VARARGS_H
+#  define PREFER_VARARGS
+#  define USE_VARARGS
+#  include <varargs.h>
+# endif
+#endif
+
+#ifdef HAVE_ASPRINTF
+# ifdef __GNUC__
+# define zasprintf(X,F,A...) asprintf(X,F, ## A)
+# else
+# define zasprintf asprintf
+# endif
+#endif
+
 #ifdef HAVE_FCNTL_H
 # include <fcntl.h>
 #else

Re: PATCH: zasprintf

[Bart Schaefer]

On Sep 16,  2:53pm, Clint Adams wrote:
} Subject: PATCH: zasprintf
}
} In continuation of the crusade against PATH_MAX, this implements
} zasprintf.  It will break on systems where there is a stdarg or
} varargs implementation but neither asprintf nor vsnprintf.

This seems to me to be the wrong way to approach this issue.  If you
can't provide a non-broken implementation -- and I don't see how you
can, if you don't plan to implement a printf-format-string parser --
then you should try harder to restrict the problem domain to something
for which you CAN provide a working implementation.

In particular, zsh appears to use PATH_MAX in four cases (plus one
special case):

1) Feeping or issuing an error message when a string that might be a
   path name is "too long," even though that string isn't immediately
   going to be copied into a buffer or even used as a pathname.  In
   most of these cases I think we could just drop the test entirely.

2) Copying a string known to be a path name into a temp buffer assumed
   to be large enough to hold it.  A call to dupstring() or ztrdup()
   would suffice.

3) Pasting a string obtained from readdir() onto a known path prefix.
   In this case, it would be sufficient to use NAME_MAX + strlen()
   (and use pathconf() to get NAME_MAX if necessary).

4) Pasting together two partial path names to make one longer path.
   This is the case where the patch in 12814 uses zasprintf() -- but
   it's overkill, it'd be sufficient to call strlen() on each of the
   two parts to preallocate a large enough buffer.  A simple function
   similar to dyncat() or tricat() would work.

None of those require varargs, stdarg, snprintf, etc.

The last, special case is to create a "big enough" buffer for use by
readlink().  In this case I think we could use lstat() to read the size
of the link itself, and use that to allocate a buffer.  Does anyone
know of an operating system where that would fail?

-- 
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   

PATH_MAX again

[Bart Schaefer]

On Sep 17, 12:47am, Bart Schaefer wrote:
} 
} 1) Feeping or issuing an error message when a string that might be a
}    path name is "too long," even though that string isn't immediately
}    going to be copied into a buffer or even used as a pathname.  In
}    most of these cases I think we could just drop the test entirely.

Along those lines, does anyone know what the test of

	    if (sbptr == PATH_MAX) {
		feep = 1;
		continue;
	    }

in zle_hist.c:doisearch() is intended to accomplish?

-- 
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   

Re: PATCH: zasprintf

[Clint Adams]

> This seems to me to be the wrong way to approach this issue.  If you
> can't provide a non-broken implementation -- and I don't see how you
> can, if you don't plan to implement a printf-format-string parser --
> then you should try harder to restrict the problem domain to something
> for which you CAN provide a working implementation.

I don't think it's impossible to provide a non-broken implementation;
I just haven't succeeded in doing so in all possible eventualities.
One merely has to solve the annoying gcc prototype problem; deal with
platforms that have no asprintf(), no stdarg, and no varargs, of which
I am unfamiliar; provide a compatibility function for vsnprintf();
and realloc in the event of the limit being reached; and it should be
decently portable.

However, I agree that working everywhere is better than working most
places, so below is a patch to replace the call to zasprintf.
It should now be non-broken and still an improvement.

> 1) Feeping or issuing an error message when a string that might be a
> 2) Copying a string known to be a path name into a temp buffer assumed

Agreed on both cases.

> 3) Pasting a string obtained from readdir() onto a known path prefix.
>    In this case, it would be sufficient to use NAME_MAX + strlen()
>    (and use pathconf() to get NAME_MAX if necessary).

I'm uncomfortable with this after the other pathconf episode, especially
since pathconf() might return -1 for NAME_MAX, and then we've got
to deal with that by either arbitrarily setting a value or by dynamically
resizing, whereas I presume asprintf() would be cleaner and more
efficient.

> 4) Pasting together two partial path names to make one longer path.
>    This is the case where the patch in 12814 uses zasprintf() -- but
>    it's overkill, it'd be sufficient to call strlen() on each of the
>    two parts to preallocate a large enough buffer.  A simple function
>    similar to dyncat() or tricat() would work.

tricat() looks suitable to me; is there some reason it isn't?

> The last, special case is to create a "big enough" buffer for use by
> readlink().  In this case I think we could use lstat() to read the size
> of the link itself, and use that to allocate a buffer.  Does anyone
> know of an operating system where that would fail?

No idea.

Index: Src/init.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/init.c,v
retrieving revision 1.12
diff -u -r1.12 init.c
--- Src/init.c	2000/09/16 18:57:45	1.12
+++ Src/init.c	2000/09/17 03:45:35
@@ -1033,7 +1033,7 @@
 	return;
     }
 #endif
-    zasprintf(&buf, "%s/%s", h, s);
+    buf = tricat(h, "/", s);
     source(buf);
     zsfree(buf);
 }

Re: PATCH: zasprintf

[Bart Schaefer]

On Sep 17, 12:25am, Clint Adams wrote:
} Subject: Re: PATCH: zasprintf
}
} > This seems to me to be the wrong way to approach this issue.  If you
} > can't provide a non-broken implementation -- and I don't see how you
} > can, if you don't plan to implement a printf-format-string parser --
} 
} I don't think it's impossible to provide a non-broken implementation;

I don't think it's impossible either, but the only way to "provide a
compatibility function for vsnprintf()" is to implement a parser for
printf format-strings.  There simply is no way to fake it if you don't
have it.  I don't believe there's any use zsh could make of asprintf()
that isn't better solved another way.  In particular:

} > 3) Pasting a string obtained from readdir() onto a known path prefix.
} >    In this case, it would be sufficient to use NAME_MAX + strlen()
} >    (and use pathconf() to get NAME_MAX if necessary).
} 
} I'm uncomfortable with this after the other pathconf episode, especially
} since pathconf() might return -1 for NAME_MAX, and then we've got
} to deal with that by either arbitrarily setting a value or by dynamically
} resizing, whereas I presume asprintf() would be cleaner and more
} efficient.

I don't have any such confidence that asprintf() would be cleaner or more
efficient than, say, a realloc'ing version of tricat() that takes a char**
to an allocated string as its first parameter.

As for pathconf() ... as far as I can tell NAME_MAX has none of the odd
problems associated with PATH_MAX.  It doesn't, for example, potentially
vary from call to call within the same directory.  The only tricky bit
is dealing with -1, which brings us back to the realloc'ing tricat().

-- 
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   

Re: PATCH: zasprintf

[Clint Adams]

> I don't think it's impossible either, but the only way to "provide a
> compatibility function for vsnprintf()" is to implement a parser for
> printf format-strings.  There simply is no way to fake it if you don't

Which is not unheard of.  sendmail, for example, has compatibility
snprintf/vsnprintf functions.

> have it.  I don't believe there's any use zsh could make of asprintf()
> that isn't better solved another way.  In particular:

If by "better" you mean "avoiding asprintf and dealing with a specialized
case."

> I don't have any such confidence that asprintf() would be cleaner or more
> efficient than, say, a realloc'ing version of tricat() that takes a char**
> to an allocated string as its first parameter.

Why, other than portability?  The difference in speed between asprintf()
and the real tricat() is negligible.

Speaking of the real tricat, this eliminates bin_dot's dependence on PATH_MAX:

Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.32
diff -u -r1.32 builtin.c
--- Src/builtin.c	2000/09/14 15:55:00	1.32
+++ Src/builtin.c	2000/09/17 14:27:56
@@ -3250,11 +3250,10 @@
 {
     char **old, *old0 = NULL;
     int ret, diddot = 0, dotdot = 0;
-    char buf[PATH_MAX];
-    char *s, **t, *enam, *arg0;
+    char *s, **t, *enam, *arg0, *buf;
     struct stat st;
 
-    if (!*argv || strlen(*argv) >= PATH_MAX)
+    if (!*argv)
 	return 0;
     old = pparams;
     /* get arguments for the script */
@@ -3295,18 +3294,18 @@
 		    if (diddot)
 			continue;
 		    diddot = 1;
-		    strcpy(buf, arg0);
-		} else {
-		    if (strlen(*t) + strlen(arg0) + 1 >= PATH_MAX)
-			continue;
-		    sprintf(buf, "%s/%s", *t, arg0);
-		}
+		    buf = ztrdup(arg0);
+		} else
+		    buf = tricat(*t, "/", arg0);
+
 		s = unmeta(buf);
 		if (access(s, F_OK) == 0 && stat(s, &st) >= 0
 		    && !S_ISDIR(st.st_mode)) {
 		    ret = source(enam = buf);
+		    zsfree(buf);
 		    break;
 		}
+		zsfree(buf);
 	    }
 	}
     }

Re: PATCH: zasprintf

[Bart Schaefer]

On Sep 17, 11:04am, Clint Adams wrote:
} Subject: Re: PATCH: zasprintf
}
} > I don't believe there's any use zsh could make of asprintf()
} > that isn't better solved another way.
} 
} If by "better" you mean "avoiding asprintf and dealing with a specialized
} case."

I do, mostly because I don't think there are very many specialized cases.
There are 96 uses of sprintf() in zsh, nearly all of them used to convert
integers to strings (which hardly requires a dynamically allocated buffer).
Several of the remaining ones are used for path concatenation.  None have
particularly complex format strings.

I simply think that asprintf() is over-generalized.

} > I don't have any such confidence that asprintf() would be cleaner or more
} > efficient than, say, a realloc'ing version of tricat()
} 
} Why, other than portability?

For one thing, because it can't realloc().  That means that if you use it
in a loop, you have to allocate and free the buffer every time around,
even if you're always writing approximately the same size string.

For another thing:

} Speaking of the real tricat, this eliminates bin_dot's dependence on
} PATH_MAX:

This is a case where a zsh-heap-allocating version of tricat() would be
slightly preferable to a permanent-heap-allocating version.

-- 
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   

Re: PATCH: zasprintf

[Clint Adams]

> This is a case where a zsh-heap-allocating version of tricat() would be
> slightly preferable to a permanent-heap-allocating version.

Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.34
diff -u -r1.34 builtin.c
--- Src/builtin.c	2000/09/17 16:24:06	1.34
+++ Src/builtin.c	2000/09/17 16:38:39
@@ -3297,25 +3297,25 @@
 		break;
 	    }
 	if (!*s || (ret && isset(PATHDIRS) && diddot < 2 && dotdot == 0)) {
+	    pushheap();
 	    /* search path for script */
 	    for (t = path; *t; t++) {
 		if (!(*t)[0] || ((*t)[0] == '.' && !(*t)[1])) {
 		    if (diddot)
 			continue;
 		    diddot = 1;
-		    buf = ztrdup(arg0);
+		    buf = dupstring(arg0);
 		} else
-		    buf = tricat(*t, "/", arg0);
+		    buf = zhtricat(*t, "/", arg0);
 
 		s = unmeta(buf);
 		if (access(s, F_OK) == 0 && stat(s, &st) >= 0
 		    && !S_ISDIR(st.st_mode)) {
 		    ret = source(enam = buf);
-		    zsfree(buf);
 		    break;
 		}
-		zsfree(buf);
 	    }
+	    popheap();
 	}
     }
     /* clean up and return */
Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.14
diff -u -r1.14 utils.c
--- Src/utils.c	2000/08/29 20:27:48	1.14
+++ Src/utils.c	2000/09/17 16:38:46
@@ -3473,6 +3473,20 @@
 }
 
 /**/
+mod_export char *
+zhtricat(char const *s1, char const *s2, char const *s3)
+{
+    char *ptr;
+    
+    ptr = (char *)zhalloc(strlen(s1) + strlen(s2) + strlen(s3) + 1);
+    strcpy(ptr, s1);
+    strcat(ptr, s2);
+    strcat(ptr, s3);
+    return ptr;
+}
+
+
+/**/
 static int
 upchdir(int n)
 {

Re: PATCH: zasprintf

[Zefram]

Bart Schaefer wrote:
>The last, special case is to create a "big enough" buffer for use by
>readlink().  In this case I think we could use lstat() to read the size
>of the link itself, and use that to allocate a buffer.  Does anyone
>know of an operating system where that would fail?

$ uname -sr
Linux 2.2.12-20
$ ls -l /proc/self/cwd
lrwx------   1 zefram   zefram          0 Sep 17 13:17 /proc/self/cwd -> /home/zefram
$

Some other bits of /proc do this too.  And on any system, a link could
be replaced between the lstat() and the readlink().  I think it would
be reasonable to use lstat() for a first guess, but we have to fall back
on an algorithm of increasing the buffer size until it's big enough.

-zefram

RE: PATCH: zasprintf

[Andrej Borsenkow]

> +
> +    *strp = buf;
> +    return strlen(buf);


It is wrong IIRC in case of (v|)snprintf. snprintf may leave buffer not
NULL-terminated in case of overflow. Even worse is, that there seems to be no
standard return value to indicate overflow (some systems return -1; some
buffer length. There was very interesting comment from Mark Crispin on
c-client list recently). And some systems (mine including) do not even
document snprintf.

Somehow I agree with Bart - if we can avoid it, let's avoid it.

-andrej