diff mbox series

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

Message ID 20210311195210.3153729-5-adhemerval.zanella@linaro.org
State Superseded
Delegated to: Florian Weimer
Headers
Series [v2,1/8] posix: Consolidate register-atfork |

Commit Message

Adhemerval Zanella Netto March 11, 2021, 7:52 p.m. UTC 
  Changes from v1:
* Move the errno set/restore to parent branch.
---
Checked on x86_64-linux-gnu.
---
 posix/fork.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Florian Weimer March 11, 2021, 9:04 p.m. UTC | #1 
* Adhemerval Zanella via Libc-alpha:

> Changes from v1:
> * Move the errno set/restore to parent branch.
> ---
> Checked on x86_64-linux-gnu.
> ---
>  posix/fork.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/posix/fork.c b/posix/fork.c
> index cc1bdc1232..a4fdd44e1e 100644
> --- a/posix/fork.c
> +++ b/posix/fork.c
> @@ -68,7 +68,6 @@ __libc_fork (void)
>      }
>  
>    pid_t pid = _Fork ();
> -
>    if (pid == 0)
>      {
>        fork_system_setup ();
> @@ -99,6 +98,11 @@ __libc_fork (void)
>      }
>    else
>      {
> +      /* If _Fork failed, preserve its errno value.  */
> +      int save_errno;
> +      if (pid < 0)
> +	save_errno = errno;
> +
>        /* Release acquired locks in the multi-threaded case.  */
>        if (multiple_threads)
>  	{
> @@ -111,6 +115,9 @@ __libc_fork (void)
>  
>        /* Run the handlers registered for the parent.  */
>        __run_fork_handlers (atfork_run_parent, multiple_threads);
> +
> +      if (pid < 0)
> +       __set_errno (save_errno);
>      }
>  
>    return pid;

This is okay.  GCC does not warn about the uninitialized save_errno
here; I checked with build-many-glibcs.py.  Apparently the middle-end is
smart enough to detect that the conditions match.

Thanks,
Florian
Andreas Schwab March 11, 2021, 10:21 p.m. UTC | #2 
On Mär 11 2021, Florian Weimer via Libc-alpha wrote:

> * Adhemerval Zanella via Libc-alpha:
>
>> Changes from v1:
>> * Move the errno set/restore to parent branch.
>> ---
>> Checked on x86_64-linux-gnu.
>> ---
>>  posix/fork.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/posix/fork.c b/posix/fork.c
>> index cc1bdc1232..a4fdd44e1e 100644
>> --- a/posix/fork.c
>> +++ b/posix/fork.c
>> @@ -68,7 +68,6 @@ __libc_fork (void)
>>      }
>>  
>>    pid_t pid = _Fork ();
>> -
>>    if (pid == 0)
>>      {
>>        fork_system_setup ();
>> @@ -99,6 +98,11 @@ __libc_fork (void)
>>      }
>>    else
>>      {
>> +      /* If _Fork failed, preserve its errno value.  */
>> +      int save_errno;
>> +      if (pid < 0)
>> +	save_errno = errno;
>> +
>>        /* Release acquired locks in the multi-threaded case.  */
>>        if (multiple_threads)
>>  	{
>> @@ -111,6 +115,9 @@ __libc_fork (void)
>>  
>>        /* Run the handlers registered for the parent.  */
>>        __run_fork_handlers (atfork_run_parent, multiple_threads);
>> +
>> +      if (pid < 0)
>> +       __set_errno (save_errno);
>>      }
>>  
>>    return pid;
>
> This is okay.  GCC does not warn about the uninitialized save_errno
> here; I checked with build-many-glibcs.py.  Apparently the middle-end is
> smart enough to detect that the conditions match.

But I think it wouldn't be bad to make that unconditional.

Andreas.
Florian Weimer March 11, 2021, 10:31 p.m. UTC | #3 
* Andreas Schwab:

> On Mär 11 2021, Florian Weimer via Libc-alpha wrote:
>
>> * Adhemerval Zanella via Libc-alpha:
>>
>>> Changes from v1:
>>> * Move the errno set/restore to parent branch.
>>> ---
>>> Checked on x86_64-linux-gnu.
>>> ---
>>>  posix/fork.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/posix/fork.c b/posix/fork.c
>>> index cc1bdc1232..a4fdd44e1e 100644
>>> --- a/posix/fork.c
>>> +++ b/posix/fork.c
>>> @@ -68,7 +68,6 @@ __libc_fork (void)
>>>      }
>>>  
>>>    pid_t pid = _Fork ();
>>> -
>>>    if (pid == 0)
>>>      {
>>>        fork_system_setup ();
>>> @@ -99,6 +98,11 @@ __libc_fork (void)
>>>      }
>>>    else
>>>      {
>>> +      /* If _Fork failed, preserve its errno value.  */
>>> +      int save_errno;
>>> +      if (pid < 0)
>>> +	save_errno = errno;
>>> +
>>>        /* Release acquired locks in the multi-threaded case.  */
>>>        if (multiple_threads)
>>>  	{
>>> @@ -111,6 +115,9 @@ __libc_fork (void)
>>>  
>>>        /* Run the handlers registered for the parent.  */
>>>        __run_fork_handlers (atfork_run_parent, multiple_threads);
>>> +
>>> +      if (pid < 0)
>>> +       __set_errno (save_errno);
>>>      }
>>>  
>>>    return pid;
>>
>> This is okay.  GCC does not warn about the uninitialized save_errno
>> here; I checked with build-many-glibcs.py.  Apparently the middle-end is
>> smart enough to detect that the conditions match.
>
> But I think it wouldn't be bad to make that unconditional.

Works for me too.

Thanks,
Florian
diff mbox series

Patch

diff --git a/posix/fork.c b/posix/fork.c
index cc1bdc1232..a4fdd44e1e 100644
--- a/posix/fork.c
+++ b/posix/fork.c
@@ -68,7 +68,6 @@  __libc_fork (void)
     }
 
   pid_t pid = _Fork ();
-
   if (pid == 0)
     {
       fork_system_setup ();
@@ -99,6 +98,11 @@  __libc_fork (void)
     }
   else
     {
+      /* If _Fork failed, preserve its errno value.  */
+      int save_errno;
+      if (pid < 0)
+	save_errno = errno;
+
       /* Release acquired locks in the multi-threaded case.  */
       if (multiple_threads)
 	{
@@ -111,6 +115,9 @@  __libc_fork (void)
 
       /* Run the handlers registered for the parent.  */
       __run_fork_handlers (atfork_run_parent, multiple_threads);
+
+      if (pid < 0)
+       __set_errno (save_errno);
     }
 
   return pid;