Re: bug #17457: "grep -r foo . > somefile" goes into an infinite loop

[Jim Meyering]

Jim Meyering wrote:
...
> Here's a preliminary patch.
> I will add a test case and update NEWS.  In the mean time, if anyone knows
> who submitted the proposed patch, I'll mention that it inspired mine.
>
> With that change, now I see this behavior:
>
>     $ mkdir d && touch d/k && ./grep -r pat d > d/k
>     ./grep: input file `d/k' is also the output
>     [Exit 2]
>
> Before, depending on pattern/contents, it would
> either exit 0, 1 or fill your partition.
...
> +      if (S_ISREG (stats->stat.st_mode) && out_stat.st_ino
> +          && SAME_REGULAR_FILE (stats->stat, out_stat))
> +        {
> +          if (! suppress_errors)
> +            error (0, 0, _("input file %s is also the output"),
> +                   quote (file));
> +          errseen = 1;
> +          return 1;
> +        }

I've made the diagnostic unconditional,
added a test script and a NEWS entry:
(FYI, you don't need to use -r to trigger the bug -- see the test)

>From e79a76228ec9fb586809394208461a1969168d61 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sat, 23 Jul 2011 22:52:06 +0200
Subject: [PATCH 1/2] fail with exit status 2 when an input file is the same
 as the output

This avoids a potential "infinite" disk-filling loop.
Reported in http://savannah.gnu.org/patch/?5316
and http://savannah.gnu.org/bugs/?17457.
* src/main.c: Include "quote.h".
(out_stat): New global.
(grepfile): Compare each regular file's dev/ino/etc.
with those from the file on stdout (if it too is regular).
(main): Set out_stat, if stdout is a regular file.
* src/system.h: Include "same-inode.h".
(same_file_attributes): Define.  From diffutils.
(SAME_REGULAR_FILE): Define.
* bootstrap.conf (gnulib_modules): Use quote, not quotearg.
Use same-inode.
* NEWS (Bug fixes): Mention it.
---
 NEWS           |    5 +++++
 bootstrap.conf |    3 ++-
 src/main.c     |   26 ++++++++++++++++++++++++++
 src/system.h   |   42 +++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 74 insertions(+), 2 deletions(-)

diff --git a/NEWS b/NEWS
index 6e5d38b..7d87bb1 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,11 @@ GNU grep NEWS                                    -*- outline 
-*-

 * Noteworthy changes in release ?.? (????-??-??) [?]

+  grep now rejects a command like "grep -r pattern . > out",
+  in which the output file is also one of the inputs,
+  because it can result in an "infinite" disk-filling loop.
+  [bug introduced in grep-???]
+

 * Noteworthy changes in release 2.9 (2011-06-21) [stable]

diff --git a/bootstrap.conf b/bootstrap.conf
index 143d7b5..66c239e 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -57,10 +57,11 @@ obstack
 open
 progname
 propername
-quotearg
+quote
 readme-release
 realloc-gnu
 regex
+same-inode
 ssize_t
 stddef
 stdlib
diff --git a/src/main.c b/src/main.c
index 4e01a17..fd9f662 100644
--- a/src/main.c
+++ b/src/main.c
@@ -44,6 +44,7 @@
 #include "isdir.h"
 #include "progname.h"
 #include "propername.h"
+#include "quote.h"
 #include "savedir.h"
 #include "version-etc.h"
 #include "xalloc.h"
@@ -59,6 +60,11 @@
   proper_name ("Mike Haertel"), \
   _("others, see <http://git.sv.gnu.org/cgit/grep.git/tree/AUTHORS>")

+/* When stdout is connected to a regular file, save its stat
+   information here, so that we can automatically skip it, thus
+   avoiding a potential (racy) infinite loop.  */
+static struct stat out_stat;
+
 struct stats
 {
   struct stats const *parent;
@@ -1212,6 +1218,22 @@ grepfile (char const *file, struct stats *stats)
                                       || S_ISSOCK (stats->stat.st_mode)
                                       || S_ISFIFO (stats->stat.st_mode)))
         return 1;
+
+      /* If there's a regular file on stdout and the current file refers
+         to the same i-node, we have to report the problem and skip it.
+         Otherwise when matching lines from some other input reach the
+         disk before we open this file, we can end up reading and matching
+         those lines and appending them to the file from which we're reading.
+         Then we'd have what appears to be an infinite loop that'd terminate
+         only upon filling the output file system or reaching a quota.  */
+      if (S_ISREG (stats->stat.st_mode) && out_stat.st_ino
+          && SAME_REGULAR_FILE (stats->stat, out_stat))
+        {
+          error (0, 0, _("input file %s is also the output"), quote (file));
+          errseen = 1;
+          return 1;
+        }
+
       while ((desc = open (file, O_RDONLY)) < 0 && errno == EINTR)
         continue;

@@ -2121,6 +2143,10 @@ main (int argc, char **argv)
   if (show_help)
     usage (EXIT_SUCCESS);

+  struct stat tmp_stat;
+  if (fstat (STDOUT_FILENO, &tmp_stat) == 0 && S_ISREG (tmp_stat.st_mode))
+    out_stat = tmp_stat;
+
   if (keys)
     {
       if (keycc == 0)
diff --git a/src/system.h b/src/system.h
index 199b485..e7afc46 100644
--- a/src/system.h
+++ b/src/system.h
@@ -27,6 +27,7 @@
 #include "configmake.h"
 #include "dirname.h"
 #include "minmax.h"
+#include "same-inode.h"

 #if O_BINARY
 # define HAVE_DOS_FILE_CONTENTS 1
@@ -53,8 +54,47 @@ enum { EXIT_TROUBLE = 2 };
 #include <locale.h>

 #ifndef initialize_main
-#define initialize_main(argcp, argvp)
+# define initialize_main(argcp, argvp)
 #endif

+/* Do struct stat *S, *T have the same file attributes?
+
+   POSIX says that two files are identical if st_ino and st_dev are
+   the same, but many file systems incorrectly assign the same (device,
+   inode) pair to two distinct files, including:
+
+   - GNU/Linux NFS servers that export all local file systems as a
+     single NFS file system, if a local device number (st_dev) exceeds
+     255, or if a local inode number (st_ino) exceeds 16777215.
+
+   - Network Appliance NFS servers in snapshot directories; see
+     Network Appliance bug #195.
+
+   - ClearCase MVFS; see bug id ATRia04618.
+
+   Check whether two files that purport to be the same have the same
+   attributes, to work around instances of this common bug.  Do not
+   inspect all attributes, only attributes useful in checking for this
+   bug.
+
+   It's possible for two distinct files on a buggy file system to have
+   the same attributes, but it's not worth slowing down all
+   implementations (or complicating the configuration) to cater to
+   these rare cases in buggy implementations.  */
+
+#ifndef same_file_attributes
+# define same_file_attributes(s, t) \
+   ((s)->st_mode == (t)->st_mode \
+    && (s)->st_nlink == (t)->st_nlink \
+    && (s)->st_uid == (t)->st_uid \
+    && (s)->st_gid == (t)->st_gid \
+    && (s)->st_size == (t)->st_size \
+    && (s)->st_mtime == (t)->st_mtime \
+    && (s)->st_ctime == (t)->st_ctime)
+#endif
+
+#define SAME_REGULAR_FILE(s, t) \
+  (SAME_INODE (s, t) && same_file_attributes (&s, &t))
+
 #include "unlocked-io.h"
 #endif
--
1.7.6.609.gbf6a9


>From e4627d23797a467c62456eeaed722a1141655eb6 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sun, 24 Jul 2011 12:10:31 +0200
Subject: [PATCH 2/2] tests: add a test to trigger the bug

* tests/Makefile.am (TESTS): Add it.
* tests/in-eq-out-infloop: Exercise the bug/fix.
---
 tests/Makefile.am       |    1 +
 tests/in-eq-out-infloop |   26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)
 create mode 100755 tests/in-eq-out-infloop

diff --git a/tests/Makefile.am b/tests/Makefile.am
index a3a0f33..1baf269 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -61,6 +61,7 @@ TESTS =                                               \
   grep-dir                                     \
   help-version                                 \
   ignore-mmap                                  \
+  in-eq-out-infloop                             \
   include-exclude                              \
   inconsistent-range                            \
   khadafy                                      \
diff --git a/tests/in-eq-out-infloop b/tests/in-eq-out-infloop
new file mode 100755
index 0000000..726accb
--- /dev/null
+++ b/tests/in-eq-out-infloop
@@ -0,0 +1,26 @@
+#!/bin/sh
+# Demonstrate the disk-filling infloop when redirecting to an input file.
+. "${srcdir=.}/init.sh"; path_prepend_ ../src
+require_timeout_
+
+# Use an input file large enough that the problem is reproducible in spite
+# of buffering effects.  Just larger than 256KB should be adequate.
+v=$(printf %063d 0)'
+'
+# 64 * 2^12 = 256k
+for i in 1 2 3 4 5 6 7 8 9 10 11 12; do
+  v="$v$v"
+done
+
+echo "$v" > out || framework_failure_
+echo 'grep: input file `out'\'' is also the output' \
+    > err.exp || framework_failure_
+
+# Require an exit status of 2.
+# grep-2.8 and earlier would infloop.
+timeout 10 grep 0 out >> out 2> err; st=$?
+test $st = 2 || fail=1
+
+compare err.exp err || fail=1
+
+Exit $fail
--
1.7.6.609.gbf6a9