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

Wed Nov 8 05:36:41 UTC 2023

On Wed, Nov 01, 2023 at 03:31:05PM +0000, Nick Alcock wrote:
> 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>

Reviewed-by: Kris Van Hees <kris.van.hees 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
> 
>