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

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(a)redhat.com&gt; --- Notes: 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 81bb850c7d8c..cb3b1c6f4ede 100644 --- a/generator/states-connect-socket-activation.c +++ b/generator/states-connect-socket-activation.c @@ -93,22 +93,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 @@ -140,25 +141,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) { set_error (errno, "prepare_socket_activation_environment"); - 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) { @@ -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);