[PATCH] Improved temp-file creation

[PATCH] Improved temp-file creation

[Wayne Davison]

[-- 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. */

Re: [PATCH] Improved temp-file creation

[Geoff Wing]

Wayne Davison <wayned@users.sourceforge.net> output:
:--- Src/builtin.c	18 Oct 2004 19:07:50 -0000	1.130
:+++ Src/builtin.c	19 Oct 2004 05:32:17 -0000
[...]
:-	if (((tempfd = open(fil, O_WRONLY | O_CREAT | O_EXCL | O_NOCTTY, 0600))

:--- Src/exec.c	18 Oct 2004 19:07:55 -0000	1.75
:+++ Src/exec.c	19 Oct 2004 05:32:19 -0000
[...]
:-    if (!s || (fd = open(s, O_CREAT|O_WRONLY|O_EXCL|O_NOCTTY, 0600)) == -1)

:+gettempfile(const char *prefix, int use_heap, char **tempname)
[...]
:+	if ((fd = open(fn, O_RDWR | O_CREAT | O_EXCL, 0600)) >= 0)

You've replaced a couple of open()s which were O_NOCTTY with one that isn't.
Does this affect anything?  Were they erroneously O_NOCTTY or were they
specifically that way for scripts or something else?  You haven't mentioned
either here or in the ChangeLog that this was intentional.

Regards,
Geoff

Re: [PATCH] Improved temp-file creation

[Wayne Davison]

On Fri, Oct 29, 2004 at 05:01:20PM +1000, Geoff Wing wrote:
> You've replaced a couple of open()s which were O_NOCTTY with one that isn't.

Correct.  I had meant to discuss that, but neglected to do so.

Since we're using O_CREAT to ensure that we open a new file, I didn't
see any need to also specify O_NOCTTY -- i.e. how can the file be a tty
if it was just created?

Since some of the replaced open() calls used O_CREAT included O_NOCTTY
and some didn't, I chose to settle on leaving it out.  If I was wrong
about the flag being useless, we can add it back in.

Thanks for checking over the changes!

..wayne..