Jim Meyering <jim <at> meyering.net> writes: > > The call to error() flushes stderr (so even if fd 2 is pointing to a file > > and stderr is not line-buffered, the error message is still output), but > > we are failing to check ferror(stderr), when we proceed to blindly invoke > > the subsidiary program even though we had a write failure. Should we > > change the code to fail with EXIT_CANCELED if we detect failure to print > > the advisory message? > > Good catch. > > At first glance, this looked like a close call, but upon > reflection, it is clear that it does deserve a non-zero exit. > If we fail to diagnose the initial problem via stderr, > we have an obligation to escalate. Failure to change > priority, for whatever reason, must be diagnosed.
nohup and su were also affected (although I don't know how to write a test for su without requiring the test operator to enter a password). I performed an audit of all remaining clients of exec[lv]* in coreutils; the remaining programs are safe because they either call fork() and don't touch stderr in the child until after the exec attempt (such as sort), or it can be proven that all call paths that use stderr dead-end in a call to exit() rather than falling through to the exec attempt (such as chroot). How does this look? From: Eric Blake <[email protected]> Date: Wed, 28 Oct 2009 14:36:09 -0600 Subject: [PATCH] nice, nohup, su: detect write failure to stderr These programs can print non-fatal diagnostics to stderr prior to exec'ing a subsidiary program. However, if we thought the situation warranted a diagnostic, we insist that the diagnostic be printed without error, rather than blindly exec, as it may be a security risk. For an example, try 'nice -n -1 nice 2>/dev/full'. Failure to raise priority (by lowering niceness) is not fatal, but failure to inform the user about failure to change priority is dangerous. * src/nice.c (main): Declare failure if writing advisory message to stderr fails. * src/nohup.c (main): Likewise. * src/su.c (main): Likewise. * tests/misc/nice: Test this. * tests/misc/nohup: Likewise. * NEWS: Document this. --- NEWS | 4 ++++ src/nice.c | 13 +++++++++++-- src/nohup.c | 9 +++++++++ src/su.c | 8 ++++++++ tests/misc/nice | 6 ++++++ tests/misc/nohup | 15 +++++++++++++++ 6 files changed, 53 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index abf2466..0760775 100644 --- a/NEWS +++ b/NEWS @@ -21,6 +21,10 @@ GNU coreutils NEWS -*- outline -*- call fails with errno == EACCES. [the bug dates back to the initial implementation] + nice, nohup, and su now refuse to execute the subsidiary program if + they detect write failure in printing an otherwise non-fatal warning + message to stderr. + stat -f recognizes more file system types: afs, cifs, anon-inode FS, btrfs, cgroupfs, cramfs-wend, debugfs, futexfs, hfs, inotifyfs, minux3, nilfs, securityfs, selinux, xenfs diff --git a/src/nice.c b/src/nice.c index e157db8..a16066b 100644 --- a/src/nice.c +++ b/src/nice.c @@ -185,8 +185,17 @@ main (int argc, char **argv) ok = (setpriority (PRIO_PROCESS, 0, current_niceness + adjustment) == 0); #endif if (!ok) - error (perm_related_errno (errno) ? 0 - : EXIT_CANCELED, errno, _("cannot set niceness")); + { + error (perm_related_errno (errno) ? 0 + : EXIT_CANCELED, errno, _("cannot set niceness")); + /* error() flushes stderr, but does not check for write failure. + Normally, we would catch this via our atexit() hook of + close_stdout, but execvp() gets in the way. If stderr + encountered a write failure, there is no need to try calling + error() again. */ + if (ferror (stderr)) + exit (EXIT_CANCELED); + } execvp (argv[i], &argv[i]); diff --git a/src/nohup.c b/src/nohup.c index 880cc74..1f92c3f 100644 --- a/src/nohup.c +++ b/src/nohup.c @@ -203,6 +203,15 @@ main (int argc, char **argv) close (out_fd); } + /* error() flushes stderr, but does not check for write failure. + Normally, we would catch this via our atexit() hook of + close_stdout, but execvp() gets in the way. If stderr + encountered a write failure, there is no need to try calling + error() again, particularly since we may have just changed the + underlying fd out from under stderr. */ + if (ferror (stderr)) + exit (exit_internal_failure); + signal (SIGHUP, SIG_IGN); { diff --git a/src/su.c b/src/su.c index add100a..2648d17 100644 --- a/src/su.c +++ b/src/su.c @@ -506,5 +506,13 @@ main (int argc, char **argv) if (simulate_login && chdir (pw->pw_dir) != 0) error (0, errno, _("warning: cannot change directory to %s"), pw->pw_dir); + /* error() flushes stderr, but does not check for write failure. + Normally, we would catch this via our atexit() hook of + close_stdout, but execv() gets in the way. If stderr + encountered a write failure, there is no need to try calling + error() again. */ + if (ferror (stderr)) + exit (EXIT_CANCELED); + run_shell (shell, command, argv + optind, MAX (0, argc - optind)); } diff --git a/tests/misc/nice b/tests/misc/nice index cf4d96b..f85666e 100755 --- a/tests/misc/nice +++ b/tests/misc/nice @@ -82,6 +82,12 @@ if test x`nice -n -1 nice 2> /dev/null` = x0 ; then mv err exp || framework_failure nice --1 true 2> err || fail=1 compare exp err || fail=1 + # Failure to write advisory message is fatal. Buggy through coreutils 8.0. + if test -w /dev/full && test -c /dev/full; then + nice -n -1 nice > out 2> /dev/full + test $? = 125 || fail=1 + test -s out && fail=1 + fi else # superuser - change succeeds nice -n -1 nice 2> err || fail=1 diff --git a/tests/misc/nohup b/tests/misc/nohup index ad04a1c..d1c2119 100755 --- a/tests/misc/nohup +++ b/tests/misc/nohup @@ -64,6 +64,21 @@ test -f nohup.out && fail=1 rm -f nohup.out err # ---------------------- +# Bug present through coreutils 8.0: failure to print advisory message +# to stderr must be fatal. Requires stdout to be terminal. +if test -w /dev/full && test -c /dev/full; then +( + exec >/dev/tty + test -t 1 || exit 0 + nohup true 2> /dev/full + test $? = 125 || fail=1 + test -f nohup.out || fail=1 + test -s nohup.out && fail=1 + rm -f nohup.out + exit $fail +) || fail=1 +fi + nohup no-such-command 2> err errno=$? if test -t 1; then -- 1.6.4.2
