Libguestfs - May 2019 - Re: [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

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

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