On Wed, Feb 15, 2023 at 03:11:46PM +0100, Laszlo Ersek wrote:
> 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 <ler...@redhat.com>
> ---
>
> 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);
Reviewed-by: Richard W.M. Jones <rjo...@redhat.com>
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
