Libguestfs - Apr 2020 - [nbdkit PATCH v2 0/3] more consistent stdin/out handling

In v2:
- use int instead of bool in the public header
- split the tests from the code
- don't overload test-layers; instead, add new tests
- add a missing fflush exposed by the new tests
- other minor cleanups

Eric Blake (3):
  server: Add nbdkit_stdio_safe
  server: Sanitize stdin/out before running plugin code
  server: More tests of stdin/out handling

 docs/nbdkit-plugin.pod          |  23 ++++-
 plugins/sh/nbdkit-sh-plugin.pod |   4 +-
 include/nbdkit-common.h         |   1 +
 tests/Makefile.am               |  23 +++++
 server/internal.h               |   2 +
 server/background.c             |  14 +--
 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-single-sh.sh         |  78 +++++++++++++++
 tests/test-stdio.sh             |  95 ++++++++++++++++++
 tests/test-stdio-plugin.c       | 170 ++++++++++++++++++++++++++++++++
 16 files changed, 489 insertions(+), 30 deletions(-)
 create mode 100755 tests/test-single-sh.sh
 create mode 100755 tests/test-stdio.sh
 create mode 100644 tests/test-stdio-plugin.c

-- 
2.26.0

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.

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

diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 6c1311eb..a12ecfd5 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
+
+The C<nbdkit_stdio_safe> utility function returns non-zero 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.
+
+ int 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..47288050 100644
--- a/include/nbdkit-common.h
+++ b/include/nbdkit-common.h
@@ -106,6 +106,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 int 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/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..4bb1f2e0 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. */
+int
+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

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.

Note that if a plugin consumes some, but not all, input during
.config, then we want --run to pick up with reading the rest of the
input.  For that to happen, we need to flush any buffered read-ahead
data from stdin.  POSIX says fflush(NULL) is supposed to be sufficient
for the task, but in practice, glibc still has a bug that requires an
additional fflush(stdin).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 server/internal.h    |  2 ++
 server/background.c  | 14 +++++---------
 server/captive.c     | 10 ++++++++--
 server/connections.c | 12 ------------
 server/main.c        | 38 +++++++++++++++++++++++++++++++++++++-
 5 files changed, 52 insertions(+), 24 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..72ab1ef6 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,15 +70,11 @@ 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);
+    dup2 (STDOUT_FILENO, STDERR_FILENO);

   forked_into_background = true;
   debug ("forked into background (new pid = %d)", getpid ());
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 054e60b9..bbb416c3 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,40 @@ 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.  POSIX says that 'fflush(NULL)' is
+   * supposed to reset the underlying offset of seekable stdin, but
+   * glibc is buggy and requires an explicit fflush(stdin) as
+   * well. https://sourceware.org/bugzilla/show_bug.cgi?id=12799
+   */
+  fflush (stdin);
+  fflush (NULL);
+  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 +954,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;
   }

-- 
2.26.0

Enhance the testsuite to ensure we don't regress with recent changes
to stdin/out handling.  This adds:
- test-single-sh.sh: prove that 'nbdkit -s sh script' is viable
- test-stdio.sh: create plugin that checks stdin/out match /dev/null,
then run it with -s, --run, -f

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/Makefile.am         |  23 ++++++
 tests/test-single-sh.sh   |  78 +++++++++++++++++
 tests/test-stdio.sh       |  95 +++++++++++++++++++++
 tests/test-stdio-plugin.c | 170 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 366 insertions(+)
 create mode 100755 tests/test-single-sh.sh
 create mode 100755 tests/test-stdio.sh
 create mode 100644 tests/test-stdio-plugin.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 2aa95890..63d47fc9 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -189,6 +189,7 @@ EXTRA_DIST = \
 	test-retry-reopen-fail.sh \
 	test-retry-zero-flags.sh \
 	test-ssh.sh \
+	test-stdio.sh \
 	test-swap.sh \
 	test.tcl \
 	test-shebang-perl.sh \
@@ -200,6 +201,7 @@ EXTRA_DIST = \
 	test-sh-extents.sh \
 	test-single.sh \
 	test-single-from-file.sh \
+	test-single-sh.sh \
 	test-split-extents.sh \
 	test-start.sh \
 	test-random-sock.sh \
@@ -263,6 +265,8 @@ TESTS += \
 	test-start.sh \
 	test-single.sh \
 	test-single-from-file.sh \
+	test-single-sh.sh \
+	test-stdio.sh \
 	test-captive.sh \
 	test-random-sock.sh \
 	test-tls.sh \
@@ -289,6 +293,25 @@ test_socket_activation_CPPFLAGS = \
 	$(NULL)
 test_socket_activation_CFLAGS = $(WARNINGS_CFLAGS)

+# check_LTLIBRARIES won't build a shared library (see automake manual).
+# So we have to do this and add a dependency.
+noinst_LTLIBRARIES += \
+	test-stdio-plugin.la \
+	$(NULL)
+test-stdio.sh: test-stdio-plugin.la
+
+test_stdio_plugin_la_SOURCES = \
+	test-stdio-plugin.c \
+	$(top_srcdir)/include/nbdkit-plugin.h \
+	$(NULL)
+test_stdio_plugin_la_CPPFLAGS = -I$(top_srcdir)/include
+test_stdio_plugin_la_CFLAGS = $(WARNINGS_CFLAGS)
+# For use of the -rpath option, see:
+# https://lists.gnu.org/archive/html/libtool/2007-07/msg00067.html
+test_stdio_plugin_la_LDFLAGS = \
+	-module -avoid-version -shared $(SHARED_LDFLAGS) -rpath /nowhere \
+	$(NULL)
+
 # check_LTLIBRARIES won't build a shared library (see automake manual).
 # So we have to do this and add a dependency.
 noinst_LTLIBRARIES += \
diff --git a/tests/test-single-sh.sh b/tests/test-single-sh.sh
new file mode 100755
index 00000000..a1586868
--- /dev/null
+++ b/tests/test-single-sh.sh
@@ -0,0 +1,78 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2020 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+source ./functions.sh
+set -e
+set -x
+
+requires nbdsh --version
+
+files="single-sh.script single-sh.log"
+rm -f $files
+
+cleanup_fn rm -f $files
+
+fail=0
+# Inline scripts are incompatible with -s
+if nbdkit -s sh - >/dev/null <<EOF
+echo "oops: should not have run '$@'" >>single-sh.log
+EOF
+then
+    echo "$0: failed to diagnose -s vs. 'sh -'"
+    fail=1
+fi
+if test -f single-sh.log; then
+    echo "$0: script unexpectedly ran"
+    cat single-sh.log
+    fail=1
+fi
+
+cat >single-sh.script <<\EOF
+case $1 in
+  get_size) echo 1m ;;
+  pread) dd if=/dev/zero count=$3 iflag=count_bytes ;;
+  *) exit 2 ;;
+esac
+EOF
+chmod +x single-sh.script
+
+# The sh plugin sets up pipes to handle stdin/out per each run of the
+# script, but this is not incompatible with using -s for the client
+nbdsh -c '
+h.connect_command (["nbdkit", "-s", "sh",
"single-sh.script"])
+assert h.get_size() == 1024 * 1024
+buf1 = h.pread (512, 0)
+buf2 = bytearray (512)
+assert buf1 == buf2
+'
+
+exit $fail
diff --git a/tests/test-stdio.sh b/tests/test-stdio.sh
new file mode 100755
index 00000000..43913d66
--- /dev/null
+++ b/tests/test-stdio.sh
@@ -0,0 +1,95 @@
+#!/usr/bin/env bash
+# nbdkit
+# 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
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+source ./functions.sh
+set -xe
+
+requires nbdsh -c 'exit (not h.supports_uri ())'
+
+plugin=.libs/test-stdio-plugin.so
+requires test -f $plugin
+
+sock1=`mktemp -u`
+sock2=`mktemp -u`
+files="test-stdio.in test-stdio.out test-stdio.err
+  test-stdio.pid1 test-stdio.pid2 $sock1 $sock2"
+rm -f $files
+cleanup_fn rm -f $files
+
+# Using a seekable file lets us prove that if the plugin consumes less
+# than the full input, the next process sees the rest
+cat >test-stdio.in <<EOF
+string1
+string2
+EOF
+
+# .dump_plugin using stdout is normal; using stdin is odd, but viable
+{ nbdkit --dump-plugin $plugin; printf 'rest='; cat
+} < test-stdio.in > test-stdio.out
+grep "input=string1" test-stdio.out
+grep "rest=string2" test-stdio.out
+
+# Test with --run.
+nbdkit -v $plugin one=1 --run 'printf cmd=; cat' \
+    < test-stdio.in > test-stdio.out
+cat test-stdio.out
+grep "one=string1" test-stdio.out
+grep "cmd=string2" test-stdio.out
+
+# Test with -f; we have to repeat body of start_nbdkit ourselves
+echo "string" | nbdkit -P test-stdio.pid1 -v --filter=exitlast \
+    -f -U $sock1 $plugin two=2 | tee test-stdio.out & pid=$!
+for i in {1..60}; do
+    if test -s test-stdio.pid1; then
+        break
+    fi
+    sleep 1
+done
+if ! test -s test-stdio.pid1; then
+    echo "$0: PID file $pidfile was not created"
+    exit 1
+fi
+nbdsh -u "nbd+unix:///?socket=$sock1" -c 'buf = h.pread (512,
0)'
+wait $pid
+grep "two=string" test-stdio.out
+
+# Test as daemon
+echo "string" | start_nbdkit -P test-stdio.pid2 --filter=exitlast \
+    -U $sock2 $plugin three=3 | tee test-stdio.out
+nbdsh -u "nbd+unix:///?socket=$sock2" -c 'buf = h.pread (512,
0)'
+grep "three=string" test-stdio.out
+
+# Test with -s; here, the plugin produces no output.
+nbdsh -c '
+h.connect_command (["nbdkit", "-v", "-s",
"'$plugin'", "four=4"])
+buf = h.pread (512, 0)
+'
diff --git a/tests/test-stdio-plugin.c b/tests/test-stdio-plugin.c
new file mode 100644
index 00000000..6af1ba90
--- /dev/null
+++ b/tests/test-stdio-plugin.c
@@ -0,0 +1,170 @@
+/* nbdkit
+ * 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
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <config.h>
+
+#include <assert.h>
+#include <fcntl.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#define NBDKIT_API_VERSION 2
+
+#include <nbdkit-plugin.h>
+
+static const char *msg = "input";
+
+/* Check whether stdin/out match /dev/null */
+static bool
+stdio_check (void)
+{
+  static int dn = -1;
+  struct stat st1, st2;
+
+  if (dn == -1) {
+    dn = open ("/dev/null", O_RDONLY);
+    assert (dn > STDERR_FILENO);
+  }
+  if (fstat (dn, &st1) == -1)
+    assert (false);
+
+  if (fstat (STDIN_FILENO, &st2) == -1)
+    assert (false);
+  if (st1.st_dev != st2.st_dev || st1.st_ino != st2.st_ino)
+    return false;
+
+  if (fstat (STDOUT_FILENO, &st2) == -1)
+    assert (false);
+  if (st1.st_dev != st2.st_dev || st1.st_ino != st2.st_ino)
+    return false;
+
+  return true;
+}
+
+static void
+stdio_dump_plugin (void)
+{
+  char *buf = NULL;
+  size_t len = 0;
+  bool check = stdio_check ();
+
+  assert (check == false);
+
+  /* Reading from stdin during .dump_plugin is unusual, but not forbidden */
+  if (getline (&buf, &len, stdin) == -1)
+    assert (false);
+  /* The point of .dump_plugin is to extend details sent to stdout */
+  printf ("%s=%s\n", msg, buf);
+  free (buf);
+}
+
+static int
+stdio_config (const char *key, const char *value)
+{
+  bool check = stdio_check ();
+  assert (check == false);
+  msg = key;
+  return 0;
+}
+
+static int
+stdio_config_complete (void)
+{
+  bool check = stdio_check ();
+  assert (check == false);
+  if (nbdkit_stdio_safe ()) {
+    char *buf = NULL;
+    size_t len = 0;
+
+    /* Reading from stdin during .config_complete is safe except under -s */
+    if (getline (&buf, &len, stdin) == -1)
+      assert (false);
+    /* Output during .config_complete is unusual, but not forbidden */
+    printf ("%s=%s\n", msg, buf);
+    free (buf);
+  }
+  return 0;
+}
+
+static int
+stdio_get_ready (void)
+{
+  bool check = stdio_check ();
+  assert (check == true);
+  return 0;
+}
+
+static void *
+stdio_open (int readonly)
+{
+  bool check = stdio_check ();
+  assert (check == true);
+  return NBDKIT_HANDLE_NOT_NEEDED;
+}
+
+static int64_t
+stdio_get_size (void *handle)
+{
+  bool check = stdio_check ();
+  assert (check == true);
+  return 1024*1024;
+}
+
+#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
+
+static int
+stdio_pread (void *handle, void *buf, uint32_t count, uint64_t offset,
+             uint32_t flags)
+{
+  bool check = stdio_check ();
+  assert (check == true);
+  memset (buf, 0, count);
+  return 0;
+}
+
+static struct nbdkit_plugin plugin = {
+  .name              = "stdio",
+  .version           = PACKAGE_VERSION,
+  .dump_plugin       = stdio_dump_plugin,
+  .config            = stdio_config,
+  .config_complete   = stdio_config_complete,
+  .get_ready         = stdio_get_ready,
+  .open              = stdio_open,
+  .get_size          = stdio_get_size,
+  .pread             = stdio_pread,
+};
+
+NBDKIT_REGISTER_PLUGIN(plugin)
-- 
2.26.0

On Mon, Apr 13, 2020 at 07:28:59PM -0500, Eric Blake wrote:
[...]

This patch is fine and can be pushed if you want, but I've got some
small comments.
> +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).
I wonder if we want to say "returns 1" rather than true, to give
ourselves wiggle room in future in case we suddenly decided that we
needed this to return an error indication?  On the other hand, maybe
errors can never happen in any conceivable situation.
> @@ -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) {
I think this could be clearer written the other way around:

    if (fd < STDERR_FILENO && !nbdkit_stdio_safe ()) {

but then thinking about this more, why isn't it this?

    if (fd == STDIN_FILENO && !nbdkit_stdio_safe ()) {

Anyway, these are minor points, ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Mon, Apr 13, 2020 at 07:29:00PM -0500, Eric Blake
wrote:
> For that to happen, we need to flush any buffered read-ahead
> data from stdin.  POSIX says fflush(NULL) is supposed to be sufficient
> for the task, but in practice, glibc still has a bug that requires an
> additional fflush(stdin).
Is there a link to the bug?
> @@ -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;
Personally I think it would be clearer if these were called orig_stdin
& orig_stdout, or saved_stdin & saved_stdout.

But the patch overall looks fine, so ACK.

Rich.

-- 
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

On Mon, Apr 13, 2020 at 07:29:01PM -0500, Eric Blake
wrote:
> Enhance the testsuite to ensure we don't regress with recent changes
> to stdin/out handling.  This adds:
> - test-single-sh.sh: prove that 'nbdkit -s sh script' is viable
> - test-stdio.sh: create plugin that checks stdin/out match /dev/null,
> then run it with -s, --run, -f
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
...
> +/* Check whether stdin/out match /dev/null */
> +static bool
> +stdio_check (void)
> +{
> +  static int dn = -1;
> +  struct stat st1, st2;
> +
> +  if (dn == -1) {
> +    dn = open ("/dev/null", O_RDONLY);
> +    assert (dn > STDERR_FILENO);
> +  }
> +  if (fstat (dn, &st1) == -1)
> +    assert (false);
> +
> +  if (fstat (STDIN_FILENO, &st2) == -1)
> +    assert (false);
> +  if (st1.st_dev != st2.st_dev || st1.st_ino != st2.st_ino)
> +    return false;
> +
> +  if (fstat (STDOUT_FILENO, &st2) == -1)
> +    assert (false);
> +  if (st1.st_dev != st2.st_dev || st1.st_ino != st2.st_ino)
> +    return false;
> +
> +  return true;
> +}
Interesting - I think I would have done a Linux-specific hack
involving /proc/fd, but your way should be better :-)

All looks good, ACK.

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/