On Sat, Feb 08, 2020 at 02:32:54PM -0800, Colin Percival wrote:
> On 2020-02-08 13:05, Peter Pentchev wrote:
> > Apparently glibc really, really doesn't believe in cancelling a thread
> > that has already completed... I'm not really sure what to think about
> > that, since, well, the thread hasn't been joined yet, so technically it
> > still exists, albeit in a zombie-like state - and a little test program
> > of mine shows that even after a thread has exited and pthread_cancel()
> > returns ESRCH, a call to pthread_join() will succeed and will provide
> > the correct value returned by the thread function. I'm not sure whether
> > this is glibc or the Linux kernel or something else - the behavior is
> > the same on three systems I've tested it on: [...]
> Ok, this is definitely a bug: pthread_cancel is absolutely supposed to
> work on threads which have exited but not joined yet.
>
> Can you produce a minimal test case which exhibits this so that it can
> be reported upstream (to glibc or Linux kernel folks -- I'm not sure who
> but hopefully they can forward to each other if we get it wrong)?
>
> > So I could keep the check for ESRCH as a Debian-specific patch, or you
> > could make it conditional on __linux__ or __gnu_linux__ or something
> > similar. What do you think?
>
> My general policy with "operating system doesn't do what POSIX says it
> should" is to have "#ifdef POSIXFAIL_FOO" in the code and a test as part
> of libcperciva/POSIX/posix-cflags.sh which defines that macro on the
> appropriate systems (which might just be "anything Linux" for now, but in
> the future might depend on the version of glibc).
Right. Sorry, I'm really not quite in my right frame of mind today,
otherwise I would have actually remembered (or taken a look) that spiped
does it this way (quite sensibly, too).
So what do you think about the attached patch that addresses both your
questions: a minimal test case for the kernel/libc maintainers (but see
below) and a -DPOSIXFAIL_PTHREAD_CANCEL test for spiped? My builds of
spiped now produce a Debian package that passes the tests.
Of course, if you think the patch is too verbose (I have intentionally
not muted the test program's standard error output) or too embellished,
feel free to pare it down to whatever feels right for spiped.
(and, of course, if you decide it to include in spiped, the Makefile
bits will probably be handled via your Makefile generation process)
BTW funny thing: I tried building an out-of-the-box spiped-1.6.1a on
a FreeBSD 11.2-RELEASE-p11 system (with a GENERIC kernel) and it
exhibited exactly the same behavior: "make test" works, but
"make test < /dev/null" fails in the pushbits test with the same error.
So it turns out that the problem might be even more widespread...
Thanks for bearing with me, and thanks a lot for your patience!
G'luck,
Peter
--
Peter Pentchev roam@{ringlet.net,debian.org,FreeBSD.org} pp@storpool.com
PGP key: http://people.FreeBSD.org/~roam/roam.key.asc
Key fingerprint 2EE7 A7A5 17FC 124C F115 C354 651E EFB0 2527 DF13
Description: Detect pthread_cancel() returning ESRCH.
It seems that some implementations of pthread_cancel() will
return a "no such thread/process" error if called for a thread that
has already terminated.
Forwarded: no
Author: Peter Pentchev <roam@ringlet.net>
Last-Update: 2020-02-09
--- a/tests/pushbits/main.c
+++ b/tests/pushbits/main.c
@@ -9,6 +9,7 @@
#include <time.h>
#include <unistd.h>
+#include "fixed_pthread_cancel.h"
#include "getopt.h"
#include "noeintr.h"
#include "parsenum.h"
@@ -74,7 +75,7 @@
/* Wait a short while, then cancel the thread. */
sleep(1);
- if (pthread_cancel(thread)) {
+ if (fixed_pthread_cancel(thread)) {
warnp("pthread_cancel");
goto err0;
}
--- a/libcperciva/POSIX/posix-cflags.sh
+++ b/libcperciva/POSIX/posix-cflags.sh
@@ -50,4 +50,22 @@
printf %s "-std=c99"
fi
fi
+if ! ${CC} -D_POSIX_C_SOURCE=200809L $D/posix-pthread_cancel.c -lpthread 2>/dev/null; then
+ [ ${FIRST} = "NO" ] && printf " "; FIRST=NO
+ printf %s "-DPOSIXFAIL_PTHREAD_CANCEL"
+ echo "WARNING: POSIX violation: cannot compile the pthread_cancel() ESRCH test program" 1>&2
+else
+ # This is a runtime test, so running the binary is necessary.
+ ./a.out
+ rc="$?"
+ if [ "$rc" = 2 ]; then
+ [ ${FIRST} = "NO" ] && printf " "; FIRST=NO
+ printf %s "-DPOSIXFAIL_PTHREAD_CANCEL"
+ echo "WARNING: POSIX violation: pthread_cancel() fails on threads that have exited" 1>&2
+ elif [ "$rc" != 0 ]; then
+ [ ${FIRST} = "NO" ] && printf " "; FIRST=NO
+ printf %s "-DPOSIXFAIL_PTHREAD_CANCEL"
+ echo "WARNING: POSIX violation: the pthread_cancel() ESRCH test program failed" 1>&2
+ fi
+fi
rm -f a.out
--- /dev/null
+++ b/libcperciva/POSIX/posix-pthread_cancel.c
@@ -0,0 +1,42 @@
+#include <errno.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <unistd.h>
+
+static void *
+thrfunc(void * const arg)
+{
+ (*(int *)arg)++;
+ return arg;
+}
+
+int
+main(void)
+{
+ int token = 616;
+ pthread_t thrid;
+ int rc = pthread_create(&thrid, NULL, thrfunc, &token);
+ if (rc != 0) {
+ fprintf(stderr, "pthread_create() returned %d\n", rc);
+ return 1;
+ }
+ sleep(1);
+ rc = pthread_cancel(thrid);
+ if (rc != 0) {
+ if (rc == ESRCH)
+ return 2;
+ fprintf(stderr, "pthread_cancel() returned %d\n", rc);
+ return 1;
+ }
+ void *res;
+ rc = pthread_join(thrid, &res);
+ if (rc != 0) {
+ fprintf(stderr, "pthread_join() returned %d\n", rc);
+ return 1;
+ }
+ if (*(int *)res != 617) {
+ fprintf(stderr, "the thread function returned %d\n", *(int *)res);
+ return 1;
+ }
+ return 0;
+}
--- /dev/null
+++ b/libcperciva/util/fixed_pthread_cancel.c
@@ -0,0 +1,18 @@
+#ifdef POSIXFAIL_PTHREAD_CANCEL
+
+#include <errno.h>
+#include <pthread.h>
+
+#include "fixed_pthread_cancel.h"
+
+int
+fixed_pthread_cancel(const pthread_t thread)
+{
+ const int rc = pthread_cancel(thread);
+
+ if (rc == ESRCH)
+ return 0;
+ return rc;
+}
+
+#endif
--- /dev/null
+++ b/libcperciva/util/fixed_pthread_cancel.h
@@ -0,0 +1,14 @@
+#ifndef _FIXED_PTHREAD_CANCEL_H
+#define _FIXED_PTHREAD_CANCEL_H
+
+#ifndef POSIXFAIL_PTHREAD_CANCEL
+
+#define fixed_pthread_cancel(x) pthread_cancel(x)
+
+#else
+
+int fixed_pthread_cancel(pthread_t thread);
+
+#endif
+
+#endif
--- a/spipe/main.c
+++ b/spipe/main.c
@@ -8,6 +8,7 @@
#include <unistd.h>
#include "events.h"
+#include "fixed_pthread_cancel.h"
#include "getopt.h"
#include "graceful_shutdown.h"
#include "parsenum.h"
@@ -70,7 +71,7 @@
* shutdown() that socket.
*/
for (i = 0; i < 2; i++) {
- if ((rc = pthread_cancel(ET->threads[i])) != 0) {
+ if ((rc = fixed_pthread_cancel(ET->threads[i])) != 0) {
warn0("pthread_cancel: %s", strerror(rc));
goto err0;
}
@@ -294,12 +295,12 @@
exit(0);
err5:
- if ((rc = pthread_cancel(ET.threads[0])) != 0)
+ if ((rc = fixed_pthread_cancel(ET.threads[0])) != 0)
warn0("pthread_cancel: %s", strerror(rc));
if ((rc = pthread_join(ET.threads[0], NULL)) != 0)
warn0("pthread_join: %s", strerror(rc));
err4:
- if ((rc = pthread_cancel(ET.threads[1])) != 0)
+ if ((rc = fixed_pthread_cancel(ET.threads[1])) != 0)
warn0("pthread_cancel: %s", strerror(rc));
if ((rc = pthread_join(ET.threads[1], NULL)) != 0)
warn0("pthread_join: %s", strerror(rc));
--- a/spipe/Makefile
+++ b/spipe/Makefile
@@ -2,7 +2,7 @@
# AUTOGENERATED FILE, DO NOT EDIT
PROG=spipe
MAN1=spipe.1
-SRCS=main.c pushbits.c proto_conn.c proto_crypt.c proto_handshake.c proto_pipe.c graceful_shutdown.c sha256.c sha256_shani.c elasticarray.c ptrheap.c timerqueue.c asprintf.c entropy.c getopt.c insecure_memzero.c monoclock.c noeintr.c sock.c warnp.c cpusupport_x86_aesni.c cpusupport_x86_rdrand.c cpusupport_x86_shani.c cpusupport_x86_ssse3.c events_immediate.c events_network.c events_network_selectstats.c events_timer.c events.c network_connect.c network_read.c network_write.c crypto_aes.c crypto_aes_aesni.c crypto_aesctr.c crypto_dh.c crypto_dh_group14.c crypto_entropy.c crypto_entropy_rdrand.c crypto_verify_bytes.c
+SRCS=main.c pushbits.c proto_conn.c proto_crypt.c proto_handshake.c proto_pipe.c graceful_shutdown.c sha256.c sha256_shani.c elasticarray.c ptrheap.c timerqueue.c asprintf.c entropy.c fixed_pthread_cancel.c getopt.c insecure_memzero.c monoclock.c noeintr.c sock.c warnp.c cpusupport_x86_aesni.c cpusupport_x86_rdrand.c cpusupport_x86_shani.c cpusupport_x86_ssse3.c events_immediate.c events_network.c events_network_selectstats.c events_timer.c events.c network_connect.c network_read.c network_write.c crypto_aes.c crypto_aes_aesni.c crypto_aesctr.c crypto_dh.c crypto_dh_group14.c crypto_entropy.c crypto_entropy_rdrand.c crypto_verify_bytes.c
IDIRS=-I../proto -I../lib/util -I../libcperciva/alg -I../libcperciva/datastruct -I../libcperciva/util -I../libcperciva/cpusupport -I../libcperciva/events -I../libcperciva/network -I../libcperciva/crypto
LDADD_REQ=-lcrypto -lpthread
SUBDIR_DEPTH=..
@@ -61,6 +61,8 @@
${CC} ${CFLAGS_POSIX} -D_POSIX_C_SOURCE=200809L -D_XOPEN_SOURCE=700 -DCPUSUPPORT_CONFIG_FILE=\"cpusupport-config.h\" -I.. ${IDIRS} ${CPPFLAGS} ${CFLAGS} -c ../libcperciva/util/asprintf.c -o asprintf.o
entropy.o: ../libcperciva/util/entropy.c ../libcperciva/util/warnp.h ../libcperciva/util/entropy.h
${CC} ${CFLAGS_POSIX} -D_POSIX_C_SOURCE=200809L -D_XOPEN_SOURCE=700 -DCPUSUPPORT_CONFIG_FILE=\"cpusupport-config.h\" -I.. ${IDIRS} ${CPPFLAGS} ${CFLAGS} -c ../libcperciva/util/entropy.c -o entropy.o
+fixed_pthread_cancel.o: ../libcperciva/util/fixed_pthread_cancel.c ../libcperciva/util/fixed_pthread_cancel.h
+ ${CC} ${CFLAGS_POSIX} -D_POSIX_C_SOURCE=200809L -D_XOPEN_SOURCE=700 -DCPUSUPPORT_CONFIG_FILE=\"cpusupport-config.h\" -I.. ${IDIRS} ${CPPFLAGS} ${CFLAGS} -c ../libcperciva/util/fixed_pthread_cancel.c -o fixed_pthread_cancel.o
getopt.o: ../libcperciva/util/getopt.c ../libcperciva/util/getopt.h
${CC} ${CFLAGS_POSIX} -D_POSIX_C_SOURCE=200809L -D_XOPEN_SOURCE=700 -DCPUSUPPORT_CONFIG_FILE=\"cpusupport-config.h\" -I.. ${IDIRS} ${CPPFLAGS} ${CFLAGS} -c ../libcperciva/util/getopt.c -o getopt.o
insecure_memzero.o: ../libcperciva/util/insecure_memzero.c ../libcperciva/util/insecure_memzero.h
--- a/tests/pushbits/Makefile
+++ b/tests/pushbits/Makefile
@@ -1,7 +1,7 @@
.POSIX:
# AUTOGENERATED FILE, DO NOT EDIT
PROG=test_pushbits
-SRCS=main.c getopt.c noeintr.c warnp.c pushbits.c
+SRCS=main.c fixed_pthread_cancel.c getopt.c noeintr.c warnp.c pushbits.c
IDIRS=-I../../libcperciva/util -I../../spipe
LDADD_REQ=-lpthread
SUBDIR_DEPTH=../..
@@ -22,8 +22,10 @@
${PROG}:${SRCS:.c=.o}
${CC} -o ${PROG} ${SRCS:.c=.o} ${LDFLAGS} ${LDADD_EXTRA} ${LDADD_REQ} ${LDADD_POSIX}
-main.o: main.c ../../libcperciva/util/getopt.h ../../libcperciva/util/noeintr.h ../../libcperciva/util/parsenum.h ../../libcperciva/util/warnp.h ../../spipe/pushbits.h
+main.o: main.c ../../libcperciva/util/fixed_pthread_cancel.h ../../libcperciva/util/getopt.h ../../libcperciva/util/noeintr.h ../../libcperciva/util/parsenum.h ../../libcperciva/util/warnp.h ../../spipe/pushbits.h
${CC} ${CFLAGS_POSIX} -D_POSIX_C_SOURCE=200809L -D_XOPEN_SOURCE=700 -DCPUSUPPORT_CONFIG_FILE=\"cpusupport-config.h\" -I../.. ${IDIRS} ${CPPFLAGS} ${CFLAGS} -c main.c -o main.o
+fixed_pthread_cancel.o: ../../libcperciva/util/fixed_pthread_cancel.c ../../libcperciva/util/fixed_pthread_cancel.h
+ ${CC} ${CFLAGS_POSIX} -D_POSIX_C_SOURCE=200809L -D_XOPEN_SOURCE=700 -DCPUSUPPORT_CONFIG_FILE=\"cpusupport-config.h\" -I../.. ${IDIRS} ${CPPFLAGS} ${CFLAGS} -c ../../libcperciva/util/fixed_pthread_cancel.c -o fixed_pthread_cancel.o
getopt.o: ../../libcperciva/util/getopt.c ../../libcperciva/util/getopt.h
${CC} ${CFLAGS_POSIX} -D_POSIX_C_SOURCE=200809L -D_XOPEN_SOURCE=700 -DCPUSUPPORT_CONFIG_FILE=\"cpusupport-config.h\" -I../.. ${IDIRS} ${CPPFLAGS} ${CFLAGS} -c ../../libcperciva/util/getopt.c -o getopt.o
noeintr.o: ../../libcperciva/util/noeintr.c ../../libcperciva/util/noeintr.h
Attachment:
signature.asc
Description: PGP signature