Richard W.M. Jones
2019-May-25 18:33 UTC
[Libguestfs] [PATCH libnbd] states: connect_command: Don't set O_NONBLOCK on socket passed to child.
I also made the code a bit more robust about closing the socket along error paths. --- generator/states-connect.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/generator/states-connect.c b/generator/states-connect.c index ba8b240..a69b70f 100644 --- a/generator/states-connect.c +++ b/generator/states-connect.c @@ -27,6 +27,7 @@ #include <stdbool.h> #include <string.h> #include <unistd.h> +#include <fcntl.h> #include <errno.h> #include <signal.h> #include <netdb.h> @@ -183,12 +184,12 @@ CONNECT_COMMAND.START: int sv[2]; pid_t pid; + int flags; assert (!h->sock); assert (h->argv); assert (h->argv[0]); - if (socketpair (AF_UNIX, SOCK_STREAM|SOCK_NONBLOCK|SOCK_CLOEXEC, 0, - sv) == -1) { + if (socketpair (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, sv) == -1) { SET_NEXT_STATE (%.DEAD); set_error (errno, "socketpair"); return -1; @@ -219,13 +220,27 @@ } /* Parent. */ + close (sv[1]); + h->pid = pid; + + /* The socket must be set to non-blocking only in the parent, + * because the child may not be expecting a non-blocking socket. + */ + flags = fcntl (sv[0], F_GETFL, 0); + if (flags == -1 || + fcntl (sv[0], F_SETFL, flags|O_NONBLOCK) == -1) { + SET_NEXT_STATE (%.DEAD); + close (sv[0]); + return -1; + } + h->sock = nbd_internal_socket_create (sv[0]); if (!h->sock) { SET_NEXT_STATE (%.DEAD); + close (sv[0]); return -1; } - close (sv[1]); /* The sockets are connected already, we can jump directly to * receiving the server magic. -- 2.21.0
Eric Blake
2019-May-25 20:22 UTC
Re: [Libguestfs] [PATCH libnbd] states: connect_command: Don't set O_NONBLOCK on socket passed to child.
On 5/25/19 1:33 PM, Richard W.M. Jones wrote:> I also made the code a bit more robust about closing the socket along > error paths. > --- > generator/states-connect.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) >> assert (!h->sock); > assert (h->argv); > assert (h->argv[0]); > - if (socketpair (AF_UNIX, SOCK_STREAM|SOCK_NONBLOCK|SOCK_CLOEXEC, 0, > - sv) == -1) { > + if (socketpair (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, sv) == -1) {Is it any easier to keep SOCK_NONBLOCK here and then clear O_NONBLOCK in the child process? It may matter if we try to port to a system that lacks SOCK_CLOEXEC (SOCK_NONBLOCK and SOCK_CLOEXEC were added at the same time). But we can deal with portability when someone reports a problem. For now the patch looks fine. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Eric Blake
2019-May-25 20:30 UTC
Re: [Libguestfs] [PATCH libnbd] states: connect_command: Don't set O_NONBLOCK on socket passed to child.
On 5/25/19 3:22 PM, Eric Blake wrote:> On 5/25/19 1:33 PM, Richard W.M. Jones wrote: >> I also made the code a bit more robust about closing the socket along >> error paths. >> --- >> generator/states-connect.c | 21 ++++++++++++++++++--- >> 1 file changed, 18 insertions(+), 3 deletions(-) >> > >> assert (!h->sock); >> assert (h->argv); >> assert (h->argv[0]); >> - if (socketpair (AF_UNIX, SOCK_STREAM|SOCK_NONBLOCK|SOCK_CLOEXEC, 0, >> - sv) == -1) { >> + if (socketpair (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, sv) == -1) { > > Is it any easier to keep SOCK_NONBLOCK here and then clear O_NONBLOCK in > the child process? It may matter if we try to port to a system that > lacks SOCK_CLOEXEC (SOCK_NONBLOCK and SOCK_CLOEXEC were added at the > same time). But we can deal with portability when someone reports a > problem. For now the patch looks fine.Hmm. Your use of SOCK_CLOEXEC here made me look for other fds that we might inadvertently destroy or leak in a multi-threaded process that does fork/exec (or even if the program linking against libnbd does connect_command() in two separate threads on two different nbd objects). I found: lib/crypto.c: fp = fopen (pskfile, "r"); We need to use either fopen(pskfile, "re") (if libc is new-enough to support "e" for O_CLOEXEC) or raw open(O_CLOEXEC) + fdopen() instead. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Seemingly Similar Threads
- Re: [PATCH libnbd] states: connect_command: Don't set O_NONBLOCK on socket passed to child.
- Re: [PATCH libnbd] states: connect_command: Don't set O_NONBLOCK on socket passed to child.
- [PATCH libnbd 0/2] api: Add support for AF_VSOCK.
- Re: Patchable build problems on OS X 10.10
- [PATCH 0/4] Small refactorings of the protocol layer.