pthread_create_blocking_np: don't fail after the thread has started

lib/util/pthread_create_blocking_np.c

@@ -1,6 +1,9 @@
 #include <errno.h>
 #include <pthread.h>
 #include <stdlib.h>
+#include <string.h>
+
+#include "warnp.h"
 
 #include "pthread_create_blocking_np.h"
 
@@ -78,6 +81,7 @@ pthread_create_blocking_np(pthread_t * restrict thread,
 {
 	struct wrapped_cookie * U;
 	int rc;
+	int rc_cleanup;
 
 	/*
 	 * Allocate our cookie and store parameters.  The C standard does not
@@ -126,15 +130,31 @@ pthread_create_blocking_np(pthread_t * restrict thread,
 		if ((rc = U->rc_sync) != 0)
 			goto err5;
 	}
-	if ((rc = pthread_mutex_unlock(&U->mutex)) != 0)
-		goto err4;
 
-	/* Clean up synchronization-related variables. */
-	if ((rc = pthread_cond_destroy(&U->cond)) != 0)
-		goto err2;
-	if ((rc = pthread_mutex_destroy(&U->mutex)) != 0)
-		goto err1;
+	/*
+	 * The thread is running, so it owns ${arg} and will run
+	 * ${start_routine} to completion.  From here we must report
+	 * success: this function promises that on failure the provided
+	 * routine was not run, and a caller which frees ${arg} after an
+	 * error return would be freeing memory the thread is still using.
+	 *
+	 * Anything which fails below is one of our own synchronization
+	 * variables.  The caller cannot act on that, so warn and carry on.
+	 */
+	if ((rc_cleanup = pthread_mutex_unlock(&U->mutex)) != 0) {
+		warn0("pthread_mutex_unlock: %s", strerror(rc_cleanup));
+
+		/* Do not destroy a mutex which we might still hold. */
+		goto done;
+	}
+	if ((rc_cleanup = pthread_cond_destroy(&U->cond)) != 0) {
+		warn0("pthread_cond_destroy: %s", strerror(rc_cleanup));
+		goto done;
+	}
+	if ((rc_cleanup = pthread_mutex_destroy(&U->mutex)) != 0)
+		warn0("pthread_mutex_destroy: %s", strerror(rc_cleanup));
 
+done:
 	/* Clean up. */
 	free(U);
 
@@ -147,7 +167,6 @@ pthread_create_blocking_np(pthread_t * restrict thread,
 	 * is more important.
 	 */
 	pthread_mutex_unlock(&U->mutex);
-err4:
 	pthread_cancel(*thread);
 	pthread_join(*thread, NULL);
 err3:

tests/pthread_create_blocking_np/Makefile

@@ -1,7 +1,7 @@
 .POSIX:
 # AUTOGENERATED FILE, DO NOT EDIT
 PROG=test_pthread_create_blocking_np
-SRCS=main.c timing.c
+SRCS=main.c timing.c fault.c
 IDIRS=-I../../libcperciva/util -I../../lib/util
 LDADD_REQ=-lpthread
 SUBDIR_DEPTH=../..
@@ -23,7 +23,9 @@ clean:
 ${PROG}:${SRCS:.c=.o} ${LIBALL}
 	${CC} -o ${PROG} ${SRCS:.c=.o} ${LIBALL} ${LDFLAGS} ${LDADD_EXTRA} ${LDADD_REQ} ${LDADD_POSIX}
 
-main.o: main.c ../../lib/util/pthread_create_blocking_np.h timing.h ../../libcperciva/util/warnp.h
+main.o: main.c fault.h ../../lib/util/pthread_create_blocking_np.h timing.h ../../libcperciva/util/warnp.h
 	${CC} ${CFLAGS_POSIX} -D_POSIX_C_SOURCE=200809L -D_XOPEN_SOURCE=700 -DCPUSUPPORT_CONFIG_FILE=\"cpusupport-config.h\" -DAPISUPPORT_CONFIG_FILE=\"apisupport-config.h\" -I../.. ${IDIRS} ${CPPFLAGS} ${CFLAGS} -c main.c -o main.o
+fault.o: fault.c fault.h ../../lib/util/pthread_create_blocking_np.c ../../lib/util/pthread_create_blocking_np.h ../../libcperciva/util/warnp.h
+	${CC} ${CFLAGS_POSIX} -D_POSIX_C_SOURCE=200809L -D_XOPEN_SOURCE=700 -DCPUSUPPORT_CONFIG_FILE=\"cpusupport-config.h\" -DAPISUPPORT_CONFIG_FILE=\"apisupport-config.h\" -I../.. ${IDIRS} ${CPPFLAGS} ${CFLAGS} -c fault.c -o fault.o
 timing.o: timing.c ../../libcperciva/util/millisleep.h ../../libcperciva/util/monoclock.h ../../lib/util/pthread_create_blocking_np.h ../../libcperciva/util/warnp.h timing.h
 	${CC} ${CFLAGS_POSIX} -D_POSIX_C_SOURCE=200809L -D_XOPEN_SOURCE=700 -DCPUSUPPORT_CONFIG_FILE=\"cpusupport-config.h\" -DAPISUPPORT_CONFIG_FILE=\"apisupport-config.h\" -I../.. ${IDIRS} ${CPPFLAGS} ${CFLAGS} -c timing.c -o timing.o

tests/pthread_create_blocking_np/Makefile.BSD

@@ -14,6 +14,7 @@ LIB_DIR		=	../../lib
 # Main test code
 SRCS	=	main.c
 SRCS	+=	timing.c
+SRCS	+=	fault.c
 
 # libcperciva includes
 IDIRS	+=	-I${LIBCPERCIVA_DIR}/util

tests/pthread_create_blocking_np/fault.c

@@ -0,0 +1,150 @@
+#include <errno.h>
+#include <pthread.h>
+#include <string.h>
+
+#include "fault.h"
+#include "warnp.h"
+
+/*
+ * Compile the implementation under an alternate public symbol and route only
+ * its cleanup pthread calls through these wrappers.  This keeps the regression
+ * portable -- in particular, it does not depend on GNU ld's --wrap support.
+ */
+static int test_pthread_mutex_unlock(pthread_mutex_t *);
+static int test_pthread_cond_destroy(pthread_cond_t *);
+static int test_pthread_mutex_destroy(pthread_mutex_t *);
+
+#define pthread_create_blocking_np pthread_create_blocking_np_faultable
+#define pthread_mutex_unlock test_pthread_mutex_unlock
+#define pthread_cond_destroy test_pthread_cond_destroy
+#define pthread_mutex_destroy test_pthread_mutex_destroy
+#include "../../lib/util/pthread_create_blocking_np.c"
+#undef pthread_mutex_destroy
+#undef pthread_cond_destroy
+#undef pthread_mutex_unlock
+#undef pthread_create_blocking_np
+
+static pthread_t parent_thread;
+static const char * failpoint;
+static int injected;
+
+static int
+should_fail(const char * point)
+{
+
+	/* The worker also unlocks the same mutex; only fail in the parent. */
+	if (!pthread_equal(pthread_self(), parent_thread))
+		return (0);
+	if (injected || strcmp(failpoint, point))
+		return (0);
+
+	injected = 1;
+	return (1);
+}
+
+static int
+test_pthread_mutex_unlock(pthread_mutex_t * mutex)
+{
+
+	if (should_fail("unlock"))
+		return (EPERM);
+	return (pthread_mutex_unlock(mutex));
+}
+
+static int
+test_pthread_cond_destroy(pthread_cond_t * cond)
+{
+
+	if (should_fail("cond"))
+		return (EBUSY);
+	return (pthread_cond_destroy(cond));
+}
+
+static int
+test_pthread_mutex_destroy(pthread_mutex_t * mutex)
+{
+
+	if (should_fail("mutex"))
+		return (EBUSY);
+	return (pthread_mutex_destroy(mutex));
+}
+
+static void *
+workfunc_fault(void * cookie)
+{
+
+	return (cookie);
+}
+
+static int
+run_fault(const char * point, int expect_injected)
+{
+	pthread_t thr;
+	void * rc_join = NULL;
+	int marker;
+	int rc;
+
+	parent_thread = pthread_self();
+	failpoint = point;
+	injected = 0;
+	marker = 443;
+
+	/*
+	 * Every synthetic failure below happens after wrapped_thread() has copied
+	 * ${arg} and marked itself running.  The public contract therefore requires
+	 * success: the worker, not the caller, owns ${arg} at that point.
+	 */
+	if ((rc = pthread_create_blocking_np_faultable(&thr, NULL,
+	    workfunc_fault, &marker)) != 0) {
+		warn0("pthread_create_blocking_np (%s): %s",
+		    point, strerror(rc));
+		goto err0;
+	}
+	if ((rc = pthread_join(thr, &rc_join)) != 0) {
+		warn0("pthread_join (%s): %s", point, strerror(rc));
+		goto err0;
+	}
+	if (rc_join != &marker) {
+		warn0("pthread join value changed under %s cleanup failure", point);
+		goto err0;
+	}
+	if (injected != expect_injected) {
+		warn0("%s cleanup failure injection mismatch", point);
+		goto err0;
+	}
+
+	/* Success! */
+	return (0);
+
+err0:
+	/* Failure! */
+	return (-1);
+}
+
+/**
+ * check_faults(void):
+ * Check the ownership contract when cleanup fails after the child is running.
+ */
+int
+check_faults(void)
+{
+
+	/* Control: wrappers must leave the ordinary success path unchanged. */
+	if (run_fault("none", 0))
+		goto err0;
+
+	/* These three used to return an error after the worker owned ${arg}. */
+	if (run_fault("unlock", 1))
+		goto err0;
+	if (run_fault("cond", 1))
+		goto err0;
+	if (run_fault("mutex", 1))
+		goto err0;
+
+	/* Success! */
+	return (0);
+
+err0:
+	/* Failure! */
+	return (-1);
+}

tests/pthread_create_blocking_np/fault.h

@@ -0,0 +1,6 @@
+#ifndef _FAULT_H_
+#define _FAULT_H_
+
+int check_faults(void);
+
+#endif /* !_FAULT_H_ */

tests/pthread_create_blocking_np/main.c

@@ -2,6 +2,7 @@
 #include <stdlib.h>
 #include <string.h>
 
+#include "fault.h"
 #include "pthread_create_blocking_np.h"
 #include "timing.h"
 #include "warnp.h"
@@ -67,6 +68,10 @@ main(int argc, char ** argv)
 	if (check_basic())
 		goto err0;
 
+	/* Check failures while cleaning up after the thread has started. */
+	if (check_faults())
+		goto err0;
+
 	/* Check the relative timing of points in the code. */
 	if (check_timing())
 		goto err0;