GNU bug report logs -
#8154
du: issue with `--files0-from=DIR'
Previous Next
Reported by: Stefan Vargyas <stvar <at> yahoo.com>
Date: Wed, 2 Mar 2011 14:23:01 UTC
Severity: normal
Fixed in version 8.11
Done: Pádraig Brady <P <at> draigBrady.com>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 8154 in the body.
You can then email your comments to 8154 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#8154; Package
coreutils.
(Wed, 02 Mar 2011 14:23:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Stefan Vargyas <stvar <at> yahoo.com>:
New bug report received and forwarded. Copy sent to
bug-coreutils <at> gnu.org.
(Wed, 02 Mar 2011 14:23:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Dear maintainers,
While building and running coreutils v8.9, I came across the following issue of 'du':
$ mkdir /tmp/foo
$ du --files0-from=/tmp/foo
du: `/tmp/foo': read error: Is a directory
...
The program enters an infinite loop -- continuously printing on stderr the error message shown above. Although such usage pattern of 'du' is erroneous, it better not behave this way. Looking into 'du.c', I found that the unending loop is caused by a misconceived 'continue' statement placed after a call to 'error' (the one labeled by 'case AI_ERR_READ'). A plausible fixing patch is immediate: see it enclosed.
Sincerely,
Stefan Vargyas.
[bug-report-du-files0-from-dir.patch (text/plain, attachment)]
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#8154; Package
coreutils.
(Wed, 02 Mar 2011 14:47:02 GMT)
Full text and
rfc822 format available.
Message #8 received at 8154 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
[adding bug-gnulib]
On 03/02/2011 06:28 AM, Stefan Vargyas wrote:
> Dear maintainers,
>
> While building and running coreutils v8.9, I came across the following issue of 'du':
>
> $ mkdir /tmp/foo
> $ du --files0-from=/tmp/foo
> du: `/tmp/foo': read error: Is a directory
> ...
>
> The program enters an infinite loop
Thanks for the report. This is indeed a bug.
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).
--
Eric Blake eblake <at> redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#8154; Package
coreutils.
(Wed, 02 Mar 2011 15:10:02 GMT)
Full text and
rfc822 format available.
Message #11 received at 8154 <at> debbugs.gnu.org (full text, mbox):
Eric Blake wrote:
> [adding bug-gnulib]
>
> On 03/02/2011 06:28 AM, Stefan Vargyas wrote:
>> Dear maintainers,
>>
>> While building and running coreutils v8.9, I came across the
>> following issue of 'du':
>>
>> $ mkdir /tmp/foo
>> $ du --files0-from=/tmp/foo
>> du: `/tmp/foo': read error: Is a directory
>> ...
>>
>> The program enters an infinite loop
Thanks! That affects the latest (coreutils-8.10), too.
> Thanks for the report. This is indeed a bug.
>
> 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.
diff --git a/src/du.c b/src/du.c
index 671cac7..e205cd5 100644
--- a/src/du.c
+++ b/src/du.c
@@ -889,6 +889,8 @@ main (int argc, char **argv)
quote (files_from));
ai = argv_iter_init_stream (stdin);
+ if (ai == NULL && errno == EISDIR)
+ error (EXIT_FAILURE, errno, _("invalid file: %s"), quote (files_from));
/* It's not easy here to count the arguments, so assume the
worst. */
diff --git a/lib/argv-iter.c b/lib/argv-iter.c
index 340e588..f0445f1 100644
--- a/lib/argv-iter.c
+++ b/lib/argv-iter.c
@@ -21,6 +21,8 @@
#include <stdlib.h>
#include <string.h>
+#include <sys/stat.h>
+#include <errno.h>
struct argv_iterator
{
@@ -49,11 +51,21 @@ argv_iter_init_argv (char **argv)
}
/* Initialize to read from the stream, FP.
- The input is expected to contain a list of NUL-delimited tokens. */
+ The input is expected to contain a list of NUL-delimited tokens.
+ If FP refers to a directory, set errno to EISDIR and return NULL. */
struct argv_iterator *
argv_iter_init_stream (FILE *fp)
{
- 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);
if (!ai)
return NULL;
ai->fp = fp;
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#8154; Package
coreutils.
(Wed, 02 Mar 2011 15:17:01 GMT)
Full text and
rfc822 format available.
Message #14 received at 8154 <at> debbugs.gnu.org (full text, mbox):
On 02/03/11 13:28, Stefan Vargyas wrote:
> Dear maintainers,
>
> While building and running coreutils v8.9, I came across the following issue of 'du':
>
> $ mkdir /tmp/foo
> $ du --files0-from=/tmp/foo
> du: `/tmp/foo': read error: Is a directory
> ...
>
> The program enters an infinite loop -- continuously printing on stderr the error message shown above.
Oh, clang pointed out a very similar issue with wc:
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=0eaa993a
We'll amend your patch as per above I think?
Unfortunately du didn't do anything that clang noticed as suspicious,
but I should have grepped for AI_ERR_READ to show that wc and du
are the only utils to check it.
cheers,
Pádraig.
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#8154; Package
coreutils.
(Wed, 02 Mar 2011 15:25:01 GMT)
Full text and
rfc822 format available.
Message #17 received at 8154 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
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).
> +++ 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.
--
Eric Blake eblake <at> redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#8154; Package
coreutils.
(Wed, 02 Mar 2011 15:27:01 GMT)
Full text and
rfc822 format available.
Message #20 received at 8154 <at> debbugs.gnu.org (full text, mbox):
Stefan Vargyas wrote:
> Dear maintainers,
>
> While building and running coreutils v8.9, I came across the following
> issue of 'du':
>
> $ mkdir /tmp/foo
> $ du --files0-from=/tmp/foo
> du: `/tmp/foo': read error: Is a directory
> ...
>
> The program enters an infinite loop -- continuously printing on stderr
> the error message shown above. Although such usage pattern of 'du' is
> erroneous, it better not behave this way. Looking into 'du.c', I found
> that the unending loop is caused by a misconceived 'continue'
> statement placed after a call to 'error' (the one labeled by 'case
> AI_ERR_READ'). A plausible fixing patch is immediate: see it enclosed.
>
> Sincerely,
>
> Stefan Vargyas.
>
>
>
>
> --- coreutils-8.9/src/du.c 2011-01-01 23:19:23.000000000 +0200
> +++ 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;
> + goto out_argv_iter;
>
> case AI_ERR_MEM:
> xalloc_die ();
> @@ -978,6 +980,7 @@
> ok &= du_files (temp_argv, bit_flags);
> }
> }
> + out_argv_iter:
Thanks for the patch.
Contrary to what my alternative patch suggested, part of yours is required.
The part that exits the loop upon read error.
Though note that as you wrote it, the goto was unreachable,
since error (EXIT_FAILURE, ... never returns, while error (0, ...
merely issues a warning.
I've taken the opportunity (of this new label) to move the test
for EOF into the case alongside the other AI_ERR_* values:
diff --git a/src/du.c b/src/du.c
index 671cac7..6270092 100644
--- a/src/du.c
+++ b/src/du.c
@@ -889,6 +889,8 @@ main (int argc, char **argv)
quote (files_from));
ai = argv_iter_init_stream (stdin);
+ if (ai == NULL && errno == EISDIR)
+ error (EXIT_FAILURE, errno, _("invalid file: %s"), quote (files_from));
/* It's not easy here to count the arguments, so assume the
worst. */
@@ -926,15 +928,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 (0, errno, _("%s: read error"), quote (files_from));
- continue;
+ ok = false;
+ goto argv_iter_done;
case AI_ERR_MEM:
xalloc_die ();
@@ -985,6 +989,7 @@ main (int argc, char **argv)
ok &= du_files (temp_argv, bit_flags);
}
}
+ argv_iter_done:
argv_iter_free (ai);
di_set_free (di_set);
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#8154; Package
coreutils.
(Wed, 02 Mar 2011 17:11:02 GMT)
Full text and
rfc822 format available.
Message #23 received at 8154 <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#8154; Package
coreutils.
(Wed, 02 Mar 2011 17:13:02 GMT)
Full text and
rfc822 format available.
Message #26 received at 8154 <at> debbugs.gnu.org (full text, mbox):
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
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#8154; Package
coreutils.
(Wed, 02 Mar 2011 17:26:02 GMT)
Full text and
rfc822 format available.
Message #29 received at 8154 <at> debbugs.gnu.org (full text, mbox):
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.
Thanks!
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#8154; Package
coreutils.
(Wed, 02 Mar 2011 17:34:01 GMT)
Full text and
rfc822 format available.
Message #32 received at 8154 <at> debbugs.gnu.org (full text, mbox):
On 02/03/11 17:10, 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 thought that at first too, but it seems like
it might cause more issues that not, by allowing it?
Testing bsd here shows that valid NUL terminated file names
are returned, but also there is other info interspersed.
cheers,
Pádraig.
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#8154; Package
coreutils.
(Wed, 02 Mar 2011 17:36:01 GMT)
Full text and
rfc822 format available.
Message #35 received at 8154 <at> debbugs.gnu.org (full text, mbox):
Pádraig Brady wrote:
> On 02/03/11 17:10, 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 thought that at first too, but it seems like
> it might cause more issues that not, by allowing it?
> Testing bsd here shows that valid NUL terminated file names
> are returned, but also there is other info interspersed.
I'm planning to skip the new tests when "cat some-dir" succeeds.
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#8154; Package
coreutils.
(Wed, 02 Mar 2011 17:47:02 GMT)
Full text and
rfc822 format available.
Message #38 received at 8154 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 03/02/2011 10:10 AM, 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?
Because other GNU apps do so? For example, POSIX requires that input
files to m4 must be text files. If you do 'm4 dir/', you are therefore
violating POSIX, because dir/ is not a text file. However, I've chosen
to make m4 uniformly fail with EISDIR on opening the file, rather than
deal with the vaguaries of different platforms (some forbid
fopen("dir/"), although that is in itself a POSIX violation; most get
past the fopen(), but then it's arbitrary whether the fread() sees EOF,
reads garbage, or fails with EISDIR). See:
http://lists.gnu.org/archive/html/m4-patches/2008-09/msg00004.html
http://git.savannah.gnu.org/cgit/m4.git/commit?id=eed62f0d
> 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.
Yes, the code would be simpler by letting the directory do what it
normally does (be that nothing, error, or raw data), but in the case of
raw data, acting on that garbage is likely to lead to a host of other
error messages.
--
Eric Blake eblake <at> redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#8154; Package
coreutils.
(Wed, 02 Mar 2011 18:33:02 GMT)
Full text and
rfc822 format available.
Message #41 received at 8154 <at> debbugs.gnu.org (full text, mbox):
Jim,
Maybe you would consider appropriate to keep 'wc.c' in sync with 'du.c':
with respect to 'argv_iter' processing, both files had similar code
patterns. Padraig indirectly suggested this too above in the thread.
Stefan.
> Date: Wed, 02 Mar 2011 16:26:03 +0100
> From: Jim Meyering <jim <at> meyering.net>
> Subject: Re: bug#8154: du: issue with `--files0-from=DIR'
>
> [...]
>
> I've taken the opportunity (of this new label) to move the test
> for EOF into the case alongside the other AI_ERR_* values:
>
> diff --git a/src/du.c b/src/du.c
> index 671cac7..6270092 100644
> --- a/src/du.c
> +++ b/src/du.c
> @@ -889,6 +889,8 @@ main (int argc, char **argv)
> quote (files_from));
>
> ai = argv_iter_init_stream (stdin);
> + if (ai == NULL && errno == EISDIR)
> + error (EXIT_FAILURE, errno, _("invalid file: %s"), quote
> (files_from));
>
> /* It's not easy here to count the arguments, so assume the
> worst. */
> @@ -926,15 +928,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 (0, errno, _("%s: read error"), quote (files_from));
> - continue;
> + ok = false;
> + goto argv_iter_done;
>
> case AI_ERR_MEM:
> xalloc_die ();
> @@ -985,6 +989,7 @@ main (int argc, char **argv)
> ok &= du_files (temp_argv, bit_flags);
> }
> }
> + argv_iter_done:
>
> argv_iter_free (ai);
> di_set_free (di_set);
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#8154; Package
coreutils.
(Wed, 02 Mar 2011 18:50:02 GMT)
Full text and
rfc822 format available.
Message #44 received at 8154 <at> debbugs.gnu.org (full text, mbox):
Stefan Vargyas wrote:
> Maybe you would consider appropriate to keep 'wc.c' in sync with 'du.c':
> with respect to 'argv_iter' processing, both files had similar code
> patterns. Padraig indirectly suggested this too above in the thread.
Yes, definitely, thanks.
Perhaps you haven't seen later replies?
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8154
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#8154; Package
coreutils.
(Wed, 02 Mar 2011 18:54:01 GMT)
Full text and
rfc822 format available.
Message #47 received at 8154 <at> debbugs.gnu.org (full text, mbox):
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 <meyering <at> redhat.com>
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 <meyering <at> redhat.com>
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 sonnenburg <at> informatik.hu-berlin.de
Solar Designer solar <at> owl.openwall.com
Stanislav Ievlev inger <at> altlinux.ru
Stavros Passas stabat <at> ics.forth.gr
+Stefan Vargyas stvar <at> yahoo.com
Stéphane Chazelas Stephane_CHAZELAS <at> yahoo.fr
Stephen Depooter sbdep <at> myrealbox.com
Stephen Eglen eglen <at> pcg.wustl.edu
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
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#8154; Package
coreutils.
(Wed, 02 Mar 2011 19:02:01 GMT)
Full text and
rfc822 format available.
Message #50 received at 8154 <at> debbugs.gnu.org (full text, mbox):
Indeed I wasn't noticing the developments before sending the reply.
> Date: Wed, 02 Mar 2011 19:49:29 +0100
> From: Jim Meyering <jim <at> meyering.net>
> Subject: Re: bug#8154: du: issue with `--files0-from=DIR'
>
> [...]
>
> Yes, definitely, thanks.
> Perhaps you haven't seen later replies?
>
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8154
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#8154; Package
coreutils.
(Wed, 02 Mar 2011 19:26:01 GMT)
Full text and
rfc822 format available.
Message #53 received at 8154 <at> debbugs.gnu.org (full text, mbox):
Eric Blake wrote:
> On 03/02/2011 10:10 AM, 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?
>
> Because other GNU apps do so? For example, POSIX requires that input
> files to m4 must be text files. If you do 'm4 dir/', you are therefore
That's fine for m4, but the --files0-from argument
is a file containing NUL-delimited file names: not a text file.
And obviously not covered by POSIX, besides.
However, my primary motivation was this: if we special-case
directories, should we also special-case char- and block-devices?
That seems inappropriate.
> violating POSIX, because dir/ is not a text file. However, I've chosen
> to make m4 uniformly fail with EISDIR on opening the file, rather than
> deal with the vaguaries of different platforms (some forbid
> fopen("dir/"), although that is in itself a POSIX violation; most get
> past the fopen(), but then it's arbitrary whether the fread() sees EOF,
> reads garbage, or fails with EISDIR). See:
> http://lists.gnu.org/archive/html/m4-patches/2008-09/msg00004.html
> http://git.savannah.gnu.org/cgit/m4.git/commit?id=eed62f0d
>
>> 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.
>
> Yes, the code would be simpler by letting the directory do what it
> normally does (be that nothing, error, or raw data), but in the case of
> raw data, acting on that garbage is likely to lead to a host of other
> error messages.
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#8154; Package
coreutils.
(Thu, 03 Mar 2011 08:12:02 GMT)
Full text and
rfc822 format available.
Message #56 received at 8154 <at> debbugs.gnu.org (full text, mbox):
Jim Meyering wrote:
> 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:
>
> Subject: [PATCH 1/2] wc: avoid NULL dereference on out-of-memory error
I've pushed that, with adjusted logs:
(mainly, I dug up and noted each bug-introducing commit)
commit caaf2899f67d312d76af91add2a4d9f7be2d5c61
Author: Jim Meyering <meyering <at> redhat.com>
Date: Wed Mar 2 19:16:46 2011 +0100
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.
Bug introduced by 2008-11-24 commit 031e2fb5, "du: read and
process --files0-from= input a name at a time,".
* 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.
commit 7cfd12c78e0be4c90f29c99ab383163aa1471504
Author: Jim Meyering <meyering <at> redhat.com>
Date: Wed Mar 2 18:54:43 2011 +0100
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.
Bug introduced by 2008-11-25 commit c2e56e0d,
"wc: read and process --files0-from= input a name at a time,".
* NEWS (Bug fixes): Mention it.
bug marked as fixed in version 8.11, send any further explanations to
8154 <at> debbugs.gnu.org and Stefan Vargyas <stvar <at> yahoo.com>
Request was from
Pádraig Brady <P <at> draigBrady.com>
to
control <at> debbugs.gnu.org.
(Thu, 24 Mar 2011 00:39:02 GMT)
Full text and
rfc822 format available.
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Thu, 21 Apr 2011 11:24:05 GMT)
Full text and
rfc822 format available.
This bug report was last modified 14 years and 64 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.