[Libguestfs] [nbdkit PATCH 2/3] sh: Avoid setenv after fork

[Richard W.M. Jones]

On Thu, Aug 01, 2019 at 10:42:58PM -0500, Eric Blake wrote:
> setenv() is not async-signal-safe and as such should not be used
> between fork/exec of a multi-threaded app: if one thread is
> manipulating the current environment (which may entail obtaining a
> malloc() mutex) when another thread calls fork(), the resulting
> child's attempt to use setenv() could deadlock or see a broken environ
> because the thread owning the lock no longer exists to release it.
> Besides, it is more efficient to update the environment exactly once
> in the parent, rather than after every fork().
> 
> While at it, check for (unlikely) failure of setenv.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  plugins/sh/call.c | 3 ---
>  plugins/sh/sh.c   | 6 ++++++
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/plugins/sh/call.c b/plugins/sh/call.c
> index 871de5c6..9b8b48e2 100644
> --- a/plugins/sh/call.c
> +++ b/plugins/sh/call.c
> @@ -127,9 +127,6 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to stdin */
>      /* Restore SIGPIPE back to SIG_DFL, since shell can't undo SIG_IGN */
>      signal (SIGPIPE, SIG_DFL);
> 
> -    /* Set $tmpdir for the script. */
> -    setenv ("tmpdir", tmpdir, 1);
> -
>      execvp (argv[0], (char **) argv);
>      perror (argv[0]);
>      _exit (EXIT_FAILURE);
> diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
> index 737c38cf..e3d3c2f1 100644
> --- a/plugins/sh/sh.c
> +++ b/plugins/sh/sh.c
> @@ -60,6 +60,12 @@ sh_load (void)
>      nbdkit_error ("mkdtemp: /tmp: %m");
>      exit (EXIT_FAILURE);
>    }
> +  /* Set $tmpdir for the script. */
> +  if (setenv ("tmpdir", tmpdir, 1) == -1) {
> +    nbdkit_error ("setenv: tmpdir=%s: %m", tmpdir);
> +    exit (EXIT_FAILURE);
> +  }
> +
>    nbdkit_debug ("sh: load: tmpdir: %s", tmpdir);
>  }

This sets $tmpdir in the whole nbdkit process ... which may or may not
be a problem.  It's probably not a problem in reality, but I wonder if
it's better to to use ‘execvpe’ (or more portably but more difficult
to use ‘execve’) to set the environment in the exec call?  I guess
we'd have to copy and extend environ in the parent since we want to
pass existing environment variables through.

Rich.

-- 
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