Re: env+nice, one bug fix, two test corrections

[Eric Blake]

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 <address@hidden>
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