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