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
Eric Blake
2020-Apr-04 22:02 UTC
[Libguestfs] [nbdkit PATCH 1/2] server: Add nbdkit_stdio_safe
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
Eric Blake
2020-Apr-04 22:02 UTC
[Libguestfs] [nbdkit PATCH 2/2] server: Sanitize stdin/out before running plugin code
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
Richard W.M. Jones
2020-Apr-07 07:55 UTC
Re: [Libguestfs] [nbdkit PATCH 0/2] stdin/out cleanups
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
Richard W.M. Jones
2020-Apr-07 07:59 UTC
Re: [Libguestfs] [nbdkit PATCH 1/2] server: Add nbdkit_stdio_safe
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
Richard W.M. Jones
2020-Apr-07 08:06 UTC
Re: [Libguestfs] [nbdkit PATCH 2/2] server: Sanitize stdin/out before running plugin code
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
Possibly Parallel Threads
- [nbdkit PATCH v2 0/3] more consistent stdin/out handling
- [nbdkit PATCH 2/2] server: Sanitize stdin/out before running plugin code
- [nbdkit PATCH v2 2/3] server: Sanitize stdin/out before running plugin code
- server: Fix reading passwords interactively.
- [libnbd PATCH v3 00/19] pass LISTEN_FDNAMES with systemd socket activation