Libguestfs - Aug 2019 - [PATCH nbdkit] server: Try hard to maintain invariant that fds 0, 1 and 2 are always open.

https://www.redhat.com/archives/libguestfs/2019-August/thread.html#00347

Thanks: Eric Blake and Daniel P. Berrangé
---
 common/utils/utils.h |  1 +
 server/connections.c |  4 ++--
 server/crypto.c      |  5 +++--
 server/main.c        | 23 +++++++++++++++++++++++
 common/utils/utils.c | 29 +++++++++++++++++++++++++++++
 5 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/common/utils/utils.h b/common/utils/utils.h
index ebd5f66..a77d2cd 100644
--- a/common/utils/utils.h
+++ b/common/utils/utils.h
@@ -38,5 +38,6 @@ extern void uri_quote (const char *str, FILE *fp);
 extern int exit_status_to_nbd_error (int status, const char *cmd);
 extern int set_cloexec (int fd);
 extern int set_nonblock (int fd);
+extern void close_or_nullify_fd (int fd);
 
 #endif /* NBDKIT_UTILS_H */
diff --git a/server/connections.c b/server/connections.c
index c173df8..f57ab3e 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -489,7 +489,7 @@ static void
 raw_close (struct connection *conn)
 {
   if (conn->sockin >= 0)
-    close (conn->sockin);
+    close_or_nullify_fd (conn->sockin);
   if (conn->sockout >= 0 && conn->sockin != conn->sockout)
-    close (conn->sockout);
+    close_or_nullify_fd (conn->sockout);
 }
diff --git a/server/crypto.c b/server/crypto.c
index 9cd1bb0..6f97f2c 100644
--- a/server/crypto.c
+++ b/server/crypto.c
@@ -47,6 +47,7 @@
 #include <assert.h>
 
 #include "internal.h"
+#include "utils.h"
 
 #ifdef HAVE_GNUTLS
 
@@ -404,9 +405,9 @@ crypto_close (struct connection *conn)
   gnutls_bye (session, GNUTLS_SHUT_RDWR);
 
   if (sockin >= 0)
-    close (sockin);
+    close_or_nullify_fd (sockin);
   if (sockout >= 0 && sockin != sockout)
-    close (sockout);
+    close_or_nullify_fd (sockout);
 
   gnutls_deinit (session);
   conn->crypto_session = NULL;
diff --git a/server/main.c b/server/main.c
index 65025a6..65bef30 100644
--- a/server/main.c
+++ b/server/main.c
@@ -60,6 +60,7 @@ static struct backend *open_filter_so (struct backend *next,
size_t i, const cha
 static void start_serving (void);
 static void write_pidfile (void);
 static bool is_config_key (const char *key, size_t len);
+static void open_std_file_descriptors (void);
 
 struct debug_flag *debug_flags; /* -D */
 bool exit_with_parent;          /* --exit-with-parent */
@@ -149,6 +150,13 @@ main (int argc, char *argv[])
   size_t i;
   const char *magic_config_key;
 
+  /* Ensures that fds 0, 1 and 2 are open (on /dev/null if nothing
+   * else).  This is so that nbdkit and plugins can assume these file
+   * descriptors are always open, which makes certain code easier to
+   * write.
+   */
+  open_std_file_descriptors ();
+
   threadlocal_init ();
 
   /* The default setting for TLS depends on whether we were
@@ -930,3 +938,18 @@ is_config_key (const char *key, size_t len)
 
   return true;
 }
+
+static void
+open_std_file_descriptors (void)
+{
+  int fds[] = { STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO };
+  int i, fd, fl;
+
+  for (i = 0; i < sizeof fds / sizeof fds[0]; ++i) {
+    fd = fds[i];
+    fl = fcntl (fd, F_GETFL, NULL);
+    if (fl == -1 && errno == EBADF)
+      /* This is best effort - don't fail. */
+      open ("/dev/null", fd == STDIN_FILENO ? O_RDONLY : O_WRONLY);
+  }
+}
diff --git a/common/utils/utils.c b/common/utils/utils.c
index 32bc96a..c818d4c 100644
--- a/common/utils/utils.c
+++ b/common/utils/utils.c
@@ -190,3 +190,32 @@ set_nonblock (int fd) {
   }
   return fd;
 }
+
+/* Calls close (fd).  As a special case if fd is stdin/stdout/stderr
+ * then it reopens the file descriptor on /dev/null.  This allows us
+ * to maintain certain invariants for nbdkit and plugins.  In all
+ * cases this ignores errors.
+ */
+void
+close_or_nullify_fd (int fd)
+{
+  int flags;
+  int nfd;
+
+  close (fd);
+
+  if (fd == STDIN_FILENO)
+    flags = O_RDONLY;
+  else if (fd == STDOUT_FILENO || fd == STDERR_FILENO)
+    flags = O_WRONLY;
+  else
+    return;
+
+  nfd = open ("/dev/null", flags);
+  if (nfd == -1)
+    return;
+  if (nfd != fd) {
+    dup2 (nfd, fd);
+    close (nfd);
+  }
+}
-- 
2.22.0

On 8/27/19 10:26 AM, Richard W.M. Jones wrote:
> https://www.redhat.com/archives/libguestfs/2019-August/thread.html#00347
> 
> Thanks: Eric Blake and Daniel P. Berrangé
> ---
>  common/utils/utils.h |  1 +
>  server/connections.c |  4 ++--
>  server/crypto.c      |  5 +++--
>  server/main.c        | 23 +++++++++++++++++++++++
>  common/utils/utils.c | 29 +++++++++++++++++++++++++++++
>  5 files changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/common/utils/utils.h b/common/utils/utils.h
> index ebd5f66..a77d2cd 100644
> --- a/common/utils/utils.h
> +++ b/common/utils/utils.h
> @@ -38,5 +38,6 @@ extern void uri_quote (const char *str, FILE *fp);
>  extern int exit_status_to_nbd_error (int status, const char *cmd);
>  extern int set_cloexec (int fd);
>  extern int set_nonblock (int fd);
> +extern void close_or_nullify_fd (int fd);
I like the idea of a helper function...
>  
>  #endif /* NBDKIT_UTILS_H */
> diff --git a/server/connections.c b/server/connections.c
> index c173df8..f57ab3e 100644
> --- a/server/connections.c
> +++ b/server/connections.c
> @@ -489,7 +489,7 @@ static void
>  raw_close (struct connection *conn)
>  {
>    if (conn->sockin >= 0)
> -    close (conn->sockin);
> +    close_or_nullify_fd (conn->sockin);
>    if (conn->sockout >= 0 && conn->sockin !=
conn->sockout)
> -    close (conn->sockout);
> +    close_or_nullify_fd (conn->sockout);
and your patch calls it closer to the point of use than mine.

> +++ b/server/main.c
> @@ -60,6 +60,7 @@ static struct backend *open_filter_so (struct backend
*next, size_t i, const cha
>  static void start_serving (void);
>  static void write_pidfile (void);
>  static bool is_config_key (const char *key, size_t len);
> +static void open_std_file_descriptors (void);
>  
>  struct debug_flag *debug_flags; /* -D */
>  bool exit_with_parent;          /* --exit-with-parent */
> @@ -149,6 +150,13 @@ main (int argc, char *argv[])
>    size_t i;
>    const char *magic_config_key;
>  
> +  /* Ensures that fds 0, 1 and 2 are open (on /dev/null if nothing
> +   * else).  This is so that nbdkit and plugins can assume these file
> +   * descriptors are always open, which makes certain code easier to
> +   * write.
> +   */
> +  open_std_file_descriptors ();
But this part, I'm less sure of. If a user calls 'nbdkit -s
<&-', this
would let us start trying to interact with /dev/null as the socket.
It's user error for telling us (via -s) that stdin was important, so we
are better off diagnosing it up front and loudly, rather than
cryptically (or not at all) when we eventual read from /dev/null and see
EOF.  And once we've figured out how to diagnose missing fds for -s,
it's just as easy to use that diagnosis even when -s is not in use.

> +
> +static void
> +open_std_file_descriptors (void)
> +{
> +  int fds[] = { STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO };
> +  int i, fd, fl;
> +
> +  for (i = 0; i < sizeof fds / sizeof fds[0]; ++i) {
> +    fd = fds[i];
> +    fl = fcntl (fd, F_GETFL, NULL);
> +    if (fl == -1 && errno == EBADF)
> +      /* This is best effort - don't fail. */
> +      open ("/dev/null", fd == STDIN_FILENO ? O_RDONLY :
O_WRONLY);
I also like the hack of assigning O_WRONLY for stdin and O_RDONLY for
stdout (opposite your patch), so that attempts to use stdin/out still
give EBADF (which calls attention to the odd setup).

> +void
> +close_or_nullify_fd (int fd)
> +{
> +  int flags;
> +  int nfd;
> +
> +  close (fd);
> +
> +  if (fd == STDIN_FILENO)
> +    flags = O_RDONLY;
> +  else if (fd == STDOUT_FILENO || fd == STDERR_FILENO)
> +    flags = O_WRONLY;
The fd == STDERR_FILENO case should never happen if we properly
sanitized 0/1/2 in main().
> +  else
> +    return;
> +
> +  nfd = open ("/dev/null", flags);
Missing O_CLOEXEC.
> +  if (nfd == -1)
> +    return;
> +  if (nfd != fd) {
Possible if we are racing with another thread for opening fds - but if
we are racing, then losing the race is bad.  I'd rather just assert that
there is no race (that is, we should only be needing to do this when -s
was in force - at which point we know there is only a single connection
and thus no other connection thread competing with us).
> +    dup2 (nfd, fd);
> +    close (nfd);
> +  }
> +}
> 
So while our patches had a similar idea, I think I'll go with my
counterproposal as simpler (and we discussed that a bit more on IRC as
well).

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