Subject: bug#21611: closed (Re: bug#21611: probable bug in tee: tee overwrites argv[argc] (coreutils-8.24))

From: [email-masked] (GNU bug Tracking System)
Date: Sun, 04 Oct 2015 00:07:03 +0000

Your bug report

#21611: probable bug in tee: tee overwrites argv[argc] (coreutils-8.24)

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
21611: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=21611
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#21611: probable bug in tee: tee overwrites argv[argc] (coreutils-8.24)
From: Paul Eggert <[email-masked]>
To: Rainer Deyke <[email-masked]>, [email-masked]
Date: Sat, 3 Oct 2015 17:06:52 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

Rainer Deyke wrote:
> As mentioned in a comment on line 198 of 'tee.c', 'tee' overwrites
> 'argv[argc]'.  This is very bad style at best, and probably undefined behavior
> on at least some platforms.

Thanks for the report.  It's well-defined behavior on any platform conforming to 
the C standard, so it should be quite safe.  That being said, it's a bit faster 
to subtract 1 from a pointer than to do the equivalent of a memmove of an array, 
so it sounds a bit faster to do things the way you suggest.  Cleaner, too.  I 
installed the attached patch.
From 50c41891523e079f87ba2849b0b47c5c9eb3a3ca Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Sat, 3 Oct 2015 17:01:32 -0700
Subject: [PATCH] tee: simplify argv handling

* src/tee.c (tee_files): Last arg is now char ** instead of char
const **, as that is a bit simpler.  All callers changed.  Modify
files[-1], not files[nfiles], as that is a bit faster and simpler.
Latter problem pointed out by Rainer Deyke in:
http://bugs.gnu.org/21611
---
 src/tee.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/src/tee.c b/src/tee.c
index 35120f8..350a201 100644
--- a/src/tee.c
+++ b/src/tee.c
@@ -36,7 +36,7 @@
   proper_name ("Richard M. Stallman"), \
   proper_name ("David MacKenzie")
 
-static bool tee_files (int nfiles, const char **files);
+static bool tee_files (int nfiles, char **files);
 
 /* If true, append to output files rather than truncating them. */
 static bool append;
@@ -168,7 +168,7 @@ main (int argc, char **argv)
   /* Do *not* warn if tee is given no file arguments.
      POSIX requires that it work when given no arguments.  */
 
-  ok = tee_files (argc - optind, (const char **) &argv[optind]);
+  ok = tee_files (argc - optind, &argv[optind]);
   if (close (STDIN_FILENO) != 0)
     error (EXIT_FAILURE, errno, _("standard input"));
 
@@ -176,11 +176,11 @@ main (int argc, char **argv)
 }
 
 /* Copy the standard input into each of the NFILES files in FILES
-   and into the standard output.
+   and into the standard output.  As a side effect, modify FILES[-1].
    Return true if successful.  */
 
 static bool
-tee_files (int nfiles, const char **files)
+tee_files (int nfiles, char **files)
 {
   size_t n_outputs = 0;
   FILE **descriptors;
@@ -193,13 +193,6 @@ tee_files (int nfiles, const char **files)
      ? (append ? "ab" : "wb")
      : (append ? "a" : "w"));
 
-  descriptors = xnmalloc (nfiles + 1, sizeof *descriptors);
-
-  /* Move all the names 'up' one in the argv array to make room for
-     the entry for standard output.  This writes into argv[argc].  */
-  for (i = nfiles; i >= 1; i--)
-    files[i] = files[i - 1];
-
   if (O_BINARY && ! isatty (STDIN_FILENO))
     xfreopen (NULL, "rb", stdin);
   if (O_BINARY && ! isatty (STDOUT_FILENO))
@@ -207,10 +200,13 @@ tee_files (int nfiles, const char **files)
 
   fadvise (stdin, FADVISE_SEQUENTIAL);
 
-  /* In the array of NFILES + 1 descriptors, make
-     the first one correspond to standard output.   */
+  /* Set up FILES[0 .. NFILES] and DESCRIPTORS[0 .. NFILES].
+     In both arrays, entry 0 corresponds to standard output.  */
+
+  descriptors = xnmalloc (nfiles + 1, sizeof *descriptors);
+  files--;
   descriptors[0] = stdout;
-  files[0] = _("standard output");
+  files[0] = bad_cast (_("standard output"));
   setvbuf (stdout, NULL, _IONBF, 0);
   n_outputs++;
 
-- 
2.1.0

Subject: probable bug in tee: tee overwrites argv[argc] (coreutils-8.24)
From: Rainer Deyke <[email-masked]>
To: [email-masked]
Date: Sat, 3 Oct 2015 11:48:59 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

As mentioned in a comment on line 198 of 'tee.c', 'tee' overwrites 
'argv[argc]'.  This is very bad style at best, and probably undefined 
behavior on at least some platforms.  The attached patch replaces the 
current hack with another hack that is still bad style, but probably 
safer than the current hack.

-- 
Rainer Deyke ([email-masked])
--- tee.c	2015-06-26 19:04:19.000000000 +0200
+++ tee-fix.c	2015-10-03 11:35:37.190245786 +0200
@@ -194,10 +194,12 @@
 
   descriptors = xnmalloc (nfiles + 1, sizeof *descriptors);
 
-  /* Move all the names 'up' one in the argv array to make room for
-     the entry for standard output.  This writes into argv[argc].  */
-  for (i = nfiles; i >= 1; i--)
-    files[i] = files[i - 1];
+  /* Effectively move all the names 'up' one in the argv array to
+     make room for the entry for standard output.  This will cause
+     the element of argv immediately before the list of file
+     (either an options argument or argv[0]) to be overwritten
+     later. */
+  files--;
 
   if (O_BINARY && ! isatty (STDIN_FILENO))
     xfreopen (NULL, "rb", stdin);