[v3,1/6] nptl: Make pthread_kill async-signal-safe
Commit Message
Changes from v2:
* Rebase against master.
Changes from v1:
* Move __libc_signal_block_all prior TID read.
---
Simiar to raise (BZ #15368), pthread_kill is also defined as
async-signal-safe (POSIX.1-2008 TC1). However, similar to raise
it has the same issues relating to signal handling.
Different than raise, all the signal are blocked so it would be
possible to implement pthread_cancel (which also should be
async-cancel safe).
Checked on x86_64-linux-gnu.
---
nptl/pthread_kill.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
Comments
* Adhemerval Zanella via Libc-alpha:
> Changes from v2:
> * Rebase against master.
>
> Changes from v1:
> * Move __libc_signal_block_all prior TID read.
>
> ---
>
> Simiar to raise (BZ #15368), pthread_kill is also defined as
> async-signal-safe (POSIX.1-2008 TC1). However, similar to raise
> it has the same issues relating to signal handling.
>
> Different than raise, all the signal are blocked so it would be
> possible to implement pthread_cancel (which also should be
> async-cancel safe).
I think it would make sense to spell out what the actual bug fix is.
It's hard to tell whether pthread_kill is now truly async-signal-safe,
given that there is still a race with regards to thread exit and TID
reuse.
Thanks,
Florian
On 11/05/2021 11:25, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> Changes from v2:
>> * Rebase against master.
>>
>> Changes from v1:
>> * Move __libc_signal_block_all prior TID read.
>>
>> ---
>>
>> Simiar to raise (BZ #15368), pthread_kill is also defined as
>> async-signal-safe (POSIX.1-2008 TC1). However, similar to raise
>> it has the same issues relating to signal handling.
>>
>> Different than raise, all the signal are blocked so it would be
>> possible to implement pthread_cancel (which also should be
>> async-cancel safe).
>
> I think it would make sense to spell out what the actual bug fix is.
>
> It's hard to tell whether pthread_kill is now truly async-signal-safe,
> given that there is still a race with regards to thread exit and TID
> reuse.
But this is another issue [1] and it would require a different fix
than this one (possibly a additional per-thread lock to synchronize
with pthread_create and pthread_kill).
Blocking the signal help regarding cancellation, but we will need a
different to handle the pthread_exit.
[1] https://sourceware.org/bugzilla/show_bug.cgi?id=12889
* Adhemerval Zanella:
> On 11/05/2021 11:25, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>>
>>> Changes from v2:
>>> * Rebase against master.
>>>
>>> Changes from v1:
>>> * Move __libc_signal_block_all prior TID read.
>>>
>>> ---
>>>
>>> Simiar to raise (BZ #15368), pthread_kill is also defined as
>>> async-signal-safe (POSIX.1-2008 TC1). However, similar to raise
>>> it has the same issues relating to signal handling.
>>>
>>> Different than raise, all the signal are blocked so it would be
>>> possible to implement pthread_cancel (which also should be
>>> async-cancel safe).
>>
>> I think it would make sense to spell out what the actual bug fix is.
>>
>> It's hard to tell whether pthread_kill is now truly async-signal-safe,
>> given that there is still a race with regards to thread exit and TID
>> reuse.
>
> But this is another issue [1] and it would require a different fix
> than this one (possibly a additional per-thread lock to synchronize
> with pthread_create and pthread_kill).
>
> Blocking the signal help regarding cancellation, but we will need a
> different to handle the pthread_exit.
>
> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=12889
Sure, but I don't really see how this improves async-signal-safety given
that we do not block signals around fork (which we should perhaps do; I
think there's nothing preventing us from doing that).
Thanks,
Florian
On 11/05/2021 11:44, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> On 11/05/2021 11:25, Florian Weimer wrote:
>>> * Adhemerval Zanella via Libc-alpha:
>>>
>>>> Changes from v2:
>>>> * Rebase against master.
>>>>
>>>> Changes from v1:
>>>> * Move __libc_signal_block_all prior TID read.
>>>>
>>>> ---
>>>>
>>>> Simiar to raise (BZ #15368), pthread_kill is also defined as
>>>> async-signal-safe (POSIX.1-2008 TC1). However, similar to raise
>>>> it has the same issues relating to signal handling.
>>>>
>>>> Different than raise, all the signal are blocked so it would be
>>>> possible to implement pthread_cancel (which also should be
>>>> async-cancel safe).
>>>
>>> I think it would make sense to spell out what the actual bug fix is.
>>>
>>> It's hard to tell whether pthread_kill is now truly async-signal-safe,
>>> given that there is still a race with regards to thread exit and TID
>>> reuse.
>>
>> But this is another issue [1] and it would require a different fix
>> than this one (possibly a additional per-thread lock to synchronize
>> with pthread_create and pthread_kill).
>>
>> Blocking the signal help regarding cancellation, but we will need a
>> different to handle the pthread_exit.
>>
>> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=12889
>
> Sure, but I don't really see how this improves async-signal-safety given
> that we do not block signals around fork (which we should perhaps do; I
> think there's nothing preventing us from doing that).
Right, I think this patch came from a previous attempt to fix
bz12889, but thinking twice it does not add much without fixing
the race condition in pthread_kill first.
@@ -28,6 +28,11 @@ __pthread_kill (pthread_t threadid, int signo)
if (__is_internal_signal (signo))
return EINVAL;
+ sigset_t set;
+ __libc_signal_block_all (&set);
+
+ int val;
+
/* Force load of pd->tid into local variable or register. Otherwise
if a thread exits between ESRCH test and tgkill, we might return
EINVAL, because pd->tid would be cleared by the kernel. */
@@ -35,14 +40,20 @@ __pthread_kill (pthread_t threadid, int signo)
pid_t tid = atomic_forced_read (pd->tid);
if (__glibc_unlikely (tid <= 0))
/* Not a valid thread handle. */
- return ESRCH;
+ val = ESRCH;
+ else
+ {
+ /* We have a special syscall to do the work. */
+ pid_t pid = __getpid ();
+
+ val = INTERNAL_SYSCALL_CALL (tgkill, pid, tid, signo);
+ val = (INTERNAL_SYSCALL_ERROR_P (val)
+ ? INTERNAL_SYSCALL_ERRNO (val) : 0);
+ }
- /* We have a special syscall to do the work. */
- pid_t pid = __getpid ();
+ __libc_signal_restore_set (&set);
- int val = INTERNAL_SYSCALL_CALL (tgkill, pid, tid, signo);
- return (INTERNAL_SYSCALL_ERROR_P (val)
- ? INTERNAL_SYSCALL_ERRNO (val) : 0);
+ return val;
}
versioned_symbol (libc, __pthread_kill, pthread_kill, GLIBC_2_34);