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

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190802/1fef8614/attachment.sig>