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 protected]> >>> 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 protected]> 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 protected]> 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
