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

On Fri, Aug 02, 2019 at 02:26:15PM -0500, Eric Blake wrote: 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/79b26ee95034d9c90913da3b5db5965... 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