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

[Frederic Berat <fberat@redhat.com>]

On Tue, Jun 6, 2023 at 8:21 AM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
>
>
>
> 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?

Yep, I went too far on v2 when starting to use xread.

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

On my original change, I couldn't find any reason for this call to exist.
As far as I understand, pipes are open blocking by default, which
means read(fd[2]) should block until anything is written to fd[3].
Since there is no write, "read(fd[0])" should never be executed, and
cancellation should hit on "read(fd[2])".

If the first read succeeds, the test should fail.
If the thread isn't marked as cancelled and simply continues its
execution, the test should fail.

Since the potential race between the close and the cancel got fixed by
d0e3ffb7a58854248f1d5e737610d50cd0a60f46, I assume the second read is
superfluous.

Yet, I may have misunderstood the intent on this second read, in which
case some comment would be useful.

>
> >     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);
>