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

[Jim Meyering]

Eric Blake wrote:
> On 03/02/2011 08:09 AM, Jim Meyering wrote:
>>> I wonder if the better fix would be to modify the gnulib argv-iter
>>> module to make argv_iter_init_stream to fail if fileno(fp) is a
>>> directory, since not all platforms reliably fail with EISDIR when doing
>>> read() on a directory (some, like BSD, successfully return EOF, and some
>>> older systems even read raw directory contents).
>>
>> That's just what I've done.
>> Here's a tentative patch, both for du.c and argv-iter.c in gnulib.
>> A probably-identical change to the du.c part will be required for wc.c.
>> I'll add tests and update NEWS, too.
>
> Yes, that looks nicer for directories.  Still, I can't help wonder if
> the same infloop would happen if we encounter EIO or other (rare) error
> while reading a non-directory, in which case a variant of Stefan's patch
> is also needed (even though with Jim's patch, it won't trip for the
> original case of directories that sparked this thread).

As you noticed, I thought the same thing and adjusted my patch,
which I now see I sent two minutes after you posted this.

>> +++ coreutils-8.9-stev/src/du.c      2011-03-02 03:32:04.000000000 +0200
>> @@ -926,8 +926,10 @@
>>            switch (ai_err)
>>              {
>>              case AI_ERR_READ:
>> -              error (0, errno, _("%s: read error"), quote (files_from));
>> -              continue;
>> +              error (EXIT_FAILURE, errno, _("%s: read error"),
>> +                     quote (files_from));
>> +              ok = false;
>
> This line would never be reached - once you call error() with a non-zero
> first argument, it calls exit().  The question is whether it is ever
> worth trying to continue after a read error on the files-from argument,
> or whether bailing out like this is the only sane approach.  Or maybe we
> still need the error(0) to issue the warning and affect final exit
> status, while making sure we break out of the loop rather than continue
> to retry a read that will likely fail again, so that we at least print
> the summary of all files successfully read prior to the read error on
> the files-from argument.

Unifying argv_iter* handling between du.c and wc.c led me to
find another bug just now:

A failed ai = argv_iter_init_* would leave ai set to NULL,
yet there was no check for that in wc.c before the dereference via
argv_iter (ai, ...

Here's the full wc.c patch, so far, but I'm about to commit
that bug-fix (the 2nd hunk, below) separately:

diff --git a/src/wc.c b/src/wc.c
index 3afd4de..0a5bfa9 100644
--- a/src/wc.c
+++ b/src/wc.c
@@ -699,6 +699,9 @@ main (int argc, char **argv)
           files = NULL;
           nfiles = 0;
           ai = argv_iter_init_stream (stream);
+          if (ai == NULL && errno == EISDIR)
+            error (EXIT_FAILURE, errno, _("invalid file: %s"),
+                   quote (files_from));
         }
     }
   else
@@ -709,6 +712,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);

@@ -719,16 +725,16 @@ 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"), quote (files_from));
+              ok = false;
+              goto argv_iter_done;
             case AI_ERR_MEM:
               xalloc_die ();
             default:
@@ -770,6 +776,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