Libguestfs - Aug 2019 - [nbdkit PATCH] server: Enforce sane stdin/out/err

Refuse to run if stdin/out/err start life closed.  stdin/out are
essential when using -s (if they aren't present, we want to fail
fast), and should remain unused otherwise (but ensuring they are open
means we don't have to worry about other fd creation events
accidentally colliding).  We also want to ensure that nothing opens
into the slot for stderr, as any error message we produce might then
corrupt the wrong file.

https://git.savannah.gnu.org/cgit/gnulib.git/tree/doc/xstdopen.texi is
an interesting read on the matters of dealing with closed fds; but as
we are unable to use gnulib for licensing reasons, we'll just roll our
own solution.

Note that checking that we have valid fds on entry covers most cases,
but there is still one more situation to worry about (in fact, the one
that triggered this patch in the first place), namely, when -s implies
that our socket is fd 0 and 1, but we are executing cleanup code after
closing the connection to the client.  For a simple script ("case $1
in get_size) echo 1m; *) exit 2;; esac"), the following program would
fail an assertion, because the .close callback of the sh plugin ended
up with a pipe() call unexpectedly re-establishing stdin/out in the
parent process:

$ nbdsh -c 'h.connect_command
(["nbdkit","sh","script",
            "-s","--exit-with-parent"])' -c
'print("%r"%h.get_size())'
1048576
nbdkit: call.c:155: call3: Assertion `in_fd[0] > STDERR_FILENO &&
in_fd[1] > STDERR_FILENO && out_fd[0] > STDERR_FILENO &&
out_fd[1] > STDERR_FILENO && err_fd[0] > STDERR_FILENO &&
err_fd[1] > STDERR_FILENO' failed.

With this patch, the assertion can remain in place.

Reported-by:  Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---

This is my counter-proposal patch which leaves the assertion in the sh
plugin untouched, without any fd shuffling, by instead ensuring that
nbdkit never leaves 0/1/2 vacant.

 server/internal.h    |  1 +
 server/connections.c | 12 ++++++++++++
 server/main.c        | 11 +++++++++++
 3 files changed, 24 insertions(+)

diff --git a/server/internal.h b/server/internal.h
index 22e13b6d..a9692bbc 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -92,6 +92,7 @@ extern bool no_sr;
 extern const char *port;
 extern bool readonly;
 extern const char *run;
+extern bool listen_stdin;
 extern const char *selinux_label;
 extern int threads;
 extern int tls;
diff --git a/server/connections.c b/server/connections.c
index c173df8d..0184afea 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -366,6 +366,18 @@ free_connection (struct connection *conn)

   threadlocal_set_conn (NULL);
   conn->close (conn);
+  if (listen_stdin) {
+    int fd;
+
+    /* Restore something to stdin/out so the rest of our code can
+     * continue to assume that all new fds will be above stderr.
+     * Swap directions to get EBADF on improper use of stdin/out.
+     */
+    fd = open ("/dev/null", O_WRONLY | O_CLOEXEC);
+    assert (fd == 0);
+    fd = open ("/dev/null", O_RDONLY | O_CLOEXEC);
+    assert (fd == 1);
+  }

   /* Don't call the plugin again if quit has been set because the main
    * thread will be in the process of unloading it.  The plugin.unload
diff --git a/server/main.c b/server/main.c
index 65025a62..124e19b7 100644
--- a/server/main.c
+++ b/server/main.c
@@ -149,6 +149,17 @@ main (int argc, char *argv[])
   size_t i;
   const char *magic_config_key;

+  /* Refuse to run if stdin/out/err are closed, whether or not -s is used. */
+  if (fcntl (STDERR_FILENO, F_GETFL) == -1) {
+    /* Nowhere we can report the error. Oh well. */
+    exit (EXIT_FAILURE);
+  }
+  if (fcntl (STDIN_FILENO, F_GETFL) == -1 ||
+      fcntl (STDOUT_FILENO, F_GETFL) == -1) {
+    perror ("expecting stdin/stdout to be opened");
+    exit (EXIT_FAILURE);
+  }
+
   threadlocal_init ();

   /* The default setting for TLS depends on whether we were
-- 
2.21.0