[v7,1/4] tests: replace read by xread
Checks
Commit Message
With fortification enabled, read calls return result needs to be checked,
has it gets the __wur macro enabled.
Note on read call removal from sysdeps/pthread/tst-cancel20.c and
sysdeps/pthread/tst-cancel21.c:
It is assumed that this second read call was there to overcome the race
condition between pipe closure and thread cancellation that could happen
in the original code. Since this race condition got fixed by
d0e3ffb7a58854248f1d5e737610d50cd0a60f46 the second call seems
superfluous. Hence, instead of checking for the return value of read, it
looks reasonable to simply remove it.
---
Changes since v6:
- Add note for read call removal
- Fixed support/Makefile ordering
- Revert faulty change in behavior for first read call in tst-cancel{20,21}.c
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 | 2 --
sysdeps/pthread/tst-cancel21.c | 2 --
sysdeps/pthread/tst-fini1mod.c | 4 +++-
11 files changed, 54 insertions(+), 10 deletions(-)
create mode 100644 support/xread.c
Comments
On Mon, 12 Jun 2023, Frédéric Bérat via Libc-alpha wrote:
> @@ -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));
Since the comment suggests this code is actually testing a property of the
read function, it seems doubtful whether it's appropriate to change it to
call xread - which would impose an API requirement on xread that it calls
read in exactly the way under test. Calling x* functions is fine in
testcases when the function in question is not under test - for example,
when the point of calling xread is just to read some data, with the
details of how that's done being irrelevant - but questionable when the
test is a test of read itself.
> @@ -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");
The same might apply here.
On Mon, Jun 12, 2023 at 6:58 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Mon, 12 Jun 2023, Frédéric Bérat via Libc-alpha wrote:
>
> > @@ -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));
>
> Since the comment suggests this code is actually testing a property of the
> read function, it seems doubtful whether it's appropriate to change it to
> call xread - which would impose an API requirement on xread that it calls
> read in exactly the way under test. Calling x* functions is fine in
> testcases when the function in question is not under test - for example,
> when the point of calling xread is just to read some data, with the
> details of how that's done being irrelevant - but questionable when the
> test is a test of read itself.
>
My understanding is that's not really a property of read that is
tested here, since it could be theoretically replaced by anything that
blocks and is a cancellation point. So in principle, the way it is
done is still irrelevant.
That said, I see your point. The wrapper may bring opacity that is not
wanted here.
> > @@ -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");
>
> The same might apply here.
>
> --
> Joseph S. Myers
> joseph@codesourcery.com
@@ -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;
@@ -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);
}
@@ -195,6 +195,7 @@ libsupport-routines = \
xpthread_spin_lock \
xpthread_spin_unlock \
xraise \
+ xread \
xreadlink \
xrealloc \
xrecvfrom \
@@ -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);
new file mode 100644
@@ -0,0 +1,36 @@
+/* read with error checking and retries.
+ Copyright (C) 2023 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 <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;
+ }
+}
@@ -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);
@@ -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
@@ -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);
@@ -84,8 +84,6 @@ tf_body (void)
exit (1);
}
- read (fd[0], &c, 1);
-
pthread_cleanup_pop (0);
}
@@ -85,8 +85,6 @@ tf_body (void)
exit (1);
}
- read (fd[0], &c, 1);
-
pthread_cleanup_pop (0);
}
@@ -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);