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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 9762 in the body.
You can then email your comments to 9762 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Ambrose Feinstein <ambrose <at> google.com>
To: bug-coreutils <at> gnu.org
Subject: tac fails when given multiple non-seekable inputs due to misuse of
	mkstemp()
Date: Sat, 15 Oct 2011 13:40:17 -0700

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.

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Erik Auerswald <auerswal <at> unix-ag.uni-kl.de>
To: bug-coreutils <at> gnu.org
Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs
	due to misuse of mkstemp()
Date: Sun, 16 Oct 2011 01:13:52 +0200

Hi,

On Sat, Oct 15, 2011 at 01:40:17PM -0700, Ambrose Feinstein wrote:
> Trivial reproduction:
> 
> $ true | tac - -
> tac: cannot create temporary file in `/tmp': Invalid argument
> 
> This is present in coreutils 8.14.

This is present in coreutils 8.13 as well:

$ tac <(echo a) <(echo b)
tac: cannot create temporary file in `/tmp': Invalid argument
a
$ tac --version
tac (GNU coreutils) 8.13
Copyright (C) 2011 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later
<http://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by Jay Lepreau and David MacKenzie.


Erik

Message #11 received at 9762 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Ambrose Feinstein <ambrose <at> google.com>
Cc: 9762 <at> debbugs.gnu.org
Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due
	to misuse of mkstemp()
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 <meyering <at> redhat.com>
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 <meyering <at> redhat.com>
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 <meyering <at> redhat.com>
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

Message #14 received at 9762 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Ambrose Feinstein <ambrose <at> google.com>
Cc: 9762 <at> debbugs.gnu.org
Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due
	to misuse of mkstemp()
Date: Sun, 16 Oct 2011 21:12:31 +0200

Jim Meyering wrote:
...
> 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 95fa11b63f0a5c983723f02afa7c5b896e5a0a97 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering <at> redhat.com>
> 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;

There was also an error-path leak of "tmp_file", even with the patch
above (which was not pushed).  I've adjusted it to fix that, too:


From 3f468dfce95ae301359511e19b13658f35a90444 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering <at> redhat.com>
Date: Sun, 16 Oct 2011 12:14:05 +0200
Subject: [PATCH] tac: don't leak a file descriptor for each non-seekable
 input

* src/tac.c (tac_nonseekable): Call fclose and free tmp_file after
each successful call to copy_to_temp.
---
 src/tac.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/src/tac.c b/src/tac.c
index 2d8d6ea..13b05fb 100644
--- a/src/tac.c
+++ b/src/tac.c
@@ -512,16 +512,13 @@ tac_nonseekable (int input_fd, const char *file)
 {
   FILE *tmp_stream;
   char *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;
-        }
-    }
+  if (!copy_to_temp (&tmp_stream, &tmp_file, input_fd, file))
+    return false;

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

 /* Print FILE in reverse, copying it to a temporary
--
1.7.7

Message #17 received at 9762 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Ambrose Feinstein <ambrose <at> google.com>
Cc: 9762 <at> debbugs.gnu.org
Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due
	to misuse of mkstemp()
Date: Mon, 17 Oct 2011 17:18:28 +0200

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 <meyering <at> redhat.com>
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

Message #20 received at 9762 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Ambrose Feinstein <ambrose <at> google.com>
Cc: 9762 <at> debbugs.gnu.org
Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due
	to misuse of mkstemp()
Date: Mon, 17 Oct 2011 17:55:06 +0200

Jim Meyering wrote:
...
>>     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
...
>>    /* 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 <meyering <at> redhat.com>
> 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"

And I nearly left it that way.
Tweaked to insert the new include in alphabetical order
like all of the others, and pushed.

Message #23 received at 9762 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Jim Meyering <jim <at> meyering.net>
Cc: 9762 <at> debbugs.gnu.org
Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due
	to misuse of mkstemp()
Date: Mon, 17 Oct 2011 10:24:19 -0700

On 10/17/11 08:18, Jim Meyering wrote:
>> +      char const * const Template = "tacXXXXXX";

That "const * const" prompted me to suggest a minor
improvement.  Since Template's address is never taken,
better would be

  char const Template[] = "tacXXXXXX";

Hmm, or better yet, get rid of Template entirely:

--- a/src/tac.c
+++ b/src/tac.c
@@ -430,12 +430,11 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file)
 
   if (template == NULL)
     {
-      char const * const Template = "tacXXXXXX";
       tempdir = getenv ("TMPDIR");
       if (tempdir == NULL)
         tempdir = DEFAULT_TMPDIR;
 
-      template = file_name_concat (tempdir, Template, NULL);
+      template = file_name_concat (tempdir, "tacXXXXXX", NULL);
     }
 
   /* FIXME: there's a small window between a successful mkstemp call

Message #26 received at 9762 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 9762 <at> debbugs.gnu.org
Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due
	to misuse of mkstemp()
Date: Mon, 17 Oct 2011 19:31:15 +0200

Paul Eggert wrote:
> On 10/17/11 08:18, Jim Meyering wrote:
>>> +      char const * const Template = "tacXXXXXX";
>
> That "const * const" prompted me to suggest a minor
> improvement.  Since Template's address is never taken,
> better would be
>
>   char const Template[] = "tacXXXXXX";
>
> Hmm, or better yet, get rid of Template entirely:

Cleaner and one line shorter.  Thanks!

> --- a/src/tac.c
> +++ b/src/tac.c
> @@ -430,12 +430,11 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file)
>
>    if (template == NULL)
>      {
> -      char const * const Template = "tacXXXXXX";
>        tempdir = getenv ("TMPDIR");
>        if (tempdir == NULL)
>          tempdir = DEFAULT_TMPDIR;
>
> -      template = file_name_concat (tempdir, Template, NULL);
> +      template = file_name_concat (tempdir, "tacXXXXXX", NULL);
>      }
>
>    /* FIXME: there's a small window between a successful mkstemp call

Message #29 received at 9762 <at> debbugs.gnu.org (full text, mbox):

From: Ambrose Feinstein <ambrose <at> google.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: 9762 <at> debbugs.gnu.org
Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due
	to misuse of mkstemp()
Date: Mon, 17 Oct 2011 18:08:29 -0700

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?

Message #32 received at 9762 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 9762 <at> debbugs.gnu.org
Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due
	to misuse of mkstemp()
Date: Tue, 18 Oct 2011 07:46:33 +0200

Paul Eggert wrote:

> On 10/17/11 08:18, Jim Meyering wrote:
>>> +      char const * const Template = "tacXXXXXX";
>
> That "const * const" prompted me to suggest a minor
> improvement.  Since Template's address is never taken,
> better would be
>
>   char const Template[] = "tacXXXXXX";
>
> Hmm, or better yet, get rid of Template entirely:
>
> --- a/src/tac.c
> +++ b/src/tac.c
> @@ -430,12 +430,11 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file)
>
>    if (template == NULL)
>      {
> -      char const * const Template = "tacXXXXXX";
>        tempdir = getenv ("TMPDIR");
>        if (tempdir == NULL)
>          tempdir = DEFAULT_TMPDIR;
>
> -      template = file_name_concat (tempdir, Template, NULL);
> +      template = file_name_concat (tempdir, "tacXXXXXX", NULL);
>      }
>
>    /* FIXME: there's a small window between a successful mkstemp call

I wrote the log for you, and while doing that realized that I found
the following clearer still: (and shorter)

      char *t = getenv ("TMPDIR");
      tempdir = t ? t : DEFAULT_TMPDIR;
      template = file_name_concat (tempdir, "tacXXXXXX", NULL);

With your name on it, I'll wait for an ACK before pushing:

From 385634c8dd512fc4fae46310266247b8fcf2a85a Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Tue, 18 Oct 2011 07:43:58 +0200
Subject: [PATCH] maint: make tac.c slightly cleaner

* src/tac.c (copy_to_temp): Now that the template string tacXXXXXX
is used in only one place, don't bother using a separate variable.
Also, using three unconditional assignments seems slightly clearer.
---
 src/tac.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/tac.c b/src/tac.c
index 97b19ae..7d99595 100644
--- a/src/tac.c
+++ b/src/tac.c
@@ -430,12 +430,9 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file)

   if (template == NULL)
     {
-      char const * const Template = "tacXXXXXX";
-      tempdir = getenv ("TMPDIR");
-      if (tempdir == NULL)
-        tempdir = DEFAULT_TMPDIR;
-
-      template = file_name_concat (tempdir, Template, NULL);
+      char *t = getenv ("TMPDIR");
+      tempdir = t ? t : DEFAULT_TMPDIR;
+      template = file_name_concat (tempdir, "tacXXXXXX", NULL);
     }

   /* FIXME: there's a small window between a successful mkstemp call
--
1.7.6.4

Message #35 received at 9762 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Jim Meyering <jim <at> meyering.net>
Cc: 9762 <at> debbugs.gnu.org
Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due
	to	misuse of mkstemp()
Date: Mon, 17 Oct 2011 23:22:20 -0700

On 10/17/11 22:46, Jim Meyering wrote:
> With your name on it, I'll wait for an ACK before pushing:

Looks good.  Your name should really be on it now....
Thanks.

Message #38 received at 9762 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Ambrose Feinstein <ambrose <at> google.com>
Cc: 9762 <at> debbugs.gnu.org
Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due
	to misuse of mkstemp()
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 <meyering <at> redhat.com>
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 <meyering <at> redhat.com>
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 <meyering <at> redhat.com>
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

Message #41 received at 9762 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Jim Meyering <jim <at> meyering.net>
Cc: 9762 <at> debbugs.gnu.org, Ambrose Feinstein <ambrose <at> google.com>
Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due
	to misuse of mkstemp()
Date: Tue, 18 Oct 2011 09:08:56 -0700

On 10/18/11 05:38, Jim Meyering wrote:
> * src/tac.c (temp_stream): Don't exit immediately upon failed heap
> allocation, here.  That would inhibit processing of any additional
> command-line arguments.

It doesn't matter that much either way here, but in general I'm
leery about any attempt to do useful work after a memory allocation
failure in C.  The output of 'tac' will be wrong anyway, and many
users would prefer 'tac' to simply exit, than to go on and generate
a much larger (but still wrong) output, consuming needless resources.

Also (as my recent experiences with Emacs have shown),
once malloc has failed, other code can start to go wrong,
including library code that we have no control over.  Sometimes
the failures are subtle, and sometimes they're spectacular; either
way it's typically better to steer clear of that particular chasm.

After malloc fails, Emacs typically enters a "restricted" mode,
where it first releases a memory reserve, and then continually
asks you to save your work and exit as soon as possible.  This
is good advice, and even then one is not entirely sure of exiting
safely.

Message #44 received at 9762 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 9762 <at> debbugs.gnu.org, Ambrose Feinstein <ambrose <at> google.com>
Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due
	to misuse of mkstemp()
Date: Tue, 18 Oct 2011 18:27:46 +0200

Paul Eggert wrote:
> On 10/18/11 05:38, Jim Meyering wrote:
>> * src/tac.c (temp_stream): Don't exit immediately upon failed heap
>> allocation, here.  That would inhibit processing of any additional
>> command-line arguments.

Hi Paul,

Perhaps I should add this to the log:

...which may require no additional memory allocation.

> It doesn't matter that much either way here, but in general I'm
> leery about any attempt to do useful work after a memory allocation
> failure in C.  The output of 'tac' will be wrong anyway, and many
> users would prefer 'tac' to simply exit, than to go on and generate
> a much larger (but still wrong) output, consuming needless resources.
>
> Also (as my recent experiences with Emacs have shown),
> once malloc has failed, other code can start to go wrong,
> including library code that we have no control over.  Sometimes
> the failures are subtle, and sometimes they're spectacular; either
> way it's typically better to steer clear of that particular chasm.
>
> After malloc fails, Emacs typically enters a "restricted" mode,
> where it first releases a memory reserve, and then continually
> asks you to save your work and exit as soon as possible.  This
> is good advice, and even then one is not entirely sure of exiting
> safely.

That makes sense for a monolithic, long-running program
like emacs, but command-line tools like tac...
There is little risk.  tac will still exit nonzero,
and that should be sufficient to tell any caller that
there's been a failure.  What is to be gained?
Diagnostics about other file arguments and a greater
percentage of output.

In this case continuing to process arguments after such a failure is
easy and imho, desirable, since a non-seekable input is unusual and
likely to be followed by a seekable one (if by anything).

i.e., if this command fails while processing arg2,

    tac foo <(some command) bar unreadable

it should be able to process "bar" and then diagnose
that the final argument is an unreadable file.

The principle is the same as the one that prompts
POSIX to require (at least from what I recall) that
all arguments be processed, whenever possible.

Message #49 received at 9762-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Ambrose Feinstein <ambrose <at> google.com>
Cc: 9762-done <at> debbugs.gnu.org
Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due
	to misuse of mkstemp()
Date: Wed, 19 Oct 2011 09:34:39 +0200

Jim Meyering wrote:
> 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 <meyering <at> redhat.com>
> 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.

Added this, and pushed:


Suggested by Ambrose Feinstein.
* THANKS.in: Update.


diff --git a/THANKS.in b/THANKS.in
index 23ac679..83a7864 100644
--- a/THANKS.in
+++ b/THANKS.in
@@ -37,6 +37,7 @@ Alexandre Duret-Lutz                duret_g <at> epita.fr
 Alexey Solovyov                     alekso <at> math.uu.se
 Alexey Vyskubov                     alexey <at> pippuri.mawhrin.net
 Alfred M. Szmidt                    ams <at> kemisten.nu
+Ambrose Feinstein                   ambrose <at> google.com
 Andi Kleen                          freitag <at> alancoxonachip.com
 Andre Novaes Cunha                  Andre.Cunha <at> br.global-one.net
 Andreas Frische                     andreasfrische <at> gmail.com

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.