Subject: bug#50012: closed (Re: bug#50012: [PATCH] df: Fixed usage of incorrect stat data when using automount)

From: [email-masked] (GNU bug Tracking System)
Date: Wed, 11 Aug 2021 18:25:01 +0000

Your bug report

#50012: [PATCH] df: Fixed usage of incorrect stat data when using automount

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
50012: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=50012
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#50012: [PATCH] df: Fixed usage of incorrect stat data when using automount
From: Paul Eggert <[email-masked]>
To: Thimo Emmerich <[email-masked]>
Cc: [email-masked]
Date: Wed, 11 Aug 2021 11:24:03 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

On 8/11/21 8:59 AM, Thimo Emmerich wrote:

> Since the stat in [2]/df.c, line 1775 is necessary for the following 
> fifo evaluation, it seems to be necessary to update the stat information 
> after a successful open.

Thanks for reporting the bug. I installed the attached. It differs a bit 
from what you proposed in that it prefers fstat to stat, as fstat should 
be a bit faster and should avoid some unlikely races (there are many 
more races, but one fix at a time).
>From 48f59e058b4347172496bede193ade9fbf94d58e Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Wed, 11 Aug 2021 11:16:05 -0700
Subject: [PATCH] df: fix bug with automounted

If the command-line argument is automounted, df would use
stat info that became wrong after the following open.
* NEWS: Mention the fix (bug#50012).
* src/df.c (automount_stat_err): New function.
This fixes the hang on fifos in a better way, by using O_NONBLOCK.
(main): Use it.
---
 NEWS     |  3 +++
 src/df.c | 34 +++++++++++++++++++++++++---------
 2 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/NEWS b/NEWS
index 49ee083e4..e14de1397 100644
--- a/NEWS
+++ b/NEWS
@@ -16,6 +16,9 @@ GNU coreutils NEWS                                    -*- outline -*-
   df no longer outputs duplicate remote mounts in the presence of bind mounts.
   [bug introduced in coreutils-8.26]
 
+  df no longer mishandles command-line args that it pre-mounts
+  [bug introduced in coreutils-8.29]
+
   du no longer crashes on XFS file systems when the directory hierarchy is
   heavily changed during the run.
   [bug introduced in coreutils-8.25]
diff --git a/src/df.c b/src/df.c
index 4534935f5..48025b9fc 100644
--- a/src/df.c
+++ b/src/df.c
@@ -276,6 +276,28 @@ static struct option const long_options[] =
   {NULL, 0, NULL, 0}
 };
 
+/* Stat FILE and put the results into *ST.  Return 0 if successful, an
+   error number otherwise.  Try to open FILE before statting, to
+   trigger automounts.  */
+
+static int
+automount_stat_err (char const *file, struct stat *st)
+{
+  int fd = open (file, O_RDONLY | O_NOCTTY | O_NONBLOCK);
+  if (fd < 0)
+    {
+      if (errno == ENOENT || errno == ENOTDIR)
+        return errno;
+      return stat (file, st) == 0 ? 0 : errno;
+    }
+  else
+    {
+      int err = fstat (fd, st) == 0 ? 0 : errno;
+      close (fd);
+      return err;
+    }
+}
+
 /* Replace problematic chars with '?'.
    Since only control characters are currently considered,
    this should work in all encodings.  */
@@ -1772,19 +1794,13 @@ main (int argc, char **argv)
       stats = xnmalloc (argc - optind, sizeof *stats);
       for (int i = optind; i < argc; ++i)
         {
-          if (stat (argv[i], &stats[i - optind]))
+          int err = automount_stat_err (argv[i], &stats[i - optind]);
+          if (err != 0)
             {
-              error (0, errno, "%s", quotef (argv[i]));
+              error (0, err, "%s", quotef (argv[i]));
               exit_status = EXIT_FAILURE;
               argv[i] = NULL;
             }
-          else if (! S_ISFIFO (stats[i - optind].st_mode))
-            {
-              /* open() is needed to automount in some cases.  */
-              int fd = open (argv[i], O_RDONLY | O_NOCTTY);
-              if (0 <= fd)
-                close (fd);
-            }
         }
     }
 
-- 
2.30.2

Subject: [PATCH] df: Fixed usage of incorrect stat data when using automount
From: Thimo Emmerich <[email-masked]>
To: [email-masked]
Date: Wed, 11 Aug 2021 17:59:29 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

This seems to be a regression from b04ce61958c1: "df: fix hang with fifo 
argument".
When using df on an browsable automount map (in my case NFS mounts) df 
returns a screwed up information if the mount is not existing.
During execution it triggers the mount via open() but it uses previously 
collected stat information for the output.
A second run leads to the correct information due to the mount in the 
first run.
Example:
me@app07:/psg$ ls -ld psg01bu
drwxr-xr-x 2 root root 0 Jul 23 17:33 psg01bu
me@app07:/psg$ df -h psg01bu
Filesystem      Size  Used Avail Use% Mounted on
auto_psg        2.0T   80M  1.9T   1% /psg
me@app07:/psg$ ls -ld psg01bu
drwxrwsr-x 2 root abc 4096 May  5 11:37 psg01bu
me@app07:/psg$ df -h psg01bu
Filesystem               Size  Used Avail Use% Mounted on
fil36:/vg1/lv06/psg01bu  2.0T   80M  1.9T   1% /psg/psg01bu

This behaviour could be observed on Ubuntu 20.04's stock df command 
(coreutils 8.30-3ubuntu2) as well as on latest GIT master. An old CentOS 
7 machine is not affected.

In general the comment in df.h "stat each of the given entries to make 
sure any corresponding partition is automounted." is suspicious since in 
[1] it is explicitly stated that "Any access to this directory beyond a 
“stat” will (normally) cause the d_op->d_automount() dentry operation to 
be called.".

Since the stat in [2]/df.c, line 1775 is necessary for the following 
fifo evaluation, it seems to be necessary to update the stat information 
after a successful open.

Best regards,
  Thimo

[1]: https://www.kernel.org/doc/html/latest/filesystems/autofs.html
[2]: https://github.com/coreutils/coreutils/blob/master/src/df.c#L1775

 From a586ab553d9e6f463049349a8ca88d7e727804df Mon Sep 17 00:00:00 2001
From: Thimo Emmerich <[email-masked]>
Date: Wed, 11 Aug 2021 17:50:22 +0200
Subject: [PATCH] df: Fixed usage of incorrect stat data when using automount

This code re-runs stat() after open()ing to update possibly
invalid data. Invalid data can occur in case of using df on
an unmounted directory in a browsable automount map.
---
  src/df.c | 16 +++++++++++-----
  1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/df.c b/src/df.c
index 4534935f5..a5288cb6a 100644
--- a/src/df.c
+++ b/src/df.c
@@ -1766,9 +1766,10 @@ main (int argc, char **argv)

    if (optind < argc)
      {
-      /* stat each of the given entries to make sure any corresponding
-         partition is automounted.  This must be done before reading the
-         file system table.  */
+      /* get the stat() information for the fifo check.
+         In case of a browsable automount map this stat will
+         not trigger the mount. Thus the information needs
+         to be updated.  */
        stats = xnmalloc (argc - optind, sizeof *stats);
        for (int i = optind; i < argc; ++i)
          {
@@ -1780,10 +1781,15 @@ main (int argc, char **argv)
              }
            else if (! S_ISFIFO (stats[i - optind].st_mode))
              {
-              /* open() is needed to automount in some cases.  */
+                /* open() each of the given entries to make sure any 
corresponding
+                 partition is automounted.  This must be done before 
reading the
+                 file system table.  */
                int fd = open (argv[i], O_RDONLY | O_NOCTTY);
                if (0 <= fd)
-                close (fd);
+                {
+                  close (fd);
+                  stat (argv[i], &stats[i - optind]);
+                }
              }
          }
      }
--
2.25.1

-- 
Mit freundlichen Grüßen / with kind regards

   Thimo Emmerich

-------------------------------------------

R&D Center Europe Stuttgart Laboratory 01

Phone:    +49 (0) 711 / 5858 - 282
E-mail:   [email-masked]

Sony Europe B.V., Zweigniederlassung Deutschland
Stuttgart Technology Center
Hedelfinger Str. 61, 70327 Stuttgart, Germany

Registered at the Court of Berlin-Charlottenburg HRB 129332 B

Main Establishment: Sony Europe B.V.
Registered Office: The Heights, Brooklands, Weybridge, Surrey, KT13 0XW, UK
Incorporated in the Netherlands No. 71682147
Directors: Hideyuki Furumi, Masaki Kurebayashi, Ricky Londema