Re: [PATCH v7 4/4] posix: Add posix_spawn_file_actions_addclosefrom_np

[Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org>]

On 08/07/2021 18:54, H.J. Lu wrote:
> On Thu, Jul 8, 2021 at 9:12 AM Adhemerval Zanella via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
>>
>>
>>
>> On 08/07/2021 11:34, Florian Weimer wrote:
>>> Some of the new files use:
>>>
>>>> +   <http://www.gnu.org/licenses/>.  */
>>>
>>> That should be: https://
>>
>> Ack.
>>
>>>
>>>> +/* Called on process re-execution.  The arguments are the expected opened
>>>> +   file descriptors.  */
>>>> +_Noreturn static void
>>>> +handle_restart (int argc, char *argv[])
>>>> +{
>>>> +  size_t nfds = argc > 1 ? argc - 1 : 0;
>>>> +  struct fd_t
>>>> +  {
>>>> +    int fd;
>>>> +    _Bool found;
>>>> +  } *fds = xmalloc (sizeof (struct fd_t) * nfds);
>>>> +  for (int i = 0; i < nfds; i++)
>>>> +    {
>>>> +      char *endptr;
>>>> +      long unsigned int fd = strtoul (argv[i+1], &endptr, 10);
>>>> +      if (*endptr != '\0' || fd > INT_MAX)
>>>> +        FAIL_EXIT1 ("readdir: invalid file descriptor value: %s", argv[i]);
>>>
>>> Should be:
>>>
>>>   FAIL_EXIT1 ("argv[%d]: invalid file descriptor value: %s", i, argv[i]);
>>
>> Ack.
>>
>>>
>>>> @@ -344,7 +346,9 @@ __spawnix (pid_t * pid, const char *file,
>>>>    /* We need at least a few pages in case the compiler's stack checking is
>>>>       enabled.  In some configs, it is known to use at least 24KiB.  We use
>>>>       32KiB to be "safe" from anything the compiler might do.  Besides, the
>>>> -     extra pages won't actually be allocated unless they get used.  */
>>>> +     extra pages won't actually be allocated unless they get used.
>>>> +     It also acts the slack for spawn_closefrom (including MIPS64 getdents64
>>>> +     where it might use about 1k extra stack space.  */
>>>
>>> Missing closing parenthesis.
>>>
>>> Rest looks okay to me, thanks.  No need to post a new verison, I think.
>>
>> Thanks, I will respin another round of tests and commit.
> 
> On x86-64, I saw:
> 
> FAIL: posix/tst-spawn5
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=28067
> 
> Has anyone else seen it?

I reproduced it with --enable-hardcoded-path-in-tests, this is most
likely something wrong I doing with the parameters to respawn the
process. I will fix it.