Re: [PATCH] Fix some temp file issues

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: Paul Eggert <eggert@cs.ucla.edu>
> 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/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.
> (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?