redirect unused fds to /dev/null in pipestream

Closing an fd like stdout and stderr will cause EBADF on access
which may not be handled in a child process. Thus redirect them
to /dev/null if the user requests that their output be ignored.

diff --git a/src/pipestream.cpp b/src/pipestream.cpp
index 66ee39b..b9cdcf7 100644 (file)
--- a/src/pipestream.cpp
+++ b/src/pipestream.cpp
@@ -27,6 +27,7 @@ on this file might be covered by the GNU General Public License.
 #include <errno.h>
 #include <stdio.h>
 #include <string.h>
+#include <fcntl.h>
 #include <sys/wait.h>
 #include <unistd.h>
 
@@ -203,6 +204,34 @@ mk_argv (const std::vector<std::string> &command)
     return boost::shared_array<char *> (ret);
 }
 
+/** @brief      Helper for redirecting a file descriptor to \c /dev/null.
+ *              This will only acquire an fd the first time it is called
+ *              or if it is called after unsuccessfully attempting to
+ *              acquire one.
+ *
+ *  @param fd         The open file descriptor to operate on.
+ *
+ *  @returns          \c true on success, \c false otherwise (the call to
+ *                    either \c open(2) or \c dup2(2) failed).
+ */
+static bool
+redirect_devnull (const int fd)
+{
+    static int nullfd = -1;
+    
+    errno = 0;
+    if (nullfd == -1 && (nullfd = open ("/dev/null", O_RDWR)) == -1) {
+        return false;
+    }
+
+    errno = 0;
+    if (dup2 (nullfd, fd) == -1) {
+        return false;
+    }
+
+    return true;
+}
+
 /** @brief      Helper aggregating common code for the shell-free ctors.
  *
  *  @param argv       Argument list prepared for \c execve(2).
@@ -242,14 +271,20 @@ inpipebuf::init_without_shell (const char *const *argv,
             close (pipefd [0]);
 
             if (!out) {
-                close (STDOUT_FILENO);
+                if (!redirect_devnull (STDOUT_FILENO)) {
+                    fprintf(stderr, "redirect_devnull/stdout: %m\n");
+                    /* XXX should we bail here? */
+                }
             } else if (dup2 (pipefd[1], STDOUT_FILENO) == -1) {
                 fprintf(stderr, "dup2/stdout: %m\n");
                 exit(EXIT_FAILURE);
             }
 
             if (!err) {
-                close (STDERR_FILENO);
+                if (!redirect_devnull (STDERR_FILENO)) {
+                    fprintf(stderr, "redirect_devnull/stderr: %m\n");
+                    /* XXX should we bail here? */
+                }
             } else if (dup2 (pipefd[1], STDERR_FILENO) == -1) {
                 fprintf(stderr, "dup2/stderr: %m\n");
                 exit(EXIT_FAILURE);

diff --git a/test/test_pipestream.cpp b/test/test_pipestream.cpp
index bf7b068..1b4bf8e 100644 (file)
--- a/test/test_pipestream.cpp
+++ b/test/test_pipestream.cpp
@@ -112,6 +112,7 @@ BOOST_AUTO_TEST_SUITE(pipestream)
             const std::string result =
                     capture_exec (I2n::join_string (bad_command, " "));
 
+            BOOST_CHECK(!exres.normal_exit);
             BOOST_CHECK_EQUAL(result.size (), 0);
         }
 
@@ -122,6 +123,7 @@ BOOST_AUTO_TEST_SUITE(pipestream)
             ExecResult exres = ExecResult ();
             const std::string result = capture_exec (bad_command, exres);
 
+            BOOST_CHECK(exres.normal_exit);
             BOOST_CHECK(!exres.terminated_by_signal);
             BOOST_CHECK_EQUAL(result.size (), 0);
         }
@@ -133,7 +135,9 @@ BOOST_AUTO_TEST_SUITE(pipestream)
             ExecResult exres = ExecResult ();
             const std::string result = capture_exec (bad_command, exres, false, true);
 
+            BOOST_CHECK(exres.normal_exit);
             BOOST_CHECK(!exres.terminated_by_signal);
+            BOOST_CHECK_EQUAL(exres.return_code, EXIT_FAILURE);
             BOOST_CHECK_EQUAL(result.size (), 0);
         }
 
@@ -208,6 +212,85 @@ BOOST_AUTO_TEST_SUITE(pipestream)
             BOOST_CHECK_EQUAL(result.size (), 0);
         }
 
+        const char *const echo_abs = "/bin/echo";
+        const char *const echo_rel = "echo";
+
+        static std::vector<std::string>
+        mk_echo_argv (const std::string &text, const bool absolute=true)
+        {
+            std::vector<std::string> ret;
+
+            ret.push_back (absolute ? echo_abs : echo_rel);
+            ret.push_back ("-n");
+            ret.push_back (text);
+
+            return ret;
+        }
+
+        BOOST_AUTO_TEST_CASE(abspath_echo_noshell_capture_ok)
+        {
+            ExecResult exres = ExecResult ();
+            const std::string text = "The significant owl hoots in the night.";
+            const std::vector<std::string> argv = mk_echo_argv (text);
+            const std::string result = capture_exec (argv, exres, true, false, true);
+
+            BOOST_CHECK(exres.normal_exit);
+            BOOST_CHECK_EQUAL(exres.return_code, EXIT_SUCCESS);
+            BOOST_CHECK_EQUAL(result, text);
+        }
+
+        BOOST_AUTO_TEST_CASE(relpath_echo_noshell_capture_ok)
+        {
+            ExecResult exres = ExecResult ();
+            const std::string text = "Yet many grey lords go sadly to the masterless men.";
+            const std::vector<std::string> argv = mk_echo_argv (text, false);
+            const std::string result = capture_exec (argv, exres, true, false, true);
+
+            BOOST_CHECK(exres.normal_exit);
+            BOOST_CHECK_EQUAL(exres.return_code, EXIT_SUCCESS);
+            BOOST_CHECK_EQUAL(result, text);
+        }
+
+        static std::vector<std::string>
+        mk_errcho_argv (const std::string &text)
+        {
+            /*
+             * Hack cause there’s no way to make echo print to stderr without
+             * redirection.
+             */
+            std::vector<std::string> ret;
+
+            ret.push_back ("/bin/sh");
+            ret.push_back ("-c");
+            ret.push_back (std::string ("1>&- 1>&2 echo -n '") + text + "'"); /* brr */
+
+            return ret;
+        }
+
+        BOOST_AUTO_TEST_CASE(sh_errcho_capture_ok)
+        {
+            ExecResult exres = ExecResult ();
+            const std::string text = "Hooray, hooray for the spinster’s sister’s daughter.";
+            const std::vector<std::string> argv = mk_errcho_argv (text);
+            const std::string result = capture_exec (argv, exres, false, true, true);
+
+            BOOST_CHECK(exres.normal_exit);
+            BOOST_CHECK_EQUAL(exres.return_code, EXIT_SUCCESS);
+            BOOST_CHECK_EQUAL(result, text);
+        }
+
+        BOOST_AUTO_TEST_CASE(sh_errcho_stdout_empty_ok)
+        {
+            ExecResult exres = ExecResult ();
+            const std::string text = "To the axeman, all supplicants are the same height.";
+            const std::vector<std::string> argv = mk_errcho_argv (text);
+            const std::string result = capture_exec (argv, exres, true, false, true);
+
+            BOOST_CHECK(exres.normal_exit);
+            BOOST_CHECK_EQUAL(exres.return_code, EXIT_SUCCESS);
+            BOOST_CHECK_EQUAL(result.size (), 0);
+        }
+
     BOOST_AUTO_TEST_SUITE_END() /* [pipestream->read] */
 
 BOOST_AUTO_TEST_SUITE_END() /* [pipestream] */