bug-coreutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: tail aborts while following by name if using inotify


From: Jim Meyering
Subject: Re: tail aborts while following by name if using inotify
Date: Tue, 29 Dec 2009 17:03:30 +0100

Giuseppe Scrivano wrote:
> Subject: [PATCH] tail: remove `fdspec' from the hash table before change its 
> key
>
> * src/tail.c (tail_forever_inotify): Avoid to modify fdspec->wd until it
> is contained in the wd_to_name hash table with a different key value.
> Once it is removed, it can be added using the new `wd' as key for the
> hash table.
> ---
>  src/tail.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/src/tail.c b/src/tail.c
> index 8e6c8ac..83b4253 100644
> --- a/src/tail.c
> +++ b/src/tail.c
> @@ -1474,15 +1474,18 @@ tail_forever_inotify (int wd, struct File_spec *f, 
> size_t n_files,
>              continue;
>
>            /* It's fine to add the same file more than once.  */
> -          f[j].wd = inotify_add_watch (wd, f[j].name, inotify_wd_mask);
> -
> -          if (f[j].wd < 0)
> +          int new_wd = inotify_add_watch (wd, f[j].name, inotify_wd_mask);
> +          if (new_wd < 0)
>              {
>                error (0, errno, _("cannot watch %s"), quote (f[j].name));
>                continue;
>              }
>
>            fspec = &(f[j]);
> +
> +          /* Remove `fspec' and re-add it using `new_fd' as its key.  */
> +          hash_delete (wd_to_name, fspec);
> +          f[j].wd = new_wd;

Thanks again.  I've just pushed your fix, along with the two
corresponding tests that now pass.  I've made one small change:
I prefer to use fspec-> rather than the technically equivalent f[j]. here:

-            f[j].wd = new_wd;
+            fspec->wd = new_wd;


>From 07f15147eb72e9c60686068ddea9c6c5b1ac62e3 Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <address@hidden>
Date: Tue, 29 Dec 2009 14:59:24 +0100
Subject: [PATCH 1/3] tail: remove `fdspec' from the hash table before changing 
its key

* src/tail.c (tail_forever_inotify): Avoid modifying fdspec->wd while
it is in the wd_to_name hash table.  Once it is removed, it can be
added using the new `wd' as key for the hash table.  This fixes the
abort-inducing bug reported by Rob Wortman in
http://thread.gmane.org/gmane.comp.gnu.coreutils.bugs/19372
---
 src/tail.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/tail.c b/src/tail.c
index 8e6c8ac..3d5e221 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -1474,15 +1474,18 @@ tail_forever_inotify (int wd, struct File_spec *f, 
size_t n_files,
             continue;

           /* It's fine to add the same file more than once.  */
-          f[j].wd = inotify_add_watch (wd, f[j].name, inotify_wd_mask);
-
-          if (f[j].wd < 0)
+          int new_wd = inotify_add_watch (wd, f[j].name, inotify_wd_mask);
+          if (new_wd < 0)
             {
               error (0, errno, _("cannot watch %s"), quote (f[j].name));
               continue;
             }

           fspec = &(f[j]);
+
+          /* Remove `fspec' and re-add it using `new_fd' as its key.  */
+          hash_delete (wd_to_name, fspec);
+          fspec->wd = new_wd;
           if (hash_insert (wd_to_name, fspec) == NULL)
             xalloc_die ();

--
1.6.6.301.g5794


>From bd26efaaa3e251c4a1bed308ec5ee500ecfd7d25 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 28 Dec 2009 15:42:10 +0100
Subject: [PATCH 2/3] tail: add a test to exercise abort-inducing flaw in tail -F
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* tests/tail-2/inotify-hash-abuse: New file, derived from
a report by Rob Wortman.
* tests/Makefile.am (TESTS): Add it.
Improved by: Pádraig Brady.
---
 THANKS                          |    1 +
 tests/Makefile.am               |    1 +
 tests/tail-2/inotify-hash-abuse |   69 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+), 0 deletions(-)
 create mode 100755 tests/tail-2/inotify-hash-abuse

diff --git a/THANKS b/THANKS
index 668d59f..86be252 100644
--- a/THANKS
+++ b/THANKS
@@ -512,6 +512,7 @@ Richard Sharman                     address@hidden
 Rick Sladkey                        address@hidden
 Rik Faith                           address@hidden
 Risto Kankkunen                     address@hidden
+Rob Wortman                         address@hidden
 Robert H. de Vries                  address@hidden
 Robert Lindgren                     address@hidden
 Robert Millan                       address@hidden
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 93d4275..35ea8b2 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -80,6 +80,7 @@ TESTS =                                               \
   rm/ext3-perf                                 \
   rm/cycle                                     \
   cp/link-heap                                 \
+  tail-2/inotify-hash-abuse                    \
   tail-2/inotify-rotate                                \
   chmod/no-x                                   \
   chgrp/basic                                  \
diff --git a/tests/tail-2/inotify-hash-abuse b/tests/tail-2/inotify-hash-abuse
new file mode 100755
index 0000000..bc86398
--- /dev/null
+++ b/tests/tail-2/inotify-hash-abuse
@@ -0,0 +1,69 @@
+#!/bin/sh
+# Exercise an abort-inducing flaw in inotify-enabled tail -F.
+
+# Copyright (C) 2009 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/>.
+
+if test "$VERBOSE" = yes; then
+  set -x
+  tail --version
+fi
+
+. $srcdir/test-lib.sh
+
+# 9 is a magic number, related to internal details of tail.c and hash.c
+n=9
+seq $n | xargs touch || framework_failure
+
+debug='---disable-inotify -s .01'
+debug=
+tail $debug -qF $(seq $n) > out 2>&1 & pid=$!
+
+# Wait until tail has started...
+echo x > $n
+until grep '^x$' out >/dev/null 2>&1; do :; done
+
+mv 1 f || fail=1
+
+# Wait for this diagnostic:
+# tail: `1' has become inaccessible: No such file or directory
+until grep inaccessible out >/dev/null 2>&1; do :; done
+
+# Trigger the bug.  Before the fix, this would provoke the abort.
+echo a > 1 || fail=1
+
+# Wait up to 2s for the buggy tail to die,
+# or for the "tail: `1' has appeared;  following end of new file" output
+dead=0
+for i in $(seq 10); do
+  kill -0 $pid || { dead=1; break; }
+  grep 'has appeared;' out > /dev/null && break
+  sleep .2
+done
+
+# Fixed tail will not have aborted.  Kill it.
+test $dead = 0 && kill -HUP $pid
+
+wait $pid
+st=$?
+
+case $st in
+  129) ;;
+  *) echo tail died via unexpected signal: $st; fail=1;;
+esac
+
+cat out
+
+Exit $fail
--
1.6.6.301.g5794


>From fd93fc3ef4947f4712a4bda08b6e11c0d6cdf48a Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Tue, 29 Dec 2009 13:47:10 +0100
Subject: [PATCH 3/3] tail: add another test to exercise abort-inducing flaw in 
tail -F

* tests/tail-2/inotify-hash-abuse2: New test, based on a reproducer
by Rob Wortman.
* tests/Makefile.am (TESTS): Add it.
---
 tests/Makefile.am                |    1 +
 tests/tail-2/inotify-hash-abuse2 |   50 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 0 deletions(-)
 create mode 100755 tests/tail-2/inotify-hash-abuse2

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 35ea8b2..6ef7ad8 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -81,6 +81,7 @@ TESTS =                                               \
   rm/cycle                                     \
   cp/link-heap                                 \
   tail-2/inotify-hash-abuse                    \
+  tail-2/inotify-hash-abuse2                   \
   tail-2/inotify-rotate                                \
   chmod/no-x                                   \
   chgrp/basic                                  \
diff --git a/tests/tail-2/inotify-hash-abuse2 b/tests/tail-2/inotify-hash-abuse2
new file mode 100755
index 0000000..2f59eb4
--- /dev/null
+++ b/tests/tail-2/inotify-hash-abuse2
@@ -0,0 +1,50 @@
+#!/bin/sh
+# Exercise an abort-inducing flaw in inotify-enabled tail -F.
+# Like inotify-hash-abuse, but without a hard-coded "9".
+
+# Copyright (C) 2009 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/>.
+
+if test "$VERBOSE" = yes; then
+  set -x
+  tail --version
+fi
+
+. $srcdir/test-lib.sh
+
+touch f || framework_failure
+
+debug='---disable-inotify -s .001'
+debug=
+timeout 10 tail $debug -F f & pid=$!
+
+for i in $(seq 200); do
+  kill -0 $pid || break;
+  mv f g
+  touch f
+done
+
+# Fixed tail will not have aborted.  Kill it.
+kill -HUP $pid
+
+wait $pid
+st=$?
+
+case $st in
+  129) ;;
+  *) echo tail died via unexpected signal: $st; fail=1;;
+esac
+
+Exit $fail
--
1.6.6.301.g5794




reply via email to

[Prev in Thread] Current Thread [Next in Thread]