diff mbox series

[v2,17/19] nptl: Use tidlock when accessing TID on pthread_sigqueue

Message ID 20210823195047.543237-18-adhemerval.zanella@linaro.org
State Superseded
Headers
Series Fix various NPTL synchronization issues |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Adhemerval Zanella Aug. 23, 2021, 7:50 p.m. UTC 
  Checked on x86_64-linux-gnu.
---
 nptl/pthread_sigqueue.c | 52 +++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 25 deletions(-)

Comments

Florian Weimer Aug. 26, 2021, 2:43 p.m. UTC | #1 
* 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
Adhemerval Zanella Aug. 26, 2021, 5:49 p.m. UTC | #2 
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
>
Florian Weimer Aug. 30, 2021, 9:26 a.m. UTC | #3 
* 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
diff mbox series

Patch

diff --git a/nptl/pthread_sigqueue.c b/nptl/pthread_sigqueue.c
index 2a0467ad7a..731e83884d 100644
--- a/nptl/pthread_sigqueue.c
+++ b/nptl/pthread_sigqueue.c
@@ -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);