[DTrace-devel] [PATCH 2/2] EINTR: make safe under signal hits

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
I guess.  I'm not real familiar with EINTR.

But need copyright years for dof_parser_host.c, dt_pcap.c, and elfish.c.

Also... s/but not only that// in the opening sentence.

On 10/31/23 14:18, Nick Alcock via DTrace-devel 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.  (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).
>
> Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
> ---
>   libcommon/dof_parser_host.c | 6 ++++--
>   libdtrace/dt_consume.c      | 9 ++++++---
>   libdtrace/dt_pcap.c         | 5 +++--
>   libdtrace/dt_proc.c         | 2 +-
>   libproc/Pcontrol.c          | 2 +-
>   libproc/elfish.c            | 5 +++--
>   6 files changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/libcommon/dof_parser_host.c b/libcommon/dof_parser_host.c
> index feb719020dc18..5c6db673fe810 100644
> --- a/libcommon/dof_parser_host.c
> +++ b/libcommon/dof_parser_host.c
> @@ -92,10 +92,12 @@ dof_parser_host_read(int in, int timeout)
>   	for (i = 0, sz = offsetof(dof_parsed_t, type); i < sz;) {
>   		size_t ret;
>   
> -		if ((ret = poll(&fd, 1, timeout * 1000)) <= 0)
> +		while ((ret = poll(&fd, 1, timeout * 1000)) <= 0 && errno == EINTR);
> +		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_consume.c b/libdtrace/dt_consume.c
> index f5dfc373caea7..e25f6759b7270 100644
> --- a/libdtrace/dt_consume.c
> +++ b/libdtrace/dt_consume.c
> @@ -3065,9 +3065,12 @@ dtrace_consume(dtrace_hdl_t *dtp, FILE *fp, dtrace_consume_probe_f *pf,
>   	 * We therefore need to convert the value.
>   	 */
>   	interval /= NANOSEC / MILLISEC;
> -	cnt = epoll_wait(dtp->dt_poll_fd, events, dtp->dt_conf.num_online_cpus,
> -			 interval);
> -	if (cnt < 0) {
> +
> +	while ((cnt = epoll_wait(dtp->dt_poll_fd, events,
> +				 dtp->dt_conf.num_online_cpus, interval)) < 0 &&
> +	       errno == EINTR);
> +
> +        if (cnt < 0) {
>   		dt_set_errno(dtp, errno);
>   		return DTRACE_WORKSTATUS_ERROR;
>   	}
> 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;