Re: [coreutils] Re: Bug#609049: du: process_file: Assertion `level == prev_level - 1' failed

[Jim Meyering]

Jim Meyering wrote:
...
> While I do have a reproducer that will become a test suite addition
> (coming soon), it relies on python (a first) and the python-inotify
> package, so I'll have to be careful to skip the test when those
> prerequisites aren't installed.  Also, there's an inherent race condition,
> so I'll have to find the right compromise between absolute test-robustness
> (too expensive in time and inodes) and reasonableness.

Here's a complete patch, including the test.
I would have preferred to avoid the one-second sleep.
Maybe someone here can see a better way.

>From 9bf47055f64efb56d022feca01ad901e85e21bc1 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sat, 8 Jan 2011 17:44:55 +0100
Subject: [PATCH] du: don't abort when a subdir is renamed during traversal

* NEWS (Bug fixes): Mention it.
* src/du.c (prev_level): Move declaration "up" to file-scope global.
(du_files): Reset prev_level to 0 upon abnormal fts_read termination.
Reported by Johathan Nieder in http://bugs.debian.org/609049
Also, improve a diagnostic.
* tests/du/move-dir-while-traversing: Test for the above.
* tests/Makefile.am (TESTS): Add it.
---
 NEWS                               |    8 +++
 src/du.c                           |   15 +++++--
 tests/Makefile.am                  |    1 +
 tests/du/move-dir-while-traversing |   85 ++++++++++++++++++++++++++++++++++++
 4 files changed, 105 insertions(+), 4 deletions(-)
 create mode 100755 tests/du/move-dir-while-traversing

diff --git a/NEWS b/NEWS
index 2a71ca6..5a70243 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,14 @@ GNU coreutils NEWS                                    -*- 
outline -*-

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

+** Bug fixes
+
+  du would abort with a failed assertion when two conditions are met:
+  part of the hierarchy being traversed is moved to a higher level in the
+  directory tree, and there is at least one more command line directory
+  argument following the one containing the moved sub-tree.
+  [bug introduced in coreutils-5.1.0]
+

 * Noteworthy changes in release 8.9 (2011-01-04) [stable]

diff --git a/src/du.c b/src/du.c
index 77deb0c..671cac7 100644
--- a/src/du.c
+++ b/src/du.c
@@ -63,8 +63,11 @@ extern bool fts_debug;
 /* A set of dev/ino pairs.  */
 static struct di_set *di_set;

-/* Define a class for collecting directory information. */
+/* Keep track of the preceding "level" (depth in hierarchy)
+   from one call of process_file to the next.  */
+static size_t prev_level;

+/* Define a class for collecting directory information. */
 struct duinfo
 {
   /* Size of files in directory.  */
@@ -399,7 +402,6 @@ process_file (FTS *fts, FTSENT *ent)
   struct duinfo dui;
   struct duinfo dui_to_print;
   size_t level;
-  static size_t prev_level;
   static size_t n_alloc;
   /* First element of the structure contains:
      The sum of the st_size values of all entries in the single directory
@@ -582,10 +584,15 @@ du_files (char **files, int bit_flags)
             {
               if (errno != 0)
                 {
-                  /* FIXME: try to give a better message  */
-                  error (0, errno, _("fts_read failed"));
+                  error (0, errno, _("fts_read failed: %s"),
+                         quotearg_colon (fts->fts_path));
                   ok = false;
                 }
+
+              /* When exiting this loop early, be careful to reset the
+                 global, prev_level, used in process_file.  Otherwise, its
+                 (level == prev_level - 1) assertion could fail.  */
+              prev_level = 0;
               break;
             }
           FTS_CROSS_CHECK (fts);
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 06d81f0..a5dbd3e 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -370,6 +370,7 @@ TESTS =                                             \
   du/long-from-unreadable                      \
   du/long-sloop                                        \
   du/max-depth                                 \
+  du/move-dir-while-traversing                 \
   du/no-deref                                  \
   du/no-x                                      \
   du/one-file-system                           \
diff --git a/tests/du/move-dir-while-traversing 
b/tests/du/move-dir-while-traversing
new file mode 100755
index 0000000..e42bc93
--- /dev/null
+++ b/tests/du/move-dir-while-traversing
@@ -0,0 +1,85 @@
+#!/bin/sh
+# Trigger a failed assertion in coreutils-8.9 and earlier.
+
+# 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
+
+# We use a python-inotify script, so...
+python -m pyinotify -h > /dev/null \
+  || skip_ 'python-inotify package not installed'
+
+# Move a directory "up" while du is processing its sub-directories.
+# While du is processing a hierarchy .../B/C/D/... this script
+# detects when du opens D/, and then moves C/ "up" one level
+# so that it is a sibling of B/.
+# Given the inherent race condition, we have to add enough "weight"
+# under D/ so that in most cases, the monitor performs the single
+# rename syscall before du finishes processing the subtree under D/.
+
+cat <<'EOF' > inotify-watch-for-dir-access.py
+#!/usr/bin/env python
+from pyinotify import *
+dir = sys.argv[1]
+dest_parent = os.path.dirname(os.path.dirname(dir))
+dest = os.path.join(dest_parent, os.path.basename(dir))
+
+class ProcessDir(ProcessEvent):
+
+    def process_IN_OPEN(self, event):
+        os.rename(dir, dest)
+        sys.exit(0)
+
+    def process_default(self, event):
+        pass
+
+wm = WatchManager()
+notifier = Notifier(wm)
+wm.watch_transient_file(dir, IN_OPEN, ProcessDir)
+print 'started'
+notifier.loop()
+EOF
+chmod a+x inotify-watch-for-dir-access.py
+
+long=d/e/f/g/h/i/j/k/l/m/n/o/p/q/r/s/t/u/v/w/x/y/z
+t=T/U
+mkdir -p $t/3/a/b/c/$long d2 || framework_failure
+timeout 6 ./inotify-watch-for-dir-access.py $t/3/a/b > start-msg &
+
+# Wait for the watcher to start...
+nonempty() { test -s start-msg && return 0; sleep $1; }
+retry_delay_ nonempty .1 5
+
+# The above delay is insufficient in ~50% of my trials.
+# Sometimes, when under heavy load, a parallel "make check" would
+# fail this test when sleeping just 0.1 seconds here.
+sleep 1
+
+# The above watches for an IN_OPEN event on $t/3/a/b,
+# and when it triggers, moves the parent, $t/3/a, up one level
+# so it's directly under $t.
+
+du -a $t d2 2> err
+# Before coreutils-8.10, du would abort.
+test $? = 1 || fail=1
+
+# check for the new diagnostic
+printf "du: fts_read failed: $t/3/a/b: No such file or directory\n" > exp \
+  || fail=1
+compare err exp || fail=1
+
+Exit $fail
--
1.7.3.5