Libguestfs - Apr 2020 - [nbdkit PATCH 2/2] server: Sanitize stdin/out before running plugin code

This is what I've been playing with in response to my earlier question
about what to do with 'nbdkit -s sh -'
(https://www.redhat.com/archives/libguestfs/2020-April/msg00032.html)

I'm still open to ideas on a better name, and/or whether adding
<stdbool.h> to our public include files is a good idea (if not,
returning int instead of bool is tolerable).

Eric Blake (2):
  server: Add nbdkit_stdio_safe
  server: Sanitize stdin/out before running plugin code

 docs/nbdkit-plugin.pod          | 23 +++++++++++++++++++-
 plugins/sh/nbdkit-sh-plugin.pod |  4 +++-
 include/nbdkit-common.h         |  2 ++
 server/internal.h               |  2 ++
 server/background.c             | 12 ++++-------
 server/captive.c                | 10 +++++++--
 server/connections.c            | 12 -----------
 server/main.c                   | 38 ++++++++++++++++++++++++++++++---
 server/nbdkit.syms              |  1 +
 server/public.c                 | 18 +++++++++++++++-
 server/test-public.c            | 23 ++++++++++++++++++--
 plugins/sh/sh.c                 |  7 +++++-
 tests/test-layers-plugin.c      | 12 ++++++++++-
 tests/test-layers.c             |  4 +++-
 14 files changed, 135 insertions(+), 33 deletions(-)

-- 
2.26.0.rc2

Trying to read a password or inline script from stdin when the -s
option was used to connect to our client over stdin is not going to
work.  It would be nice to fail such usage as soon as possible, by
giving plugins a means to decide if it is safe to use stdio during
.config.  Update nbdkit_read_password and the sh plugin to take
advantage of the new entry point.

Also, treat 'nbdkit -s --dump-plugin' as an error, as .dump_plugin is
supposed to interact with stdout.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/nbdkit-plugin.pod          | 23 ++++++++++++++++++++++-
 plugins/sh/nbdkit-sh-plugin.pod |  4 +++-
 include/nbdkit-common.h         |  2 ++
 server/main.c                   |  5 +++--
 server/nbdkit.syms              |  1 +
 server/public.c                 | 18 +++++++++++++++++-
 server/test-public.c            | 23 +++++++++++++++++++++--
 plugins/sh/sh.c                 |  7 ++++++-
 8 files changed, 75 insertions(+), 8 deletions(-)

diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 6c1311eb..822e19df 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -388,6 +388,10 @@ should probably look at other plugins and follow the same
conventions.
 If the value is a relative path, then note that the server changes
 directory when it starts up.  See L</FILENAMES AND PATHS> above.

+If C<nbdkit_stdio_safe> returns true, the value of the configuration
+parameter may be used to trigger reading additional data through stdin
+(such as a password or inline script).
+
 If the C<.config> callback is not provided by the plugin, and the user
 tries to specify any C<key=value> arguments, then nbdkit will exit
 with an error.
@@ -1148,6 +1152,23 @@ C<str> can be a string containing a
case-insensitive form of various
 common toggle values.  The function returns 0 or 1 if the parse was
 successful.  If there was an error, it returns C<-1>.

+=head2 Interacting with stdin and stdout
+
+Use the C<nbdkit_stdio_safe> utility function to determine if it is
+safe to interact with stdin and stdout during C<.config>.  When nbdkit
+is started under the single connection mode (C<-s>), the plugin must
+not directly interact with stdin, because that would interfere with
+the client.  The result of this function only matters prior to
+C<.config_complete>; once nbdkit reaches C<.get_ready>, the plugin
+should assume that nbdkit may have closed the original stdin and
+stdout in order to become a daemon.
+
+ bool nbdkit_stdio_safe (void);
+
+As an example, the L<nbdkit-sh-plugin(3)> uses this function to
+determine whether it is safe to support C<script=-> to read a script
+from stdin.
+
 =head2 Reading passwords

 The C<nbdkit_read_password> utility function can be used to read
@@ -1180,7 +1201,7 @@ used directly on the command line, eg:
  nbdkit myplugin password=mostsecret

 But more securely this function can also read a password
-interactively:
+interactively (if C<nbdkit_stdio_safe> returns true):

  nbdkit myplugin password=-

diff --git a/plugins/sh/nbdkit-sh-plugin.pod b/plugins/sh/nbdkit-sh-plugin.pod
index ffd0310f..20a2b785 100644
--- a/plugins/sh/nbdkit-sh-plugin.pod
+++ b/plugins/sh/nbdkit-sh-plugin.pod
@@ -48,7 +48,9 @@ as the name of the script, like this:
  EOF

 By default the inline script runs under F</bin/sh>.  You can add a
-shebang (C<#!>) to use other scripting languages.
+shebang (C<#!>) to use other scripting languages.  Of course, reading
+an inline script from stdin is incompatible with the C<-s>
+(C<--single>) mode of nbdkit that connects a client on stdin.

 =head1 WRITING AN NBDKIT SH PLUGIN

diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h
index 9d1d89d0..9b10500a 100644
--- a/include/nbdkit-common.h
+++ b/include/nbdkit-common.h
@@ -38,6 +38,7 @@
 #endif

 #include <stdarg.h>
+#include <stdbool.h>
 #include <stdint.h>
 #include <errno.h>
 #include <sys/socket.h>
@@ -106,6 +107,7 @@ extern int nbdkit_parse_int64_t (const char *what, const
char *str,
                                  int64_t *r);
 extern int nbdkit_parse_uint64_t (const char *what, const char *str,
                                   uint64_t *r);
+extern bool nbdkit_stdio_safe (void);
 extern int nbdkit_read_password (const char *value, char **password);
 extern char *nbdkit_realpath (const char *path);
 extern int nbdkit_nanosleep (unsigned sec, unsigned nsec);
diff --git a/server/main.c b/server/main.c
index 62598b81..748122fc 100644
--- a/server/main.c
+++ b/server/main.c
@@ -529,12 +529,13 @@ main (int argc, char *argv[])
       (port && listen_stdin) ||
       (unixsocket && listen_stdin) ||
       (listen_stdin && run) ||
+      (listen_stdin && dump_plugin) ||
       (vsock && unixsocket) ||
       (vsock && listen_stdin) ||
       (vsock && run)) {
     fprintf (stderr,
-             "%s: -p, --run, -s, -U or --vsock options cannot be used
"
-             "in this combination\n",
+             "%s: --dump-plugin, -p, --run, -s, -U or --vsock options
"
+             "cannot be used in this combination\n",
              program_name);
     exit (EXIT_FAILURE);
   }
diff --git a/server/nbdkit.syms b/server/nbdkit.syms
index 111223f2..20c390a9 100644
--- a/server/nbdkit.syms
+++ b/server/nbdkit.syms
@@ -65,6 +65,7 @@
     nbdkit_realpath;
     nbdkit_set_error;
     nbdkit_shutdown;
+    nbdkit_stdio_safe;
     nbdkit_vdebug;
     nbdkit_verror;

diff --git a/server/public.c b/server/public.c
index 3fd11253..f19791bc 100644
--- a/server/public.c
+++ b/server/public.c
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2020 Red Hat Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -404,6 +404,13 @@ nbdkit_parse_bool (const char *str)
   return -1;
 }

+/* Return true if it is safe to read from stdin during configuration. */
+bool
+nbdkit_stdio_safe (void)
+{
+  return !listen_stdin;
+}
+
 /* Read a password from configuration value. */
 static int read_password_from_fd (const char *what, int fd, char **password);

@@ -419,6 +426,11 @@ nbdkit_read_password (const char *value, char **password)

   /* Read from stdin. */
   if (strcmp (value, "-") == 0) {
+    if (!nbdkit_stdio_safe ()) {
+      nbdkit_error ("stdin is not available for reading password");
+      return -1;
+    }
+
     printf ("password: ");

     /* Set no echo. */
@@ -455,6 +467,10 @@ nbdkit_read_password (const char *value, char **password)

     if (nbdkit_parse_int ("password file descriptor", &value[1],
&fd) == -1)
       return -1;
+    if (!nbdkit_stdio_safe () && fd < STDERR_FILENO) {
+      nbdkit_error ("stdin/out is not available for reading
password");
+      return -1;
+    }
     if (read_password_from_fd (&value[1], fd, password) == -1)
       return -1;
   }
diff --git a/server/test-public.c b/server/test-public.c
index fe347d44..d4903420 100644
--- a/server/test-public.c
+++ b/server/test-public.c
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2018-2019 Red Hat Inc.
+ * Copyright (C) 2018-2020 Red Hat Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -53,6 +53,8 @@ nbdkit_error (const char *fs, ...)
   error_flagged = true;
 }

+bool listen_stdin;
+
 volatile int quit;
 int quit_fd = -1;

@@ -427,7 +429,24 @@ test_nbdkit_read_password (void)
     pass = false;
   }

-  /* XXX Testing reading from stdin would require setting up a pty */
+  /* XXX Testing reading from stdin would require setting up a pty. But
+   * we can test that it is forbidden with -s.
+   */
+  listen_stdin = true;
+  if (nbdkit_read_password ("-", &pw) != -1) {
+    fprintf (stderr, "Failed to diagnose failed password from stdin with
-s\n");
+    pass = false;
+  }
+  else if (pw != NULL) {
+    fprintf (stderr, "Failed to set password to NULL on failure\n");
+    pass = false;
+  }
+  else if (!error_flagged) {
+    fprintf (stderr, "Wrong error message handling\n");
+    pass = false;
+  }
+  error_flagged = false;
+
   return pass;
 }

diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
index 736b8ef0..c8a321f1 100644
--- a/plugins/sh/sh.c
+++ b/plugins/sh/sh.c
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2018-2019 Red Hat Inc.
+ * Copyright (C) 2018-2020 Red Hat Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -116,6 +116,11 @@ inline_script (void)
   char *filename = NULL;
   CLEANUP_FREE char *cmd = NULL;

+  if (!nbdkit_stdio_safe ()) {
+    nbdkit_error ("inline script is incompatible with -s");
+    return NULL;
+  }
+
   if (asprintf (&filename, "%s/%s", tmpdir, scriptname) == -1) {
     nbdkit_error ("asprintf: %m");
     goto err;
-- 
2.26.0.rc2

As shown in the previous patch, plugins may choose to use stdin or
stdout during .config.  But from .get_ready onwards, well-written
plugins shouldn't be needing any further use of stdin/out.  We already
swapped stdin/out to /dev/null while daemonizing, but did not do do
during -f or --run, which leads to some surprising inconsistency when
trying to debug a plugin that works in the foreground but fails when
daemonized.  What's more, while the task executed by --run should use
the original stdin/out (and we even have tests that would fail without
this), we don't want the plugin to interfere with whatever --run is
doing.  So it's time to move the swap over to /dev/null into a central
location, right before .get_ready.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 server/internal.h          |  2 ++
 server/background.c        | 12 ++++--------
 server/captive.c           | 10 ++++++++--
 server/connections.c       | 12 ------------
 server/main.c              | 33 ++++++++++++++++++++++++++++++++-
 tests/test-layers-plugin.c | 12 +++++++++++-
 tests/test-layers.c        |  4 +++-
 7 files changed, 60 insertions(+), 25 deletions(-)

diff --git a/server/internal.h b/server/internal.h
index 67eb6a32..79e1906c 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -132,6 +132,8 @@ extern bool tls_verify_peer;
 extern char *unixsocket;
 extern const char *user, *group;
 extern bool verbose;
+extern int orig_in;
+extern int orig_out;

 /* Linked list of backends.  Each backend struct is followed by either
  * a filter or plugin struct.  "top" points to the first one.  They
diff --git a/server/background.c b/server/background.c
index 531ba470..8388d9c8 100644
--- a/server/background.c
+++ b/server/background.c
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2019 Red Hat Inc.
+ * Copyright (C) 2019-2020 Red Hat Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -70,13 +70,9 @@ fork_into_background (void)
   chdir ("/");
 #pragma GCC diagnostic pop

-  /* Close stdin/stdout and redirect to /dev/null. */
-  close (0);
-  close (1);
-  open ("/dev/null", O_RDONLY);
-  open ("/dev/null", O_WRONLY);
-
-  /* If not verbose, set stderr to the same as stdout as well. */
+  /* By this point, stdin/out have been redirected to /dev/null.
+   * If not verbose, set stderr to the same as stdout as well.
+   */
   if (!verbose)
     dup2 (1, 2);

diff --git a/server/captive.c b/server/captive.c
index 0a243d2b..1ff10192 100644
--- a/server/captive.c
+++ b/server/captive.c
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2019 Red Hat Inc.
+ * Copyright (C) 2019-2020 Red Hat Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -151,7 +151,13 @@ run_command (void)
   }

   if (pid > 0) {              /* Parent process is the run command. */
-    r = system (cmd);
+    /* Restore original stdin/out */
+    if (dup2 (orig_in, STDIN_FILENO) == -1 ||
+        dup2 (orig_out, STDOUT_FILENO) == -1) {
+      r = -1;
+    }
+    else
+      r = system (cmd);
     if (r == -1) {
       nbdkit_error ("failure to execute external command: %m");
       r = EXIT_FAILURE;
diff --git a/server/connections.c b/server/connections.c
index c54c71c1..c7b55ca1 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -335,18 +335,6 @@ free_connection (struct connection *conn)
     return;

   conn->close ();
-  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 748122fc..596dd306 100644
--- a/server/main.c
+++ b/server/main.c
@@ -101,6 +101,8 @@ const char *user, *group;       /* -u & -g */
 bool verbose;                   /* -v */
 bool vsock;                     /* --vsock */
 unsigned int socket_activation  /* $LISTEN_FDS and $LISTEN_PID set */;
+int orig_in = -1;               /* dup'd stdin during -s/--run */
+int orig_out = -1;              /* dup'd stdout during -s/--run */

 /* The linked list of zero or more filters, and one plugin. */
 struct backend *top;
@@ -694,6 +696,35 @@ main (int argc, char *argv[])

   top->config_complete (top);

+  /* Sanitize stdin/stdout to /dev/null, after saving the originals
+   * when needed.  We are still single-threaded at this point, and
+   * already checked that stdin/out were open, so we don't have to
+   * worry about other threads accidentally grabbing our intended fds,
+   * or races on FD_CLOEXEC.
+   */
+  if (listen_stdin || run) {
+#ifndef F_DUPFD_CLOEXEC
+#define F_DUPFD_CLOEXEC F_DUPFD
+#endif
+    orig_in = fcntl (STDIN_FILENO, F_DUPFD_CLOEXEC, STDERR_FILENO + 1);
+    orig_out = fcntl (STDOUT_FILENO, F_DUPFD_CLOEXEC, STDERR_FILENO + 1);
+#if F_DUPFD == F_DUPFD_CLOEXEC
+    orig_in = set_cloexec (orig_in);
+    orig_out = set_cloexec (orig_out);
+#endif
+    if (orig_in == -1 || orig_out == -1) {
+      perror ("fcntl");
+      exit (EXIT_FAILURE);
+    }
+  }
+  close (STDIN_FILENO);
+  close (STDOUT_FILENO);
+  if (open ("/dev/null", O_RDONLY) != STDIN_FILENO ||
+      open ("/dev/null", O_WRONLY) != STDOUT_FILENO) {
+    perror ("open");
+    exit (EXIT_FAILURE);
+  }
+
   /* Select the correct thread model based on config. */
   lock_init_thread_model ();

@@ -918,7 +949,7 @@ start_serving (void)
     change_user ();
     write_pidfile ();
     threadlocal_new_server_thread ();
-    handle_single_connection (0, 1);
+    handle_single_connection (orig_in, orig_out);
     return;
   }

diff --git a/tests/test-layers-plugin.c b/tests/test-layers-plugin.c
index 8858bede..5a6b3020 100644
--- a/tests/test-layers-plugin.c
+++ b/tests/test-layers-plugin.c
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2018-2019 Red Hat Inc.
+ * Copyright (C) 2018-2020 Red Hat Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -36,6 +36,7 @@
 #include <stdlib.h>
 #include <stdint.h>
 #include <string.h>
+#include <unistd.h>

 #define NBDKIT_API_VERSION 2
 #include <nbdkit-plugin.h>
@@ -75,7 +76,16 @@ test_layers_plugin_config_complete (void)
 static int
 test_layers_plugin_get_ready (void)
 {
+  char c = 'X';
   DEBUG_FUNCTION;
+
+  /* Test that stdin/stdout have been sanitized */
+  if (write (STDOUT_FILENO, &c, 1) != 1 ||
+      read (STDIN_FILENO, &c, 1) != 0) {
+    nbdkit_error ("unusual stdio behavior");
+    return -1;
+  }
+
   return 0;
 }

diff --git a/tests/test-layers.c b/tests/test-layers.c
index 33ae5a75..5bdce54e 100644
--- a/tests/test-layers.c
+++ b/tests/test-layers.c
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2018-2019 Red Hat Inc.
+ * Copyright (C) 2018-2020 Red Hat Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -125,6 +125,8 @@ main (int argc, char *argv[])
   if (pid == 0) {               /* Child. */
     dup2 (sfd[1], 0);
     dup2 (sfd[1], 1);
+    close (sfd[0]);
+    close (sfd[1]);
     close (pfd[0]);
     dup2 (pfd[1], 2);
     close (pfd[1]);
-- 
2.26.0.rc2

On Sat, Apr 04, 2020 at 05:02:37PM -0500, Eric Blake
wrote:
> This is what I've been playing with in response to my earlier question
> about what to do with 'nbdkit -s sh -'
> (https://www.redhat.com/archives/libguestfs/2020-April/msg00032.html)
> 
> I'm still open to ideas on a better name, and/or whether adding
> <stdbool.h> to our public include files is a good idea (if not,
> returning int instead of bool is tolerable).
The answer is: no, we cannot use <stdbool.h>.  This is because we
advertise that we support using the headers in plugins which are pure
C90 code.

We also test this -- see tests/Makefile.am test-ansi-c-plugin.la --
however it may be that the test doesn't work properly if it didn't
fail after your change.

The solution is quite simple - return an int instead.
Cf. nbdkit_parse_bool

Rich.
> Eric Blake (2):
>   server: Add nbdkit_stdio_safe
>   server: Sanitize stdin/out before running plugin code
> 
>  docs/nbdkit-plugin.pod          | 23 +++++++++++++++++++-
>  plugins/sh/nbdkit-sh-plugin.pod |  4 +++-
>  include/nbdkit-common.h         |  2 ++
>  server/internal.h               |  2 ++
>  server/background.c             | 12 ++++-------
>  server/captive.c                | 10 +++++++--
>  server/connections.c            | 12 -----------
>  server/main.c                   | 38 ++++++++++++++++++++++++++++++---
>  server/nbdkit.syms              |  1 +
>  server/public.c                 | 18 +++++++++++++++-
>  server/test-public.c            | 23 ++++++++++++++++++--
>  plugins/sh/sh.c                 |  7 +++++-
>  tests/test-layers-plugin.c      | 12 ++++++++++-
>  tests/test-layers.c             |  4 +++-
>  14 files changed, 135 insertions(+), 33 deletions(-)
> 
> -- 
> 2.26.0.rc2
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

nbdkit_stdio_safe should return an int, but apart from that the rest
of the patch looked fine to me, except for:

On Sat, Apr 04, 2020 at 05:02:38PM -0500, Eric Blake
wrote:
> Also, treat 'nbdkit -s --dump-plugin' as an error, as .dump_plugin
is
> supposed to interact with stdout.
> diff --git a/server/main.c b/server/main.c
> index 62598b81..748122fc 100644
> --- a/server/main.c
> +++ b/server/main.c
> @@ -529,12 +529,13 @@ main (int argc, char *argv[])
>        (port && listen_stdin) ||
>        (unixsocket && listen_stdin) ||
>        (listen_stdin && run) ||
> +      (listen_stdin && dump_plugin) ||
>        (vsock && unixsocket) ||
>        (vsock && listen_stdin) ||
>        (vsock && run)) {
>      fprintf (stderr,
> -             "%s: -p, --run, -s, -U or --vsock options cannot be used
"
> -             "in this combination\n",
> +             "%s: --dump-plugin, -p, --run, -s, -U or --vsock options
"
> +             "cannot be used in this combination\n",
>               program_name);
>      exit (EXIT_FAILURE);
>    }
I feel this would be better as a separate commit.  It seems incidental
to the main thrust of this change.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On Sat, Apr 04, 2020 at 05:02:39PM -0500, Eric Blake
wrote:
> As shown in the previous patch, plugins may choose to use stdin or
> stdout during .config.  But from .get_ready onwards, well-written
> plugins shouldn't be needing any further use of stdin/out.  We already
> swapped stdin/out to /dev/null while daemonizing, but did not do do
> during -f or --run, which leads to some surprising inconsistency when
> trying to debug a plugin that works in the foreground but fails when
> daemonized.  What's more, while the task executed by --run should use
> the original stdin/out (and we even have tests that would fail without
> this), we don't want the plugin to interfere with whatever --run is
> doing.  So it's time to move the swap over to /dev/null into a central
> location, right before .get_ready.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  server/internal.h          |  2 ++
>  server/background.c        | 12 ++++--------
>  server/captive.c           | 10 ++++++++--
>  server/connections.c       | 12 ------------
>  server/main.c              | 33 ++++++++++++++++++++++++++++++++-
>  tests/test-layers-plugin.c | 12 +++++++++++-
>  tests/test-layers.c        |  4 +++-
>  7 files changed, 60 insertions(+), 25 deletions(-)
> 
> diff --git a/server/internal.h b/server/internal.h
> index 67eb6a32..79e1906c 100644
> --- a/server/internal.h
> +++ b/server/internal.h
> @@ -132,6 +132,8 @@ extern bool tls_verify_peer;
>  extern char *unixsocket;
>  extern const char *user, *group;
>  extern bool verbose;
> +extern int orig_in;
> +extern int orig_out;
> 
>  /* Linked list of backends.  Each backend struct is followed by either
>   * a filter or plugin struct.  "top" points to the first one. 
They
> diff --git a/server/background.c b/server/background.c
> index 531ba470..8388d9c8 100644
> --- a/server/background.c
> +++ b/server/background.c
> @@ -1,5 +1,5 @@
>  /* nbdkit
> - * Copyright (C) 2019 Red Hat Inc.
> + * Copyright (C) 2019-2020 Red Hat Inc.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -70,13 +70,9 @@ fork_into_background (void)
>    chdir ("/");
>  #pragma GCC diagnostic pop
> 
> -  /* Close stdin/stdout and redirect to /dev/null. */
> -  close (0);
> -  close (1);
> -  open ("/dev/null", O_RDONLY);
> -  open ("/dev/null", O_WRONLY);
> -
> -  /* If not verbose, set stderr to the same as stdout as well. */
> +  /* By this point, stdin/out have been redirected to /dev/null.
> +   * If not verbose, set stderr to the same as stdout as well.
> +   */
>    if (!verbose)
>      dup2 (1, 2);
> 
> diff --git a/server/captive.c b/server/captive.c
> index 0a243d2b..1ff10192 100644
> --- a/server/captive.c
> +++ b/server/captive.c
> @@ -1,5 +1,5 @@
>  /* nbdkit
> - * Copyright (C) 2019 Red Hat Inc.
> + * Copyright (C) 2019-2020 Red Hat Inc.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -151,7 +151,13 @@ run_command (void)
>    }
> 
>    if (pid > 0) {              /* Parent process is the run command. */
> -    r = system (cmd);
> +    /* Restore original stdin/out */
> +    if (dup2 (orig_in, STDIN_FILENO) == -1 ||
> +        dup2 (orig_out, STDOUT_FILENO) == -1) {
> +      r = -1;
> +    }
> +    else
> +      r = system (cmd);
>      if (r == -1) {
>        nbdkit_error ("failure to execute external command: %m");
>        r = EXIT_FAILURE;
> diff --git a/server/connections.c b/server/connections.c
> index c54c71c1..c7b55ca1 100644
> --- a/server/connections.c
> +++ b/server/connections.c
> @@ -335,18 +335,6 @@ free_connection (struct connection *conn)
>      return;
> 
>    conn->close ();
> -  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 748122fc..596dd306 100644
> --- a/server/main.c
> +++ b/server/main.c
> @@ -101,6 +101,8 @@ const char *user, *group;       /* -u & -g */
>  bool verbose;                   /* -v */
>  bool vsock;                     /* --vsock */
>  unsigned int socket_activation  /* $LISTEN_FDS and $LISTEN_PID set */;
> +int orig_in = -1;               /* dup'd stdin during -s/--run */
> +int orig_out = -1;              /* dup'd stdout during -s/--run */
> 
>  /* The linked list of zero or more filters, and one plugin. */
>  struct backend *top;
> @@ -694,6 +696,35 @@ main (int argc, char *argv[])
> 
>    top->config_complete (top);
> 
> +  /* Sanitize stdin/stdout to /dev/null, after saving the originals
> +   * when needed.  We are still single-threaded at this point, and
> +   * already checked that stdin/out were open, so we don't have to
> +   * worry about other threads accidentally grabbing our intended fds,
> +   * or races on FD_CLOEXEC.
> +   */
> +  if (listen_stdin || run) {
> +#ifndef F_DUPFD_CLOEXEC
> +#define F_DUPFD_CLOEXEC F_DUPFD
> +#endif
> +    orig_in = fcntl (STDIN_FILENO, F_DUPFD_CLOEXEC, STDERR_FILENO + 1);
> +    orig_out = fcntl (STDOUT_FILENO, F_DUPFD_CLOEXEC, STDERR_FILENO + 1);
> +#if F_DUPFD == F_DUPFD_CLOEXEC
> +    orig_in = set_cloexec (orig_in);
> +    orig_out = set_cloexec (orig_out);
> +#endif
> +    if (orig_in == -1 || orig_out == -1) {
> +      perror ("fcntl");
> +      exit (EXIT_FAILURE);
> +    }
> +  }
> +  close (STDIN_FILENO);
> +  close (STDOUT_FILENO);
> +  if (open ("/dev/null", O_RDONLY) != STDIN_FILENO ||
> +      open ("/dev/null", O_WRONLY) != STDOUT_FILENO) {
> +    perror ("open");
> +    exit (EXIT_FAILURE);
> +  }
> +
>    /* Select the correct thread model based on config. */
>    lock_init_thread_model ();
> 
> @@ -918,7 +949,7 @@ start_serving (void)
>      change_user ();
>      write_pidfile ();
>      threadlocal_new_server_thread ();
> -    handle_single_connection (0, 1);
> +    handle_single_connection (orig_in, orig_out);
>      return;
>    }
All the way down to here, we're all fine, ACK.

Can the part below be a separate test rather than overloading the
existing (layers) test?  I wonder if it could be done fairly easily
using a test with sh or eval plugin.

It would also be nice to have tests of -s and --run and that their
stdin/stdout behaviour is still correct: for example that the --run
subscript is connected to the same stdin/stdout/stderr as the parent
process (although obviously that's a bunch more work).
> diff --git a/tests/test-layers-plugin.c b/tests/test-layers-plugin.c
> index 8858bede..5a6b3020 100644
> --- a/tests/test-layers-plugin.c
> +++ b/tests/test-layers-plugin.c
> @@ -1,5 +1,5 @@
>  /* nbdkit
> - * Copyright (C) 2018-2019 Red Hat Inc.
> + * Copyright (C) 2018-2020 Red Hat Inc.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -36,6 +36,7 @@
>  #include <stdlib.h>
>  #include <stdint.h>
>  #include <string.h>
> +#include <unistd.h>
> 
>  #define NBDKIT_API_VERSION 2
>  #include <nbdkit-plugin.h>
> @@ -75,7 +76,16 @@ test_layers_plugin_config_complete (void)
>  static int
>  test_layers_plugin_get_ready (void)
>  {
> +  char c = 'X';
>    DEBUG_FUNCTION;
> +
> +  /* Test that stdin/stdout have been sanitized */
> +  if (write (STDOUT_FILENO, &c, 1) != 1 ||
> +      read (STDIN_FILENO, &c, 1) != 0) {
> +    nbdkit_error ("unusual stdio behavior");
> +    return -1;
> +  }
> +
>    return 0;
>  }
> 
> diff --git a/tests/test-layers.c b/tests/test-layers.c
> index 33ae5a75..5bdce54e 100644
> --- a/tests/test-layers.c
> +++ b/tests/test-layers.c
> @@ -1,5 +1,5 @@
>  /* nbdkit
> - * Copyright (C) 2018-2019 Red Hat Inc.
> + * Copyright (C) 2018-2020 Red Hat Inc.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -125,6 +125,8 @@ main (int argc, char *argv[])
>    if (pid == 0) {               /* Child. */
>      dup2 (sfd[1], 0);
>      dup2 (sfd[1], 1);
> +    close (sfd[0]);
> +    close (sfd[1]);
>      close (pfd[0]);
>      dup2 (pfd[1], 2);
>      close (pfd[1]);
Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On 4/7/20 2:55 AM, Richard W.M. Jones wrote:
> On Sat, Apr 04, 2020 at 05:02:37PM -0500, Eric Blake wrote:
>> This is what I've been playing with in response to my earlier
question
>> about what to do with 'nbdkit -s sh -'
>> (https://www.redhat.com/archives/libguestfs/2020-April/msg00032.html)
>>
>> I'm still open to ideas on a better name, and/or whether adding
>> <stdbool.h> to our public include files is a good idea (if not,
>> returning int instead of bool is tolerable).
> 
> The answer is: no, we cannot use <stdbool.h>.  This is because we
> advertise that we support using the headers in plugins which are pure
> C90 code.
> 
> We also test this -- see tests/Makefile.am test-ansi-c-plugin.la --
> however it may be that the test doesn't work properly if it didn't
> fail after your change.
Interesting - even with -std=c90 -pedantic, gcc is NOT flagging the use 
of <stdbool.h> as a problem.  Maybe we should open a glibc bug 
requesting that it be taught to make <stdbool.h> error out if compiler 
witness macros indicate it is being sucked in during pedantic c90 
compilation.  But it was easy enough to strengthen the test (now committed):

diff --git c/tests/test-ansi-c-plugin.c w/tests/test-ansi-c-plugin.c
index ff0b34d0..140d29ce 100644
--- c/tests/test-ansi-c-plugin.c
+++ w/tests/test-ansi-c-plugin.c
@@ -36,6 +36,16 @@

  #include <nbdkit-plugin.h>

+/* C90 lacks <stdbool.h>, and even in C99 and later, we are permitted
+ * to use 'bool', 'true', and 'false' however we want
if that header
+ * is not included.  Prove that nbdkit did not pollute the namespace.
+ */
+#if __bool_true_false_are_defined
+#error nbdkit polluted namespace with stdbool.h
+#else
+extern int true, false;
+#endif
+
  /* This looks like a 100MB disk with one empty partition. */
  static unsigned char bootsector[512] = {
    0xfa, 0xb8, 0x00, 0x10, 0x8e, 0xd0, 0xbc, 0x00,



Also, the term 'ANSI C' is now ambiguous: newer ANSI has moved to 
include C11 by reference, so you have to be clear when you mean ANSI 
C89/ISO C90. 
https://stackoverflow.com/questions/1608318/is-bool-a-native-c-type


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org