[DTrace-devel] [PATCH 2/2] EINTR: make safe under signal hits
Kris Van Hees
kris.van.hees at oracle.com
Wed Nov 1 03:42:38 UTC 2023
On Tue, Oct 31, 2023 at 06:18:46PM +0000, 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);
Is this functionally the same though? With the single conditional, you were
doing a poll() that waits at most timeout * 1000 ms. Now you are performing
a loop that at each iteration waits up to timeout * 1000 ms. Therefore, it is
possible that an EINTR will cause the wait to be longer than timeout * 1000 ms.
Shouldn't the loop be adjusting the timeout in the case of EINTR to take
another iteration for the remainder of the timeout rather than the full
duration?
> + 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);
Same comment as above.
> +
> + 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;
> --
> 2.42.0.271.g85384428f1
>
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list