[v2] debug: test for more required cacellation points (BZ# 29274)

  Generalize the test for cancellation point in __read_chk to also test
the other fortified functions with required cancellation points.

Since there is not easy way to force some syscalls to block (for
instance pread) the test tests two modes: cancellation on blocked
syscalls and early cancellation on pending request.

Checked on aarch64-linux-gnu.

Co-authored-by: Andreas Schwab <schwab@suse.de>
---
 debug/Makefile              |   6 +-
 debug/tst-chk-cancel.c      | 272 ++++++++++++++++++++++++++++++++++++
 debug/tst-read-chk-cancel.c |  50 -------
 3 files changed, 275 insertions(+), 53 deletions(-)
 create mode 100644 debug/tst-chk-cancel.c
 delete mode 100644 debug/tst-read-chk-cancel.c
  

On 7/11/22 12:53, Adhemerval Zanella wrote:
> Generalize the test for cancellation point in __read_chk to also test
> the other fortified functions with required cancellation points.
> 
> Since there is not easy way to force some syscalls to block (for
> instance pread) the test tests two modes: cancellation on blocked
> syscalls and early cancellation on pending request.
> 
> Checked on aarch64-linux-gnu.
> 
> Co-authored-by: Andreas Schwab <schwab@suse.de>

Needs a v3. See P&C comments below.

As I understand the test we want to test being in one of two regions:

- After the syscall registers cancellation.

- Before the syscall registers cancellation.

The !only_early test (the first test) uses a wait to ensure that the
cleanup region is registered, and then a clock_nanosleep to let the
test get the region that is after the syscall registers cancelaltion.

The only_early test (the second test) uses a wait to ensure that the
cleanup region is registered, but does NOT sleep in an attempt to
test the region that is just before the syscall registers cancellation.

Please correct me if I'm wrong.

> ---
>  debug/Makefile              |   6 +-
>  debug/tst-chk-cancel.c      | 272 ++++++++++++++++++++++++++++++++++++
>  debug/tst-read-chk-cancel.c |  50 -------
>  3 files changed, 275 insertions(+), 53 deletions(-)
>  create mode 100644 debug/tst-chk-cancel.c
>  delete mode 100644 debug/tst-read-chk-cancel.c
> 
> diff --git a/debug/Makefile b/debug/Makefile
> index 456b349c4d..99cdf3221b 100644
> --- a/debug/Makefile
> +++ b/debug/Makefile
> @@ -110,7 +110,7 @@ CPPFLAGS-tst-longjmp_chk2.c += -D_FORTIFY_SOURCE=1
>  CFLAGS-tst-longjmp_chk3.c += -fexceptions -fasynchronous-unwind-tables
>  CPPFLAGS-tst-longjmp_chk3.c += -D_FORTIFY_SOURCE=1
>  CPPFLAGS-tst-realpath-chk.c += -D_FORTIFY_SOURCE=2
> -CPPFLAGS-tst-read-chk-cancel.c += -D_FORTIFY_SOURCE=2
> +CPPFLAGS-tst-chk-cancel.c += -D_FORTIFY_SOURCE=2

OK.

>  
>  # _FORTIFY_SOURCE tests.
>  # Auto-generate tests for _FORTIFY_SOURCE for different levels, compilers and
> @@ -206,7 +206,7 @@ tests += tst-ssp-1
>  endif
>  
>  ifeq ($(have-thread-library), yes)
> -tests += tst-read-chk-cancel
> +tests += tst-chk-cancel

OK.

>  endif
>  
>  ifeq (,$(CXX))
> @@ -248,4 +248,4 @@ $(objpfx)xtrace: xtrace.sh
>  	    -e 's|@REPORT_BUGS_TO@|$(REPORT_BUGS_TO)|' $^ > $@.new \
>  	&& rm -f $@ && mv $@.new $@ && chmod +x $@
>  
> -$(objpfx)tst-read-chk-cancel: $(shared-thread-library)
> +$(objpfx)tst-chk-cancel: $(shared-thread-library)

OK. Still required for htl.

> diff --git a/debug/tst-chk-cancel.c b/debug/tst-chk-cancel.c
> new file mode 100644
> index 0000000000..ccc688eda8
> --- /dev/null
> +++ b/debug/tst-chk-cancel.c
> @@ -0,0 +1,272 @@
> +/* Test for required cancellation points in fortified functions (BZ #29274)

OK.

> +   Copyright (C) 2022 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <array_length.h>
> +#include <errno.h>
> +#include <poll.h>
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <support/check.h>
> +#include <support/xthread.h>
> +#include <support/xunistd.h>
> +#include <sys/socket.h>
> +
> +/* Cleanup handling test.  */
> +static int cl_called;
> +
> +static void
> +cl (void *arg)
> +{
> +  ++cl_called;
> +}
> +
> +static int fds[2];
> +static pthread_barrier_t barrier;
> +
> +static void *
> +tf_read (void *n)
> +{
> +  pthread_cleanup_push (cl, NULL);
> +
> +  xpthread_barrier_wait (&barrier);
> +
> +  if (n != (void *) 1L)
> +    xpthread_barrier_wait (&barrier);

If I understand the intent of the tests then we don't need this second wait?

We only need the first wait to register the cleanup handler in all cases.

> +
> +  /* This call should be forwarded to __read_chk because the buffer size
> +     is known, but the read length is non-constant.  */
> +  char c;
> +  if (read (fds[0], &c, (uintptr_t) n) != 1)
> +    return (void *) -1L;
> +
> +  pthread_cleanup_pop (0);
> +  return 0;
> +}
> +
> +static void *
> +tf_pread (void *n)
> +{
> +  pthread_cleanup_push (cl, NULL);
> +
> +  xpthread_barrier_wait (&barrier);
> +
> +  if (n != (void *) 1L)
> +    xpthread_barrier_wait (&barrier);
> +
> +  /* This call should be forwarded to __pread_chk because the buffer size
> +     is known, but the read length is non-constant.  */
> +  char c;
> +  if (pread (fds[0], &c, (uintptr_t) n, 0) != 1)
> +    return (void *) -1L;
> +
> +  pthread_cleanup_pop (0);
> +  return 0;
> +}
> +
> +static void *
> +tf_pread64 (void *n)
> +{
> +  pthread_cleanup_push (cl, NULL);
> +
> +  xpthread_barrier_wait (&barrier);
> +
> +  if (n != (void *) 1L)
> +    xpthread_barrier_wait (&barrier);
> +
> +  /* This call should be forwarded to __pread64_chk because the buffer size
> +     is known, but the read length is non-constant.  */
> +  char c;
> +  if (pread64 (fds[0], &c, (uintptr_t) n, 0) != 1)
> +    return (void *) -1L;
> +
> +  pthread_cleanup_pop (0);
> +  return 0;
> +}
> +
> +static void *
> +tf_poll (void *n)
> +{
> +  pthread_cleanup_push (cl, NULL);
> +
> +  xpthread_barrier_wait (&barrier);
> +
> +  if (n != (void *) 1L)
> +    xpthread_barrier_wait (&barrier);
> +
> +  /* This call should be forwarded to __poll_chk because the pollfd size
> +     is known, but the number of entries is non-constant.  */
> +  struct pollfd pfd = { fds[0], POLLIN, 0 };
> +  if (poll (&pfd, (uintptr_t) n, -1) != 1)
> +    return (void *) -1L;
> +
> +  pthread_cleanup_pop (0);
> +  return 0;
> +}
> +
> +static void *
> +tf_ppoll (void *n)
> +{
> +  pthread_cleanup_push (cl, NULL);
> +
> +  xpthread_barrier_wait (&barrier);
> +
> +  if (n != (void *) 1L)
> +    xpthread_barrier_wait (&barrier);
> +
> +  /* This call should be forwarded to __ppoll_chk because the pollfd size
> +     is known, but the number of entries is non-constant.  */
> +  struct pollfd pfd = { fds[0], POLLIN, 0 };
> +  if (ppoll (&pfd, (uintptr_t) n, 0, 0) != 1)
> +    return (void *) -1L;
> +
> +  pthread_cleanup_pop (0);
> +  return 0;
> +}
> +
> +static void *
> +tf_recv (void *n)
> +{
> +  pthread_cleanup_push (cl, NULL);
> +
> +  xpthread_barrier_wait (&barrier);
> +
> +  if (n != (void *) 1L)
> +    xpthread_barrier_wait (&barrier);
> +
> +  /* This call should be forwarded to __ppoll_chk because the pollfd size
> +     is known, but the number of entries is non-constant.  */
> +  char c;
> +  if (recv (fds[0], &c, (uintptr_t) n, 0) != 1)
> +    return (void *) -1L;
> +
> +  pthread_cleanup_pop (0);
> +  return 0;
> +}
> +
> +static void *
> +tf_recvfrom (void *n)
> +{
> +  pthread_cleanup_push (cl, NULL);
> +
> +  xpthread_barrier_wait (&barrier);
> +
> +  if (n != (void *) 1L)
> +    xpthread_barrier_wait (&barrier);
> +
> +  /* This call should be forwarded to __ppoll_chk because the pollfd size
> +     is known, but the number of entries is non-constant.  */
> +  char c;
> +  if (recvfrom (fds[0], &c, (uintptr_t) n, 0, NULL, NULL) != 1)
> +    return (void *) -1L;
> +
> +  pthread_cleanup_pop (0);
> +  return 0;
> +}
> +
> +static struct cancel_tests
> +{
> +  const char *name;
> +  void *(*tf) (void *);
> +  bool only_early;
> +#define ADD_TEST(name, early) { #name, tf_##name, early }
> +} tests[] =
> +{
> +  ADD_TEST (poll,     false),
> +  ADD_TEST (ppoll,    false),
> +  ADD_TEST (pread,    true),
> +  ADD_TEST (pread64,  true),
> +  ADD_TEST (read,     false),
> +  ADD_TEST (recv,     false),
> +  ADD_TEST (recvfrom, false),
> +};
> +
> +/* Set the send buffer of socket S to 1 byte so any send operation
> +   done with WRITE_BUFFER_SIZE bytes will force syscall blocking.  */
> +static void
> +set_socket_buffer (int s)
> +{
> +  int val = 1;
> +  socklen_t len = sizeof (val);
> +
> +  TEST_VERIFY_EXIT (setsockopt (s, SOL_SOCKET, SO_SNDBUF, &val,
> +                    sizeof (val)) == 0);
> +  TEST_VERIFY_EXIT (getsockopt (s, SOL_SOCKET, SO_SNDBUF, &val, &len) == 0);
> +  printf ("%s: got size %d\n", __func__, val);
> +}
> +
> +static int
> +do_test (void)
> +{
> +  xpthread_barrier_init (&barrier, 0, 2);
> +
> +  if (socketpair (AF_UNIX, SOCK_STREAM, 0, fds) != 0)
> +    FAIL_EXIT1 ("socketpair: %m");
> +  set_socket_buffer (fds[1]);
> +

Add comment:

/* This is the !only_early test. It is a late cancel test that has a sleep
   in the main thread in an attempt to allow the child thread to reach and
   block on the syscall. The cancellation should happen with high probability
   when the child thread blocked on the syscall, and that is the intent of
   the test (syscall cancellation registration complete).  */


> +  for (int i = 0; i < array_length (tests); i++)
> +    {
> +      if (tests[i].only_early)
> +	continue;
> +
> +      xpthread_barrier_init (&barrier, NULL, 2);
> +      /* Reset the counter for the cleanup handler.  */
> +      cl_called = 0;
> +
> +      pthread_t thr = xpthread_create (0, tests[i].tf, (void *) 1L);
> +      /* After this wait the threads cancellation handler is installed.  */
> +      xpthread_barrier_wait (&barrier);

OK. Correct. So now we are somewhere between the first wait and the syscall entry in
the child thread.

> +
> +      struct timespec ts = { .tv_sec = 0, .tv_nsec = 100000000 };
> +      TEMP_FAILURE_RETRY (clock_nanosleep (CLOCK_REALTIME, 0, &ts, &ts));

OK. Wait a bit. The duration doesn't actually matter given the happens before of the wait
and the registration of the handler. So no spurious failures. This is a "in-time" cancel
test which hopes to cancel near the syscall.

> +
> +      xpthread_cancel (thr);

OK. Cancel will run and ++cl_called.

> +
> +      void *status = xpthread_join (thr);
> +      TEST_VERIFY (status == PTHREAD_CANCELED);
> +      TEST_COMPARE (cl_called, 1);
> +
> +      printf ("in-time cancel test of '%s' successful\n", tests[i].name);
> +    }
> +

Add a comment:

/* This test is the early cancel version of the first test, and the intent is
   to have the cancellation happen at one of two regions:
   * Before the cancellable syscall registers cancellation.
   * After the cancellable syscall registers cancellation.

   This test will exercise if syscall cancellation registration is an atomic
   operation or not since the transition from the regions is designed to be
   atomic.

   We can't control when the cancellation happens, but it will happen in one
   of the two regions. The first test (the !only_early version) attempts to
   test the second region, while this test attempts to test the first region
   and the transition with some probability.  */

> +  for (int i = 0; i < array_length (tests); i++)

OK.

> +    {
> +      xpthread_barrier_init (&barrier, NULL, 2);


> +      /* Reset the counter for the cleanup handler.  */
> +      cl_called = 0;
> +
> +      /* After this wait the cancellation handler is in place.  */
> +      pthread_t thr = xpthread_create (0, tests[i].tf, NULL);
> +      xpthread_barrier_wait (&barrier);

OK. After this point the two waits have occurred and the thread is somewhere
between the first barrier wait and the blocking syscall.

OK. No wait issued becuase we want to early cancel.

> +
> +      xpthread_cancel (thr);

OK. Send the cancel.

> +      xpthread_barrier_wait (&barrier);

We don't need this xpthread_barrier_wait?

We set the count to 2, and the thread and the main thread both executed
the wait. After that the first thread is free running towards the syscall
cancellation registration and we want to cancel it right away (which you do).

> +
> +      void *status = xpthread_join (thr);
> +      TEST_VERIFY (status == PTHREAD_CANCELED);
> +      TEST_COMPARE (cl_called, 1);
> +
> +      printf ("early cancel test of '%s' successful\n", tests[i].name);
> +    }
> +
> +  xpthread_barrier_destroy (&barrier);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/debug/tst-read-chk-cancel.c b/debug/tst-read-chk-cancel.c
> deleted file mode 100644
> index 7e06afb596..0000000000
> --- a/debug/tst-read-chk-cancel.c
> +++ /dev/null
> @@ -1,50 +0,0 @@
> -/* Test that __read_chk is a cancellation point (BZ #29274)

OK. Remove.

> -   Copyright (C) 2022 Free Software Foundation, Inc.
> -   This file is part of the GNU C Library.
> -
> -   The GNU C Library is free software; you can redistribute it and/or
> -   modify it under the terms of the GNU Lesser General Public
> -   License as published by the Free Software Foundation; either
> -   version 2.1 of the License, or (at your option) any later version.
> -
> -   The GNU C Library is distributed in the hope that it will be useful,
> -   but WITHOUT ANY WARRANTY; without even the implied warranty of
> -   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -   Lesser General Public License for more details.
> -
> -   You should have received a copy of the GNU Lesser General Public
> -   License along with the GNU C Library; if not, see
> -   <https://www.gnu.org/licenses/>.  */
> -
> -#include <stdint.h>
> -#include <support/xunistd.h>
> -#include <support/xthread.h>
> -
> -static int pipe_fds[2];
> -static pthread_barrier_t barrier;
> -
> -static void *
> -read_thread (void *n)
> -{
> -  xpthread_barrier_wait (&barrier);
> -  char c;
> -  /* This call should be forwarded to __read_chk because the buffer size
> -     is known, but the read length is non-constant.  */
> -  if (read (pipe_fds[0], &c, (uintptr_t) n) != 1)
> -    return (void *) -1L;
> -  return 0;
> -}
> -
> -static int
> -do_test (void)
> -{
> -  xpthread_barrier_init (&barrier, 0, 2);
> -  xpipe (pipe_fds);
> -  pthread_t thr = xpthread_create (0, read_thread, (void *) 1L);
> -  xpthread_barrier_wait (&barrier);
> -  xpthread_cancel (thr);
> -  xpthread_join (thr);
> -  return 0;
> -}
> -
> -#include <support/test-driver.c>
  

On 18/07/22 11:29, Carlos O'Donell wrote:
> On 7/11/22 12:53, Adhemerval Zanella wrote:
>> Generalize the test for cancellation point in __read_chk to also test
>> the other fortified functions with required cancellation points.
>>
>> Since there is not easy way to force some syscalls to block (for
>> instance pread) the test tests two modes: cancellation on blocked
>> syscalls and early cancellation on pending request.
>>
>> Checked on aarch64-linux-gnu.
>>
>> Co-authored-by: Andreas Schwab <schwab@suse.de>
> 
> Needs a v3. See P&C comments below.
> 
> As I understand the test we want to test being in one of two regions:
> 
> - After the syscall registers cancellation.
> 
> - Before the syscall registers cancellation.
> 
> The !only_early test (the first test) uses a wait to ensure that the
> cleanup region is registered, and then a clock_nanosleep to let the
> test get the region that is after the syscall registers cancelaltion.
> 
> The only_early test (the second test) uses a wait to ensure that the
> cleanup region is registered, but does NOT sleep in an attempt to
> test the region that is just before the syscall registers cancellation.

Not really the cleanup region, but whether we will trigger the cancellation
signal handler or not.  The tests checks for two possible cancellation 
triggers: 

  1. when the cancellable entrypoint is blocked in kernel (!only_early) 
     and then the cancellation signal handler (sigcancel_handler) will 
     act uppon, set the thread cancelled, issue issue do_cancel.

  2. when thread was already cancelled prior the cancellation entrypoint,
     so __pthread_enable_asynccancel will set the cancelling bit, se if
     the thread is cancelled, and issue do_cancel.

> 
> Please correct me if I'm wrong.
> 
>> ---
>>  debug/Makefile              |   6 +-
>>  debug/tst-chk-cancel.c      | 272 ++++++++++++++++++++++++++++++++++++
>>  debug/tst-read-chk-cancel.c |  50 -------
>>  3 files changed, 275 insertions(+), 53 deletions(-)
>>  create mode 100644 debug/tst-chk-cancel.c
>>  delete mode 100644 debug/tst-read-chk-cancel.c
>>
>> diff --git a/debug/Makefile b/debug/Makefile
>> index 456b349c4d..99cdf3221b 100644
>> --- a/debug/Makefile
>> +++ b/debug/Makefile
>> @@ -110,7 +110,7 @@ CPPFLAGS-tst-longjmp_chk2.c += -D_FORTIFY_SOURCE=1
>>  CFLAGS-tst-longjmp_chk3.c += -fexceptions -fasynchronous-unwind-tables
>>  CPPFLAGS-tst-longjmp_chk3.c += -D_FORTIFY_SOURCE=1
>>  CPPFLAGS-tst-realpath-chk.c += -D_FORTIFY_SOURCE=2
>> -CPPFLAGS-tst-read-chk-cancel.c += -D_FORTIFY_SOURCE=2
>> +CPPFLAGS-tst-chk-cancel.c += -D_FORTIFY_SOURCE=2
> 
> OK.
> 
>>  
>>  # _FORTIFY_SOURCE tests.
>>  # Auto-generate tests for _FORTIFY_SOURCE for different levels, compilers and
>> @@ -206,7 +206,7 @@ tests += tst-ssp-1
>>  endif
>>  
>>  ifeq ($(have-thread-library), yes)
>> -tests += tst-read-chk-cancel
>> +tests += tst-chk-cancel
> 
> OK.
> 
>>  endif
>>  
>>  ifeq (,$(CXX))
>> @@ -248,4 +248,4 @@ $(objpfx)xtrace: xtrace.sh
>>  	    -e 's|@REPORT_BUGS_TO@|$(REPORT_BUGS_TO)|' $^ > $@.new \
>>  	&& rm -f $@ && mv $@.new $@ && chmod +x $@
>>  
>> -$(objpfx)tst-read-chk-cancel: $(shared-thread-library)
>> +$(objpfx)tst-chk-cancel: $(shared-thread-library)
> 
> OK. Still required for htl.
> 
>> diff --git a/debug/tst-chk-cancel.c b/debug/tst-chk-cancel.c
>> new file mode 100644
>> index 0000000000..ccc688eda8
>> --- /dev/null
>> +++ b/debug/tst-chk-cancel.c
>> @@ -0,0 +1,272 @@
>> +/* Test for required cancellation points in fortified functions (BZ #29274)
> 
> OK.
> 
>> +   Copyright (C) 2022 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <https://www.gnu.org/licenses/>.  */
>> +
>> +#include <array_length.h>
>> +#include <errno.h>
>> +#include <poll.h>
>> +#include <stdbool.h>
>> +#include <stdint.h>
>> +#include <stdio.h>
>> +#include <support/check.h>
>> +#include <support/xthread.h>
>> +#include <support/xunistd.h>
>> +#include <sys/socket.h>
>> +
>> +/* Cleanup handling test.  */
>> +static int cl_called;
>> +
>> +static void
>> +cl (void *arg)
>> +{
>> +  ++cl_called;
>> +}
>> +
>> +static int fds[2];
>> +static pthread_barrier_t barrier;
>> +
>> +static void *
>> +tf_read (void *n)
>> +{
>> +  pthread_cleanup_push (cl, NULL);
>> +
>> +  xpthread_barrier_wait (&barrier);
>> +
>> +  if (n != (void *) 1L)
>> +    xpthread_barrier_wait (&barrier);
> 
> If I understand the intent of the tests then we don't need this second wait?
> 
> We only need the first wait to register the cleanup handler in all cases.

Yeah, you are right.  We *used* to require a second wait because we run the
pthread_create bits without sigcancel blocked (it was fixed by b3cae39dcbfa2432b3f3aa28854d8ac57f0de1b8).

> 
>> +
>> +  /* This call should be forwarded to __read_chk because the buffer size
>> +     is known, but the read length is non-constant.  */
>> +  char c;
>> +  if (read (fds[0], &c, (uintptr_t) n) != 1)
>> +    return (void *) -1L;
>> +
>> +  pthread_cleanup_pop (0);
>> +  return 0;
>> +}
>> +
>> +static void *
>> +tf_pread (void *n)
>> +{
>> +  pthread_cleanup_push (cl, NULL);
>> +
>> +  xpthread_barrier_wait (&barrier);
>> +
>> +  if (n != (void *) 1L)
>> +    xpthread_barrier_wait (&barrier);
>> +
>> +  /* This call should be forwarded to __pread_chk because the buffer size
>> +     is known, but the read length is non-constant.  */
>> +  char c;
>> +  if (pread (fds[0], &c, (uintptr_t) n, 0) != 1)
>> +    return (void *) -1L;
>> +
>> +  pthread_cleanup_pop (0);
>> +  return 0;
>> +}
>> +
>> +static void *
>> +tf_pread64 (void *n)
>> +{
>> +  pthread_cleanup_push (cl, NULL);
>> +
>> +  xpthread_barrier_wait (&barrier);
>> +
>> +  if (n != (void *) 1L)
>> +    xpthread_barrier_wait (&barrier);
>> +
>> +  /* This call should be forwarded to __pread64_chk because the buffer size
>> +     is known, but the read length is non-constant.  */
>> +  char c;
>> +  if (pread64 (fds[0], &c, (uintptr_t) n, 0) != 1)
>> +    return (void *) -1L;
>> +
>> +  pthread_cleanup_pop (0);
>> +  return 0;
>> +}
>> +
>> +static void *
>> +tf_poll (void *n)
>> +{
>> +  pthread_cleanup_push (cl, NULL);
>> +
>> +  xpthread_barrier_wait (&barrier);
>> +
>> +  if (n != (void *) 1L)
>> +    xpthread_barrier_wait (&barrier);
>> +
>> +  /* This call should be forwarded to __poll_chk because the pollfd size
>> +     is known, but the number of entries is non-constant.  */
>> +  struct pollfd pfd = { fds[0], POLLIN, 0 };
>> +  if (poll (&pfd, (uintptr_t) n, -1) != 1)
>> +    return (void *) -1L;
>> +
>> +  pthread_cleanup_pop (0);
>> +  return 0;
>> +}
>> +
>> +static void *
>> +tf_ppoll (void *n)
>> +{
>> +  pthread_cleanup_push (cl, NULL);
>> +
>> +  xpthread_barrier_wait (&barrier);
>> +
>> +  if (n != (void *) 1L)
>> +    xpthread_barrier_wait (&barrier);
>> +
>> +  /* This call should be forwarded to __ppoll_chk because the pollfd size
>> +     is known, but the number of entries is non-constant.  */
>> +  struct pollfd pfd = { fds[0], POLLIN, 0 };
>> +  if (ppoll (&pfd, (uintptr_t) n, 0, 0) != 1)
>> +    return (void *) -1L;
>> +
>> +  pthread_cleanup_pop (0);
>> +  return 0;
>> +}
>> +
>> +static void *
>> +tf_recv (void *n)
>> +{
>> +  pthread_cleanup_push (cl, NULL);
>> +
>> +  xpthread_barrier_wait (&barrier);
>> +
>> +  if (n != (void *) 1L)
>> +    xpthread_barrier_wait (&barrier);
>> +
>> +  /* This call should be forwarded to __ppoll_chk because the pollfd size
>> +     is known, but the number of entries is non-constant.  */
>> +  char c;
>> +  if (recv (fds[0], &c, (uintptr_t) n, 0) != 1)
>> +    return (void *) -1L;
>> +
>> +  pthread_cleanup_pop (0);
>> +  return 0;
>> +}
>> +
>> +static void *
>> +tf_recvfrom (void *n)
>> +{
>> +  pthread_cleanup_push (cl, NULL);
>> +
>> +  xpthread_barrier_wait (&barrier);
>> +
>> +  if (n != (void *) 1L)
>> +    xpthread_barrier_wait (&barrier);
>> +
>> +  /* This call should be forwarded to __ppoll_chk because the pollfd size
>> +     is known, but the number of entries is non-constant.  */
>> +  char c;
>> +  if (recvfrom (fds[0], &c, (uintptr_t) n, 0, NULL, NULL) != 1)
>> +    return (void *) -1L;
>> +
>> +  pthread_cleanup_pop (0);
>> +  return 0;
>> +}
>> +
>> +static struct cancel_tests
>> +{
>> +  const char *name;
>> +  void *(*tf) (void *);
>> +  bool only_early;
>> +#define ADD_TEST(name, early) { #name, tf_##name, early }
>> +} tests[] =
>> +{
>> +  ADD_TEST (poll,     false),
>> +  ADD_TEST (ppoll,    false),
>> +  ADD_TEST (pread,    true),
>> +  ADD_TEST (pread64,  true),
>> +  ADD_TEST (read,     false),
>> +  ADD_TEST (recv,     false),
>> +  ADD_TEST (recvfrom, false),
>> +};
>> +
>> +/* Set the send buffer of socket S to 1 byte so any send operation
>> +   done with WRITE_BUFFER_SIZE bytes will force syscall blocking.  */
>> +static void
>> +set_socket_buffer (int s)
>> +{
>> +  int val = 1;
>> +  socklen_t len = sizeof (val);
>> +
>> +  TEST_VERIFY_EXIT (setsockopt (s, SOL_SOCKET, SO_SNDBUF, &val,
>> +                    sizeof (val)) == 0);
>> +  TEST_VERIFY_EXIT (getsockopt (s, SOL_SOCKET, SO_SNDBUF, &val, &len) == 0);
>> +  printf ("%s: got size %d\n", __func__, val);
>> +}
>> +
>> +static int
>> +do_test (void)
>> +{
>> +  xpthread_barrier_init (&barrier, 0, 2);
>> +
>> +  if (socketpair (AF_UNIX, SOCK_STREAM, 0, fds) != 0)
>> +    FAIL_EXIT1 ("socketpair: %m");
>> +  set_socket_buffer (fds[1]);
>> +
> 
> Add comment:
> 
> /* This is the !only_early test. It is a late cancel test that has a sleep
>    in the main thread in an attempt to allow the child thread to reach and
>    block on the syscall. The cancellation should happen with high probability
>    when the child thread blocked on the syscall, and that is the intent of
>    the test (syscall cancellation registration complete).  */
> 
> 
>> +  for (int i = 0; i < array_length (tests); i++)
>> +    {
>> +      if (tests[i].only_early)
>> +	continue;
>> +
>> +      xpthread_barrier_init (&barrier, NULL, 2);
>> +      /* Reset the counter for the cleanup handler.  */
>> +      cl_called = 0;
>> +
>> +      pthread_t thr = xpthread_create (0, tests[i].tf, (void *) 1L);
>> +      /* After this wait the threads cancellation handler is installed.  */
>> +      xpthread_barrier_wait (&barrier);
> 
> OK. Correct. So now we are somewhere between the first wait and the syscall entry in
> the child thread.
> 
>> +
>> +      struct timespec ts = { .tv_sec = 0, .tv_nsec = 100000000 };
>> +      TEMP_FAILURE_RETRY (clock_nanosleep (CLOCK_REALTIME, 0, &ts, &ts));
> 
> OK. Wait a bit. The duration doesn't actually matter given the happens before of the wait
> and the registration of the handler. So no spurious failures. This is a "in-time" cancel
> test which hopes to cancel near the syscall.
> 
>> +
>> +      xpthread_cancel (thr);
> 
> OK. Cancel will run and ++cl_called.
> 
>> +
>> +      void *status = xpthread_join (thr);
>> +      TEST_VERIFY (status == PTHREAD_CANCELED);
>> +      TEST_COMPARE (cl_called, 1);
>> +
>> +      printf ("in-time cancel test of '%s' successful\n", tests[i].name);
>> +    }
>> +
> 
> Add a comment:
> 
> /* This test is the early cancel version of the first test, and the intent is
>    to have the cancellation happen at one of two regions:
>    * Before the cancellable syscall registers cancellation.
>    * After the cancellable syscall registers cancellation.
> 
>    This test will exercise if syscall cancellation registration is an atomic
>    operation or not since the transition from the regions is designed to be
>    atomic.
> 
>    We can't control when the cancellation happens, but it will happen in one
>    of the two regions. The first test (the !only_early version) attempts to
>    test the second region, while this test attempts to test the first region
>    and the transition with some probability.  */
> 
>> +  for (int i = 0; i < array_length (tests); i++)
> 
> OK.
> 
>> +    {
>> +      xpthread_barrier_init (&barrier, NULL, 2);
> 
> 
>> +      /* Reset the counter for the cleanup handler.  */
>> +      cl_called = 0;
>> +
>> +      /* After this wait the cancellation handler is in place.  */
>> +      pthread_t thr = xpthread_create (0, tests[i].tf, NULL);
>> +      xpthread_barrier_wait (&barrier);
> 
> OK. After this point the two waits have occurred and the thread is somewhere
> between the first barrier wait and the blocking syscall.
> 
> OK. No wait issued becuase we want to early cancel.
> 
>> +
>> +      xpthread_cancel (thr);
> 
> OK. Send the cancel.
> 
>> +      xpthread_barrier_wait (&barrier);
> 
> We don't need this xpthread_barrier_wait?
> 
> We set the count to 2, and the thread and the main thread both executed
> the wait. After that the first thread is free running towards the syscall
> cancellation registration and we want to cancel it right away (which you do).

I will remove them.

> 
>> +
>> +      void *status = xpthread_join (thr);
>> +      TEST_VERIFY (status == PTHREAD_CANCELED);
>> +      TEST_COMPARE (cl_called, 1);
>> +
>> +      printf ("early cancel test of '%s' successful\n", tests[i].name);
>> +    }
>> +
>> +  xpthread_barrier_destroy (&barrier);
>> +
>> +  return 0;
>> +}
>> +
>> +#include <support/test-driver.c>
>> diff --git a/debug/tst-read-chk-cancel.c b/debug/tst-read-chk-cancel.c
>> deleted file mode 100644
>> index 7e06afb596..0000000000
>> --- a/debug/tst-read-chk-cancel.c
>> +++ /dev/null
>> @@ -1,50 +0,0 @@
>> -/* Test that __read_chk is a cancellation point (BZ #29274)
> 
> OK. Remove.
> 
>> -   Copyright (C) 2022 Free Software Foundation, Inc.
>> -   This file is part of the GNU C Library.
>> -
>> -   The GNU C Library is free software; you can redistribute it and/or
>> -   modify it under the terms of the GNU Lesser General Public
>> -   License as published by the Free Software Foundation; either
>> -   version 2.1 of the License, or (at your option) any later version.
>> -
>> -   The GNU C Library is distributed in the hope that it will be useful,
>> -   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> -   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> -   Lesser General Public License for more details.
>> -
>> -   You should have received a copy of the GNU Lesser General Public
>> -   License along with the GNU C Library; if not, see
>> -   <https://www.gnu.org/licenses/>.  */
>> -
>> -#include <stdint.h>
>> -#include <support/xunistd.h>
>> -#include <support/xthread.h>
>> -
>> -static int pipe_fds[2];
>> -static pthread_barrier_t barrier;
>> -
>> -static void *
>> -read_thread (void *n)
>> -{
>> -  xpthread_barrier_wait (&barrier);
>> -  char c;
>> -  /* This call should be forwarded to __read_chk because the buffer size
>> -     is known, but the read length is non-constant.  */
>> -  if (read (pipe_fds[0], &c, (uintptr_t) n) != 1)
>> -    return (void *) -1L;
>> -  return 0;
>> -}
>> -
>> -static int
>> -do_test (void)
>> -{
>> -  xpthread_barrier_init (&barrier, 0, 2);
>> -  xpipe (pipe_fds);
>> -  pthread_t thr = xpthread_create (0, read_thread, (void *) 1L);
>> -  xpthread_barrier_wait (&barrier);
>> -  xpthread_cancel (thr);
>> -  xpthread_join (thr);
>> -  return 0;
>> -}
>> -
>> -#include <support/test-driver.c>
> 
>