[DTrace-devel] [PATCH v2 3/4] EINTR: make safe under signal hits

[Nick Alcock]

This commit audits DTrace (especially libproc and dt_proc, but not only
that) for syscalls and library calls that may return EINTR but are not
suitably guarded, and guards them, adjusting any that use timeouts so that
the timeout ignores the interruption.  (The plan was also to have any of
them that needed to actually *use* this EINTR to trigger an early return to
do that as well, but it turns out that none do, not even the ones in
libproc.)

This matters for libproc and dt_proc in particular now because the monitor
thread is routinely hit by signals and so has to expect EINTR returns from
appropriate syscalls (and consider what to do if that happens, because it
means a proxy request is waiting and the DTrace main thread is blocked on
it).

This does not touch the core polling loop in dt_consume, since that loop is
*expected* to return -EINTR, triggering an -EINTR return from dtrace_go()
(see cmd/dtrace.c's call to that function).  This may be a bug, since it
means that repeatedly changing the window size is enough to cause DTrace to
ignore the switchrate -- but the caller might well *want* an -EINTR return
so it can handle whatever signal just hit.

Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
---
 libcommon/dof_parser_host.c | 29 +++++++++++++++++++++++++++--
 libdtrace/dt_pcap.c         |  5 +++--
 libdtrace/dt_proc.c         |  2 +-
 libproc/Pcontrol.c          |  2 +-
 libproc/elfish.c            |  5 +++--
 5 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/libcommon/dof_parser_host.c b/libcommon/dof_parser_host.c
index feb719020dc18..071f8e80ebc01 100644
--- a/libcommon/dof_parser_host.c
+++ b/libcommon/dof_parser_host.c
@@ -10,6 +10,7 @@
 #include <stddef.h>
 #include <stdlib.h>
 #include <string.h>
+#include <time.h>
 #include <unistd.h>
 
 #include "dof_parser.h"
@@ -88,14 +89,38 @@ dof_parser_host_read(int in, int timeout)
 	 * only after that, both to make sure we don't underread and to make
 	 * sure we don't *overread* and concatenate part of another message
 	 * onto this one.
+	 *
+	 * Adjust the timeout whenever we are interrupted.  If we can't figure
+	 * out the time, don't bother adjusting, but still read: a read taking
+	 * longer than expected is better than no read at all.
 	 */
 	for (i = 0, sz = offsetof(dof_parsed_t, type); i < sz;) {
 		size_t ret;
+		struct timespec start, end;
+		int no_adjustment = 0;
+		long timeout_msec = timeout * 1000;
 
-		if ((ret = poll(&fd, 1, timeout * 1000)) <= 0)
+		if (clock_gettime(CLOCK_REALTIME, &start) < 0)
+			no_adjustment = 1;
+
+                while ((ret = poll(&fd, 1, timeout_msec)) <= 0 && errno == EINTR) {
+
+                        if (no_adjustment || clock_gettime(CLOCK_REALTIME, &end) < 0)
+				continue; /* Abandon timeout adjustment */
+
+			timeout_msec -= ((end.tv_sec + ((unsigned long long) end.tv_nsec * NANOSEC)) -
+					 (start.tv_sec + ((unsigned long long) start.tv_nsec * NANOSEC))) /
+					MICROSEC;
+
+			if (timeout_msec < 0)
+				timeout_msec = 0;
+		}
+
+                if (ret < 0)
 			goto err;
 
-		ret = read(in, ((char *) reply) + i, sz - i);
+		while ((ret = read(in, ((char *) reply) + i, sz - i)) < 0 &&
+		       errno == EINTR);
 
 		if (ret <= 0)
 			goto err;
diff --git a/libdtrace/dt_pcap.c b/libdtrace/dt_pcap.c
index 484816deccacc..bb41759eb8226 100644
--- a/libdtrace/dt_pcap.c
+++ b/libdtrace/dt_pcap.c
@@ -98,7 +98,8 @@ dt_pcap_destroy(dtrace_hdl_t *dtp)
 		 */
 		close(dtp->dt_pcap.dt_pcap_pipe[1]);
 		pthread_join(dtp->dt_pcap.dt_pcap_output, NULL);
-		waitpid(dtp->dt_pcap.dt_pcap_pid, NULL, 0);
+		while (waitpid(dtp->dt_pcap.dt_pcap_pid, NULL, 0) < 0 &&
+		       errno == EINTR);
 	}
 }
 
@@ -368,7 +369,7 @@ dt_pcap_filename(dtrace_hdl_t *dtp, FILE *fp)
 			 */
 			close(pipe_in[1]);
 			close(pipe_out[0]);
-			waitpid(pid, NULL, 0);
+			while (waitpid(pid, NULL, 0) < 0 && errno == EINTR);
 			dtp->dt_pcap.dt_pcap_pid = -1;
 			return NULL;
 	}
diff --git a/libdtrace/dt_proc.c b/libdtrace/dt_proc.c
index ed142c3bfd46f..64d0a763cd56d 100644
--- a/libdtrace/dt_proc.c
+++ b/libdtrace/dt_proc.c
@@ -1138,7 +1138,7 @@ dt_proc_loop(dt_proc_t *dpr, int awaiting_continue)
 			jmp_buf this_exec_jmp, *old_exec_jmp;
 			volatile int did_exec_retry = 0;
 
-			read(dpr->dpr_proxy_fd[0], &junk, 1);
+			while (read(dpr->dpr_proxy_fd[0], &junk, 1) < 0 && errno == EINTR);
 			pfd.revents = 0;
 
 			/*
diff --git a/libproc/Pcontrol.c b/libproc/Pcontrol.c
index 0cc1692f3b489..eb450bdc29814 100644
--- a/libproc/Pcontrol.c
+++ b/libproc/Pcontrol.c
@@ -211,7 +211,7 @@ Pcreate(
 	}
 	close(forkblock[1]);
 
-	waitpid(pid, &status, 0);
+	while (waitpid(pid, &status, 0) < 0 && errno == EINTR);
 	if (!WIFSTOPPED(status) || WSTOPSIG(status) != SIGTRAP ||
 	    (status >> 8) != (SIGTRAP | PTRACE_EVENT_EXEC << 8)) {
 		rc = ENOENT;
diff --git a/libproc/elfish.c b/libproc/elfish.c
index f4e54e8324001..210e5ff0f5db5 100644
--- a/libproc/elfish.c
+++ b/libproc/elfish.c
@@ -35,7 +35,7 @@
 int
 Pread_isa_info(struct ps_prochandle *P, const char *procname)
 {
-	int fd;
+	int fd, err;
 	Elf64_Ehdr hdr;
 
 	if ((fd = open(procname, O_RDONLY)) < 0) {
@@ -43,7 +43,8 @@ Pread_isa_info(struct ps_prochandle *P, const char *procname)
 		return -1;
 	}
 
-	if (read(fd, &hdr, sizeof(hdr)) < 0) {
+	while ((err = read(fd, &hdr, sizeof(hdr))) < 0 && errno == EINTR);
+        if (err < 0) {
 		_dprintf("%s is not an ELF file\n", procname);
 		close(fd);
 		return -1;
-- 
2.42.0.271.g85384428f1