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