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

From: Jim Meyering <[email-masked]>
Date: Sun, 16 Oct 2011 12:22:47 +0200

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
    tac: don't misbehave with multiple non-seekable inputs
    tac: don't leak a file descriptor for each non-seekable input

Before the primary bug fix (2/3), tac could leak only one file
descriptor, because that bug prevented us from opening 2nd and
subsequent files.  But once fixed, it would leak an FD for each "-"
(or other nonseekable) command line argument.

>From cdd328f232a93fb40aec25d0681ef191eaeba2da Mon Sep 17 00:00:00 2001
From: Jim Meyering <[email-masked]>
Date: Sun, 16 Oct 2011 10:35:56 +0200
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
@@ -451,8 +448,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 +457,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

>From 608f9d9d0daef48c957fe38570e5d0f293c0f1eb Mon Sep 17 00:00:00 2001
From: Jim Meyering <[email-masked]>
Date: Sun, 16 Oct 2011 12:07:05 +0200
Subject: [PATCH 2/3] tac: don't misbehave with multiple non-seekable inputs

* src/tac.c (copy_to_temp): Do not reuse the template buffer.
Instead, scribble only on a freshly-xstrdup'd copy each time.
Free that buffer both here, upon failure, and ...
(tac_nonseekable): ...free the buffer in caller, upon success.
* tests/misc/tac-2-nonseekable: New file.
* tests/Makefile.am (TESTS): Add it.
* NEWS (Bug fixes): Mention it.
Reported by Ambrose Feinstein in http://debbugs.gnu.org/9762.
---
 NEWS                         |    5 +++++
 src/tac.c                    |   19 +++++++++++++++----
 tests/Makefile.am            |    1 +
 tests/misc/tac-2-nonseekable |   27 +++++++++++++++++++++++++++
 4 files changed, 48 insertions(+), 4 deletions(-)
 create mode 100755 tests/misc/tac-2-nonseekable

diff --git a/NEWS b/NEWS
index 4c8e162..3ed44b2 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,11 @@ GNU coreutils NEWS                                    -*- outline -*-

 * Noteworthy changes in release ?.? (????-??-??) [?]

+** Bug fixes
+
+  tac no longer fails to handle two or more non-seekable inputs
+  [bug introduced in coreutils-5.3.0]
+

 * Noteworthy changes in release 8.14 (2011-10-12) [stable]

diff --git a/src/tac.c b/src/tac.c
index c572862..2d8d6ea 100644
--- a/src/tac.c
+++ b/src/tac.c
@@ -448,12 +448,13 @@ 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.  */

-  char *tempfile = template;
-  int fd = mkstemp (template);
+  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;
     }

@@ -463,6 +464,7 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file)
       error (0, errno, _("cannot open %s for writing"), quote (tempfile));
       close (fd);
       unlink (tempfile);
+      free (tempfile);
       return false;
     }

@@ -498,6 +500,7 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file)

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

@@ -509,8 +512,16 @@ tac_nonseekable (int input_fd, const char *file)
 {
   FILE *tmp_stream;
   char *tmp_file;
-  return (copy_to_temp (&tmp_stream, &tmp_file, input_fd, file)
-          && tac_seekable (fileno (tmp_stream), tmp_file));
+  if (copy_to_temp (&tmp_stream, &tmp_file, input_fd, file))
+    {
+      if (tac_seekable (fileno (tmp_stream), tmp_file))
+        {
+          free (tmp_file);
+          return true;
+        }
+    }
+
+  return false;
 }

 /* Print FILE in reverse, copying it to a temporary
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 9c9a1b8..5021c18 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -274,6 +274,7 @@ TESTS =						\
   misc/sum-sysv					\
   misc/tac					\
   misc/tac-continue				\
+  misc/tac-2-nonseekable			\
   misc/tail					\
   misc/tee					\
   misc/tee-dash					\
diff --git a/tests/misc/tac-2-nonseekable b/tests/misc/tac-2-nonseekable
new file mode 100755
index 0000000..7b48773
--- /dev/null
+++ b/tests/misc/tac-2-nonseekable
@@ -0,0 +1,27 @@
+#!/bin/sh
+# ensure that tac works with two or more non-seekable inputs
+
+# Copyright (C) 2011 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/init.sh"; path_prepend_ ../src
+print_ver_ tac
+
+echo x | tac - - > out 2> err || fail=1
+echo x > exp || fail=1
+compare out exp || fail=1
+compare err /dev/null || fail=1
+
+Exit $fail
--
1.7.7

>From 95fa11b63f0a5c983723f02afa7c5b896e5a0a97 Mon Sep 17 00:00:00 2001
From: Jim Meyering <[email-masked]>
Date: Sun, 16 Oct 2011 12:14:05 +0200
Subject: [PATCH 3/3] tac: don't leak a file descriptor for each non-seekable
 input

* src/tac.c (tac_nonseekable): Call fclose after each successful
call to copy_to_temp.
---
 src/tac.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/src/tac.c b/src/tac.c
index 2d8d6ea..ebafa18 100644
--- a/src/tac.c
+++ b/src/tac.c
@@ -516,9 +516,11 @@ tac_nonseekable (int input_fd, const char *file)
     {
       if (tac_seekable (fileno (tmp_stream), tmp_file))
         {
+          fclose (tmp_stream);
           free (tmp_file);
           return true;
         }
+      fclose (tmp_stream);
     }

   return false;
--
1.7.7