bug#29038: df hangs on fifos/named pipes

[Stephane Chazelas]

2017-10-30 09:53:01 -0700, Pádraig Brady:
[...]
> I pushed my change with the comments fixed up.
> 
> I thought about O_NONBLOCK and O_PATH but thought these might not
> induce or wait for the auto mount to occur.
[...]

Note that:

df /dev/watchdog

as root still causes the system to reboot on Linux. Not a POSIX
conformance issue as the behaviour is unspecified for device
files without a mounted file system on, but still not ideal.

The point is that opening files has or can have side effects and
df is not *meant* to open files.

I would think that automounting directories assuming we do want
it to happen should be enough unless someone can come up with a
realistic use case where non-directory files are being
automounted (and assuming that's possible).

-- 
Stephane

bug#29038: df hangs on fifos/named pipes

[Pádraig Brady]

On 29/10/17 23:16, Paul Eggert wrote:
> Pádraig Brady wrote:
> 
>> I suppose we could stat() and if that succeeds && !fifo, only then call 
>> open() ?
>> Patch to do that is attached.
> 
> Better is to use open with O_NONBLOCK, as this avoids interpreting the file 
> name 
> twice in the usual case. Better yet is to use O_PATH if available, as this 
> avoids interpreting the file name twice even when the file is unreadable or 
> is a 
> FIFO that would block. Also, the comment just before the code should be 
> changed 
> to match the altered code. Proposed followup patch attached.
> 
> It is puzzling that df calls fstat or stat, when it should just be calling 
> fstatfs or statfs. But that is a different matter.

I pushed my change with the comments fixed up.

I thought about O_NONBLOCK and O_PATH but thought these might not
induce or wait for the auto mount to occur.

thanks for the review,
Pádraig

marking this as done...

bug#29038: df hangs on fifos/named pipes

[Stephane Chazelas]

2017-10-29 23:16:50 -0700, Paul Eggert:
> Pádraig Brady wrote:
> 
> >I suppose we could stat() and if that succeeds && !fifo, only then call 
> >open() ?
> >Patch to do that is attached.
> 
> Better is to use open with O_NONBLOCK, as this avoids interpreting the file
> name twice in the usual case.

That would still have the unwanted side effect of instantiating
(and kill thereafter) the pipe when opening a fifo that has a
writer and no reader.

As in, if some process does a fd = open("fifo", O_WRONLY)

The "df fifo" would cause that open() to return, and the next
write() to it possibly cause a SIGPIPE because df is already
gone.

It's different for O_DIRECTORY as the open() would fail on
fifos (it also fails if you don't have read permission,
which is another advantage of using O_PATH I suppose)


> Better yet is to use O_PATH if available, as
> this avoids interpreting the file name twice even when the file is
> unreadable or is a FIFO that would block.

Note that according to the man page, fstat() on fds open with
O_PATH only works since Linux 3.6. Note that we're already
interpreting the file path twice as we're not using fstatfs (as we
wouldn't want to cause a "too many open files" situation I
suppose by keeping all the files open before we get to the
second phase).

[...]
> It is puzzling that df calls fstat or stat, when it should just be calling
> fstatfs or statfs. But that is a different matter.

I suppose that's because it needs to determine if the file is a
device file or not as for device files where a fs is mounted, it
needs to report details for that fs as opposed to the fs the
device file is on.

-- 
Stephane

bug#29038: df hangs on fifos/named pipes

[Paul Eggert]

Pádraig Brady wrote:


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


Better is to use open with O_NONBLOCK, as this avoids interpreting the file name 
twice in the usual case. Better yet is to use O_PATH if available, as this 
avoids interpreting the file name twice even when the file is unreadable or is a 
FIFO that would block. Also, the comment just before the code should be changed 
to match the altered code. Proposed followup patch attached.


It is puzzling that df calls fstat or stat, when it should just be calling 
fstatfs or statfs. But that is a different matter.
From c02f0e6009d7d77ee7a8e1299e03ca10a477c9e4 Mon Sep 17 00:00:00 2001
From: Paul Eggert 
Date: Sun, 29 Oct 2017 23:09:49 -0700
Subject: [PATCH] df: improve fix for FIFOs

* src/df.c (main): Use O_PATH if available.  Otherwise,
use O_NONBLOCK.  Either way, this avoids the need to
interpret the file name twice.
---
 src/df.c | 24 ++--
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/src/df.c b/src/df.c
index ee04d51..cab1958 100644
--- a/src/df.c
+++ b/src/df.c
@@ -1708,21 +1708,25 @@ main (int argc, char **argv)
   stats = xnmalloc (argc - optind, sizeof *stats);
   for (int i = optind; i < argc; ++i)
 {
-  /* Prefer to open with O_NOCTTY and use fstat, but fall back
- on using "stat", in case the file is unreadable.  */
-  if (stat (argv[i], [i - optind]))
+#if O_PATH
+  int oflag = O_PATH;
+  bool skip_stat = true;
+#else
+  /* Fall back on stat in case the file is unreadable or is a
+ FIFO that would block.  */
+  int oflag = O_RDONLY | O_NOCTTY | O_NONBLOCK;
+  bool skip_stat = false;
+#endif
+  int fd = open (argv[i], oflag);
+  if ((fd < 0 || fstat (fd, [i - optind]) != 0)
+  && (skip_stat || stat (argv[i], [i - optind]) != 0))
 {
   error (0, errno, "%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);
-}
+  if (0 <= fd)
+close (fd);
 }
 }
 
-- 
2.7.4

bug#29038: df hangs on fifos/named pipes

[Jim Meyering]

On Sun, Oct 29, 2017 at 3:34 PM, Pádraig Brady  wrote:
...
>> 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.

Sounds good. Patch looks fine to me. Thanks!

bug#29038: df hangs on fifos/named pipes

[Pádraig Brady]

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 
>> 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?= 
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, [i - optind]))
-  && stat (argv[i], [i - optind]))
+  if (stat (argv[i], [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

bug#29038: df hangs on fifos/named pipes

[Stephane Chazelas]

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

-- 
Stephane