[PATCH] Improved temp-file creation

[Wayne Davison <wayned@users.sourceforge.net>]

[-- Attachment #1: Type: text/plain, Size: 944 bytes --]

After my recent changes to gettempname() and its callers, I decided that
it would be nice to have a similar function that works like mkstemp()
(and actually uses it, when available).  This way callers that are going
to do an immediate open() with O_CREAT|O_EXCL can all use the same code,
which will hopefully make it easier to ensure that callers don't use
mktemp() in an unsafe manner.  It also makes things more efficient when
mkstemp() is available because that call can avoid the extra lstat()
calls that mktemp() uses.

The appended patch adds the function gettempfile() that takes the same
(pretty new) parameters as gettempname(), plus an extra one that gets
set to the pointer for the name (since the function returns a file
descriptor).  I then sprinkled around the new function into the code.
One caller of gettempname() even got changed to ask for non-heap memory
instead of calling ztrdup() on the returned heap string.

..wayne..

[-- Attachment #2: temp-files.patch --]
[-- Type: text/plain, Size: 4131 bytes --]

--- Src/builtin.c	18 Oct 2004 19:07:50 -0000	1.130
+++ Src/builtin.c	19 Oct 2004 05:32:17 -0000
@@ -1445,10 +1445,8 @@ bin_fc(char *nam, char **argv, Options o
 	char *fil;
 
 	retval = 1;
-	fil = gettempname(NULL, 1);
-	if (((tempfd = open(fil, O_WRONLY | O_CREAT | O_EXCL | O_NOCTTY, 0600))
-	     == -1) ||
-	    ((out = fdopen(tempfd, "w")) == NULL)) {
+	if ((tempfd = gettempfile(NULL, 1, &fil)) < 0
+	 || ((out = fdopen(tempfd, "w")) == NULL)) {
 	    unqueue_signals();
 	    zwarnnam("fc", "can't open temp file: %e", NULL, errno);
 	} else {
@@ -3535,8 +3533,8 @@ bin_print(char *name, char **args, Optio
 	    zwarnnam(name, "open_memstream failed", NULL, 0);
 #else
 	int tempfd;
-	char *tmpf = gettempname(NULL, 1);
-	if ((tempfd = open(tmpf, O_RDWR|O_CREAT|O_EXCL, 0644)) < 0
+	char *tmpf;
+	if ((tempfd = gettempfile(NULL, 1, &tmpf)) < 0
 	 || (fout = fdopen(tempfd, "w+")) == NULL)
 	    zwarnnam(name, "can't open temp file: %e", NULL, errno);
 	unlink(tmpf);
--- Src/exec.c	18 Oct 2004 19:07:55 -0000	1.75
+++ Src/exec.c	19 Oct 2004 05:32:19 -0000
@@ -2801,8 +2801,7 @@ getherestr(struct redir *fn)
     untokenize(t);
     unmetafy(t, &len);
     t[len++] = '\n';
-    s = gettempname(NULL, 1);
-    if (!s || (fd = open(s, O_CREAT|O_WRONLY|O_EXCL|O_NOCTTY, 0600)) == -1)
+    if ((fd = gettempfile(NULL, 1, &s)) < 0)
 	return -1;
     write(fd, t, len);
     close(fd);
@@ -2975,11 +2974,9 @@ getoutputfile(char *cmd)
 	return NULL;
     if (!(prog = parsecmd(cmd)))
 	return NULL;
-    if (!(nam = gettempname(NULL, 1)))
+    if (!(nam = gettempname(NULL, 0)))
 	return NULL;
 
-    nam = ztrdup(nam);
-
     if (!jobtab[thisjob].filelist)
 	jobtab[thisjob].filelist = znewlinklist();
     zaddlinknode(jobtab[thisjob].filelist, nam);
--- Src/hist.c	18 Oct 2004 19:07:30 -0000	1.55
+++ Src/hist.c	19 Oct 2004 05:32:19 -0000
@@ -2137,8 +2137,7 @@ lockhistfile(char *fn, int keep_trying)
 
 	lockfile = bicat(unmeta(fn), ".LOCK");
 #ifdef HAVE_LINK
-	tmpfile = gettempname(fn, 0);
-	if ((fd = open(tmpfile, O_WRONLY|O_CREAT|O_EXCL, 0644)) >= 0) {
+	if ((fd = gettempfile(fn, 0, &tmpfile)) >= 0) {
 	    FILE *out = fdopen(fd, "w");
 	    if (out) {
 		fprintf(out, "%ld %s\n", (long)getpid(), getsparam("HOST"));
@@ -2163,8 +2162,8 @@ lockhistfile(char *fn, int keep_trying)
 		break;
 	    }
 	    unlink(tmpfile);
+	    free(tmpfile);
 	}
-	free(tmpfile);
 #else /* not HAVE_LINK */
 	while ((fd = open(lockfile, O_WRONLY|O_CREAT|O_EXCL, 0644)) < 0) {
 	    if (errno != EEXIST || !keep_trying)
--- Src/utils.c	18 Oct 2004 19:07:46 -0000	1.68
+++ Src/utils.c	19 Oct 2004 05:32:20 -0000
@@ -1156,6 +1156,47 @@ gettempname(const char *prefix, int use_
     return ret;
 }
 
+/**/
+mod_export int
+gettempfile(const char *prefix, int use_heap, char **tempname)
+{
+    char *fn;
+    int fd;
+#if HAVE_MKSTEMP
+    char *suffix = prefix ? ".XXXXXX" : "XXXXXX";
+
+    if (!prefix && !(prefix = getsparam("TMPPREFIX")))
+	prefix = DEFAULT_TMPPREFIX;
+    if (use_heap)
+	fn = dyncat(unmeta(prefix), suffix);
+    else
+	fn = bicat(unmeta(prefix), suffix);
+
+    fd = mkstemp(fn);
+    if (fd < 0) {
+	if (!use_heap)
+	    free(fn);
+	fn = NULL;
+    }
+#else
+    int failures = 0;
+
+    do {
+	if (!(fn = gettempname(prefix, use_heap))) {
+	    fd = -1;
+	    break;
+	}
+	if ((fd = open(fn, O_RDWR | O_CREAT | O_EXCL, 0600)) >= 0)
+	    break;
+	if (!use_heap)
+	    free(fn);
+	fn = NULL;
+    } while (errno == EEXIST && ++failures < 16);
+#endif
+    *tempname = fn;
+    return fd;
+}
+ 
 /* Check if a string contains a token */
 
 /**/
--- Src/Modules/zftp.c	18 Oct 2004 19:07:56 -0000	1.33
+++ Src/Modules/zftp.c	19 Oct 2004 05:32:21 -0000
@@ -1971,8 +1971,7 @@ zftp_open(char *name, char **args, int f
      * However, it is closed whenever there are no connections open.
      */
     if (zfstatfd == -1) {
-	fname = gettempname(NULL, 1);
-	zfstatfd = open(fname, O_RDWR|O_CREAT|O_EXCL, 0600);
+	zfstatfd = gettempfile(NULL, 1, &fname);
 	DPUTS(zfstatfd == -1, "zfstatfd not created");
 #if defined(F_SETFD) && defined(FD_CLOEXEC)
 	/* If the shell execs a program, we don't want this fd left open. */