[Libguestfs] [PATCH nbdkit] sh: Remove assert and replace with smarter file descriptor duplication. (was: Re: [nbdkit PATCH v2 14/17] sh: Use pipe2 with CLOEXEC when possible)

[Richard W.M. Jones]

On Fri, Aug 02, 2019 at 02:26:15PM -0500, Eric Blake wrote:
> +  /* Ensure that stdin/out/err of the current process were not empty
> +   * before we started creating pipes (otherwise, the close and dup2
> +   * calls below become more complex to juggle fds around correctly).
> +  */
> +  assert (in_fd[0] > STDERR_FILENO && in_fd[1] > STDERR_FILENO &&
> +          out_fd[0] > STDERR_FILENO && out_fd[1] > STDERR_FILENO &&
> +          err_fd[0] > STDERR_FILENO && err_fd[1] > STDERR_FILENO);

This assert is now being thrown whenever we use nbdkit + sh plugin +
nbdkit -s option.  It's easy to demonstrate using a libnbd one-liner:

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

(This is triggered frequently by tests in the libnbd test suite which
is where I first observed it.)

It happens during the shutdown path where there is one final call to
sh_close in the plugin:

#0  0x00007fc5d929c615 in raise () from /lib64/libc.so.6
#1  0x00007fc5d92858d9 in abort () from /lib64/libc.so.6
#2  0x00007fc5d92857a9 in __assert_fail_base.cold () from /lib64/libc.so.6
#3  0x00007fc5d9294a56 in __assert_fail () from /lib64/libc.so.6
#4  0x00007fc5d969738c in call3.constprop ()
   from /usr/lib64/nbdkit/plugins/nbdkit-sh-plugin.so
#5  0x00007fc5d9697493 in call ()
   from /usr/lib64/nbdkit/plugins/nbdkit-sh-plugin.so
#6  0x00007fc5d969833c in sh_close ()
   from /usr/lib64/nbdkit/plugins/nbdkit-sh-plugin.so
#7  0x0000561694bcf8e8 in plugin_close (b=<optimized out>, conn=0x56169df9afe0)
    at plugins.c:305
#8  0x0000561694bca649 in free_connection (conn=0x56169df9afe0)
    at connections.c:377
#9  _handle_single_connection (sockout=<optimized out>, sockin=<optimized out>)
    at connections.c:267
#10 handle_single_connection (sockin=<optimized out>, sockout=<optimized out>)
    at connections.c:277
#11 0x0000561694bc9583 in start_serving () at main.c:856
#12 main (argc=<optimized out>, argv=0x7ffd09440178) at main.c:651

This is after conn->close has been called and stdin/stdout (which were
connected to the client socket) have actually been closed.

We can easily paper over this by ignoring the assert on the close
path, but I foresee two problems:

(1) I don't believe the assert is generally correct.  While it's not
ideal for nbdkit to be run with stdin/out/err closed (they obviously
should be connected to /dev/null) it's also not impossible for that to
happen.  We don't cope well with this situation.

(2) The assert is protecting us from some minimal checks in this code,
but I think the right answer is to just add those checks.

https://github.com/libguestfs/nbdkit/blob/79b26ee95034d9c90913da3b5db596503c5d03d9/plugins/sh/call.c#L166

Anyway, a possible patch for this is attached, but I'm not very
confident it is correct.

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
-------------- next part --------------
>From 88b0663e5388b464a0b88080c8449fe94ebbdba3 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones at redhat.com>
Date: Tue, 27 Aug 2019 12:44:33 +0100
Subject: [PATCH] sh: Remove assert and replace with smarter file descriptor
 duplication.

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

On the shutdown path when nbdkit -s is used stdin/stdout (connected to
the client socket) have been closed already, so the assertion is
obviously not valid.  Remove the assertion and replace it with smarter
file descriptor duplication.

Fixes commit b5b38e4e585ec5f22ea86feba6260b314f803559.
---
 plugins/sh/call.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/plugins/sh/call.c b/plugins/sh/call.c
index b86e7c9..c1245cd 100644
--- a/plugins/sh/call.c
+++ b/plugins/sh/call.c
@@ -148,14 +148,6 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to stdin */
   }
 #endif
 
-  /* Ensure that stdin/out/err of the current process were not empty
-   * before we started creating pipes (otherwise, the close and dup2
-   * calls below become more complex to juggle fds around correctly).
-  */
-  assert (in_fd[0] > STDERR_FILENO && in_fd[1] > STDERR_FILENO &&
-          out_fd[0] > STDERR_FILENO && out_fd[1] > STDERR_FILENO &&
-          err_fd[0] > STDERR_FILENO && err_fd[1] > STDERR_FILENO);
-
   pid = fork ();
   if (pid == -1) {
     nbdkit_error ("%s: fork: %m", script);
@@ -166,12 +158,18 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to stdin */
     close (in_fd[1]);
     close (out_fd[0]);
     close (err_fd[0]);
-    dup2 (in_fd[0], 0);
-    dup2 (out_fd[1], 1);
-    dup2 (err_fd[1], 2);
-    close (in_fd[0]);
-    close (out_fd[1]);
-    close (err_fd[1]);
+    if (in_fd[0] != STDIN_FILENO) {
+      dup2 (in_fd[0], STDIN_FILENO);
+      close (in_fd[0]);
+    }
+    if (out_fd[1] != STDOUT_FILENO) {
+      dup2 (out_fd[1], STDOUT_FILENO);
+      close (out_fd[1]);
+    }
+    if (err_fd[1] != STDERR_FILENO) {
+      dup2 (err_fd[1], STDERR_FILENO);
+      close (err_fd[1]);
+    }
 
     /* Restore SIGPIPE back to SIG_DFL, since shell can't undo SIG_IGN */
     signal (SIGPIPE, SIG_DFL);
-- 
2.22.0