Subject: Re: bug#29038: df hangs on fifos/named pipes

From: Pádraig Brady <[email-masked]>
Date: Sun, 29 Oct 2017 15:34:23 -0700

On 28/10/17 00:18, Stephane Chazelas wrote:
> test case:
> 
>    mkfifo p
>    df p
> 
> That hangs, unless you make "p" non-readable or some other process
> has the fifo open in write mode.
> 
> The reason is that df tries to open the fifo in read-only mode,
> according to comments in the source code so as to trigger a
> potential automout.
> 
> That goes back to this commit:
> 
>> commit dbd17157d7e693b8de9737f802db0e235ff5a3e6
>> Author: Tomas Smetana <[email-masked]>
>> Date:   Tue Apr 28 11:21:49 2009 +0200
>>
>>     df: use open(2), not stat, to trigger automounting
>>
>>     * src/df.c (main): When iterating over command-line arguments,
>>     attempting to ensure each backing file system is mounted, use
>>     open, not stat.  stat is no longer sufficient to trigger
>>     automounting, in some cases.  Based on a suggestion from Ian Kent.
>>     More details in http://bugzilla.redhat.com/497830
> 
> More info at the bugzilla link.
> 
> It's arguable whether df, a reporting tool, should have such a
> side effect as automounting a file system.
> 
> The fifo issue though is a bug IMO, especially considering that
> POSIX explicitely says that df should work on fifos.
> 
> Here, it may be enough to add the O_DIRECTORY flag to open()
> where available if we only care about automounting files of type
> directory (or portably use opendir()).
> 
> Or use O_PATH  on Linux 3.6+ followed by openat() on non-fifos if
> open(O_PATH) is not enough to trigger the automount in the
> unlikely event we care about automounting non-directory files
> (and report their disk usage).
> 
> Or not open() at all, and not automount file systems.
> 
> Note that busybox, heirloom or ast-open df implementations on
> Linux don't have the problem (and presumably don't automount
> file systems). Nor does FreeBSD.
> 
> Reproduced with:
> 
> $ df --version
> df (GNU coreutils) 8.25
> 
> and:
> 
> $ df --version
> df (GNU coreutils) 8.27.46-e13fe
> 
> That was discovered by Martijn Dekker, CCed, when looking for a
> portable way to identify the file system of an arbitrary file.
> 

Yes we shouldn't hang.

RE side effects, open() is a fairly innocuous operation,
and we expect stat() to have side effects to auto mount.
(Note we avoid stat() with non specified arguments if possible due to this):
https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v8.27-92-ga19ff5d

I suppose we could stat() and if that succeeds && !fifo, only then call open() ?
Patch to do that is attached.

cheers,
Pádraig
From 2771d66e4fdb2e2d158d092ead543638115971f9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Sun, 29 Oct 2017 15:29:05 -0700
Subject: [PATCH] df: fix hang with fifo argument

* src/df.c (main): stat() before open(), and avoid
the optional open when given a fifo argument.
* tests/df/unreadable.sh: Add a test case.
* NEWS: Mention the fix.
Fixes https://bugs.gnu.org/29038
---
 NEWS                   |  3 +++
 src/df.c               | 13 ++++++++-----
 tests/df/unreadable.sh |  3 +++
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/NEWS b/NEWS
index 806e3ef..3e6704d 100644
--- a/NEWS
+++ b/NEWS
@@ -12,6 +12,9 @@ GNU coreutils NEWS                                    -*- outline -*-
   invalidated.  [bug introduced for "direct" in coreutils-7.5,
   and with the "nocache" implementation in coreutils-8.11]
 
+  df no longer hangs when given a fifo argument.
+  [bug introduced in coreutils-7.3]
+
   ptx -S no longer infloops for a pattern which returns zero-length matches.
   [the bug dates back to the initial implementation]
 
diff --git a/src/df.c b/src/df.c
index 7345bc9..ee04d51 100644
--- a/src/df.c
+++ b/src/df.c
@@ -1710,16 +1710,19 @@ main (int argc, char **argv)
         {
           /* Prefer to open with O_NOCTTY and use fstat, but fall back
              on using "stat", in case the file is unreadable.  */
-          int fd = open (argv[i], O_RDONLY | O_NOCTTY);
-          if ((fd < 0 || fstat (fd, &stats[i - optind]))
-              && stat (argv[i], &stats[i - optind]))
+          if (stat (argv[i], &stats[i - optind]))
             {
               error (0, errno, "%s", quotef (argv[i]));
               exit_status = EXIT_FAILURE;
               argv[i] = NULL;
             }
-          if (0 <= fd)
-            close (fd);
+          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);
+            }
         }
     }
 
diff --git a/tests/df/unreadable.sh b/tests/df/unreadable.sh
index c28cdc9..fb4c91c 100755
--- a/tests/df/unreadable.sh
+++ b/tests/df/unreadable.sh
@@ -24,6 +24,9 @@ touch unreadable || fail=1
 chmod a-r unreadable || fail=1
 df unreadable || fail=1
 
+mkfifo_or_skip_ fifo
+timeout 10 df fifo || fail=1
+
 test "$fail" = 1 && dump_mount_list_
 
 Exit $fail
-- 
2.9.3