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

  Checked on x86_64-linux-gnu.
---
 posix/fork.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
  

* 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?

I wonder if this should be part of __run_fork_handlers, so that fork
handlers always observe the original errno value.

Thanks,
Florian
  

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.

> 
> I wonder if this should be part of __run_fork_handlers, so that fork
> handlers always observe the original errno value.

I am not sure about it, checking the errno without actually calling a
function that might setting it is error-prone imho.
  

* 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?

Thanks,
Florian
  

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)
  

On Mär 11 2021, Adhemerval Zanella via Libc-alpha wrote:

> 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.  */

Perhaps: "If _Fork failed, preserve its errno value."

>   if (pid < 0)
>     __set_errno (save_errno)

Andreas.
  

* Andreas Schwab:

> On Mär 11 2021, Adhemerval Zanella via Libc-alpha wrote:
>
>> 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.  */
>
> Perhaps: "If _Fork failed, preserve its errno value."
>
>>   if (pid < 0)
>>     __set_errno (save_errno)

Agreed.  And then it's clear it should be moved to the parent branch of
the if statement above.

Thanks,
Florian
  

On 11/03/2021 11:25, Florian Weimer wrote:
> * Andreas Schwab:
> 
>> On Mär 11 2021, Adhemerval Zanella via Libc-alpha wrote:
>>
>>> 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.  */
>>
>> Perhaps: "If _Fork failed, preserve its errno value."

Ack.

>>
>>>   if (pid < 0)
>>>     __set_errno (save_errno)
> 
> Agreed.  And then it's clear it should be moved to the parent branch of
> the if statement above.
> 

But moving it before __run_fork_handlers in the the previous if (which
handles the pid != 0) will both 1. reset the errno even for success
(pid > 0) and 2. not fixing the issue since __run_fork_handlers will
be issue later. What I am missing in your suggestion?
  

* Adhemerval Zanella:

>>>>   if (pid < 0)
>>>>     __set_errno (save_errno)
>> 
>> Agreed.  And then it's clear it should be moved to the parent branch of
>> the if statement above.
>> 
>
> But moving it before __run_fork_handlers in the the previous if (which
> handles the pid != 0) will both 1. reset the errno even for success
> (pid > 0) and 2. not fixing the issue since __run_fork_handlers will
> be issue later. What I am missing in your suggestion? 

After the __run_fork_handlers call on the parent branch.

Thanks,
Florian
  

On 11/03/2021 12:32, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>>>>   if (pid < 0)
>>>>>     __set_errno (save_errno)
>>>
>>> Agreed.  And then it's clear it should be moved to the parent branch of
>>> the if statement above.
>>>
>>
>> But moving it before __run_fork_handlers in the the previous if (which
>> handles the pid != 0) will both 1. reset the errno even for success
>> (pid > 0) and 2. not fixing the issue since __run_fork_handlers will
>> be issue later. What I am missing in your suggestion? 
> 
> After the __run_fork_handlers call on the parent branch.

Why is this an improvement over doing on fork.c, now that with
this change is the only caller of __run_fork_handlers and the
routine with the information (pid result) to restore the
errno?
  

* Adhemerval Zanella:

> On 11/03/2021 12:32, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>>>>>   if (pid < 0)
>>>>>>     __set_errno (save_errno)
>>>>
>>>> Agreed.  And then it's clear it should be moved to the parent branch of
>>>> the if statement above.
>>>>
>>>
>>> But moving it before __run_fork_handlers in the the previous if (which
>>> handles the pid != 0) will both 1. reset the errno even for success
>>> (pid > 0) and 2. not fixing the issue since __run_fork_handlers will
>>> be issue later. What I am missing in your suggestion? 
>> 
>> After the __run_fork_handlers call on the parent branch.
>
> Why is this an improvement over doing on fork.c, now that with
> this change is the only caller of __run_fork_handlers and the
> routine with the information (pid result) to restore the
> errno?

I suggest to leave it in fork.c, but put it in the parent branch,
instead after the if statement.  The code only runs if fork fails, so
it's something the parent process needs to do.  It's not cleanup that
has to happen on both paths.  The compiler will likely optimize it to
identical machine code, but I think it's helpful to the reader to put it
on the parent execution path.

Thanks,
Florian
  

On 11/03/2021 16:23, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 11/03/2021 12:32, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>>>>>   if (pid < 0)
>>>>>>>     __set_errno (save_errno)
>>>>>
>>>>> Agreed.  And then it's clear it should be moved to the parent branch of
>>>>> the if statement above.
>>>>>
>>>>
>>>> But moving it before __run_fork_handlers in the the previous if (which
>>>> handles the pid != 0) will both 1. reset the errno even for success
>>>> (pid > 0) and 2. not fixing the issue since __run_fork_handlers will
>>>> be issue later. What I am missing in your suggestion? 
>>>
>>> After the __run_fork_handlers call on the parent branch.
>>
>> Why is this an improvement over doing on fork.c, now that with
>> this change is the only caller of __run_fork_handlers and the
>> routine with the information (pid result) to restore the
>> errno?
> 
> I suggest to leave it in fork.c, but put it in the parent branch,
> instead after the if statement.  The code only runs if fork fails, so
> it's something the parent process needs to do.  It's not cleanup that
> has to happen on both paths.  The compiler will likely optimize it to
> identical machine code, but I think it's helpful to the reader to put it
> on the parent execution path.

Right, I got it now your suggestion.