bug#9762: tac fails when given multiple non-seekable inputs due to misuse of mkstemp()

[Jim Meyering]

Jim Meyering wrote:
> Ambrose Feinstein wrote:
>> Trivial reproduction:
>>
>> $ true | tac - -
>> tac: cannot create temporary file in `/tmp': Invalid argument
>>
>> This is present in coreutils 8.14.
>>
>> The cause is the way "template" is reused in copy_to_temp().  The
>> "XXXXXX" suffix is clobbered by the first call to mkstemp(), so the
>> next call returns EINVAL.
>>
>> It looks like the intent is to call mkstemp() at most once and then
>> reuse that file; for example, record_or_unlink_tempfile() will delete
>> at most one file on exit.
>
> Thank you for the report.
> In fixing that, I made three changes:
>
>     maint: tac: remove sole use of sprintf in favor of stpcpy
...

I realized that this part of the first patch wasn't quite right...

> Subject: [PATCH 1/3] maint: tac: remove sole use of sprintf in favor of
>  stpcpy
>
> * src/tac.c (copy_to_temp): Use stpcpy rather than sprintf.
> Move some declarations "down" to point of initialization.
> ---
>  src/tac.c |   17 +++++++----------
>  1 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/src/tac.c b/src/tac.c
> index 65ac6a6..c572862 100644
> --- a/src/tac.c
> +++ b/src/tac.c
> @@ -426,20 +426,17 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int 
> input_fd, char const *file)
>  {
>    static char *template = NULL;
>    static char const *tempdir;
> -  char *tempfile;
> -  FILE *tmp;
> -  int fd;
>
>    if (template == NULL)
>      {
> -      char const * const Template = "%s/tacXXXXXX";
> +      char const * const Template = "tacXXXXXX";
>        tempdir = getenv ("TMPDIR");
>        if (tempdir == NULL)
>          tempdir = DEFAULT_TMPDIR;
>
> -      /* Subtract 2 for `%s' and add 1 for the trailing NUL byte.  */
> -      template = xmalloc (strlen (tempdir) + strlen (Template) - 2 + 1);
> -      sprintf (template, Template, tempdir);
> +      /* Add 1 for the slash and one for the trailing NUL byte.  */
> +      template = xmalloc (strlen (tempdir) + strlen (Template) + 1 + 1);
> +      stpcpy (stpcpy (stpcpy (template, tempdir), "/"), Template);
>      }
>
>    /* FIXME: there's a small window between a successful mkstemp call

Rather than hard-coding the "/", computing lengths, and using separate
xmalloc and 3 stpcpy calls, I prefer to use file_name_concat:

>From 54eab68b8c50f93484194090cdc1f079af5533b6 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sun, 16 Oct 2011 10:35:56 +0200
Subject: [PATCH 1/3] maint: tac: remove sole use of sprintf in favor of
 filenamecat

* src/tac.c: Include filenamecat.h.
(copy_to_temp): Use filenamecat rather than xmalloc and sprintf.
Move some declarations "down" to point of initialization.
---
 src/tac.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/src/tac.c b/src/tac.c
index 65ac6a6..95a5df7 100644
--- a/src/tac.c
+++ b/src/tac.c
@@ -49,6 +49,7 @@ tac -r -s '.\|
 #include "safe-read.h"
 #include "stdlib--.h"
 #include "xfreopen.h"
+#include "filenamecat.h"

 /* The official name of this program (e.g., no `g' prefix).  */
 #define PROGRAM_NAME "tac"
@@ -426,20 +427,15 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int 
input_fd, char const *file)
 {
   static char *template = NULL;
   static char const *tempdir;
-  char *tempfile;
-  FILE *tmp;
-  int fd;

   if (template == NULL)
     {
-      char const * const Template = "%s/tacXXXXXX";
+      char const * const Template = "tacXXXXXX";
       tempdir = getenv ("TMPDIR");
       if (tempdir == NULL)
         tempdir = DEFAULT_TMPDIR;

-      /* Subtract 2 for `%s' and add 1 for the trailing NUL byte.  */
-      template = xmalloc (strlen (tempdir) + strlen (Template) - 2 + 1);
-      sprintf (template, Template, tempdir);
+      template = file_name_concat (tempdir, Template, NULL);
     }

   /* FIXME: there's a small window between a successful mkstemp call
@@ -451,8 +447,8 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int 
input_fd, char const *file)
      FIXME: clean up upon fatal signal.  Don't block them, in case
      $TMPFILE is a remote file system.  */

-  tempfile = template;
-  fd = mkstemp (template);
+  char *tempfile = template;
+  int fd = mkstemp (template);
   if (fd < 0)
     {
       error (0, errno, _("cannot create temporary file in %s"),
@@ -460,7 +456,7 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int 
input_fd, char const *file)
       return false;
     }

-  tmp = fdopen (fd, (O_BINARY ? "w+b" : "w+"));
+  FILE *tmp = fdopen (fd, (O_BINARY ? "w+b" : "w+"));
   if (! tmp)
     {
       error (0, errno, _("cannot open %s for writing"), quote (tempfile));
--
1.7.7