diff mbox series

pthread_getcpuclockid: Add descriptive comment to smoke test

Message ID 20241128113259.2113810-1-siddhesh@sourceware.org
State New
Headers
Series pthread_getcpuclockid: Add descriptive comment to smoke test |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Test passed
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test passed

Commit Message

Siddhesh Poyarekar Nov. 28, 2024, 11:32 a.m. UTC 
  Add a descriptive comment to the tst-pthread-cpuclockid-invalid test and
also drop pthread_getcpuclockid from the TODO-testing list since it now
has full coverage.
---
 nptl/TODO-testing                        |  4 ----
 nptl/tst-pthread-getcpuclockid-invalid.c | 12 ++++++++++--
 2 files changed, 10 insertions(+), 6 deletions(-)

Comments

Florian Weimer Nov. 28, 2024, noon UTC | #1 
* Siddhesh Poyarekar:

> +/* The input thread descriptor to pthread_getcpuclockid needs to be valid when
> +   the function is called.  For the purposes of this test, this means that the
> +   thread should not be detached, have exited, but not joined.  There is also a
> +   tiny window where the TCB has been allocated but the target thread itself
> +   not created, where pthread_getcpuclockid could return ESRCH, but it's not
> +   possible to tap into that window to reliably test it.  It's not necessary
> +   anyway, since the window this test exploits is good enough to complete
> +   coverage for pthread_getcpuclockid alongside tst-clock2.  */

The kernel stores the TID to the TCB before the system call returns
(according to the clone manual page).

So the only race is for thread exit, not thread creation, and the
comment is misleading.

Thanks,
Florian
Siddhesh Poyarekar Nov. 28, 2024, 1:19 p.m. UTC | #2 
On 2024-11-28 07:00, Florian Weimer wrote:
> * Siddhesh Poyarekar:
> 
>> +/* The input thread descriptor to pthread_getcpuclockid needs to be valid when
>> +   the function is called.  For the purposes of this test, this means that the
>> +   thread should not be detached, have exited, but not joined.  There is also a
>> +   tiny window where the TCB has been allocated but the target thread itself
>> +   not created, where pthread_getcpuclockid could return ESRCH, but it's not
>> +   possible to tap into that window to reliably test it.  It's not necessary
>> +   anyway, since the window this test exploits is good enough to complete
>> +   coverage for pthread_getcpuclockid alongside tst-clock2.  */
> 
> The kernel stores the TID to the TCB before the system call returns
> (according to the clone manual page).
> 
> So the only race is for thread exit, not thread creation, and the
> comment is misleading.

I was thinking of a situation where the thread descriptor points to a 
TCB and the syscall has not been invoked yet; this would be exploitable 
when the thread descriptor itself is in memory shared with another 
thread.  But now that I'm saying it out loud, it doesn't sound like 
behaviour that ought to be relied upon, just something that the 
internals currently happen to allow.

How about this then:

/* The input thread descriptor to pthread_getcpuclockid needs to be 
valid when
    the function is called.  For the purposes of this test, this means 
that the
    thread should not be detached, have exited, but not joined.  This 
should be
    good enough to complete coverage for pthread_getcpuclockid alongside
    tst-clock2.  */

Thanks,
Sid
Florian Weimer Nov. 28, 2024, 1:49 p.m. UTC | #3 
* Siddhesh Poyarekar:

> On 2024-11-28 07:00, Florian Weimer wrote:
>> * Siddhesh Poyarekar:
>> 
>>> +/* The input thread descriptor to pthread_getcpuclockid needs to be valid when
>>> +   the function is called.  For the purposes of this test, this means that the
>>> +   thread should not be detached, have exited, but not joined.  There is also a
>>> +   tiny window where the TCB has been allocated but the target thread itself
>>> +   not created, where pthread_getcpuclockid could return ESRCH, but it's not
>>> +   possible to tap into that window to reliably test it.  It's not necessary
>>> +   anyway, since the window this test exploits is good enough to complete
>>> +   coverage for pthread_getcpuclockid alongside tst-clock2.  */
>> The kernel stores the TID to the TCB before the system call returns
>> (according to the clone manual page).
>> So the only race is for thread exit, not thread creation, and the
>> comment is misleading.
>
> I was thinking of a situation where the thread descriptor points to a
> TCB and the syscall has not been invoked yet; this would be
> exploitable when the thread descriptor itself is in memory shared with
> another thread.  But now that I'm saying it out loud, it doesn't sound
> like behaviour that ought to be relied upon, just something that the
> internals currently happen to allow.
>
> How about this then:
>
> /* The input thread descriptor to pthread_getcpuclockid needs to be
> valid when
>    the function is called.  For the purposes of this test, this means
>    that the
>    thread should not be detached, have exited, but not joined.  This
>    should be
>    good enough to complete coverage for pthread_getcpuclockid alongside
>    tst-clock2.  */

This looks reasonable to me (except for the line breaks).

Thanks,
Florian
diff mbox series

Patch

diff --git a/nptl/TODO-testing b/nptl/TODO-testing
index e076e5624f..f50d2ceb51 100644
--- a/nptl/TODO-testing
+++ b/nptl/TODO-testing
@@ -10,10 +10,6 @@  pthread_attr_[sg]etstack
 
   some more tests needed
 
-pthread_getcpuclockid
-
-  check that value is reset -> rt subdir
-
 pthread_getschedparam
 pthread_setschedparam
 
diff --git a/nptl/tst-pthread-getcpuclockid-invalid.c b/nptl/tst-pthread-getcpuclockid-invalid.c
index e88a563427..df1ad18226 100644
--- a/nptl/tst-pthread-getcpuclockid-invalid.c
+++ b/nptl/tst-pthread-getcpuclockid-invalid.c
@@ -1,5 +1,4 @@ 
-/* Smoke test to verify that pthread_getcpuclockid fails with ESRCH when the
-   thread in question has exited.
+/* pthread_getcpuclockid should fail with ESRCH when the thread exits.
    Copyright the GNU Toolchain Authors.
    This file is part of the GNU C Library.
 
@@ -17,6 +16,15 @@ 
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+/* The input thread descriptor to pthread_getcpuclockid needs to be valid when
+   the function is called.  For the purposes of this test, this means that the
+   thread should not be detached, have exited, but not joined.  There is also a
+   tiny window where the TCB has been allocated but the target thread itself
+   not created, where pthread_getcpuclockid could return ESRCH, but it's not
+   possible to tap into that window to reliably test it.  It's not necessary
+   anyway, since the window this test exploits is good enough to complete
+   coverage for pthread_getcpuclockid alongside tst-clock2.  */
+
 #include <errno.h>
 #include <pthread.h>
 #include <sched.h>