From 492ed0bdd4151e6d14599b70719526b6086ed509 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Fri, 7 Oct 2022 21:31:15 -0700 Subject: [PATCH] Fix some temp file issues This patch was prompted by a linker warning "warning: the use of `mktemp' is dangerous, better use `mkstemp' or `mkdtemp'" on Fedora 36. Avoid the use of mktemp, except on non-POSIX platforms that lack mkstemp or lack fdopen. On POSIX platforms, the only place where mktemp was plausible was fifo creation, but there is no need for mktemp there because mkfifo does not suffer from races and 'make' does not need the fifo name to be hard to predict (it can simply fall back on pipes if someone attacks the fifo's name). Also take more care to check for failure values returned from mkstemp, open, etc., and to set and use errno properly. Also, omit some unnecessary calls to umask. * src/misc.c (get_tmpbase): New function, which generalizes the old get_tmptemplate. (get_tmptemplate): Use it. (get_tmpfifoname): New function, which also uses get_tmpbase. Fifo names are now /tmp/GMfifoNNNN, where NNNN is the process id; there is no need to use mktemp for named fifos as mkfifo refuses to create a fifo if the destination already exists, and there is no denial of service as GNU Make silently falls back on a pipe if the named fifo cannot be created. Avoiding mktemp saves us some syscalls and lets us pacify the glibc linker warning. (get_tmppath) [HAVE_MKSTEMP && HAVE_FDOPEN]: Do not define, as it's no longer called in this case. This pacifies the glibc linker warning on GNUish platforms. (os_anontmp) [!WINDOWS32]: New function. (get_tmpfd) [HAVE_DUP]: (get_tmpfile) [!HAVE_FDOPEN]: Use tmpfile for anonymous files if the temporary directory is the default; on GNU/Linux this avoids a race where the file name is temporarily visible, and perhaps that will be true on other systems. (get_tmpfd) Avoid two unnecessary calls to umask when NAME is not null. Report a fatal error if mkstemp or its fallback 'open' fail, since the caller expects a nonnegative fd. (get_tmpfile) Simplify. Be consistent about opening with "wb+". Preserve errno from being modified by umask call. * src/os.h (os_anontmp) [!WINDOWS32]: Do not define as a macro, since it's now a static function. * src/posixos.c (jobserver_setup) [HAVE_MKFIFO]: Use get_tmpfifoname instead of get_tmppath. --- src/makeint.h | 1 + src/misc.c | 110 ++++++++++++++++++++++++++++++++++++-------------- src/os.h | 2 - src/posixos.c | 2 +- 4 files changed, 82 insertions(+), 33 deletions(-) diff --git a/src/makeint.h b/src/makeint.h index 0bc41eb1..04e56c92 100644 --- a/src/makeint.h +++ b/src/makeint.h @@ -569,6 +569,7 @@ int alpha_compare (const void *, const void *); void print_spaces (unsigned int); char *find_percent (char *); const char *find_percent_cached (const char **); +char *get_tmpfifoname (); char *get_tmppath (); int get_tmpfd (char **); FILE *get_tmpfile (char **); diff --git a/src/misc.c b/src/misc.c index 011e4f22..6eb228dc 100644 --- a/src/misc.c +++ b/src/misc.c @@ -586,48 +586,77 @@ get_tmpdir () } static char * -get_tmptemplate () +get_tmpbase (char const *base, int baselen) { const char *tmpdir = get_tmpdir (); char *template; size_t len; len = strlen (tmpdir); - template = xmalloc (len + CSTRLEN (DEFAULT_TMPFILE) + 2); + template = xmalloc (len + baselen + 2); strcpy (template, tmpdir); #ifdef HAVE_DOS_PATHS if (template[len - 1] != '/' && template[len - 1] != '\\') - strcat (template, "/"); -#else -# ifndef VMS + template[len++] = '/'; +#elif !defined VMS if (template[len - 1] != '/') - strcat (template, "/"); -# endif /* !VMS */ -#endif /* !HAVE_DOS_PATHS */ + template[len++] = '/'; +#endif - strcat (template, DEFAULT_TMPFILE); + strcpy (template + len, base); return template; } +static char * +get_tmptemplate () +{ + return get_tmpbase (DEFAULT_TMPFILE, CSTRLEN (DEFAULT_TMPFILE)); +} + +char * +get_tmpfifoname () +{ + static char const fifo_prefix[] = "GMfifo"; + long long pid = make_pid (); + char buf[CSTRLEN (fifo_prefix) + INTSTR_LENGTH + 1]; + int baselen = sprintf (buf, "%s%" MK_PRI64_PREFIX "d", fifo_prefix, pid); + return get_tmpbase (buf, baselen); +} + +#if !HAVE_MKSTEMP || !HAVE_FDOPEN char * get_tmppath () { char *path; -#ifdef HAVE_MKTEMP +# ifdef HAVE_MKTEMP path = get_tmptemplate(); if (*mktemp (path) == '\0') pfatal_with_name ("mktemp"); -#else +# else path = xmalloc (L_tmpnam + 1); if (tmpnam (path) == NULL) pfatal_with_name ("tmpnam"); -#endif +# endif return path; } +#endif + +/* Return a file descriptor for a new anonymous temp file, or -1. */ +#ifndef WINDOWS32 +static int +os_anontmp (void) +{ +# ifdef O_TMPFILE + return open (get_tmpdir (), O_RDWR | O_TMPFILE | O_EXCL, 0600); +# else + return -1; +# endif +} +#endif /* Generate a temporary file and return an fd for it. If name is NULL then the temp file is anonymous and will be deleted when the process exits. */ @@ -636,7 +665,6 @@ get_tmpfd (char **name) { int fd = -1; char *tmpnm; - mode_t mask; /* If there's an os-specific way to get an anoymous temp file use it. */ if (!name) @@ -644,11 +672,27 @@ get_tmpfd (char **name) fd = os_anontmp (); if (fd >= 0) return fd; +#if HAVE_DUP + /* If os_anontmp doesn't work, fall back on tmpfile + dup + fclose. + This avoids ever having a name for a file, on some platforms. + But do this only with the default temporary directory, as + tmpfile uses the default. */ + if (strcmp (get_tmpdir (), DEFAULT_TMPDIR) == 0) + { + mode_t mask = umask (0077); + FILE *tfile = tmpfile (); + if (!tfile) + pfatal_with_name ("tmpfile"); + umask (mask); + fd = dup (fileno (tfile)); + if (fd < 0) + pfatal_with_name ("dup"); + fclose (tfile); + return fd; + } +#endif } - /* Preserve the current umask, and set a restrictive one for temp files. */ - mask = umask (0077); - #if defined(HAVE_MKSTEMP) tmpnm = get_tmptemplate (); @@ -660,8 +704,8 @@ get_tmpfd (char **name) /* Can't use mkstemp(), but try to guard against a race condition. */ EINTRLOOP (fd, open (tmpnm, O_CREAT|O_EXCL|O_RDWR, 0600)); #endif - - umask (mask); + if (fd < 0) + pfatal_with_name (tmpnm); if (name) *name = tmpnm; @@ -677,29 +721,35 @@ get_tmpfd (char **name) FILE * get_tmpfile (char **name) { + /* Be consistent with tmpfile, which opens as if by "wb+". */ + static char const tmpfile_mode[] = "wb+"; + #if defined(HAVE_FDOPEN) int fd = get_tmpfd (name); - return fd < 0 ? NULL : fdopen (fd, "w"); + return fd < 0 ? NULL : fdopen (fd, tmpfile_mode); #else /* Preserve the current umask, and set a restrictive one for temp files. */ mode_t mask = umask (0077); + FILE *file; + int err; - char *tmpnm = get_tmppath (); - - /* Not secure, but...? If name is NULL we could use tmpfile()... */ - FILE *file = fopen (tmpnm, "w"); - - umask (mask); - - if (name) - *name = tmpnm; + if (!name && strcmp (get_tmpdir (), DEFAULT_TMPDIR) == 0) + file = tmpfile (); else { - unlink (tmpnm); - free (tmpnm); + char *filename = get_tmppath (); + if (name) + *name = filename; + + /* Although this fopen is insecure, it is executed only on + non-fdopen platforms, which should be a rarity nowadays. */ + file = fopen (filename, tmpfile_mode); } + err = errno; + umask (mask); + errno = err; return file; #endif } diff --git a/src/os.h b/src/os.h index 81212bc0..6c5eabb2 100644 --- a/src/os.h +++ b/src/os.h @@ -42,8 +42,6 @@ void fd_set_append (int); /* Return a file descriptor for a new anonymous temp file, or -1. */ #if defined(WINDOWS32) int os_anontmp (); -#else -# define os_anontmp() (-1) #endif /* This section provides OS-specific functions to support the jobserver. */ diff --git a/src/posixos.c b/src/posixos.c index a7ef51bf..8074aa1f 100644 --- a/src/posixos.c +++ b/src/posixos.c @@ -140,7 +140,7 @@ jobserver_setup (int slots, const char *style) #if HAVE_MKFIFO if (style == NULL || strcmp (style, "fifo") == 0) { - fifo_name = get_tmppath (); + fifo_name = get_tmpfifoname (); EINTRLOOP (r, mkfifo (fifo_name, 0600)); if (r < 0) -- 2.37.3