GNU bug report logs - #16539
df command, possible bug?

Previous Next

Full log

Message #60 received at 16539 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>, 16539 <at> debbugs.gnu.org
Subject: Re: bug#16539: More details on df command output for you
Date: Wed, 18 Jun 2014 03:19:04 +0200

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?= <P <at> draigBrady.com>
> Date: Wed, 4 Jun 2014 00:09:11 +0100
> Subject: [PATCH] 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 overmounted at that position, or due to
> access permissions.  In all these cases output "-" placeholder values
> rather than either producing an error, or in the overmount case
> outputting values for the wrong device.

Cool: finally GNU df fulfills the first sentence of the POSIX spec ;-)

  http://pubs.opengroup.org/onlinepubs/9699919799/utilities/df.html

  The df utility shall write the amount of available space ... for
  file systems on which the invoking user has appropriate read access.

I think we already discussed whether a warning would be better than
a line with "-"s (or even silently omit the line) - and seeing the
result is pleasing:

Before:
  /usr/bin/df: ‘/root/backup’: Permission denied
After:
  /dev/sda3              -         -         -    - /root/backup



> * src/df.c (device_list): A new global list updated from ...

... from what?

> (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 plaeholder values when we

s/plaeholder/placeholder/

> can't access a system specified mount point.
> (devname_for_dev): A new function to search the mount <-> dev mapping.

get_all_entries was also changed.

> * test/df/skip-duplicates.sh: Adjust accordingly.
> * NEWS: Mention the bug fixes.
> 
> Discussed at: http://bugs.gnu.org/16539

> diff --git a/src/df.c b/src/df.c
> index 10047ce..c7da208 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;

Maybe I'm old-fashioned, but I'd have separated the struct definition
from the variable definition ... well, personal style.

> @@ -610,20 +610,21 @@ excluded_fstype (const char *fstype)
>     me_mountdir wins.  */
>  
>  static void
> -filter_mount_list (void)
> +filter_mount_list (bool devices_only)

The parameter name isn't very intuitive (to me).
Would it harm to add a sentence in the function description?

> @@ -878,9 +900,37 @@ 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))
> +        {
> +          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;
> +        }

Is it possible that errno is leaking from one get_dev() invocation
to another,  i.e. that one failure eclipses another?

> +    }
> +  else if (process_all && show_all_fs)
> +    {
> +      /* Ensure we don't output incorrect stats for overmounted 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 +1280,10 @@ get_all_entries (void)
>  {
>    struct mount_entry *me;
>  
> -  if (!show_all_fs)
> -    filter_mount_list ();
> +  if (! show_all_fs)
> +    filter_mount_list (false);
> +  else
> +    filter_mount_list (true);

Shorter?
+  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..1b19149 100755
> --- a/tests/df/skip-duplicates.sh
> +++ b/tests/df/skip-duplicates.sh
> @@ -96,9 +96,10 @@ 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
> -test $(wc -l <out) -eq $(expr 1 + $unique_entries) || { fail=1; cat out; }
> +# Ensure we don't fail when unable to stat (currently) unavailable entries
> +# instead outputting placeholder information
> +LD_PRELOAD=./k.so CU_TEST_DUPE_INVALID=1 df -T >out || fail=1
> +test $(wc -l <out) -eq $(expr 2 + $unique_entries) || { fail=1; cat out; }
>  
>  # df should also prefer "/fsname" over "fsname"
>  if test "$unique_entries" = 2; then
> @@ -113,6 +114,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 overmounted "virtfs"

my Thunderbird suggests: s/overmounted/over-mounted/

> +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

Great, we're getting closer!

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.
I don't have an idea right now how to solve it. Maybe in a
couple of days ... I have to think about it.

Thanks again & have a nice day,
Berny

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.