Re: [PATCH v6 2/7] tests: replace read by xread

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 2023-06-02 11:28, Frédéric Bérat wrote:
> With fortification enabled, read calls return result needs to be checked,
> has it gets the __wur macro enabled.
> ---
> Changes since v4/v5:
>   - Rebased
> 
>   dirent/tst-fdopendir.c         |  3 ++-
>   nptl/tst-cleanup4.c            |  4 +++-
>   support/Makefile               |  1 +
>   support/test-container.c       |  3 ++-
>   support/xread.c                | 36 ++++++++++++++++++++++++++++++++++
>   support/xunistd.h              |  3 +++
>   sysdeps/pthread/Makefile       |  2 +-
>   sysdeps/pthread/tst-cancel11.c |  4 +++-
>   sysdeps/pthread/tst-cancel20.c | 10 +++-------
>   sysdeps/pthread/tst-cancel21.c |  9 ++-------
>   sysdeps/pthread/tst-fini1mod.c |  4 +++-
>   11 files changed, 59 insertions(+), 20 deletions(-)
>   create mode 100644 support/xread.c
> 
> diff --git a/dirent/tst-fdopendir.c b/dirent/tst-fdopendir.c
> index 2c9520574d..d6a24f47db 100644
> --- a/dirent/tst-fdopendir.c
> +++ b/dirent/tst-fdopendir.c
> @@ -45,7 +45,8 @@ do_test (void)
>       }
>   
>     char buf[5];
> -  read(fd, buf, sizeof (buf));
> +  xread(fd, buf, sizeof (buf));
> +
>     close(fd);
>   
>     struct stat64 st2;
> diff --git a/nptl/tst-cleanup4.c b/nptl/tst-cleanup4.c
> index 1d3d53fb5f..f2e9f263e5 100644
> --- a/nptl/tst-cleanup4.c
> +++ b/nptl/tst-cleanup4.c
> @@ -21,6 +21,8 @@
>   #include <stdlib.h>
>   #include <unistd.h>
>   
> +#include <support/xunistd.h>
> +
>   /* LinuxThreads pthread_cleanup_{push,pop} helpers.  */
>   extern void _pthread_cleanup_push (struct _pthread_cleanup_buffer *__buffer,
>                                      void (*__routine) (void *),
> @@ -64,7 +66,7 @@ fn_read (void)
>       }
>   
>     char c;
> -  read (fds[0], &c, 1);
> +  xread (fds[0], &c, 1);
>   }
>   
>   
> diff --git a/support/Makefile b/support/Makefile
> index 130de4a985..e39001ef24 100644
> --- a/support/Makefile
> +++ b/support/Makefile
> @@ -196,6 +196,7 @@ libsupport-routines = \
>     xpthread_spin_unlock \
>     xraise \
>     xreadlink \
> +  xread \
>     xrealloc \
>     xrecvfrom \
>     xsendto \
> diff --git a/support/test-container.c b/support/test-container.c
> index 20ea19af37..788b091ea0 100644
> --- a/support/test-container.c
> +++ b/support/test-container.c
> @@ -1217,7 +1217,8 @@ main (int argc, char **argv)
>   
>     /* Get our "outside" pid from our parent.  We use this to help with
>        debugging from outside the container.  */
> -  read (pipes[0], &child, sizeof(child));
> +  xread (pipes[0], &child, sizeof(child));
> +
>     close (pipes[0]);
>     close (pipes[1]);
>     sprintf (pid_buf, "%lu", (long unsigned)child);
> diff --git a/support/xread.c b/support/xread.c
> new file mode 100644
> index 0000000000..215f9b4f00
> --- /dev/null
> +++ b/support/xread.c
> @@ -0,0 +1,36 @@
> +/* read with error checking and retries.
> +   Copyright (C) 2016-2023 Free Software Foundation, Inc.

Only 2023.

> +   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 <support/xunistd.h>
> +
> +#include <support/check.h>
> +
> +void
> +xread (int fd, void *buffer, size_t length)
> +{
> +  char *p = buffer;
> +  char *end = p + length;
> +  while (p < end)
> +    {
> +      ssize_t ret = read (fd, p, end - p);
> +      if (ret < 0)
> +        FAIL_EXIT1 ("read of %zu bytes failed after %td: %m",
> +                    length, p - (char *) buffer);
> +      p += ret;
> +    }
> +}
> diff --git a/support/xunistd.h b/support/xunistd.h
> index 43a1e69fcb..0aa2638a8d 100644
> --- a/support/xunistd.h
> +++ b/support/xunistd.h
> @@ -77,6 +77,9 @@ void xclose (int);
>   /* Write the buffer.  Retry on short writes.  */
>   void xwrite (int, const void *, size_t);
>   
> +/* Read to buffer.  Retry on short reads.  */
> +void xread (int, void *, size_t);
> +
>   /* Invoke mmap with a zero file offset.  */
>   void *xmmap (void *addr, size_t length, int prot, int flags, int fd);
>   void xmprotect (void *addr, size_t length, int prot);
> diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
> index 5df1109dd3..32cf4eb119 100644
> --- a/sysdeps/pthread/Makefile
> +++ b/sysdeps/pthread/Makefile
> @@ -464,7 +464,7 @@ $(objpfx)tst-cancel28: $(librt)
>   
>   $(objpfx)tst-fini1: $(shared-thread-library) $(objpfx)tst-fini1mod.so
>   
> -$(objpfx)tst-fini1mod.so: $(shared-thread-library)
> +$(objpfx)tst-fini1mod.so: $(libsupport) $(shared-thread-library)
>   
>   $(objpfx)tst-_res1mod2.so: $(objpfx)tst-_res1mod1.so
>   LDFLAGS-tst-_res1mod1.so = -Wl,-soname,tst-_res1mod1.so
> diff --git a/sysdeps/pthread/tst-cancel11.c b/sysdeps/pthread/tst-cancel11.c
> index 4dd84d6673..449f3b9b63 100644
> --- a/sysdeps/pthread/tst-cancel11.c
> +++ b/sysdeps/pthread/tst-cancel11.c
> @@ -22,6 +22,8 @@
>   #include <string.h>
>   #include <unistd.h>
>   
> +#include <support/xunistd.h>
> +
>   
>   static pthread_barrier_t bar;
>   static int fd[2];
> @@ -56,7 +58,7 @@ tf (void *arg)
>   
>     /* This call should block and be cancelable.  */
>     char buf[20];
> -  read (fd[0], buf, sizeof (buf));
> +  xread (fd[0], buf, sizeof (buf));
>   
>     pthread_cleanup_pop (0);
>   
> diff --git a/sysdeps/pthread/tst-cancel20.c b/sysdeps/pthread/tst-cancel20.c
> index 1d5c53049b..0f1ada3742 100644
> --- a/sysdeps/pthread/tst-cancel20.c
> +++ b/sysdeps/pthread/tst-cancel20.c
> @@ -22,6 +22,8 @@
>   #include <stdlib.h>
>   #include <unistd.h>
>   
> +#include <support/xunistd.h>
> +
>   
>   static int fd[4];
>   static pthread_barrier_t b;
> @@ -43,11 +45,7 @@ sh_body (void)
>     pthread_cleanup_push (cl, (void *) 1L);
>   
>     in_sh_body = 1;
> -  if (read (fd[2], &c, 1) == 1)
> -    {
> -      puts ("read succeeded");
> -      exit (1);
> -    }
> +  xread (fd[2], &c, 1);

Uhmm, isn't read success a failure here?

>   
>     pthread_cleanup_pop (0);
>   }
> @@ -84,8 +82,6 @@ tf_body (void)
>         exit (1);
>       }
>   
> -  read (fd[0], &c, 1);
> -

No replacement for this read call?

>     pthread_cleanup_pop (0);
>   }
>   
> diff --git a/sysdeps/pthread/tst-cancel21.c b/sysdeps/pthread/tst-cancel21.c
> index bc4ff308f9..c14ed37d14 100644
> --- a/sysdeps/pthread/tst-cancel21.c
> +++ b/sysdeps/pthread/tst-cancel21.c
> @@ -23,6 +23,7 @@
>   #include <sys/wait.h>
>   #include <unistd.h>
>   
> +#include <support/xunistd.h>
>   
>   static int fd[4];
>   static pthread_barrier_t b;
> @@ -44,11 +45,7 @@ sh_body (void)
>     pthread_cleanup_push (cl, (void *) 1L);
>   
>     in_sh_body = 1;
> -  if (read (fd[2], &c, 1) == 1)
> -    {
> -      puts ("read succeeded");
> -      exit (1);
> -    }
> +  xread (fd[2], &c, 1);

Likewise, read success should be a failure?

>   
>     pthread_cleanup_pop (0);
>   }
> @@ -85,8 +82,6 @@ tf_body (void)
>         exit (1);
>       }
>   
> -  read (fd[0], &c, 1);
> -

Missing xread replacement.

>     pthread_cleanup_pop (0);
>   }
>   
> diff --git a/sysdeps/pthread/tst-fini1mod.c b/sysdeps/pthread/tst-fini1mod.c
> index cdadf034cd..0a45f6c5f2 100644
> --- a/sysdeps/pthread/tst-fini1mod.c
> +++ b/sysdeps/pthread/tst-fini1mod.c
> @@ -20,6 +20,8 @@
>   #include <stdlib.h>
>   #include <unistd.h>
>   
> +#include <support/xunistd.h>
> +
>   
>   static void *
>   tf (void *arg)
> @@ -32,7 +34,7 @@ tf (void *arg)
>       }
>   
>     char buf[10];
> -  read (fds[0], buf, sizeof (buf));
> +  xread (fds[0], buf, sizeof (buf));
>   
>     puts ("read returned");
>     exit (1);