Subject: Re: bug#16539: More details on df command output for you

From: Pádraig Brady <[email-masked]>
Date: Wed, 18 Jun 2014 18:21:50 +0100

On 06/18/2014 01:09 PM, Pádraig Brady wrote:
> On 06/18/2014 02:19 AM, Bernhard Voelker wrote:
>> On 06/17/2014 06:03 PM, Pádraig Brady wrote:
>>> From 0a4b8027049f6746a237c9fc34a0e0a4afdcfc62 Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
>>> Date: Wed, 4 Jun 2014 00:09:11 +0100
>>> Subject: [PATCH] df: output placeholder values for inaccessible mount points
> 
> I agree with all your suggested tweaks here.
> 
>> Finally, I have an edge case on my PC which is not yet covered:
>> I have a backup partition mounted on /root/backup (which is not
>> accessible by a normal user), and a read-only bind-mount of it:
>>
>>   $ mount /dev/sda1 /root/backup
>>   $ mount --bind    /root/backup /media/backup
>>   $ mount -o ro,remount,bind     /media/backup
>>
>> (The 3rd command is just there for setting the read-only flag).
>>
>> df-8.21 perfectly skipped the inaccessible entry and went ahead
>> with the read-only copy
>>
>>   $ /usr/bin/df /dev/sda3
>>   Filesystem     1K-blocks      Used Available Use% Mounted on
>>   /dev/sda3      155396036 124425132  23054156  85% /media/backup
>>
>> while the latest one from git plus these 2 patches is falling over:
>>
>>   $ src/df /dev/sda3
>>   src/df: ‘/root/backup’: Permission denied
>>
>> This is a little regression.
>> Thus said, I'm not 100% happy with the filtering yet.
> 
> The above is a separate case actually, triggered in 8.22 with:
> http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=2091f449
> Note that only triggers the issue when the last mount point for a device
> is accessible. If the /proc/mounts order was different, then you'd get the issue anyway.
> I'll fix this by adding a stat() to the selection criteria in get_disk().

The attached 2 patches on top of current master,
should address your suggested tweaks and above ordering issue.

thanks,
Pádraig.
>From 8a9d785f0145cc43c0a9da2763bcb32d7f2011f3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Wed, 4 Jun 2014 00:09:11 +0100
Subject: [PATCH 1/2] df: output placeholder values for inaccessible mount
 points

A system provided mount entry may be unavailable due to TOCTOU race,
or if another device has been over-mounted at that position, or due to
access permissions.  In all these cases output "-" placeholder values
rather than either producing an error, or in the over-mount case
outputting values for the wrong device.

* src/df.c (device_list): A new global list now updated by
filter_mount_list().
(filter_mount_list): Adjust to take a parameter as to whether
update the global mount list, or only the mount <-> device ID mapping.
(get_dev): Use the device ID mapping to ensure we're not outputting
stats for the wrong device.  Also output placeholder values when we
can't access a system specified mount point.
(get_all_entries): Set the DEVICE_ONLY param for filter_mount_list().
(devname_for_dev): A new function to search the mount <-> dev mapping.
* test/df/skip-duplicates.sh: Adjust accordingly.
* NEWS: Mention the bug fixes.

Discussed at: http://bugs.gnu.org/16539
---
 NEWS                        |    6 ++-
 src/df.c                    |  109 ++++++++++++++++++++++++++++++++-----------
 tests/df/skip-duplicates.sh |    6 ++-
 3 files changed, 89 insertions(+), 32 deletions(-)

diff --git a/NEWS b/NEWS
index 77286f8..15846b0 100644
--- a/NEWS
+++ b/NEWS
@@ -44,8 +44,10 @@ GNU coreutils NEWS                                    -*- outline -*-
 
   [These dd bugs were present in "the beginning".]
 
-  df now elides duplicates for virtual file systems like tmpfs, and will
-  display the correct device name for directories mounted multiple times.
+  df now elides duplicates for virtual file systems like tmpfs.
+  Displays the correct device details for points mounted multiple times.
+  Displays placeholder values for inaccessible file systems,
+  rather than error messages or values for the wrong file system.
   [These bugs were present in "the beginning".]
 
   head --bytes=-N and --lines=-N now handles devices more
diff --git a/src/df.c b/src/df.c
index 10047ce..d6d4b0e 100644
--- a/src/df.c
+++ b/src/df.c
@@ -45,12 +45,12 @@
 
 /* Filled with device numbers of examined file systems to avoid
    duplicities in output.  */
-struct devlist
+static struct devlist
 {
   dev_t dev_num;
   struct mount_entry *me;
   struct devlist *next;
-};
+} *device_list;
 
 /* If true, show even file systems with zero size or
    uninteresting types.  */
@@ -607,23 +607,25 @@ excluded_fstype (const char *fstype)
    In the case of duplicities - based on the device number - the mount entry
    with a '/' in its me_devname (i.e. not pseudo name like tmpfs) wins.
    If both have a real devname (e.g. bind mounts), then that with the shorter
-   me_mountdir wins.  */
+   me_mountdir wins.  With DEVICES_ONLY == true (set with df -a), only update
+   the global device_list, rather than filtering the global mount_list.  */
 
 static void
-filter_mount_list (void)
+filter_mount_list (bool devices_only)
 {
   struct mount_entry *me;
 
-  /* Store of already-processed device numbers.  */
-  struct devlist *devlist_head = NULL;
-
-  /* Sort all 'wanted' entries into the list devlist_head.  */
+  /* Sort all 'wanted' entries into the list device_list.  */
   for (me = mount_list; me;)
     {
       struct stat buf;
       struct devlist *devlist;
       struct mount_entry *discard_me = NULL;
 
+      /* TODO: On Linux we might avoid this stat() and another in get_dev()
+         by using the device IDs available from /proc/self/mountinfo.
+         read_file_system_list() could populate me_dev from those
+         for efficiency and accuracy.  */
       if (-1 == stat (me->me_mountdir, &buf))
         {
           /* Stat failed - add ME to be able to complain about it later.  */
@@ -632,7 +634,7 @@ filter_mount_list (void)
       else
         {
           /* If we've already seen this device...  */
-          for (devlist = devlist_head; devlist; devlist = devlist->next)
+          for (devlist = device_list; devlist; devlist = devlist->next)
             if (devlist->dev_num == buf.st_dev)
               break;
 
@@ -661,8 +663,9 @@ filter_mount_list (void)
 
       if (discard_me)
         {
-           me = me->me_next;
-           free_mount_entry (discard_me);
+          me = me->me_next;
+          if (! devices_only)
+            free_mount_entry (discard_me);
         }
       else
         {
@@ -670,26 +673,46 @@ filter_mount_list (void)
           devlist = xmalloc (sizeof *devlist);
           devlist->me = me;
           devlist->dev_num = buf.st_dev;
-          devlist->next = devlist_head;
-          devlist_head = devlist;
+          devlist->next = device_list;
+          device_list = devlist;
 
           me = me->me_next;
         }
     }
 
   /* Finally rebuild the mount_list from the devlist.  */
-  mount_list = NULL;
-  while (devlist_head)
+  if (! devices_only) {
+    mount_list = NULL;
+    while (device_list)
+      {
+        /* Add the mount entry.  */
+        me = device_list->me;
+        me->me_next = mount_list;
+        mount_list = me;
+        /* Free devlist entry and advance.  */
+        struct devlist *devlist = device_list->next;
+        free (device_list);
+        device_list = devlist;
+      }
+  }
+}
+
+/* Search a mount entry list for device id DEV.
+   Return the corresponding device name if found or NULL if not.  */
+
+static char const * _GL_ATTRIBUTE_PURE
+devname_for_dev (dev_t dev)
+{
+  struct devlist *dl = device_list;
+
+  while (dl)
     {
-      /* Add the mount entry.  */
-      me = devlist_head->me;
-      me->me_next = mount_list;
-      mount_list = me;
-      /* Free devlist entry and advance.  */
-      struct devlist *devlist = devlist_head->next;
-      free (devlist_head);
-      devlist_head = devlist;
+      if (dl->dev_num == dev)
+        return dl->me->me_devname;
+      dl = dl->next;
     }
+
+  return NULL;
 }
 
 /* Return true if N is a known integer value.  On many file systems,
@@ -878,9 +901,40 @@ get_dev (char const *disk, char const *mount_point, char const* file,
     fsu = *force_fsu;
   else if (get_fs_usage (stat_file, disk, &fsu))
     {
-      error (0, errno, "%s", quote (stat_file));
-      exit_status = EXIT_FAILURE;
-      return;
+      /* If we can't access a system provided entry due
+         to it not being present (now), or due to permissions,
+         just output placeholder values rather than failing.  */
+      if (process_all && (errno == EACCES || errno == ENOENT))
+        {
+          if (! show_all_fs)
+            return;
+
+          fstype = "-";
+          fsu.fsu_blocksize = fsu.fsu_blocks = fsu.fsu_bfree =
+          fsu.fsu_bavail = fsu.fsu_files = fsu.fsu_ffree = UINTMAX_MAX;
+        }
+      else
+        {
+          error (0, errno, "%s", quote (stat_file));
+          exit_status = EXIT_FAILURE;
+          return;
+        }
+    }
+  else if (process_all && show_all_fs)
+    {
+      /* Ensure we don't output incorrect stats for over-mounted directories.
+         Discard stats when the device name doesn't match.  */
+      struct stat sb;
+      if (stat (stat_file, &sb) == 0)
+        {
+          char const * devname = devname_for_dev (sb.st_dev);
+          if (devname && ! STREQ (devname, disk))
+            {
+              fstype = "-";
+              fsu.fsu_blocksize = fsu.fsu_blocks = fsu.fsu_bfree =
+              fsu.fsu_bavail = fsu.fsu_files = fsu.fsu_ffree = UINTMAX_MAX;
+            }
+        }
     }
 
   if (fsu.fsu_blocks == 0 && !show_all_fs && !show_listed_fs)
@@ -1230,8 +1284,7 @@ get_all_entries (void)
 {
   struct mount_entry *me;
 
-  if (!show_all_fs)
-    filter_mount_list ();
+  filter_mount_list (show_all_fs);
 
   for (me = mount_list; me; me = me->me_next)
     get_dev (me->me_devname, me->me_mountdir, NULL, NULL, me->me_type,
diff --git a/tests/df/skip-duplicates.sh b/tests/df/skip-duplicates.sh
index a620e73..9f0f749 100755
--- a/tests/df/skip-duplicates.sh
+++ b/tests/df/skip-duplicates.sh
@@ -96,8 +96,8 @@ test -f x || skip_ "internal test failure: maybe LD_PRELOAD doesn't work?"
 LD_PRELOAD=./k.so df -T >out || fail=1
 test $(wc -l <out) -eq $(expr 1 + $unique_entries) || { fail=1; cat out; }
 
-# Ensure we fail when unable to stat invalid entries
-LD_PRELOAD=./k.so CU_TEST_DUPE_INVALID=1 df -T >out && fail=1
+# Ensure we don't fail when unable to stat (currently) unavailable entries
+LD_PRELOAD=./k.so CU_TEST_DUPE_INVALID=1 df -T >out || fail=1
 test $(wc -l <out) -eq $(expr 1 + $unique_entries) || { fail=1; cat out; }
 
 # df should also prefer "/fsname" over "fsname"
@@ -113,6 +113,8 @@ test $(grep -c 'virtfs2.*fstype2' <out) -eq 1 || { fail=1; cat out; }
 # Ensure that filtering duplicates does not affect -a processing.
 LD_PRELOAD=./k.so df -a >out || fail=1
 test $(wc -l <out) -eq 6 || { fail=1; cat out; }
+# Ensure placeholder "-" values used for the eclipsed "virtfs"
+test $(grep -c 'virtfs *-' <out) -eq 1 || { fail=1; cat out; }
 
 # Ensure that filtering duplicates does not affect
 # argument processing (now without the fake getmntent()).
-- 
1.7.7.6

>From cc15c083795d3e97e3693a4f11ae745940cc045b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Wed, 18 Jun 2014 13:10:17 +0100
Subject: [PATCH 2/2] df: look for accessible mount points for specified
 devices

* src/df.c (get_disk): Include in the mount entry selection criteria,
whether we can access the mount directory.  This handles the case where
a device is (bind) mounted multiple times with the shortest mount path
not being accessible, while some of the other mount points are.
Discussed at: http://bugs.gnu.org/16539#63
---
 src/df.c |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/src/df.c b/src/df.c
index d6d4b0e..dc6544b 100644
--- a/src/df.c
+++ b/src/df.c
@@ -1121,6 +1121,7 @@ get_disk (char const *disk)
 {
   struct mount_entry const *me;
   struct mount_entry const *best_match = NULL;
+  bool best_match_accessible = false;
   char const *file = disk;
 
   char *resolved = canonicalize_file_name (disk);
@@ -1139,13 +1140,24 @@ get_disk (char const *disk)
       if (STREQ (disk, devname))
         {
           size_t len = strlen (me->me_mountdir);
-          if (len < best_match_len)
+
+          if (! best_match_accessible || len < best_match_len)
             {
-              best_match = me;
-              if (len == 1) /* Traditional root.  */
-                break;
-              else
-                best_match_len = len;
+              struct stat disk_stats;
+              bool this_match_accessible = false;
+
+              if (stat (me->me_mountdir, &disk_stats) == 0)
+                best_match_accessible = this_match_accessible = true;
+
+              if (this_match_accessible
+                  || (! best_match_accessible && len < best_match_len))
+                {
+                  best_match = me;
+                  if (len == 1) /* Traditional root.  */
+                    break;
+                  else
+                    best_match_len = len;
+                }
             }
         }
 
-- 
1.7.7.6