[v2,17/19] nptl: Use tidlock when accessing TID on pthread_sigqueue
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.
---
nptl/pthread_sigqueue.c | 52 +++++++++++++++++++++--------------------
1 file changed, 27 insertions(+), 25 deletions(-)
Comments
* Adhemerval Zanella:
> + /* Block all signal, since the lock is recursive and used on pthread_cancel
> + (which should be async-signal-safe). */
> + sigset_t oldmask;
> + __libc_signal_block_all (&oldmask);
> + lll_lock (pd->tidlock, LLL_PRIVATE);
> +
> + int res;
> + if (pd->tid == 0)
> + {
> + pid_t pid = getpid ();
Huh, that can't be right, should be pd->tid != 0.
Don't we have test coverage for this?
> + else
> + res = -ESRCH;
We can return 0 in this case, I think.
It's possibly that the same issue regarding synchronous signal delivery
for pthread_jill applies here.
Thanks,
Florian
On 26/08/2021 11:43, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> + /* Block all signal, since the lock is recursive and used on pthread_cancel
>> + (which should be async-signal-safe). */
>> + sigset_t oldmask;
>> + __libc_signal_block_all (&oldmask);
>> + lll_lock (pd->tidlock, LLL_PRIVATE);
>> +
>> + int res;
>> + if (pd->tid == 0)
>> + {
>> + pid_t pid = getpid ();
>
> Huh, that can't be right, should be pd->tid != 0.
>
It is definitely not, I have fixed.
> Don't we have test coverage for this?
Nops, I will add one in the next version.
>
>> + else
>> + res = -ESRCH;
>
> We can return 0 in this case, I think.
No sure about this, we can return 0 but it means signal won't be potentially
delivered.
>
> It's possibly that the same issue regarding synchronous signal delivery
> for pthread_jill applies here.
>
> Thanks,
> Florian
>
* Adhemerval Zanella:
>>> + else
>>> + res = -ESRCH;
>>
>> We can return 0 in this case, I think.
>
> No sure about this, we can return 0 but it means signal won't be potentially
> delivered.
The thread may have already started exiting and blocked the signals when
pthread_sigqueue is called. In that case, it would return 0 even though
the signal is never delivered.
Thanks,
Florian
@@ -28,41 +28,43 @@
int
__pthread_sigqueue (pthread_t threadid, int signo, const union sigval value)
{
-#ifdef __NR_rt_tgsigqueueinfo
struct pthread *pd = (struct pthread *) threadid;
- /* 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. */
- pid_t tid = atomic_forced_read (pd->tid);
- if (__glibc_unlikely (tid <= 0))
- /* Not a valid thread handle. */
- return ESRCH;
-
/* Disallow sending the signal we use for cancellation, timers,
for the setxid implementation. */
if (signo == SIGCANCEL || signo == SIGTIMER || signo == SIGSETXID)
return EINVAL;
- pid_t pid = getpid ();
+ /* Block all signal, since the lock is recursive and used on pthread_cancel
+ (which should be async-signal-safe). */
+ sigset_t oldmask;
+ __libc_signal_block_all (&oldmask);
+ lll_lock (pd->tidlock, LLL_PRIVATE);
+
+ int res;
+ if (pd->tid == 0)
+ {
+ pid_t pid = getpid ();
- /* Set up the siginfo_t structure. */
- siginfo_t info;
- memset (&info, '\0', sizeof (siginfo_t));
- info.si_signo = signo;
- info.si_code = SI_QUEUE;
- info.si_pid = pid;
- info.si_uid = __getuid ();
- info.si_value = value;
+ /* Set up the siginfo_t structure. */
+ siginfo_t info;
+ memset (&info, '\0', sizeof (siginfo_t));
+ info.si_signo = signo;
+ info.si_code = SI_QUEUE;
+ info.si_pid = pid;
+ info.si_uid = __getuid ();
+ info.si_value = value;
- /* We have a special syscall to do the work. */
- int val = INTERNAL_SYSCALL_CALL (rt_tgsigqueueinfo, pid, tid, signo,
+ res = INTERNAL_SYSCALL_CALL (rt_tgsigqueueinfo, pid, pd->tid, signo,
&info);
- return (INTERNAL_SYSCALL_ERROR_P (val)
- ? INTERNAL_SYSCALL_ERRNO (val) : 0);
-#else
- return ENOSYS;
-#endif
+ }
+ else
+ res = -ESRCH;
+
+ lll_unlock (pd->tidlock, LLL_PRIVATE);
+ __libc_signal_restore_set (&oldmask);
+
+ return -res;
}
versioned_symbol (libc, __pthread_sigqueue, pthread_sigqueue, GLIBC_2_34);