[v2,15/19] nptl: Use tidlock when accessing TID on pthread_getname_np
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_getname.c | 45 ++++++++++++++++++++++++++----------------
1 file changed, 28 insertions(+), 17 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 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
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.
* 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
@@ -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;
}