Subject: bug#8154: du: issue with `--files0-from=DIR'

From: Jim Meyering <[email-masked]>
Date: Wed, 02 Mar 2011 19:53:17 +0100

Jim Meyering wrote:
> Paul Eggert wrote:
>> On 03/02/2011 07:09 AM, Jim Meyering wrote:
>>> -  struct argv_iterator *ai = malloc (sizeof *ai);
>>> +  struct argv_iterator *ai;
>>> +  struct stat st;
>>> +
>>> +  if (fstat (fileno (fp),&st) == 0&&  S_ISDIR (st.st_mode))
>>> +    {
>>> +      errno = EISDIR;
>>> +      return NULL;
>>> +    }
>>> +
>>> +  ai = malloc (sizeof *ai);
>>
>> My kneejerk reaction is that this part of the patch
>> should not be needed (though other fixes obviously are).
>> There are lots of reasons that the stream could fail:
>> why check for directories specially?  Just let the
>> stream fail in the way that it normally would; this
>> keeps the code smaller and simpler.  On an ancient
>> host where "cat dir/" works, "du --files0-from=dir/"
>> should not arbitrarily fail.
>
> I think you're right.  The added complexity (both here
> and in each client) is not worth the theoretical gain
> in error-handling uniformity.
>
> If new tests induce false-positive failure due
> to differences in how reading from a directory works
> under the covers, we can deal with it in the tests.

If we decide it's worthwhile, we can adjust gnulib later.
For now, I'm about to fix coreutils with these two patches:

>From 59b2f5954fb3828108f1c7b7f5d1e968c0801f08 Mon Sep 17 00:00:00 2001
From: Jim Meyering <[email-masked]>
Date: Wed, 2 Mar 2011 18:54:43 +0100
Subject: [PATCH 1/2] wc: avoid NULL dereference on out-of-memory error

* src/wc.c (main): Diagnose failed argv_iter_init_* failure,
rather than falling through and dereferencing NULL.
* NEWS (Bug fixes): Mention it.
---
 NEWS     |    3 +++
 src/wc.c |    3 +++
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/NEWS b/NEWS
index a367d8d..9993469 100644
--- a/NEWS
+++ b/NEWS
@@ -8,6 +8,9 @@ GNU coreutils NEWS                                    -*- outline -*-
   delimiter and an unbounded range like "-f1234567890-".
   [bug introduced in coreutils-5.3.0]

+  wc would dereference a NULL pointer upon an early out-of-memory error
+  [bug introduced in coreutils-7.1]
+

 * Noteworthy changes in release 8.10 (2011-02-04) [stable]

diff --git a/src/wc.c b/src/wc.c
index 3afd4de..ccf4ccf 100644
--- a/src/wc.c
+++ b/src/wc.c
@@ -709,6 +709,9 @@ main (int argc, char **argv)
       ai = argv_iter_init_argv (files);
     }

+  if (!ai)
+    xalloc_die ();
+
   fstatus = get_input_fstatus (nfiles, files);
   number_width = compute_number_width (nfiles, fstatus);

--
1.7.4.1.21.g4cc62

>From 5764128e3f53474f0ea66a989290819338ebf42d Mon Sep 17 00:00:00 2001
From: Jim Meyering <[email-masked]>
Date: Wed, 2 Mar 2011 19:16:46 +0100
Subject: [PATCH 2/2] du: don't infloop for --files0-from=DIR

* src/du.c (main): Fail on AI_ERR_READ error, rather than merely
diagnosing and continuing.  Based on a patch by Stefan Vargyas.
Also move the handling of AI_ERR_EOF into the case stmt.
Do not report ferror/fclose(stdin) failure when we've
already diagnosed e.g., failure to read the DIR, above.
* src/wc.c: Handle read failure as with du: do not exit.
immediately, but rather go on to print any total and to clean-up.
As above, move the handling of AI_ERR_EOF into the case stmt.
* tests/du/files0-from-dir: New file, to test both du and wc.
* tests/Makefile.am (TESTS): Add it.
* NEWS (Bug fixes): Mention it.
---
 NEWS                     |    3 +++
 THANKS.in                |    1 +
 src/du.c                 |   15 ++++++++-------
 src/wc.c                 |   12 +++++++-----
 tests/Makefile.am        |    1 +
 tests/du/files0-from-dir |   39 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 59 insertions(+), 12 deletions(-)
 create mode 100755 tests/du/files0-from-dir

diff --git a/NEWS b/NEWS
index 9993469..658a89a 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,9 @@ GNU coreutils NEWS                                    -*- outline -*-

 ** Bug fixes

+  du would infloop when given --files0-from=DIR
+  [bug introduced in coreutils-7.1]
+
   cut could segfault when invoked with a user-specified output
   delimiter and an unbounded range like "-f1234567890-".
   [bug introduced in coreutils-5.3.0]
diff --git a/THANKS.in b/THANKS.in
index b475eef..fe53c44 100644
--- a/THANKS.in
+++ b/THANKS.in
@@ -524,6 +524,7 @@ Soeren Sonnenburg                   [email-masked]
 Solar Designer                      [email-masked]
 Stanislav Ievlev                    [email-masked]
 Stavros Passas                      [email-masked]
+Stefan Vargyas                      [email-masked]
 Stéphane Chazelas                   [email-masked]
 Stephen Depooter                    [email-masked]
 Stephen Eglen                       [email-masked]
diff --git a/src/du.c b/src/du.c
index 671cac7..67841a3 100644
--- a/src/du.c
+++ b/src/du.c
@@ -926,19 +926,19 @@ main (int argc, char **argv)
       bool skip_file = false;
       enum argv_iter_err ai_err;
       char *file_name = argv_iter (ai, &ai_err);
-      if (ai_err == AI_ERR_EOF)
-        break;
       if (!file_name)
         {
           switch (ai_err)
             {
+            case AI_ERR_EOF:
+              goto argv_iter_done;
             case AI_ERR_READ:
-              error (0, errno, _("%s: read error"), quote (files_from));
-              continue;
-
+              error (0, errno, _("%s: read error"),
+                     quotearg_colon (files_from));
+              ok = false;
+              goto argv_iter_done;
             case AI_ERR_MEM:
               xalloc_die ();
-
             default:
               assert (!"unexpected error code from argv_iter");
             }
@@ -985,11 +985,12 @@ main (int argc, char **argv)
           ok &= du_files (temp_argv, bit_flags);
         }
     }
+ argv_iter_done:

   argv_iter_free (ai);
   di_set_free (di_set);

-  if (files_from && (ferror (stdin) || fclose (stdin) != 0))
+  if (files_from && (ferror (stdin) || fclose (stdin) != 0) && ok)
     error (EXIT_FAILURE, 0, _("error reading %s"), quote (files_from));

   if (print_grand_total)
diff --git a/src/wc.c b/src/wc.c
index ccf4ccf..0399214 100644
--- a/src/wc.c
+++ b/src/wc.c
@@ -722,16 +722,17 @@ main (int argc, char **argv)
       bool skip_file = false;
       enum argv_iter_err ai_err;
       char *file_name = argv_iter (ai, &ai_err);
-      if (ai_err == AI_ERR_EOF)
-        break;
       if (!file_name)
         {
           switch (ai_err)
             {
+            case AI_ERR_EOF:
+              goto argv_iter_done;
             case AI_ERR_READ:
-              error (EXIT_FAILURE, errno, _("%s: read error"),
-                     quote (files_from));
-              continue;
+              error (0, errno, _("%s: read error"),
+                     quotearg_colon (files_from));
+              ok = false;
+              goto argv_iter_done;
             case AI_ERR_MEM:
               xalloc_die ();
             default:
@@ -773,6 +774,7 @@ main (int argc, char **argv)
       else
         ok &= wc_file (file_name, &fstatus[nfiles ? i : 0]);
     }
+ argv_iter_done:

   /* No arguments on the command line is fine.  That means read from stdin.
      However, no arguments on the --files0-from input stream is an error
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 9603441..28eafe8 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -370,6 +370,7 @@ TESTS =						\
   du/exclude					\
   du/fd-leak					\
   du/files0-from				\
+  du/files0-from-dir				\
   du/hard-link					\
   du/inacc-dest					\
   du/inacc-dir					\
diff --git a/tests/du/files0-from-dir b/tests/du/files0-from-dir
new file mode 100755
index 0000000..fc1e184
--- /dev/null
+++ b/tests/du/files0-from-dir
@@ -0,0 +1,39 @@
+#!/bin/sh
+# ensure that du and wc handle --files0-from=DIR
+
+# 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_ du wc
+
+mkdir dir
+
+# Skip this test if reading from a directory succeeds.
+# In that case, using --files0-from=dir would yield garbage,
+# interpreting the directory entry as a sequence of
+# NUL-separated file names.
+cat dir > /dev/null && skip_ "cat dir/ succeeds"
+
+for prog in du wc; do
+  $prog --files0-from=dir > /dev/null 2>err && fail=1
+  printf "$prog: dir:\n" > exp || fail=1
+  # The diagnostic string is usually "Is a directory" (ENOTDIR),
+  # but accept a different string or errno value.
+  sed 's/dir:.*/dir:/' err > k; mv k err
+  compare err exp || fail=1
+done
+
+Exit $fail
--
1.7.4.1.21.g4cc62