[v4,2/4] posix: Do not clobber errno by atfork handlers
Checks
Context |
Check |
Description |
dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
Checked on x86_64-linux-gnu.
---
posix/fork.c | 5 +++++
1 file changed, 5 insertions(+)
Comments
* Adhemerval Zanella via Libc-alpha:
> Checked on x86_64-linux-gnu.
> ---
> posix/fork.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/posix/fork.c b/posix/fork.c
> index 44caf8d166..9340511198 100644
> --- a/posix/fork.c
> +++ b/posix/fork.c
> @@ -103,6 +103,9 @@ __libc_fork (void)
> }
> else
> {
> + /* If _Fork failed, preserve its errno value. */
> + int save_errno = errno;
> +
> /* Release acquired locks in the multi-threaded case. */
> if (multiple_threads)
> {
> @@ -115,6 +118,8 @@ __libc_fork (void)
>
> /* Run the handlers registered for the parent. */
> __run_fork_handlers (atfork_run_parent, multiple_threads);
> +
> + __set_errno (save_errno);
I think you should restrict the __set_errno call to pid < 0, so that
errno is not 0 after a different value has been observed by the fork
handlers.
Thanks,
Florian
On 24/06/2021 05:19, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> Checked on x86_64-linux-gnu.
>> ---
>> posix/fork.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/posix/fork.c b/posix/fork.c
>> index 44caf8d166..9340511198 100644
>> --- a/posix/fork.c
>> +++ b/posix/fork.c
>> @@ -103,6 +103,9 @@ __libc_fork (void)
>> }
>> else
>> {
>> + /* If _Fork failed, preserve its errno value. */
>> + int save_errno = errno;
>> +
>> /* Release acquired locks in the multi-threaded case. */
>> if (multiple_threads)
>> {
>> @@ -115,6 +118,8 @@ __libc_fork (void)
>>
>> /* Run the handlers registered for the parent. */
>> __run_fork_handlers (atfork_run_parent, multiple_threads);
>> +
>> + __set_errno (save_errno);
>
> I think you should restrict the __set_errno call to pid < 0, so that
> errno is not 0 after a different value has been observed by the fork
> handlers.
OK, I can change it back. From the previous review iteration I understood
you were ok with making it unconditional [1].
[1] https://sourceware.org/pipermail/libc-alpha/2021-March/123729.html
* Adhemerval Zanella:
> On 24/06/2021 05:19, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>>
>>> Checked on x86_64-linux-gnu.
>>> ---
>>> posix/fork.c | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/posix/fork.c b/posix/fork.c
>>> index 44caf8d166..9340511198 100644
>>> --- a/posix/fork.c
>>> +++ b/posix/fork.c
>>> @@ -103,6 +103,9 @@ __libc_fork (void)
>>> }
>>> else
>>> {
>>> + /* If _Fork failed, preserve its errno value. */
>>> + int save_errno = errno;
>>> +
>>> /* Release acquired locks in the multi-threaded case. */
>>> if (multiple_threads)
>>> {
>>> @@ -115,6 +118,8 @@ __libc_fork (void)
>>>
>>> /* Run the handlers registered for the parent. */
>>> __run_fork_handlers (atfork_run_parent, multiple_threads);
>>> +
>>> + __set_errno (save_errno);
>>
>> I think you should restrict the __set_errno call to pid < 0, so that
>> errno is not 0 after a different value has been observed by the fork
>> handlers.
>
> OK, I can change it back. From the previous review iteration I understood
> you were ok with making it unconditional [1].
>
> [1] https://sourceware.org/pipermail/libc-alpha/2021-March/123729.html
Sorry, I meant making the saving unconditional, not the restoring.
Florian
On 24/06/2021 08:19, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> On 24/06/2021 05:19, Florian Weimer wrote:
>>> * Adhemerval Zanella via Libc-alpha:
>>>
>>>> Checked on x86_64-linux-gnu.
>>>> ---
>>>> posix/fork.c | 5 +++++
>>>> 1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/posix/fork.c b/posix/fork.c
>>>> index 44caf8d166..9340511198 100644
>>>> --- a/posix/fork.c
>>>> +++ b/posix/fork.c
>>>> @@ -103,6 +103,9 @@ __libc_fork (void)
>>>> }
>>>> else
>>>> {
>>>> + /* If _Fork failed, preserve its errno value. */
>>>> + int save_errno = errno;
>>>> +
>>>> /* Release acquired locks in the multi-threaded case. */
>>>> if (multiple_threads)
>>>> {
>>>> @@ -115,6 +118,8 @@ __libc_fork (void)
>>>>
>>>> /* Run the handlers registered for the parent. */
>>>> __run_fork_handlers (atfork_run_parent, multiple_threads);
>>>> +
>>>> + __set_errno (save_errno);
>>>
>>> I think you should restrict the __set_errno call to pid < 0, so that
>>> errno is not 0 after a different value has been observed by the fork
>>> handlers.
>>
>> OK, I can change it back. From the previous review iteration I understood
>> you were ok with making it unconditional [1].
>>
>> [1] https://sourceware.org/pipermail/libc-alpha/2021-March/123729.html
>
> Sorry, I meant making the saving unconditional, not the restoring.
Ok, I will fix it.
@@ -103,6 +103,9 @@ __libc_fork (void)
}
else
{
+ /* If _Fork failed, preserve its errno value. */
+ int save_errno = errno;
+
/* Release acquired locks in the multi-threaded case. */
if (multiple_threads)
{
@@ -115,6 +118,8 @@ __libc_fork (void)
/* Run the handlers registered for the parent. */
__run_fork_handlers (atfork_run_parent, multiple_threads);
+
+ __set_errno (save_errno);
}
return pid;