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

From: Jim Meyering <[email-masked]>
Date: Tue, 18 Oct 2011 14:38:21 +0200

Ambrose Feinstein wrote:
> Thanks for the quick fix!
>
> Aside from it requiring rearranging other code to not close the fd,
> was there a reason not to reuse a single temp file?

Actually that's what I started doing, but the change seemed like it'd
end up being bigger than I wanted for a bug fix.  In retrospect, now
that I've done it and see the extent, it would have been ok.

Especially once I considered another factor: we want to avoid use of
functions like xmalloc and file_name_concat that call exit upon failure.
See the log on the second patch below for why.

>From fef2bc68e36c8891780311d8869db23753c093d0 Mon Sep 17 00:00:00 2001
From: Jim Meyering <[email-masked]>
Date: Tue, 18 Oct 2011 11:44:39 +0200
Subject: [PATCH 1/3] tac: use only one temporary file, with multiple
 nonseekable inputs

* src/tac.c (temp_stream): New function, factored out of...
(copy_to_temp): ...here.
(tac_nonseekable): Don't free or fclose, now that we reuse the file.
---
 src/tac.c |  120 ++++++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 71 insertions(+), 49 deletions(-)

diff --git a/src/tac.c b/src/tac.c
index 7d99595..6a7e510 100644
--- a/src/tac.c
+++ b/src/tac.c
@@ -418,53 +418,78 @@ record_or_unlink_tempfile (char const *fn, FILE *fp ATTRIBUTE_UNUSED)

 #endif

-/* Copy from file descriptor INPUT_FD (corresponding to the named FILE) to
-   a temporary file, and set *G_TMP and *G_TEMPFILE to the resulting stream
-   and file name.  Return true if successful.  */
-
+/* A wrapper around mkstemp that gives us both an open stream pointer,
+   FP, and the corresponding FILE_NAME.  Always return the same FP/name
+   pair, rewinding/truncating it upon each reuse.  */
 static bool
-copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file)
+temp_stream (FILE **fp, char **file_name)
 {
-  static char *template = NULL;
-  static char const *tempdir;
-
-  if (template == NULL)
+  static char *tempfile = NULL;
+  static FILE *tmp_fp;
+  if (tempfile == NULL)
     {
-      char *t = getenv ("TMPDIR");
-      tempdir = t ? t : DEFAULT_TMPDIR;
-      template = file_name_concat (tempdir, "tacXXXXXX", NULL);
-    }
+      char const *t = getenv ("TMPDIR");
+      char const *tempdir = t ? t : DEFAULT_TMPDIR;
+      tempfile = file_name_concat (tempdir, "tacXXXXXX", NULL);
+
+      /* FIXME: there's a small window between a successful mkstemp call
+         and the unlink that's performed by record_or_unlink_tempfile.
+         If we're interrupted in that interval, this code fails to remove
+         the temporary file.  On systems that define DONT_UNLINK_WHILE_OPEN,
+         the window is much larger -- it extends to the atexit-called
+         unlink_tempfile.
+         FIXME: clean up upon fatal signal.  Don't block them, in case
+         $TMPFILE is a remote file system.  */
+
+      int fd = mkstemp (tempfile);
+      if (fd < 0)
+        {
+          error (0, errno, _("cannot create temporary file in %s"),
+                 quote (tempdir));
+          goto Reset;
+        }

-  /* FIXME: there's a small window between a successful mkstemp call
-     and the unlink that's performed by record_or_unlink_tempfile.
-     If we're interrupted in that interval, this code fails to remove
-     the temporary file.  On systems that define DONT_UNLINK_WHILE_OPEN,
-     the window is much larger -- it extends to the atexit-called
-     unlink_tempfile.
-     FIXME: clean up upon fatal signal.  Don't block them, in case
-     $TMPFILE is a remote file system.  */
-
-  char *tempfile = xstrdup (template);
-  int fd = mkstemp (tempfile);
-  if (fd < 0)
-    {
-      error (0, errno, _("cannot create temporary file in %s"),
-             quote (tempdir));
-      free (tempfile);
-      return false;
-    }
+      tmp_fp = fdopen (fd, (O_BINARY ? "w+b" : "w+"));
+      if (! tmp_fp)
+        {
+          error (0, errno, _("cannot open %s for writing"), quote (tempfile));
+          close (fd);
+          unlink (tempfile);
+        Reset:
+          free (tempfile);
+          tempfile = NULL;
+          return false;
+        }

-  FILE *tmp = fdopen (fd, (O_BINARY ? "w+b" : "w+"));
-  if (! tmp)
+      record_or_unlink_tempfile (tempfile, tmp_fp);
+    }
+  else
     {
-      error (0, errno, _("cannot open %s for writing"), quote (tempfile));
-      close (fd);
-      unlink (tempfile);
-      free (tempfile);
-      return false;
+      if (fseek (tmp_fp, 0, SEEK_SET) < 0
+          || ftruncate (fileno (tmp_fp), 0) < 0)
+        {
+          error (0, errno, _("failed to rewind stream for %s"),
+                 quote (tempfile));
+          return false;
+        }
     }

-  record_or_unlink_tempfile (tempfile, tmp);
+  *fp = tmp_fp;
+  *file_name = tempfile;
+  return true;
+}
+
+/* Copy from file descriptor INPUT_FD (corresponding to the named FILE) to
+   a temporary file, and set *G_TMP and *G_TEMPFILE to the resulting stream
+   and file name.  Return true if successful.  */
+
+static bool
+copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file)
+{
+  FILE *fp;
+  char *file_name;
+  if (!temp_stream (&fp, &file_name))
+    return false;

   while (1)
     {
@@ -477,26 +502,25 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file)
           goto Fail;
         }

-      if (fwrite (G_buffer, 1, bytes_read, tmp) != bytes_read)
+      if (fwrite (G_buffer, 1, bytes_read, fp) != bytes_read)
         {
-          error (0, errno, _("%s: write error"), quotearg_colon (tempfile));
+          error (0, errno, _("%s: write error"), quotearg_colon (file_name));
           goto Fail;
         }
     }

-  if (fflush (tmp) != 0)
+  if (fflush (fp) != 0)
     {
-      error (0, errno, _("%s: write error"), quotearg_colon (tempfile));
+      error (0, errno, _("%s: write error"), quotearg_colon (file_name));
       goto Fail;
     }

-  *g_tmp = tmp;
-  *g_tempfile = tempfile;
+  *g_tmp = fp;
+  *g_tempfile = file_name;
   return true;

  Fail:
-  fclose (tmp);
-  free (tempfile);
+  fclose (fp);
   return false;
 }

@@ -512,8 +536,6 @@ tac_nonseekable (int input_fd, const char *file)
     return false;

   bool ok = tac_seekable (fileno (tmp_stream), tmp_file);
-  fclose (tmp_stream);
-  free (tmp_file);
   return ok;
 }

--
1.7.6.4

>From 1b9bb7a2d29cdf3977bbe23deca3cebe6c03bfaf Mon Sep 17 00:00:00 2001
From: Jim Meyering <[email-masked]>
Date: Tue, 18 Oct 2011 12:04:02 +0200
Subject: [PATCH 2/3] tac: do not let failed allocation cause immediate exit

* src/tac.c (temp_stream): Don't exit immediately upon failed heap
allocation, here.  That would inhibit processing of any additional
command-line arguments.
---
 src/tac.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/src/tac.c b/src/tac.c
index 6a7e510..7cc562e 100644
--- a/src/tac.c
+++ b/src/tac.c
@@ -430,7 +430,12 @@ temp_stream (FILE **fp, char **file_name)
     {
       char const *t = getenv ("TMPDIR");
       char const *tempdir = t ? t : DEFAULT_TMPDIR;
-      tempfile = file_name_concat (tempdir, "tacXXXXXX", NULL);
+      tempfile = mfile_name_concat (tempdir, "tacXXXXXX", NULL);
+      if (tempdir == NULL)
+        {
+          error (0, 0, _("memory exhausted"));
+          return false;
+        }

       /* FIXME: there's a small window between a successful mkstemp call
          and the unlink that's performed by record_or_unlink_tempfile.
--
1.7.6.4

>From 424ed2cd9173ef6191bf5cf38fabc1ed7903c6ef Mon Sep 17 00:00:00 2001
From: Jim Meyering <[email-masked]>
Date: Tue, 18 Oct 2011 14:20:36 +0200
Subject: [PATCH 3/3] maint: tac: prefer "failed to" diagnostic over "cannot"

---
 src/tac.c      |    6 +++---
 tests/misc/tac |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/tac.c b/src/tac.c
index 7cc562e..36122b0 100644
--- a/src/tac.c
+++ b/src/tac.c
@@ -449,7 +449,7 @@ temp_stream (FILE **fp, char **file_name)
       int fd = mkstemp (tempfile);
       if (fd < 0)
         {
-          error (0, errno, _("cannot create temporary file in %s"),
+          error (0, errno, _("failed to create temporary file in %s"),
                  quote (tempdir));
           goto Reset;
         }
@@ -457,7 +457,7 @@ temp_stream (FILE **fp, char **file_name)
       tmp_fp = fdopen (fd, (O_BINARY ? "w+b" : "w+"));
       if (! tmp_fp)
         {
-          error (0, errno, _("cannot open %s for writing"), quote (tempfile));
+          error (0, errno, _("failed to open %s for writing"), quote (tempfile));
           close (fd);
           unlink (tempfile);
         Reset:
@@ -569,7 +569,7 @@ tac_file (const char *filename)
       fd = open (filename, O_RDONLY | O_BINARY);
       if (fd < 0)
         {
-          error (0, errno, _("cannot open %s for reading"), quote (filename));
+          error (0, errno, _("failed to open %s for reading"), quote (filename));
           return false;
         }
     }
diff --git a/tests/misc/tac b/tests/misc/tac
index 7bd07bd..5602128 100755
--- a/tests/misc/tac
+++ b/tests/misc/tac
@@ -68,7 +68,7 @@ my @Tests =
    {ENV => "TMPDIR=$bad_dir"},
    {IN_PIPE => "a\n"},
    {ERR_SUBST => "s,`$bad_dir': .*,...,"},
-   {ERR => "$prog: cannot create temporary file in ...\n"},
+   {ERR => "$prog: failed to create temporary file in ...\n"},
    {EXIT => 1}],

   # coreutils-8.5's tac would double-free its primary buffer.
--
1.7.6.4