Re: [PATCH 5/8] posix: Do not clobber errno by atfork handlers

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 11/03/2021 10:25, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 09/03/2021 08:01, Florian Weimer wrote:
>>> * Adhemerval Zanella via Libc-alpha:
>>>
>>>> Checked on x86_64-linux-gnu.
>>>> ---
>>>>  posix/fork.c | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/posix/fork.c b/posix/fork.c
>>>> index 4c9e60f187..7f27fb8338 100644
>>>> --- a/posix/fork.c
>>>> +++ b/posix/fork.c
>>>> @@ -68,7 +68,7 @@ __libc_fork (void)
>>>>      }
>>>>  
>>>>    pid_t pid = _Fork ();
>>>> -
>>>> +  int save_errno = errno;
>>>>    if (pid == 0)
>>>>      {
>>>>        /* Reset the lock state in the multi-threaded case.  */
>>>> @@ -107,6 +107,8 @@ __libc_fork (void)
>>>>    __run_fork_handlers (pid == 0 ? atfork_run_child : atfork_run_parent,
>>>>  		       multiple_threads);
>>>>  
>>>> +  if (pid < 0)
>>>> +    __set_errno (save_errno);
>>>>    return pid;
>>>>  }
>>>>  weak_alias (__libc_fork, __fork)
>>>
>>> Why is this condition correct?  Shouldn't it be pid >= 0?
>>
>> But pid >= 0 is a valid call which should not set errno.  The idea is
>> to return the _Fork error value even if some atfork handler in the
>> parent does clobber it.
> 
> Then maybe move that into the else part for the parent (on fork error
> there is no subprocess) and add a comment?

But we need to restore it *after* __run_fork_handlers calls. I will
add the comment to make it explicit:

  /* Restore the errno value even if some atfork handler in the parent                                
     clobber it.  */
  if (pid < 0)
    __set_errno (save_errno)