Re: [PATCH] Fix some temp file issues
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
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/GMfifo, where 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;
}
Re: [PATCH] Fix some temp file issues
> From: Paul Eggert
> Cc: Paul Eggert
> Date: Fri, 7 Oct 2022 00:02:25 -0700
>
> This patch was prompted by a linker warning "warning: the use of
> `mktemp' is dangerous, better use `mkstemp' or `mkdtemp'" on
> Fedora 36. It also fixes a few unlikely bugs and simplifies the
> code in a couple of places.
> * src/misc.c (get_tmpdir): Now extern, for os_anontmp.
> (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/GMfifo, where 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.
> (get_tmpfd) [HAVE_DUP && !WINDOWS32]: Use tmpfile for anonymous
> files; on GNU/Linux this avoids a race where the file name is
> temporarily visible. Avoid two unnecessary calls to umask.
> Report a fatal error if mkstemp or its fallback 'open' fail, since
> the caller expects a nonnegative fd.
> (get_tmpfile) [!HAVE_FDOPEN]: Use tmpfile for anonymous files.
> Simplify.
> * src/os.h (os_anontmp): Now always a function.
> * src/posixos.c (jobserver_setup) [HAVE_MKFIFO]:
> Use get_tmpfifoname instead of get_tmppath.
> (os_anontmp): New function, that avoids making the file
> temporarily visible, on GNUish platforms that support O_TMPFILE.
I'd appreciate a more high-level description of the idea of the
change, in addition to the gory details. It is not very easy to glean
that from the patch, since it "also fixes a few unlikely bugs and
simplifies the code in a couple of places".
> @@ -644,11 +660,22 @@ get_tmpfd (char **name)
>fd = os_anontmp ();
>if (fd >= 0)
> return fd;
> +#if HAVE_DUP && !WINDOWS32
> + 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
Why !WINDOWS32 here?
> - char *tmpnm = get_tmppath ();
> -
> - /* Not secure, but...? If name is NULL we could use tmpfile()... */
> - FILE *file = fopen (tmpnm, "w");
> + /* Although the fopen is not secure, this code is executed only on
> + non-fdopen platforms, which should be a rarity nowadays. */
> + FILE *file = name ? fopen (*name = get_tmppath (), "wb+") : tmpfile ();
This opens the file in binary mode where previously it wasn't; what is
the rationale for the change? And why the "+" part?
[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. It also fixes a few unlikely bugs and simplifies the
code in a couple of places.
* src/misc.c (get_tmpdir): Now extern, for os_anontmp.
(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/GMfifo, where 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.
(get_tmpfd) [HAVE_DUP && !WINDOWS32]: Use tmpfile for anonymous
files; on GNU/Linux this avoids a race where the file name is
temporarily visible. Avoid two unnecessary calls to umask.
Report a fatal error if mkstemp or its fallback 'open' fail, since
the caller expects a nonnegative fd.
(get_tmpfile) [!HAVE_FDOPEN]: Use tmpfile for anonymous files.
Simplify.
* src/os.h (os_anontmp): Now always a function.
* src/posixos.c (jobserver_setup) [HAVE_MKFIFO]:
Use get_tmpfifoname instead of get_tmppath.
(os_anontmp): New function, that avoids making the file
temporarily visible, on GNUish platforms that support O_TMPFILE.
---
src/makeint.h | 2 ++
src/misc.c| 79 +++
src/os.h | 4 ---
src/posixos.c | 13 -
4 files changed, 62 insertions(+), 36 deletions(-)
diff --git a/src/makeint.h b/src/makeint.h
index 0bc41eb1..0b1ba058 100644
--- a/src/makeint.h
+++ b/src/makeint.h
@@ -569,6 +569,8 @@ int alpha_compare (const void *, const void *);
void print_spaces (unsigned int);
char *find_percent (char *);
const char *find_percent_cached (const char **);
+const char *get_tmpdir ();
+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..6d77f9ce 100644
--- a/src/misc.c
+++ b/src/misc.c
@@ -566,7 +566,7 @@ umask (mode_t mask)
# endif
#endif
-static const char *
+const char *
get_tmpdir ()
{
static const char *tmpdir = NULL;
@@ -586,48 +586,64 @@ 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
/* 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. */
@@ -644,11 +660,22 @@ get_tmpfd (char **name)
fd = os_anontmp ();
if (fd >= 0)
return fd;
+#if HAVE_DUP && !WINDOWS32
+ 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 +687,8 @@ get_tmpfd (char **name)
/* Can't use mkstemp(), but try to guard against a race condition. */
EINTRLOOP (fd, open (tmpnm,
