On 10/7/22 05:36, Eli Zaretskii wrote:
I'd appreciate a more high-level description of the idea of the
change, in addition to the gory details.
I gave it a shot in the attached patch, which is an improved version of
the previous patch.
Why !WINDOWS32 here?
Earlier versions of the code avoided the use of tmpfile on MS-Windows
since it might put the temp file in the root directory even if the user
has specified a different directory in MAKE_TMPDIR or some similar
environment variable.
Come to think of it, that problem is not limited to MS-Windows; there's
a similar problem on POSIX too. So the attached patch removes the
"!WINDOWS32" here, and attacks the problem in the similar way on all
platforms, by using tmpfile only if the environment variables are unset
or specify the default directory anyway.
This opens the file in binary mode where previously it wasn't; what is
the rationale for the change? And why the "+" part?
tmpfile uses "wb+" (POSIX requires this) and we should be consistent in
all the paths that create temporary FILE *. The attached patch adds a
comment about this.
Thanks for the review.
From 492ed0bdd4151e6d14599b70719526b6086ed509 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
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
