Re: du bogus warning with bind mounts

[Jim Meyering]

Jim Meyering wrote:
> Ondrej Oprala wrote:
>>>From d8eab7abdf19b668065ff8be3700c9966eefdc39 Mon Sep 17 00:00:00 2001
>> From: Ondrej Oprala <address@hidden>
>> Date: Thu, 9 Aug 2012 17:34:09 +0200
>> Subject: [PATCH] du: Fix an issue with bogus warnings on bind-mounted
>>  directories
>>
>> * NEWS: Mention the fix.
>> * src/du.c: Include "mountlist.h".
>>   (di_mnt): New global set.
>>   (fill_mount_table): New function.
>>   (hash_ins): Add HT parameter.
>>   (process_file): Look up each dir dev/ino pair in the new set.
>>   (main): Allocate, initialize, and free the new set.
>> * tests/Makefile.am: Add a new file.
>> * tests/du/bind-mount-dir-cycle: Add a test for the fix.
>
> Thanks.
>
> Please remember to build with --enable-gcc-warnings.
> With that, compilation failed due to a shadowing warning
> with the new di_set global shadowing a local by the same name.
>
> Here's a log entry I'm adding:
>
>   (di_files): Rename global from di_set, now that there are two.
>
> In comments don't mention "table" when speaking about sets.
> The fact that we're currently using a hash table to implement
> sets is irrelevant and best not mentioned.  Otherwise, if we
> ever change the underlying data structure, the comments would
> become erroneous.
>
> In fill_mount_table, there is no need to use xmalloc to allocate a struct 
> stat.
> Also, combine declarations and initializations and move declarations
> into innermost scope.
>
> I had to remove the warning-upon-failed-stat that I had recommended,
> because otherwise, when I run the test, I would get this failure:
>
>     + diff -u exp out
>     --- exp 2012-08-17 10:53:52.832808872 +0200
>     +++ out 2012-08-17 10:53:52.835808713 +0200
>     @@ -1 +1,2 @@
>     +du: warning: unable to stat /run/user/meyering/gvfs: Permission denied
>      du: mount point 'a' already traversed
>
> I'm squashing the following into your change set and will post
> the result shortly.
>
> =====================================================================
> diff --git a/src/du.c b/src/du.c
> index bfe543e..1a11e14 100644

Actually, I had to make some fundamental changes, since
your change made the tests/du/slash test fail:

    $ src/du --exclude='[^/]*' -x /
    src/du: mount point '/' already traversed
    4       /

Such usage must not produce a diagnostic.
The fundamental change is to move the check for a bind mount
into the switch branch that handles the fts-reported case
of a directory cycle.

In the case of a bind mount, I've made du warn and exit with status of 1,
whereas before (with your change) it was merely issuing the diagnostic.
I'll still need to document that, at least in the commit log.

Also, the new test script had problems.
Stale (copied) initial comment and unused expected output
as well as ignored stdout and unchecked exit status from du.



>From 491914627db04fe789bfdbf796f9be21198a6b09 Mon Sep 17 00:00:00 2001
From: Ondrej Oprala <address@hidden>
Date: Thu, 9 Aug 2012 17:34:09 +0200
Subject: [PATCH] du: handle bind-mounted directory cycles gracefully

Before this change, a directory cycle induced by a bind mount
would be treated as a fatal (probable disk corruption) error.
However, such cycles are relatively common, and can be avoided
efficiently, so now du merely emits a warning.

* NEWS (Bug fixes): Mention it.
* src/du.c: Include "mountlist.h".
(di_mnt): New global set.
(di_files): Rename global from di_set, now that there are two.
(fill_mount_table): New function.
(hash_ins): Add HT parameter.
(process_file): Look up each dir dev/ino pair in the new set.
(main): Allocate, initialize, and free the new set.
* tests/du/bind-mount-dir-cycle: Add a test for the fix.
* tests/Makefile.am (TESTS): Add it.
---
 NEWS                          |  3 ++
 src/du.c                      | 69 ++++++++++++++++++++++++++++++++++++-------
 tests/Makefile.am             |  1 +
 tests/du/bind-mount-dir-cycle | 44 +++++++++++++++++++++++++++
 4 files changed, 107 insertions(+), 10 deletions(-)
 create mode 100755 tests/du/bind-mount-dir-cycle

diff --git a/NEWS b/NEWS
index 012a633..05072fa 100644
--- a/NEWS
+++ b/NEWS
@@ -21,6 +21,9 @@ GNU coreutils NEWS                                    -*- 
outline -*-

 ** Bug fixes

+  du no longer emits bogus warnings when traversing bind-mounted
+  directory cycles.
+
   cksum now prints checksums atomically so that concurrent
   processes will not intersperse their output.
   [the bug dates back to the initial implementation]
diff --git a/src/du.c b/src/du.c
index 7333941..be8b6e7 100644
--- a/src/du.c
+++ b/src/du.c
@@ -35,6 +35,7 @@
 #include "exclude.h"
 #include "fprintftime.h"
 #include "human.h"
+#include "mountlist.h"
 #include "quote.h"
 #include "quotearg.h"
 #include "stat-size.h"
@@ -60,8 +61,12 @@ extern bool fts_debug;
 # define FTS_CROSS_CHECK(Fts)
 #endif

-/* A set of dev/ino pairs.  */
-static struct di_set *di_set;
+/* A set of dev/ino pairs to help identify files and directories
+   whose sizes have already been counted.  */
+static struct di_set *di_files;
+
+/* A set containing a dev/ino pair for each local mount point directory.  */
+static struct di_set *di_mnt;

 /* Keep track of the preceding "level" (depth in hierarchy)
    from one call of process_file to the next.  */
@@ -333,11 +338,11 @@ Mandatory arguments to long options are mandatory for 
short options too.\n\
   exit (status);
 }

-/* Try to insert the INO/DEV pair into the global table, HTAB.
+/* Try to insert the INO/DEV pair into DI_SET.
    Return true if the pair is successfully inserted,
-   false if the pair is already in the table.  */
+   false if the pair was already there.  */
 static bool
-hash_ins (ino_t ino, dev_t dev)
+hash_ins (struct di_set *di_set, ino_t ino, dev_t dev)
 {
   int inserted = di_set_insert (di_set, dev, ino);
   if (inserted < 0)
@@ -461,7 +466,7 @@ process_file (FTS *fts, FTSENT *ent)
       if (excluded
           || (! opt_count_all
               && (hash_all || (! S_ISDIR (sb->st_mode) && 1 < sb->st_nlink))
-              && ! hash_ins (sb->st_ino, sb->st_dev)))
+              && ! hash_ins (di_files, sb->st_ino, sb->st_dev)))
         {
           /* If ignoring a directory in preorder, skip its children.
              Ignore the next fts_read output too, as it's a postorder
@@ -490,7 +495,12 @@ process_file (FTS *fts, FTSENT *ent)
         case FTS_DC:
           if (cycle_warning_required (fts, ent))
             {
-              emit_cycle_warning (file);
+              /* If this is a mount point, then diagnose it and avoid
+                 the cycle.  */
+              if (di_set_lookup (di_mnt, sb->st_dev, sb->st_ino))
+                error (0, 0, _("mount point %s already traversed"), quote 
(file));
+              else
+                emit_cycle_warning (file);
               return false;
             }
           return true;
@@ -623,6 +633,37 @@ du_files (char **files, int bit_flags)
   return ok;
 }

+/* Fill the di_mnt set with local mount point dev/ino pairs.  */
+
+static void
+fill_mount_table (void)
+{
+  struct mount_entry *mnt_ent = read_file_system_list (false);
+  while (mnt_ent)
+    {
+      struct mount_entry *mnt_free;
+      if (!mnt_ent->me_remote && !mnt_ent->me_dummy)
+        {
+          struct stat buf;
+          if (!stat (mnt_ent->me_mountdir, &buf))
+            hash_ins (di_mnt, buf.st_ino, buf.st_dev);
+          else
+            {
+              /* Ignore stat failure.  False positives are too common.
+                 E.g., "Permission denied" on /run/user/<name>/gvfs.  */
+            }
+        }
+
+      mnt_free = mnt_ent;
+      mnt_ent = mnt_ent->me_next;
+
+      free (mnt_free->me_devname);
+      free (mnt_free->me_mountdir);
+      free (mnt_free->me_type);
+      free (mnt_free);
+    }
+}
+
 int
 main (int argc, char **argv)
 {
@@ -922,8 +963,15 @@ main (int argc, char **argv)
     xalloc_die ();

   /* Initialize the set of dev,inode pairs.  */
-  di_set = di_set_alloc ();
-  if (!di_set)
+
+  di_mnt = di_set_alloc ();
+  if (!di_mnt)
+    xalloc_die ();
+
+  fill_mount_table ();
+
+  di_files = di_set_alloc ();
+  if (!di_files)
     xalloc_die ();

   /* If not hashing everything, process_file won't find cycles on its
@@ -1001,7 +1049,8 @@ main (int argc, char **argv)
  argv_iter_done:

   argv_iter_free (ai);
-  di_set_free (di_set);
+  di_set_free (di_files);
+  di_set_free (di_mnt);

   if (files_from && (ferror (stdin) || fclose (stdin) != 0) && ok)
     error (EXIT_FAILURE, 0, _("error reading %s"), quote (files_from));
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 09d2658..5c612e8 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -32,6 +32,7 @@ root_tests =                                  \
   cp/sparse-fiemap                             \
   dd/skip-seek-past-dev                                \
   df/problematic-chars                         \
+  du/bind-mount-dir-cycle                      \
   install/install-C-root                       \
   ls/capability                                        \
   ls/nameless-uid                              \
diff --git a/tests/du/bind-mount-dir-cycle b/tests/du/bind-mount-dir-cycle
new file mode 100755
index 0000000..db5a910
--- /dev/null
+++ b/tests/du/bind-mount-dir-cycle
@@ -0,0 +1,44 @@
+#!/bin/sh
+# Exercise du's new ability to handle bind-mount-induced dir cycles.
+
+# Copyright (C) 2012 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_ rm
+require_root_
+
+cleanup_()
+{
+  # When you take the undesirable shortcut of making /etc/mtab a link
+  # to /proc/mounts, unmounting "$other_partition_tmpdir" would fail.
+  # So, here we unmount a/b instead.
+  umount a/b
+}
+
+mkdir -p a/b
+mount --bind a a/b \
+  || skip_ "This test requires mount with a working --bind option."
+
+echo a > exp || framework_failure_
+echo "du: mount point 'a/b' already traversed" > exp-err || framework_failure_
+
+du a > out 2> err && fail=1
+sed 's/^[0-9][0-9]*    //' out > k && mv k out
+
+compare exp-err err || fail=1
+compare exp out || fail=1
+
+Exit $fail
--
1.7.12.rc2