Re: [Libguestfs] [nbdkit PATCH 2/3] sh: Avoid setenv after fork
On 8/2/19 2:41 AM, Richard W.M. Jones wrote:
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().
>
> +++ 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.
Correct, but .load is run exactly once, and nbdkit doesn't allow more
than one plugin to be loaded, so I don't see this as being a problem
unless down the road we find a way to load more than one plugin at once
while still honoring semantics that plugins have gotten used to of being
the only thing running.
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.
If we were doing a different $tmpdir per connection, then yes, we'd have
to do that. But with the same $tmpdir shared among all connections (the
shell script can then create a different subdir per connection and use
that as its handle, if per-connection differences are desired), I don't
see the point in the extra complexity at this time.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 488 bytes)