[Libguestfs] [libnbd PATCH v3 24/29] CONNECT_COMMAND.START: plug child process leak on error

[Laszlo Ersek]

A prior patch highlighted the following leak: when any construction step
fails in the parent after fork(), the child process is leaked.

We could plug the leak by inserting a new error handling section for
killing the child process and reaping it with waitpid(). However, it would
raise some new questions (what signal to send, ensure the child not ignore
that signal, hope that whatever process image we execute in the child
handle that signal properly, etc).

Instead, rearrange the steps in the CONNECT_COMMAND.START handler so that
fork() be the last operation in the parent process, on the construction
path. If fork() succeeds, let the entire handler succeed. For this, move
the fcntl() and nbd_internal_socket_create() calls above fork().

(The hoisting of the fcntl() calls is where we rely on the earlier
observation that O_NONBLOCK only applies to the parent-side end of the
socket pair, so it is safe to do before forking.)

The trouble in turn is that nbd_internal_socket_create() takes ownership
of the parent-side end of the socket pair (sv[0]). So once that function
returns successfully, we can't close sv[0] even on the error path -- we
close the "higher level" socket, and that invalidates sv[0] at once. Track
this ownership transfer with a new boolean flag therefore.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---

Notes:
    context:-W

 generator/states-connect.c | 48 +++++++++++---------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/generator/states-connect.c b/generator/states-connect.c
index bd78f890a2c5..283481bb7f91 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -239,103 +239,109 @@  CONNECT_TCP.NEXT_ADDRESS:
 
  CONNECT_COMMAND.START:
   enum state next;
+  bool parentfd_transferred;
   int sv[2];
-  pid_t pid;
   int flags;
   struct socket *sock;
+  pid_t pid;
 
   assert (!h->sock);
   assert (h->argv.ptr);
   assert (h->argv.ptr[0]);
 
   next = %.DEAD;
+  parentfd_transferred = false;
 
   if (nbd_internal_socketpair (AF_UNIX, SOCK_STREAM, 0, sv) == -1) {
     set_error (errno, "socketpair");
     goto done;
   }
 
   /* A process is effectively in an unusable state if any of STDIN_FILENO
    * (fd#0), STDOUT_FILENO (fd#1) and STDERR_FILENO (fd#2) don't exist. If they
    * exist however, then the socket pair created above will not intersect with
    * the fd set { 0, 1, 2 }. This is relevant for the child-side dup2() logic
    * below.
    */
   assert (sv[0] > STDERR_FILENO);
   assert (sv[1] > STDERR_FILENO);
 
+  /* Only the parent-side end of the socket pair must be set to non-blocking,
+   * 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_error (errno, "fcntl");
+    goto close_socket_pair;
+  }
+
+  sock = nbd_internal_socket_create (sv[0]);
+  if (!sock)
+    /* nbd_internal_socket_create() calls set_error() internally */
+    goto close_socket_pair;
+  parentfd_transferred = true;
+
   pid = fork ();
   if (pid == -1) {
     set_error (errno, "fork");
-    goto close_socket_pair;
+    goto close_high_level_socket;
   }
 
   if (pid == 0) {         /* child - run command */
     if (close (sv[0]) == -1) {
       nbd_internal_fork_safe_perror ("close");
       _exit (126);
     }
     if (dup2 (sv[1], STDIN_FILENO) == -1 ||
         dup2 (sv[1], STDOUT_FILENO) == -1) {
       nbd_internal_fork_safe_perror ("dup2");
       _exit (126);
     }
     NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDIN_FILENO);
     NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDOUT_FILENO);
     if (close (sv[1]) == -1) {
       nbd_internal_fork_safe_perror ("close");
       _exit (126);
     }
 
     /* Restore SIGPIPE back to SIG_DFL. */
     if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) {
       nbd_internal_fork_safe_perror ("signal");
       _exit (126);
     }
 
     execvp (h->argv.ptr[0], h->argv.ptr);
     nbd_internal_fork_safe_perror (h->argv.ptr[0]);
     if (errno == ENOENT)
       _exit (127);
     else
       _exit (126);
   }
 
-  /* Parent.
-   *
-   * Only the parent-side end of the socket pair must be set to non-blocking,
-   * 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_error (errno, "fcntl");
-    goto close_socket_pair;
-  }
-
-  sock = nbd_internal_socket_create (sv[0]);
-  if (!sock)
-    /* nbd_internal_socket_create() calls set_error() internally */
-    goto close_socket_pair;
-
-  /* Commit. */
+  /* Parent -- we're done; commit. */
   h->pid = pid;
   h->sock = sock;
 
   /* The sockets are connected already, we can jump directly to
    * receiving the server magic.
    */
   next = %^MAGIC.START;
 
   /* fall through, for releasing the temporaries */
 
-close_socket_pair:
+close_high_level_socket:
   if (next == %.DEAD)
+    sock->ops->close (sock);
+
+close_socket_pair:
+  assert (next == %.DEAD || parentfd_transferred);
+  if (!parentfd_transferred)
     close (sv[0]);
   close (sv[1]);
 
 done:
   SET_NEXT_STATE (next);
   return 0;
 
 } /* END STATE MACHINE */