[Bug nptl/17478] New: Fix off-by-one error in pthread_setname_np()

[Bug nptl/17478] New: Fix off-by-one error in pthread_setname_np()

[ryao at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=17478

            Bug ID: 17478
           Summary: Fix off-by-one error in pthread_setname_np()
           Product: glibc
           Version: unspecified
            Status: NEW
          Keywords: glibc_2.10, glibc_2.11, glibc_2.12, glibc_2.13,
                    glibc_2.14, glibc_2.15, glibc_2.16, glibc_2.17,
                    glibc_2.18, glibc_2.19, glibc_2.20
          Severity: normal
          Priority: P2
         Component: nptl
          Assignee: unassigned at sourceware dot org
          Reporter: ryao at gentoo dot org
                CC: drepper.fsp at gmail dot com
              Host: *-*-linux-gnu
            Target: *-*-linux-gnu
             Build: *-*-linux-gnu

Created attachment 7827
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7827&action=edit
This is the proposed fix. It was developed by one Gentoo developer and reviewed
by another.

The man page for pthread_setname_np() says:

> The thread name is a meaningful C language string, whose length is
> restricted to 16 characters, including the terminating null byte ('\0').

It continues to say that ERANGE will be returned on strings that do not
meet this criterium. In reality, passing a NULL terminated string with
the NULL terminating character at index 16 returns EINVAL. This is due
to an off-by-one error where strlen() is used in the comparison rather
than strlen() + 1. It is then sent to either prctl() or /proc. In the
case of /proc, it fails we can get EINVAL. The documentation for prctl()
claims that this will work. However, this is incorrect as the precise
code for Linux's kernel/sys.c will always set the 16th byte to 0 and
copy only the first 15 bytes. Consequently, we silently lose the last
character.

The corrrect way to fix the off-by-one error appears to be to add 1 to
the return value of strlen() before the comparison.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/17478] Fix off-by-one error in pthread_setname_np()

[ryao at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=17478

Richard Yao <ryao at gentoo dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|INVALID                     |---

--- Comment #2 from Richard Yao <ryao at gentoo dot org> ---
Andreas, the kernel defines the length to include the NULL terminating
character while strlen() defines the length to exclude it. This causes a
off-by-one error because the concept of how long a string can be is literally
off-by-one. If you pass a string of length 16 according to strlen(), the kernel
will interpret this as a string of length 17 and reject it with EINVAL. The
kernel field is 16 bytes, but in reality, you can only use 15 bytes because the
kernel code for copying the field ensures that the last byte is *ALWAYS* NULL.

I am certain that I am *NOT* setting the name on a different process (although
it is on a different thread). The program that I modified to use this is ZFS'
ztest. It works when I use strncpy() to copy a 16-byte string (according to
`strlen()`) to a 16 byte buffer and then pass that to `pthread_setname_np()`,
but it does not work when I pass the string directly to `pthread_setname_np()`.
If this were another process, it should return EINVAL no matter what the length
of the string is.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/17478] Fix off-by-one error in pthread_setname_np()

[ryao at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=17478

--- Comment #3 from Richard Yao <ryao at gentoo dot org> ---
A minor correction. I used `snprintf()` to do the copy, not `strncpy()`. I just
got back from LinuxCon Europe and I am semi-jet lagged. I just pushed my
patches so that you can see them:

https://github.com/ryao/zfs/compare/pthread_setname_np-for-glibc-upstream
https://github.com/ryao/zfs/commit/0e67649ebc56f40f5eebca1f86e18f2e750dc002

It literally works with that patch and hits an assertion failure without it
(the VERIFY). The assertion failure is interesting because it seems that I was
lucky enough (unlucky enough?) for the code to be passing "arc_adapt_thread" as
the thread name.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/17478] Fix off-by-one error in pthread_setname_np()

[ryao at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=17478

Richard Yao <ryao at gentoo dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |UNCONFIRMED
     Ever confirmed|1                           |0

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/17478] Fix off-by-one error in pthread_setname_np()

[ryao at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=17478

--- Comment #4 from Richard Yao <ryao at gentoo dot org> ---
I probably should also say that it shouldn't work without the second patch, but
the fact that I get EINVAL due to the length being out of range instead of
ERANGE is a bug.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/17478] Fix off-by-one error in pthread_setname_np()

[schwab@linux-m68k.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=17478

Andreas Schwab <schwab@linux-m68k.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #5 from Andreas Schwab <schwab@linux-m68k.org> ---
#define TASK_COMM_LEN 16
  size_t name_len = strlen (name);
  if (name_len >= TASK_COMM_LEN)
    return ERANGE;

This will return ERANGE if the length of name is 16.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/17478] Fix off-by-one error in pthread_setname_np()

[ryao at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=17478

Richard Yao <ryao at gentoo dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |UNCONFIRMED
         Resolution|INVALID                     |---

--- Comment #6 from Richard Yao <ryao at gentoo dot org> ---
The kernel restricts the name of the length to 15 characters. The 16th byte is
always a NULL byte. `strlen()` does not count the NULL byte.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/17478] Fix off-by-one error in pthread_setname_np()

[ryao at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=17478

--- Comment #7 from Richard Yao <ryao at gentoo dot org> ---
That should have been "The kernel restricts the length of the name to 15
characters." I am still somewhat jet lagged, but I did the analysis on this
*before* left for LinuxCon Europe.

The definition of string length used by strlen() excludes the NULL terminating
character while the definition used by the kernel includes the NULL terminating
character. The discrepancy causes an off-by-one error that must be corrected by
adding 1 to the length returned by `strlen()`.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/17478] Fix off-by-one error in pthread_setname_np()

[schwab@linux-m68k.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=17478

Andreas Schwab <schwab@linux-m68k.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #8 from Andreas Schwab <schwab@linux-m68k.org> ---
That's why pthread_setname_np returns ERANGE for anything longer.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/17478] Fix off-by-one error in pthread_setname_np()

[ryao at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=17478

--- Comment #9 from Richard Yao <ryao at gentoo dot org> ---
I figured out what happened. The number was bring printed in hexidecimal, but
lacked the 0x to indicate it as such. My apologies for the mistake.

-- 
You are receiving this mail because:
You are on the CC list for the bug.