diff mbox series

[v2,15/19] nptl: Use tidlock when accessing TID on pthread_getname_np

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

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_getname.c | 45 ++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 17 deletions(-)

Comments

Florian Weimer Aug. 26, 2021, 2:38 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 fd = __open64_nocancel (fname, O_RDONLY);
> -  if (fd == -1)
> -    return errno;
> +  char fname[sizeof ("/proc/self/task//comm" ) + INT_BUFSIZE_BOUND (pid_t)];
> +  __snprintf (fname, sizeof (fname), "/proc/self/task/%d/comm", pd->tid);
>  
>    int res = 0;
> -  ssize_t n = TEMP_FAILURE_RETRY (__read_nocancel (fd, buf, len));
> -  if (n < 0)
> -    res = errno;
> -  else
> +  int fd = __open64_nocancel (fname, O_RDONLY);
> +  if (fd != -1)
>      {
> -      if (buf[n - 1] == '\n')
> -	buf[n - 1] = '\0';
> -      else if (n == len)
> -	res = ERANGE;
> +      ssize_t n = TEMP_FAILURE_RETRY (__read_nocancel (fd, buf, len));
> +      if (n > 0)
> +	{
> +	  if (buf[n - 1] == '\n')
> +	    buf[n - 1] = '\0';
> +	  else if (n == len)
> +	    res = ERANGE;
> +	  else
> +	    buf[n] = '\0';
> +	}
>        else
> -	buf[n] = '\0';
> +	res = errno;
> +
> +      __close_nocancel_nostatus (fd);
>      }
> +  else
> +    res = errno;

ENOENT for exited threads is inconsistent with ESRCH (or 0) in other
cases.

I wonder if we should cache the thread name in the TCB.  That would be
nice for debugging coredumps, too.

Thanks,
Florian
Adhemerval Zanella Aug. 26, 2021, 5:45 p.m. UTC | #2
On 26/08/2021 11:38, 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 fd = __open64_nocancel (fname, O_RDONLY);
>> -  if (fd == -1)
>> -    return errno;
>> +  char fname[sizeof ("/proc/self/task//comm" ) + INT_BUFSIZE_BOUND (pid_t)];
>> +  __snprintf (fname, sizeof (fname), "/proc/self/task/%d/comm", pd->tid);
>>  
>>    int res = 0;
>> -  ssize_t n = TEMP_FAILURE_RETRY (__read_nocancel (fd, buf, len));
>> -  if (n < 0)
>> -    res = errno;
>> -  else
>> +  int fd = __open64_nocancel (fname, O_RDONLY);
>> +  if (fd != -1)
>>      {
>> -      if (buf[n - 1] == '\n')
>> -	buf[n - 1] = '\0';
>> -      else if (n == len)
>> -	res = ERANGE;
>> +      ssize_t n = TEMP_FAILURE_RETRY (__read_nocancel (fd, buf, len));
>> +      if (n > 0)
>> +	{
>> +	  if (buf[n - 1] == '\n')
>> +	    buf[n - 1] = '\0';
>> +	  else if (n == len)
>> +	    res = ERANGE;
>> +	  else
>> +	    buf[n] = '\0';
>> +	}
>>        else
>> -	buf[n] = '\0';
>> +	res = errno;
>> +
>> +      __close_nocancel_nostatus (fd);
>>      }
>> +  else
>> +    res = errno;
> 
> ENOENT for exited threads is inconsistent with ESRCH (or 0) in other
> cases.

Right, I think we will need to map ENOENT to ESRCH. But I also think
this is another fix.

> 
> I wonder if we should cache the thread name in the TCB.  That would be
> nice for debugging coredumps, too.

This does not make sense, although it would require some memory
allocation.
Florian Weimer Aug. 30, 2021, 9:37 a.m. UTC | #3
* Adhemerval Zanella:

>> I wonder if we should cache the thread name in the TCB.  That would be
>> nice for debugging coredumps, too.
>
> This does not make sense, although it would require some memory
> allocation.

Isn't the name limited to 16 bytes?  So it wouldn't need a separate
allocation.

Thanks,
Florian
diff mbox series

Patch

diff --git a/nptl/pthread_getname.c b/nptl/pthread_getname.c
index 8ac814366a..37dd6a360e 100644
--- a/nptl/pthread_getname.c
+++ b/nptl/pthread_getname.c
@@ -22,6 +22,7 @@ 
 #include <stdio.h>
 #include <string.h>
 #include <unistd.h>
+#include <intprops.h>
 #include <sys/prctl.h>
 #include <not-cancel.h>
 #include <shlib-compat.h>
@@ -29,7 +30,7 @@ 
 int
 __pthread_getname_np (pthread_t th, char *buf, size_t len)
 {
-  const struct pthread *pd = (const struct pthread *) th;
+  struct pthread *pd = (struct pthread *) th;
 
   /* Unfortunately the kernel headers do not export the TASK_COMM_LEN
      macro.  So we have to define it here.  */
@@ -40,29 +41,39 @@  __pthread_getname_np (pthread_t th, char *buf, size_t len)
   if (pd == THREAD_SELF)
     return __prctl (PR_GET_NAME, buf) ? errno : 0;
 
-#define FMT "/proc/self/task/%u/comm"
-  char fname[sizeof (FMT) + 8];
-  sprintf (fname, FMT, (unsigned int) pd->tid);
+  /* 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 fd = __open64_nocancel (fname, O_RDONLY);
-  if (fd == -1)
-    return errno;
+  char fname[sizeof ("/proc/self/task//comm" ) + INT_BUFSIZE_BOUND (pid_t)];
+  __snprintf (fname, sizeof (fname), "/proc/self/task/%d/comm", pd->tid);
 
   int res = 0;
-  ssize_t n = TEMP_FAILURE_RETRY (__read_nocancel (fd, buf, len));
-  if (n < 0)
-    res = errno;
-  else
+  int fd = __open64_nocancel (fname, O_RDONLY);
+  if (fd != -1)
     {
-      if (buf[n - 1] == '\n')
-	buf[n - 1] = '\0';
-      else if (n == len)
-	res = ERANGE;
+      ssize_t n = TEMP_FAILURE_RETRY (__read_nocancel (fd, buf, len));
+      if (n > 0)
+	{
+	  if (buf[n - 1] == '\n')
+	    buf[n - 1] = '\0';
+	  else if (n == len)
+	    res = ERANGE;
+	  else
+	    buf[n] = '\0';
+	}
       else
-	buf[n] = '\0';
+	res = errno;
+
+      __close_nocancel_nostatus (fd);
     }
+  else
+    res = errno;
 
-  __close_nocancel_nostatus (fd);
+  lll_unlock (pd->tidlock, LLL_PRIVATE);
+  __libc_signal_restore_set (&oldmask);
 
   return res;
 }