11460 logadm docmd() can't handle too much on stderr

Reviewed by: Toomas Soome <tsoome@me.com>
Reviewed by: Jason King <jason.king@joyent.com>
Reviewed by: Andy Fiddaman <omnios@citrus-it.co.uk>
Approved by: Robert Mustacchi <rm@joyent.com>

2 files changed, 96 insertions, 37 deletions

diff --git a/usr/src/cmd/logadm/main.c b/usr/src/cmd/logadm/main.c
index 24eb9f8a38..624397d41f 100644
--- a/usr/src/cmd/logadm/main.c
+++ b/usr/src/cmd/logadm/main.c
@@ -20,7 +20,7 @@
  */
 /*
  * Copyright (c) 2001, 2010, Oracle and/or its affiliates. All rights reserved.
- * Copyright (c) 2013, Joyent, Inc. All rights reserved.
+ * Copyright 2019 Joyent, Inc.
  *
  * logadm/main.c -- main routines for logadm
  *
@@ -41,6 +41,8 @@
 #include <sys/sysmacros.h>
 #include <time.h>
 #include <utime.h>
+#include <poll.h>
+#include <errno.h>
 #include "err.h"
 #include "lut.h"
 #include "fn.h"
@@ -1030,16 +1032,11 @@ docmd(struct opts *opts, const char *msg, const char *cmd,
 		return;		/* -n means don't really do it */
 
 	/*
-	 * run the cmd and see if it failed.  this function is *not* a
-	 * generic command runner -- we depend on some knowledge we
-	 * have about the commands we run.  first of all, we expect
-	 * errors to spew something to stderr, and that something is
-	 * typically short enough to fit into a pipe so we can wait()
-	 * for the command to complete and then fetch the error text
-	 * from the pipe.  we also expect the exit codes to make sense.
-	 * notice also that we only allow a command name which is an
-	 * absolute pathname, and two args must be supplied (the
-	 * second may be NULL, or they may both be NULL).
+	 * Run the cmd and see if it failed.  This function is *not* a generic
+	 * command runner.  The command name must be an absolute pathname, and
+	 * two args must be supplied (the second may be NULL, or they may both
+	 * be NULL).  Any output (stdout and stderr) from the child process is
+	 * logged to stderr and perhaps sent to an email recipient.
 	 */
 	if (pipe(errpipe) < 0)
 		err(EF_SYS, "pipe");
@@ -1048,25 +1045,51 @@ docmd(struct opts *opts, const char *msg, const char *cmd,
 		err(EF_SYS, "fork");
 	else if (pid) {
 		int wstat;
-		int count;
+		struct pollfd pfd;
+		boolean_t first = B_TRUE;
 
 		/* parent */
 		(void) close(errpipe[1]);
-		if (waitpid(pid, &wstat, 0) < 0)
+
+		pfd.fd = errpipe[0];
+		pfd.events = POLLIN | POLLRDNORM | POLLRDBAND | POLLPRI;
+		for (;;) {
+
+			pfd.revents = 0;
+			if (poll(&pfd, 1, -1) == -1) {
+				if (errno == EINTR) {
+					continue;
+				}
+				err(EF_SYS, "poll");
+				break;
+			}
+			if ((pfd.events & pfd.revents) != 0) {
+				if (first) {
+					err(EF_WARN,
+					    "command failed: %s%s%s%s%s%s%s",
+					    cmd,
+					    (arg1) ? " " : "",
+					    (arg1) ? arg1 : "",
+					    (arg2) ? " " : "",
+					    (arg2) ? arg2 : "",
+					    (arg3) ? " " : "",
+					    (arg3) ? arg3 : "");
+					first = B_FALSE;
+				}
+				err_fromfd(pfd.fd);
+			}
+			if ((pfd.revents & (POLLERR | POLLHUP)) != 0) {
+				break;
+			}
+		}
+		if (waitpid(pid, &wstat, 0) < 0) {
 			err(EF_SYS, "waitpid");
+			return;
+		}
 
-		/* check for stderr output */
-		if (ioctl(errpipe[0], FIONREAD, &count) >= 0 && count) {
-			err(EF_WARN, "command failed: %s%s%s%s%s%s%s",
-			    cmd,
-			    (arg1) ? " " : "",
-			    (arg1) ? arg1 : "",
-			    (arg2) ? " " : "",
-			    (arg2) ? arg2 : "",
-			    (arg3) ? " " : "",
-			    (arg3) ? arg3 : "");
-			err_fromfd(errpipe[0]);
-		} else if (WIFSIGNALED(wstat))
+		if (!first) {
+			/* Assume the command gave a useful error */
+		} else if (WIFSIGNALED(wstat)) {
 			err(EF_WARN,
 			    "command died, signal %d: %s%s%s%s%s%s%s",
 			    WTERMSIG(wstat),
@@ -1077,7 +1100,7 @@ docmd(struct opts *opts, const char *msg, const char *cmd,
 			    (arg2) ? arg2 : "",
 			    (arg3) ? " " : "",
 			    (arg3) ? arg3 : "");
-		else if (WIFEXITED(wstat) && WEXITSTATUS(wstat))
+		} else if (WIFEXITED(wstat) && WEXITSTATUS(wstat)) {
 			err(EF_WARN,
 			    "command error, exit %d: %s%s%s%s%s%s%s",
 			    WEXITSTATUS(wstat),
@@ -1088,6 +1111,7 @@ docmd(struct opts *opts, const char *msg, const char *cmd,
 			    (arg2) ? arg2 : "",
 			    (arg3) ? " " : "",
 			    (arg3) ? arg3 : "");
+		}
 
 		(void) close(errpipe[0]);
 	} else {
diff --git a/usr/src/cmd/logadm/tester b/usr/src/cmd/logadm/tester
index 89dd978448..742bc03209 100644
--- a/usr/src/cmd/logadm/tester
+++ b/usr/src/cmd/logadm/tester
@@ -21,7 +21,7 @@
 #
 #
 # Copyright (c) 2001, 2010, Oracle and/or its affiliates. All rights reserved.
-# Copyright (c) 2013, Joyent, Inc. All rights reserved.
+# Copyright 2019 Joyent, Inc.
 #
 #
 # tester - run logadm tests
@@ -31,19 +31,19 @@
 # logadm itself).
 #
 # to run all the tests:
-# 	tester [-f] <bindir>
+#	tester [-f] <bindir>
 #
 # to run just a few tests, given their names:
-# 	tester [-f] <bindir> globtest1 luttest1
+#	tester [-f] <bindir> globtest1 luttest1
 #
 # to setup a test and stop so you can run it by hand:
-# 	tester [-f] -s globtest1 <bindir>
+#	tester [-f] -s globtest1 <bindir>
 #
-# 	tester will tell you what tmp directory it created for
-# 	the test.  to run it, cd there and run:
-# 		sh runtest
-# 	to check the results, run:
-# 		sh checktest
+#	tester will tell you what tmp directory it created for
+#	the test.  to run it, cd there and run:
+#		sh runtest
+#	to check the results, run:
+#		sh checktest
 #
 # -f means "fast" -- without it, watchmalloc(3MALLOC) is setup for
 # each test and they run a zillion times slower and produce core
@@ -92,7 +92,8 @@ umask 002;
 	"logadm20",
 	"logadm21",
 	"logadm22",
-	"logadm23"
+	"logadm23",
+	"stderr1"
 );
 
 use Getopt::Std;
@@ -275,7 +276,7 @@ sub set_testconffile {
 # Default settings for system log file management.
 # The -w option to logadm(1M) is the preferred way to write to this file,
 # but if you do edit it by hand, use "logadm -V" to check it for errors.
-# 
+#
 # The format of lines in this file is:
 #       <logname> <options>
 # For each logname listed here, the default options to logadm
@@ -2214,3 +2215,37 @@ exit 0
 EOF
 
 }
+
+###########################################################################
+#
+#	stderr1 -- ensure verbose stderr does not deadlock
+#
+###########################################################################
+sub stderr1 {
+	set_file('logfile', 'initially logfile');
+
+	set_file('std.err.uniq.expect', <<'EOF');
+   1 logadm: Warning: command failed: /bin/sh -c exec 1>&2; for i in {1..250000}; do echo pre-command-stuff; done
+250000 pre-command-stuff
+   1 logadm: Warning: command failed: /bin/sh -c exec 1>&2; for i in {1..250000}; do echo post-command-stuff; done
+250000 post-command-stuff
+EOF
+
+	set_file('checktest', <<'EOF');
+[ -s std.out ] && exit 1
+/bin/diff -u std.err.uniq.expect std.err.uniq || exit 1
+exit 0
+EOF
+
+	# The output redirection below looks wrong, but it is not.  The redirect
+	# of stderr before the redirect of stdout causes stderr to be piped to
+	# uniq.
+	set_file('runtest', <<"EOF");
+# test "stderr1"
+$envsetup
+exec $bindir/logadm -f /dev/null -p now logfile \\
+    -b 'exec 1>&2; for i in {1..250000}; do echo pre-command-stuff; done' \\
+    -a 'exec 1>&2; for i in {1..250000}; do echo post-command-stuff; done' \\
+    2>&1 >std.out | uniq -c >std.err.uniq
+EOF
+}