[PATCH v2 10/13] posix: Improve thread safety of functions using environ with execveat

[Florian Weimer]

* Adhemerval Zanella Netto:

>> -  return __arch_execveat (dirfd, path, argv, envp, flags);
>> +  if (envp == NULL || envp != __environ
>
> Wouldn't 'envp != __environ' already handle the 'envp == NULL', assuming
> that __environ is always non-NULL?

Is that guaranteed?  Applications could assign NULL to environ:

  <https://codesearch.debian.net/search?q=%5Csenviron%5Cs*%3D%5Cs*%28NULL%7C0%29&literal=0>

>> +      || !__environ_is_from_array_list ((char **) envp)
>> +      || __environ_single_threaded_no_snapshot (envp))
>> +    return __arch_execveat (dirfd, path, argv, envp, flags);
>> +
>> +  size_t env_size = ENVIRON_STACK_SNAPSHOT_SIZE;
>
> The size here is only the initial size, so maybe name it hinting it?

You mean for ENVIRON_STACK_SNAPSHOT_SIZE?  That's why I put STACK there.

>> +  while (true)
>> +    {
>> +      /* Using an on-stack buffer is not ideal, but it is the most
>> +         practical way to avoid memory leaks with vfork: New mappings
>> +         created before the execveat call will be preserved in the
>> +         original process before the vfork call even if the execve at
>> +         call succeeds and replaces the current process.  */
>
> Can't we use MADV_DONTFORK here? There still the small windows between
> mmap return and madvise call, but I am not very confident that adding
> a possible unbounded dynamic stack allocation is best way forward
> here.

No, the vfork has already happened at this point.  We would need
something to unmap the mapping on a successful execve call.

>> diff --git a/stdlib/tst-environ-snapshot-skeleton.c b/stdlib/tst-environ-snapshot-skeleton.c
>> new file mode 100644
>> index 0000000000..8548e0a9e0

>> +   It laso needs to define a function
>
> s/laso/also

Fixed.

Thanks,
Florian