[Libguestfs] [libnbd PATCH v3 07/19] socket activation: replace execvp() call with fork-safe variant

[Laszlo Ersek]

Per POSIX, execvp() is not safe to call in a child process forked from a
multi-threaded process. We can now replace the execvp() call in the child
process with a call to our fork-safe (async-signal-safe) variant.

Prepare our internal execvpe context on the parent's construction path,
use the context in the child, and release the context in the parent on the
way out, regardless of whether the handler as a whole succeeded or not.
(The context is only a temporary resource.)

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v4:
    
    - pick up Rich's R-b
    
    - resolve rebase conflict near the
      prepare_socket_activation_environment() call site, due to keeping
      set_error() internal to that callee, in patch "socket activation:
      clean up responsibilities of prep.sock.act.env.()"
    
    context:-U11

 generator/states-connect-socket-activation.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c
index 752ee73bb62f..a214ffd5a6e4 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -94,22 +94,23 @@ prepare_socket_activation_environment (string_vector *env)
   string_vector_empty (env);
   return -1;
 }
 
 STATE_MACHINE {
  CONNECT_SA.START:
   enum state next;
   char *tmpdir;
   char *sockpath;
   int s;
   struct sockaddr_un addr;
+  struct execvpe execvpe_ctx;
   string_vector env;
   pid_t pid;
 
   assert (!h->sock);
   assert (h->argv.ptr);
   assert (h->argv.ptr[0]);
 
   next = %.DEAD;
 
   /* Use /tmp instead of TMPDIR because we must ensure the path is
    * short enough to store in the sockaddr_un.  On some platforms this
@@ -141,25 +142,31 @@  CONNECT_SA.START:
   memcpy (addr.sun_path, sockpath, strlen (sockpath) + 1);
   if (bind (s, (struct sockaddr *) &addr, sizeof addr) == -1) {
     set_error (errno, "bind: %s", sockpath);
     goto close_socket;
   }
 
   if (listen (s, SOMAXCONN) == -1) {
     set_error (errno, "listen");
     goto unlink_sockpath;
   }
 
+  if (nbd_internal_execvpe_init (&execvpe_ctx, h->argv.ptr[0], h->argv.len) ==
+      -1) {
+    set_error (errno, "nbd_internal_execvpe_init");
+    goto unlink_sockpath;
+  }
+
   if (prepare_socket_activation_environment (&env) == -1)
     /* prepare_socket_activation_environment() calls set_error() internally */
-    goto unlink_sockpath;
+    goto uninit_execvpe;
 
   pid = fork ();
   if (pid == -1) {
     set_error (errno, "fork");
     goto empty_env;
   }
 
   if (pid == 0) {         /* child - run command */
     if (s != FIRST_SOCKET_ACTIVATION_FD) {
       if (dup2 (s, FIRST_SOCKET_ACTIVATION_FD) == -1) {
         nbd_internal_fork_safe_perror ("dup2");
@@ -189,45 +196,47 @@  CONNECT_SA.START:
     char buf[32];
     const char *v =
       nbd_internal_fork_safe_itoa ((long) getpid (), buf, sizeof buf);
     strcpy (&env.ptr[0][PREFIX_LENGTH], v);
 
     /* Restore SIGPIPE back to SIG_DFL. */
     if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) {
       nbd_internal_fork_safe_perror ("signal");
       _exit (126);
     }
 
-    environ = env.ptr;
-    execvp (h->argv.ptr[0], h->argv.ptr);
+    (void)nbd_internal_fork_safe_execvpe (&execvpe_ctx, &h->argv, env.ptr);
     nbd_internal_fork_safe_perror (h->argv.ptr[0]);
     if (errno == ENOENT)
       _exit (127);
     else
       _exit (126);
   }
 
   /* Parent -- we're done; commit. */
   h->sact_tmpdir = tmpdir;
   h->sact_sockpath = sockpath;
   h->pid = pid;
 
   h->connaddrlen = sizeof addr;
   memcpy (&h->connaddr, &addr, h->connaddrlen);
   next = %^CONNECT.START;
 
   /* fall through, for releasing the temporaries */
 
 empty_env:
   string_vector_empty (&env);
 
+uninit_execvpe:
+  nbd_internal_execvpe_uninit (&execvpe_ctx);
+
 unlink_sockpath:
   if (next == %.DEAD)
     unlink (sockpath);
 
 close_socket:
   close (s);
 
 free_sockpath:
   if (next == %.DEAD)
     free (sockpath);