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.

diff --git a/NEWS b/NEWS
index abf2466b6a936eb705f57bfc2abb0687fffef375..0760775256fad710b80d64efb43e29946a354817 100644 (file)
--- 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 e157db80111375532d984bb5ba81f0d8a5c0f17c..a16066be8ee94fd627c0ad71a5490bd295459e80 100644 (file)
--- 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 880cc74925d78faed4c9452f56f9d81f8e1c8622..1f92c3f5a8e8d2408110252e6825ac3d479b6507 100644 (file)
--- 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 add100aeb30ae950fc2c61a8628ba8dd53e1d020..694f62eda891b4f3dfda7ba1223fb0b0963e73fb 100644 (file)
--- 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 cf4d96b230c44e9cf30e3a54c3352b01bceaab9e..f85666e34f653b35a671858f9d27a150c6b2a287 100755 (executable)
--- 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 ad04a1cb037ce3b928903be8d761636b7627fc77..96810588eeae0b0a57fcc048c4c2b50344a8d801 100755 (executable)
--- 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 echo hi 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